Page MenuHomePhabricator

[Debug-Info][DBX] DW_AT_alignment should not be generated when dwarf version is not 5
AbandonedPublic

Authored by shchenz on Apr 16 2021, 2:28 AM.

Details

Reviewers
dblaikie
aprantl
probinson
jsji
Esme
echristo
Group Reviewers
Restricted Project
Summary

As discussed in D99250 , we added a new tuning debugger DBX(D99400) to add some debug info customizations for DBX on AIX.

This is part of the customizations, not generating DWARF infos not matching the DWARF version.

This is for DW_AT_alignment attribute.

Now for DBX, we only generate this attribute when DWARF version is 5 or higher.

Diff Detail

Event Timeline

shchenz created this revision.Apr 16 2021, 2:28 AM
shchenz requested review of this revision.Apr 16 2021, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 2:28 AM

As @probinson mentioned in https://reviews.llvm.org/D99250#2651953

Our practice has been for a "tuning" option to, effectively, expand into other options to control various bits of behavior. Basically, there should never be behavior that can be controlled _only_ via the tuning option. This is a design decision that was debated at length when we introduced tuning.

(@probinson - do we have that written down somewhere?)

So this would have to be changed to add an LLVM flag (only has to be added in DwarfDebug.cpp, most likely - it doesn't have to be a published flag for Clang, and probably shouldn't be)

But this probably gets into Paul's suggestion that it might be easier to add one strict mode flag ( https://reviews.llvm.org/D100630#2694681 ) to group all this (I wouldn't mind adding that as a single flag in DwarfDebug.cpp for now - whether or not that gets plumbed up through into a CLang flag should probably be handled separately, but would be easy enough to do) under.

As @probinson mentioned in https://reviews.llvm.org/D99250#2651953

Our practice has been for a "tuning" option to, effectively, expand into other options to control various bits of behavior. Basically, there should never be behavior that can be controlled _only_ via the tuning option. This is a design decision that was debated at length when we introduced tuning.

(@probinson - do we have that written down somewhere?)

So this would have to be changed to add an LLVM flag (only has to be added in DwarfDebug.cpp, most likely - it doesn't have to be a published flag for Clang, and probably shouldn't be)

But this probably gets into Paul's suggestion that it might be easier to add one strict mode flag ( https://reviews.llvm.org/D100630#2694681 ) to group all this (I wouldn't mind adding that as a single flag in DwarfDebug.cpp for now - whether or not that gets plumbed up through into a CLang flag should probably be handled separately, but would be easy enough to do) under.

Thanks @dblaikie . Yes, I will change these patches according to Paul's suggestion. But seems we have to add this flag as a clang flag. For some cases, the mismatched DWARF infos are generated in clang FE. See D100630. BTW, do you know why there are two options -gstrict-dwarf, -gno-strict-dwarf but with no users in clang options list? Are these options used to check the DWARF infos are generated in the right DWARF version? Thanks.

shchenz planned changes to this revision.Mon, Apr 19, 8:53 PM
shchenz updated this revision to Diff 338763.Tue, Apr 20, 1:33 AM

1: use strict dwarf flag instead of DBX debugger

As @probinson mentioned in https://reviews.llvm.org/D99250#2651953

Our practice has been for a "tuning" option to, effectively, expand into other options to control various bits of behavior. Basically, there should never be behavior that can be controlled _only_ via the tuning option. This is a design decision that was debated at length when we introduced tuning.

(@probinson - do we have that written down somewhere?)

Yes, although it took me a while to find, which means it's probably not in the best place. Look for the definition of DebuggerKind enum in include/llvm/Target/TargetOptions.h. Or in the corresponding doxygen-produced docs.

shchenz abandoned this revision.Thu, Apr 22, 12:16 AM

we use a new way in D101024, so this is not needed