- Assume that clang passes non-zero alignment value to DIBuilder only in case when it was forced by C++11 'alignas', C11 '_Alignas' or compiler attribute '__attribute((aligned (N)))'.
- Emit DW_AT_alignment if alignment is specified for type/object. See: http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 for more details.
Details
- Reviewers
dblaikie echristo aprantl mehdi_amini - Commits
- rG3c9899842bf4: DebugInfo: support for DWARFv5 DW_AT_alignment attribute
rGe398c6afa9f5: DebugInfo: support for DWARFv5 DW_AT_alignment attribute
rL285189: DebugInfo: support for DWARFv5 DW_AT_alignment attribute
rL285181: DebugInfo: support for DWARFv5 DW_AT_alignment attribute
Diff Detail
Event Timeline
What's the purpose of the flag? Could we just use the non-zero value of the alignment attribute as the signal of whether to emit the DW_AT_alignment attribute? (eg: if the alignment is specified, emit it - simple?)
The alignment could be not specified and default alignment is used. Consider the following example:
struct S { } s;
This will result in the following IR code:
!5 = !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !1, line: 1, align: 8, elements: !2)
And getAlignInBits() will return 8. Thus we need to mark entities with non-default alignment.
Agreed. I also had a thought that data8 is too large for that. I just (for some reason) was sure that DWARF document explicitly stated that value should be 64 bit. Rechecked now: there is no such info in that doc, I was mistaken. Will fix.
Is the alignment used for anything in this case? Could we just remove it & make it so that whenever it's present we put the DW_AT_alignment attribute on the entity?
The DW_AT_alignment is intended to be emitted only if object's alignment is not default. In case above the default alignment for struct S is 1 byte but C++11 and C11 (or compiler-specific directives) allows user to specify different alignment value.
Alignment value (whether it is default or user-defined) must be present in IR in order to be able to generate correct code, as for new DWARF attribute: we just give debuggers information that user has explicitly set the alignment value. This knowledge can be useful in case a debugger needs to create an object for use with some expressions to be evaluated.
In D24425#539028, @dblaikie wrote:
Alignment value (whether it is default or user-defined) must be present in IR in order to be able to generate correct code,
I'm not sure I follow this ^ - the metadata here is only for debug info. It's not used for correctness of the resulting program. (the alignment of pointers in the IR for actual code generation is stored separately).
Correct. Debug Info dump doesn't influence code generation. Bad example from my side.
I'll try to rephrase:
Is the alignment currently emitted in the debug info IR metadata used for anything? Seems it's used for bitfield related things, but perhaps that's not the right solution/we could do that another way? So that the alignment just corresponds directly to the DW_AT_alignment (eg: when present, emit an alignment attribute, otherwise don't) to keep the metadata simple.
Ah, I got it now. You suggest the following way:
- Do not add DINode::FlagAlignment
- Do not add alignment to DI* objects when creating them unless alignment was forced by user
- When generating DWARF output emit DW_AT_alignment if DI* object has non-zero alignment field
I have looked through the code and you seem to be correct, actually the align value from DI* objects is used only when working with bitfields. I think I should consult with llvm-dev@ before breaking anything.
Got rid of DINode::FlagAlignment: now we assume that clang passes non-zero align value to DIBuilder only in case if alignment was forced. Thus we just check for non-zero align value and if true, emit DW_AT_alignment. Also updated related tests according to new behavior.
This might be a bit easier to review if it were broken into separate patches - it looks like there's orthogonal functionality in here (ie: some code to add the alignment attribute to global variables, some to add it to types, etc) - is that the case? Do those changes have some pieces in common that needs to be staged together in some way?
Yep, some code can be moved to separate patch. Will create a new review and put it into relationship with this one. Thanks for advice.
- Separated part of code which adds alignment param to DI*Variable family into https://reviews.llvm.org/D25073
- Update ll tests to actual ll format
include/llvm/IR/DIBuilder.h | ||
---|---|---|
139 | What is the motivation to change this to bytes? This kind of API change looks dangerous to me because frontends won't even get a type error to notify them that the API has changed. |
include/llvm/IR/DIBuilder.h | ||
---|---|---|
139 | Hmm, yep, I think you are right. I thought it would be better to pass bytes from frontend to backend as frontend has special CharUnits class for this purpose, however such changes in API can affect another frontends. Will change it back to bits and convert in backend. |
Revert API changes: DIBuilder create* methods now take alignment in bits instead of bytes, like before (conversion is done when emitting DW_AT_alignment).
include/llvm/IR/DIBuilder.h | ||
---|---|---|
212 | This should be a separate NFC commit. | |
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp | ||
779 | Can this ever be 0? | |
lib/Support/Dwarf.cpp | ||
152 | For consistency with the rest of the file, I would put this on the same line. | |
test/DebugInfo/X86/align_c11.ll | ||
52 | We typically get rid of all unnecessary attributes (= everything in quotes). | |
test/DebugInfo/X86/align_cpp11.ll | ||
114 | same here | |
unittests/Transforms/Utils/Cloning.cpp | ||
256 | This should be a separate NFC commit. |
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp | ||
---|---|---|
779 | I think it can be NULL: class DbgVariable doesn't check its DILocalVariable * member for being NULL when being constructed or accessed. | |
lib/Support/Dwarf.cpp | ||
152 | See DW_AT_MIPS_software_pipeline_depth it has same formatting (as requires coding standard and clang-format), so we already have this file formatting inconsistent. I'd prefer keeping clang-format silent on changes rather them keeping current formatting intact manually. |
- Reverted unnecessary format changes
- Removed unnecessary context from tests
- Updated documentation for SourceLevelDebugging.rst
docs/SourceLevelDebugging.rst | ||
---|---|---|
450 ↗ | (On Diff #73775) | Thanks!! |
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp | ||
779 | Hmm. I don't think it makes sense to ever construct a DbgVariable without a variable. We should probably change it to a reference at some point. Thanks for checking. | |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
1415 | What about packed structs? Sorry, I'm just being very cautious about any changes here, because every single time I touched this code in that past I broke the either LLDB or GDB ;-) A couple of examples from the testsuite you can run through clang with both -gdwarf-2 and -gdwarf-4 and verify that the DWARF output didn't change: #pragma pack(1) struct PackedBits { char a; uint32_t b : 5, c : 27 } s; #pragma pack() struct bitfield { int a : 2; int b : 32; int c : 1; int d : 28; } struct size8 { int i : 4; long long l : 60; }; struct layout0 { char l0_ofs0; struct size8 l0_ofs8; int l0_ofs16 : 1; }; struct size8_anon { int : 4; long long : 60; }; struct layout1 { char l1_ofs0; struct size8_anon l1_ofs1; int l1_ofs9 : 1; }; #pragma pack(1) struct size8_pack1 { int i : 4; long long l : 60; }; struct layout2 { char l2_ofs0; struct size8_pack1 l2_ofs1; int l2_ofs9 : 1; }; #pragma pack() #pragma pack(4) struct size8_pack4 { int i : 4; long long l : 60; }; struct layout3 { char l3_ofs0; struct size8_pack4 l3_ofs4; int l3_ofs12 : 1; }; #pragma pack() |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
---|---|---|
1415 | We already have test for that: DebugInfo/X86/debug-info-packed-struct.ll - its based on debug-info-packed-struct.c from clang (the one you quoted). I also manually checked this both with dwarf-2 and dwarf-4, dwarf output is the same for clang 3.9 release and current build with my changes. |
What is the motivation to change this to bytes? This kind of API change looks dangerous to me because frontends won't even get a type error to notify them that the API has changed.