Fix __has_unique_object_representations based on rsmith's input
Needs ReviewPublic

Authored by erichkeane on Thu, Oct 26, 3:14 PM.

Details

Summary

Corrections done for @rsmith 's comments after-commit
on cfe-commits.

Diff Detail

erichkeane created this revision.Thu, Oct 26, 3:14 PM

Added all of @rsmith's comments here, I believe I have them all and have properly responded to them.

include/clang/AST/ASTContext.h
2115

Moved because @rsmith said:

I think this would make more sense as a member of ASTContext. The Type object generally doesn't know or care about its representation.

lib/AST/Type.cpp
2212

@rsmith said: Hmm, are there any cases in which we might want to guarantee the vptr is identical across all instances?

I believe the answer to that is 'no', but if you have a case in mind, I can change this.

2218

@rsmith said: This seems really fragile to me. Empty bases may or may not occupy storage. But that's beside the point -- empty bases are just a special case of bases with tail padding, which are not properly handled here.

I really think you should be walking the record layout rather than trying to determine this from sizes alone.

Response:
I think I did that properly in this patch here. I re-did a bunch of the code around this, so hopefully it is to your liking.

2233

@rsmith said: Stray semicolon?

Yep, don't know how that got in there...

2281

@rsmith said: I don't think this is correct. As a weird GNU behaviour, we can have, say, a type with size 3 and alignment 4 (via an alignment attribute on a typedef). An array of 1 such element has size 4, and has padding even if its element type does not.

I think I added a test that covers that here. Is there a way to mark non-record types with an alignment? If not, this cause problems, because the Layout.getSize() checks in the struct handling will take care of this, since 'getSize' includes alignment.

2296

@rsmith said: The second half of this comment doesn't seem right to me. They may be represented as sequences of bits, but that doesn't make them integral.

I think you're right, I removed this part.

2303

@rsmith said: Why?

Thats a good question that I lack the answer to... I decided to remove this and let the 'struct' handling take care of it, which seems to make sense, and even better, seems to work.

2308

@rsmith said: Making these members of QualType seems counterproductive. You already have the Record here; it'd be better to make these file-static and pass that in.

Done.

Also, I should probably validate bitfields. The following SHOULD?! be unique?

struct S {
unsigned a : 1;
unsigned b : 2;
unsigned c : 3;
unsigned d : 2;
};

static_assert(__has_unique_object_representations(S), "error");

But without 'd', it should not be (tail padding).

Corrected version of the previous example:
struct S {
char a : 1;
char b : 2;
char c : 3;
char d : 2;
};
static_assert(sizeof(S) == 1, "size");
static_assert(__has_unique_object_representations(S), "error");

I haven't tested this patch agaainst this repro, but it should likely be in the tests.

Fixed for bitfields. Review by anyone greatly appreciated :)

It'd be good to have tests which have alignment directives on bitfields.

Added a test for aligned bitfield, as @majnemer requested. Is this sufficient?

Ping? This fixes a number of bugs with the previous implementation, so I'd like to see if we can get this in.