This is an archive of the discontinued LLVM Phabricator instance.

DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced
ClosedPublic

Authored by vleschuk on Sep 9 2016, 1:56 PM.

Details

Summary

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.

Diff Detail

Event Timeline

vleschuk updated this revision to Diff 70898.Sep 9 2016, 1:56 PM
vleschuk retitled this revision from to DebugInfo: use llvm::DINode::FlagAlignment to mark forcibly aligned data.
vleschuk updated this object.
vleschuk added a subscriber: cfe-commits.
aprantl added inline comments.Sep 9 2016, 2:37 PM
lib/CodeGen/CGDebugInfo.cpp
979

Could you please do the re-formatting as a separate NFC commit? No need to for a separate review.
This makes reviewing, cherry-picking, and bisection much easier.

vleschuk added inline comments.Sep 9 2016, 8:29 PM
lib/CodeGen/CGDebugInfo.cpp
979

Sure, sorry for that. Will revert.

vleschuk updated this revision to Diff 70949.Sep 9 2016, 8:59 PM

Reverted formatting changes.

vleschuk marked 2 inline comments as done.Sep 9 2016, 9:00 PM
vleschuk updated this revision to Diff 70965.Sep 11 2016, 9:02 AM
vleschuk retitled this revision from DebugInfo: use llvm::DINode::FlagAlignment to mark forcibly aligned data to DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced.
vleschuk updated this object.

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.

dblaikie edited edge metadata.Sep 12 2016, 4:06 PM

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
609

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)

3691

is max alignment the right thing here? Should it be min alignment?
(is alignment in bits the desired thing across all of this too? It looked like in the backend patch there was some division by CHAR_BITS, etc?)

probinson added inline comments.
lib/CodeGen/CGDebugInfo.cpp
3691

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).

rnk added a subscriber: rnk.Sep 12 2016, 5:21 PM
rnk added inline comments.
lib/CodeGen/CGDebugInfo.cpp
608

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.

vleschuk added inline comments.Sep 13 2016, 12:50 PM
lib/CodeGen/CGDebugInfo.cpp
3691

is max alignment the right thing here? Should it be min alignment?

The DWARF document says:

This constant describes the actual alignment used by the compiler (if there are multiple specified by the user, or if the user specified an alignment the compiler couldn't satisfy, then only the strictest alignment will be added through a DW_AT_alignment).

So I think max alignment is right choice here.

vleschuk added inline comments.Sep 13 2016, 12:53 PM
lib/CodeGen/CGDebugInfo.cpp
608

Will check if this works in all cases. I think it's worth putting this snippet into helper function within anon namespace.

vleschuk updated this revision to Diff 73228.Oct 2 2016, 6:27 PM
vleschuk edited edge metadata.
vleschuk marked 3 inline comments as done.Oct 2 2016, 6:30 PM
vleschuk added inline comments.
lib/CodeGen/CGDebugInfo.cpp
608

That is correct for types, for particular decls we still need to use hasAttr<>. Moved both cases into separate functions.

609

Default arguments now are zeros.

vleschuk marked 2 inline comments as done.Oct 2 2016, 6:30 PM
rnk added inline comments.Oct 3 2016, 8:38 AM
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
53

Please add a blank line between top level decls

vleschuk updated this revision to Diff 73318.Oct 3 2016, 11:55 AM

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
rnk accepted this revision.Oct 3 2016, 11:58 AM
rnk added a reviewer: rnk.

lgtm

lib/CodeGen/CGDebugInfo.cpp
54–56

Don't duplicate, just delegate to the Type * overload:

return getTypeAlignIfRequired(Ty.getTypePtr(), Ctx);
This revision is now accepted and ready to land.Oct 3 2016, 11:58 AM
vleschuk updated this revision to Diff 73537.Oct 4 2016, 12:49 PM
vleschuk edited edge metadata.

Pass alignment in bits instead of bytes to backend (conversion is done when emitting DW_AT_alignment).

vleschuk updated this revision to Diff 74686.Oct 14 2016, 7:38 AM

Use DIAlignment type instead of uint64_t for alignment in DebugInfo.

This revision was automatically updated to reflect the committed changes.