Add support for the C++2a [[no_unique_address]] attribute for targets using the Itanium C++ ABI.
This depends on D63371.
Paths
| Differential D63451
P0840R2: support for [[no_unique_address]] attribute ClosedPublic Authored by rsmith on Jun 17 2019, 11:38 AM.
Details Summary Add support for the C++2a [[no_unique_address]] attribute for targets using the Itanium C++ ABI. This depends on D63371.
Diff Detail
Event TimelineComment Actions Can you also add some SemaCXX tests that ensure the attribute is properly diagnosed when written on a bit-field, a static data member, a function, is given an argument, etc?
Comment Actions Can this attribute not be applied to a base class, or to a type? I think every time I've seen someone get bitten by the unique-address rule, it was because they had a base class that deleted some constructors (or something like that) and which was a base of a million different types; I'm sure the language model they'd prefer would be to just add an attribute to that one type instead of chasing down every place where they declared a field of the type.
rsmith added inline comments.
Comment Actions The attribute parts LGTM! You can change the TargetItaniumCXXABI part in a follow-up commit if you'd prefer.
This revision is now accepted and ready to land.Jun 19 2019, 8:36 AM rsmith marked 4 inline comments as done. Comment Actions
rjmccall added inline comments. Comment Actions
The standard attribute forbids that right now. We could add a custom attribute that permits it, but we're required to diagnose application of the standard attribute to a type -- though a warning would suffice to satisfy that requirement. (Because this gives "same as a base class" layout, adding it to a base class wouldn't have any effect right now, but we could certainly say that the attribute on a class type also implies the class is not POD for the purpose of layout, to permit tail padding reuse.)
The place where we expect this to be used is where a class template wants to store a copy of some user-provided type that may well be empty (eg, an allocator, a comparator, that kind of thing). There are probably more producers of such types than consumers, so putting the attribute on the consumer seems to make some sense (though it really could go in either place, and both probably have reasonable use cases). I'd be happy to go back to the committee with a proposal to extend this attribute to also apply to classes if you can provide a few convincing examples. Closed by commit rL363976: P0840R2: support for [[no_unique_address]] attribute (authored by rsmith). · Explain WhyJun 20 2019, 1:43 PM This revision was automatically updated to reflect the committed changes. Comment Actions
I think we're talking about slightly different things. You're trying to make sure that fields can take advantage of the layout optimizations available to base classes so that library code doesn't need to contort to e.g. use the empty-base-class optimization just to avoid wasting an entire pointer to store an empty allocator. This attribute seems well-targeted for that. Totally separately from that — but perhaps better-related to the name of the attribute — I know that some projects have, in the past, had surprising problems with the rule that class layout can't place empty base classes at the same offset as other objects of the same type. For example, IIRC WebKit used to have a Noncopyable class that deleted its copy constructor and copy-assignment operators, and there was an idiom to inherit from this in order to disable copying, and at one point they discovered that some fairly important class was quite a bit larger than expected because of this rule — I think they were also using the base class redundantly throughout the hierarchy because they didn't expect there to be any harm to doing so. They would've no doubt appreciated the ability to just put the attribute on Noncopyable. But of course they fixed that years ago, so I can't say that it's a serious issue for them now.
Revision Contents
Diff 205892 include/clang/AST/Decl.h
include/clang/AST/DeclCXX.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/AST/Decl.cpp
lib/AST/DeclCXX.cpp
lib/AST/RecordLayoutBuilder.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGOpenMPRuntime.cpp
lib/CodeGen/CGRecordLayoutBuilder.cpp
lib/CodeGen/CodeGenFunction.h
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDeclAttr.cpp
test/CodeGenCXX/no-unique-address.cpp
test/CodeGenCXX/tail-padding.cpp
test/Layout/no-unique-address.cpp
test/SemaCXX/cxx2a-no-unique-address.cpp
utils/TableGen/ClangAttrEmitter.cpp
www/cxx_status.html
|
Do you mind re-flowing this entire comment to 80 cols?