Check for forced alignment (alignas(), _Alignas(), __attribute(aligned (N))))) when creating DI* objects with DIBuilder. If alignment was not forced pass zero, otherwise pass actual alignment value.
Details
Diff Detail
Event Timeline
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
990–991 | Could you please do the re-formatting as a separate NFC commit? No need to for a separate review. |
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
990–991 | Sure, sorry for that. Will revert. |
Got rid of DINode::FlagAlignment. Check for forced alignment (alignas(), _Alignas(), __attribute(aligned (N))))) when creating DI* objects with DIBuilder. If alignment was not forced pass zero, otherwise pass actual alignment value. Updated related tests according to new behavior.
Like the backend patch, should/could this be broken up into separate patches for the different places/features that have alignment?
(probably just passing zero for alignment in general could be a simple cleanup patch to start, if it's otherwise unused)
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
621–622 | Maybe add a comment here about why we're specifying zero alignment (or possibly change the API to not take an alignment at all so we don't have to explain it?) (similarly for other cases where the alignment is just hardcoded to zero) | |
3661 | is max alignment the right thing here? Should it be min alignment? |
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
3661 | I should think bits is the right choice here; seems more the province of the backend to convert it into the appropriate addressable units (commonly but not universally chars). |
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
621 | IMO this is what we should be doing everywhere, rather than manually checking AlignedAttr: TypeInfo TI = CGM.getContext().getTypeInfo(Ty); uint64_t Size = TI.Width; uint64_t Align = TI.AlignIsRequired ? TI.Align : 0; This saves a hash lookup, and handles some corner cases. AlignIsRequired is already supposed to capture whether the alignment was changed. |
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
3661 |
The DWARF document says:
So I think max alignment is right choice here. |
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
621 | Will check if this works in all cases. I think it's worth putting this snippet into helper function within anon namespace. |
- Move alignment detection into helper functions in anon namespace
- Use updated LLVM DIBuilder API (from https://reviews.llvm.org/D24425)
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
47 | LLVM prefers static to anonymous namespaces http://llvm.org/docs/CodingStandards.html#anonymous-namespaces | |
48 | Instead of templating, have a const Type* overload and a QualType overload like getTypeInfo does: /// \brief Get the size and alignment of the specified complete type in bits. TypeInfo getTypeInfo(const Type *T) const; TypeInfo getTypeInfo(QualType T) const { return getTypeInfo(T.getTypePtr()); } | |
49 | LLVM style uses a leading lower case for method names: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | |
53 | Please add a blank line between top level decls |
Fix code style:
- Two overloaded methods instead of 1 template
- lower-case-headed method names
- static methods instead of anon namespace
- extra new line between methods
lgtm
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
54–56 | Don't duplicate, just delegate to the Type * overload: return getTypeAlignIfRequired(Ty.getTypePtr(), Ctx); |
Pass alignment in bits instead of bytes to backend (conversion is done when emitting DW_AT_alignment).
LLVM prefers static to anonymous namespaces http://llvm.org/docs/CodingStandards.html#anonymous-namespaces