Page MenuHomePhabricator

Fix __has_unique_object_representations with no_unique_address
AcceptedPublic

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
2628–2629

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
2638

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
2628–2629

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
1

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!