Page MenuHomePhabricator

P0840R2: support for [[no_unique_address]] attribute
ClosedPublic

Authored by rsmith on Jun 17 2019, 11:38 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

rsmith created this revision.Jun 17 2019, 11:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 11:38 AM
aaron.ballman added a subscriber: aaron.ballman.

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?

include/clang/AST/DeclCXX.h
337–341 ↗(On Diff #205134)

Do you mind re-flowing this entire comment to 80 cols?

lib/AST/Decl.cpp
3927 ↗(On Diff #205134)

const auto * (elsewhere in the function as well)

3930 ↗(On Diff #205134)

I'd prefer to see a concrete type here rather than auto.

lib/AST/RecordLayoutBuilder.cpp
1785 ↗(On Diff #205134)

Don't use auto here.

rsmith updated this revision to Diff 205487.Jun 18 2019, 6:05 PM
rsmith marked 5 inline comments as done.
  • Address review comments from @AaronBallman.
rsmith added inline comments.Jun 18 2019, 6:05 PM
include/clang/AST/DeclCXX.h
337–341 ↗(On Diff #205134)

Reflowed and turned into bulleted list for clarity.

rsmith updated this revision to Diff 205488.Jun 18 2019, 6:09 PM
  • Remove accidentally-added file

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.

include/clang/Basic/Attr.td
345 ↗(On Diff #205488)

Can you just duplicate the thing that Slava Pestov recently did for LangOpts? The tblgen enhancement really isn't very complicated, and I think we can agree that this is terrible. :)

We can probably kill the need for CXXABIs in tblgen entirely.

include/clang/Basic/AttrDocs.td
1020 ↗(On Diff #205488)

Can you include an example here? And should this documentation mention that this is a standard C++ attribute (or at least one proposed for adoption)?

lib/AST/Decl.cpp
3937 ↗(On Diff #205488)

Surely a bit-field of nonzero length is a subobject of nonzero size.

3945 ↗(On Diff #205488)

Is this a C/C++ modules interaction?

lib/CodeGen/CGExprAgg.cpp
1850 ↗(On Diff #205488)

getOverlapForFieldInit? overlap is a verb.

rsmith marked 2 inline comments as done.Jun 18 2019, 7:44 PM
rsmith added inline comments.
lib/AST/Decl.cpp
3937 ↗(On Diff #205488)

Usually, but not if it's unnamed (unnamed bit-fields aren't subobjects). In any case, this is a quote from the C++ standard.

3945 ↗(On Diff #205488)

We don't allow C modules to be imported into C++ compilations or vice versa, so this should be unreachable unless we start allowing the attribute in C. Nice catch.

I guess the question is, then: should we allow this attribute in C (either with a GNU __attribute__ spelling or as a C20 [[clang::attribute]])? I don't think it's really useful in C (empty structs are ill-formed, and you can't reuse tail padding because structs are always trivial, at least in standard C), so I'm inclined to say no.

lib/CodeGen/CGExprAgg.cpp
1850 ↗(On Diff #205488)

Good idea. (Both this and overlapForBaseInit are pre-existing; I'll rename both.)

rjmccall added inline comments.Jun 18 2019, 8:07 PM
lib/AST/Decl.cpp
3937 ↗(On Diff #205488)

Fair enough.

3945 ↗(On Diff #205488)

I agree that it seems relatively useless in C, and there's no reason to think they'd use this language design if they decided they did want it.

aaron.ballman accepted this revision.Jun 19 2019, 8:36 AM

The attribute parts LGTM! You can change the TargetItaniumCXXABI part in a follow-up commit if you'd prefer.

lib/AST/Decl.cpp
3945 ↗(On Diff #205488)

Agreed that this makes no sense in C. I was not planning on proposing this to WG14 because I couldn't think of a use case for it.

This revision is now accepted and ready to land.Jun 19 2019, 8:36 AM
rsmith updated this revision to Diff 205892.Jun 20 2019, 1:28 PM
rsmith marked 4 inline comments as done.
  • Use custom code to specify CXXABI requirements on attributes.
  • Remove dead code that would have handled [[no_unique_address]] in C.
  • Extend documentation to include an example and to mention that this is a standard C++ attribute.
lib/CodeGen/CGExprAgg.cpp
1850 ↗(On Diff #205488)

I'm going to do this in a separate change since there are quite a few uses of these and it'll add noise to the patch.

rjmccall accepted this revision.Jun 20 2019, 1:40 PM
rjmccall added inline comments.
include/clang/Basic/Attr.td
326 ↗(On Diff #205892)

Thanks!

lib/CodeGen/CGExprAgg.cpp
1850 ↗(On Diff #205488)

SGTM

Can this attribute not be applied to a base class, or to a type?

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

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.

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 1:43 PM
rsmith marked an inline comment as done.Jun 20 2019, 1:53 PM
rsmith added inline comments.
lib/CodeGen/CGExprAgg.cpp
1850 ↗(On Diff #205488)

Done in r363980.

Can this attribute not be applied to a base class, or to a type?

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

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.