This is an archive of the discontinued LLVM Phabricator instance.

Fix __has_unique_object_representations with no_unique_address
ClosedPublic

Authored by gbencze on Oct 18 2020, 10:18 AM.

Details

Summary

Fix incorrect behavior of __has_unique_object_representations when using the no_unique_address attribute.

Based on the bug report: https://bugs.llvm.org/show_bug.cgi?id=47722

Diff Detail

Event Timeline

gbencze created this revision.Oct 18 2020, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2020, 10:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gbencze requested review of this revision.Oct 18 2020, 10:18 AM
rsmith accepted this revision.Oct 18 2020, 1:47 PM

This looks fine as far as it goes, but it doesn't fix all cases of incorrect behavior of __has_unique_object_representations due to [[no_unique_address]]. Feel free to either to land this as-is and leave the other case to a separate patch, or fix it as a revision of this same change. I would expect we'll want to unify the code paths for base classes and non-static data members, which might be better done all at once.

clang/lib/AST/ASTContext.cpp
2580–2581

We need to do this for non-empty [[no_unique_address]] members of class type too, to handle tail padding reuse in cases such as:

struct A {
    ~A();
    int a;
    char b;
};
struct B {
    [[no_unique_address]] A a;
    char c[3];
};
static_assert(sizeof(B) == 8, "");
static_assert(__has_unique_object_representations(B), "");
This revision is now accepted and ready to land.Oct 18 2020, 1:47 PM
aaron.ballman added inline comments.Oct 19 2020, 8:11 AM
clang/lib/AST/ASTContext.cpp
2608

You can drop the else after this sorta-return if you'd like.

gbencze added inline comments.Oct 19 2020, 8:24 AM
clang/lib/AST/ASTContext.cpp
2580–2581

You're right, I missed the case of reusing tail padding. I'll try to update this review to include this soon.

But I don't think that this exact example is incorrect now as A is not trivially copyable due to the user defined destructor.
It should return true when the class type member is trivially copyable but non-standard-layout though:

struct A {
private:
    int a;
public:
    char b;
};
struct B {
    [[no_unique_address]] A a;
    char c[3];
};
static_assert(sizeof(B) == 8, "");
static_assert(__has_unique_object_representations(B), "");
gbencze updated this revision to Diff 303082.Nov 5 2020, 4:19 AM

Sorry for the slow update on this.
I fixed the behavior when reusing tail padding as mentioned by @rsmith and took a shot at unifying the code paths for base classes and fields. Let me know your thoughts on the approach!

gbencze added inline comments.Nov 5 2020, 4:27 AM
clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
2

Just to be sure: is the specifying the triple here enough to ensure that this always uses the Itanium ABI? I believe MSVC currently ignores the no_unique_address attribute. Or do I need to add some more flags?
Alternatively, the static_asserts could be modified to check sizeof(T) > [expected size with Itanium ABI] || __has_unique_object_representations(T)

Pinging @rsmith - could you please check the updates to this patch? Thanks!

It looks great.

clang/lib/AST/ASTContext.cpp
2575–2577

Why do you return None here?

2588–2591

Why is this a template?
Can't you just take the field_range?

By the same token, structSubobjectsHaveUniqueObjectRepresentations returns an optional int. I'm confused.

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
2

I think it's better to specify the triple exactly

43

Why is this outside of the namespace declaration?

gbencze added inline comments.Feb 8 2021, 10:06 AM
clang/lib/AST/ASTContext.cpp
2575–2577

These bits were just extracted from structHasUniqueObjectRepresentations, the underlying logic was not changed here. (originally in line 2615)
I believe this handles the case "If the width of a bit-field is larger than the width of the bit-field's type (or, in case of an enumeration type, of its underlying type), the extra bits are padding bits" from [class.bit]

2588–2591

This function is also called with (non virtual) base class subobjects, not just fields.
A None return value indicates that the subobjects do not have unique object representations, otherwise the size of the subobjects is returned. This allows us to detect for example padding between a base class and the first field.

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
43

Tbh I'm not sure. I can move it in the namespace if you think it'd be better there.

I'm adding a couple of reviewers to get this going.

Adding a few subscribers I could find from the original bugzilla entry, + perhaps a few reviewers to help us.

Gentle boop. Bugzilla is offline for me right now (and rumour has it that it will be completely shut down in the near future), so it would be good to know what is the status of this patch and how it related to the mentioned (and right now unavailable) bug report, as it is also blocking a Clang-Tidy check.

aaron.ballman accepted this revision.Jun 24 2021, 5:34 AM

LGTM!

clang/lib/AST/ASTContext.cpp
2563–2564
clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
43

No reason not to.

This revision was automatically updated to reflect the committed changes.