[PATCH] D39347: Fix __has_unique_object_representations based on rsmith's input

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:2213
+        Field->isBitField()
+            ? Field->getBitWidthValue(Context)
+            : Context.toBits(Context.getTypeSizeInChars(Field->getType()));
----------------
erichkeane wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > This isn't quite right; you want min(bitfield length, bit size of type) here. Eg, a `bool b : 8` or `int n : 1000` bitfield has padding bits.
> > I guess I'm missing something with this comment... why would a bitfield with bool b: 8 have padding?
> > 
> > Additionally, isn't int n : 1000 illegal, since 1000 > 32 (# bits in an int)?
> > Both clang and GCC reject the 2nd case.  Curiously, GCC rejects bool b: 9, but Clang seems to allow 'bool' to have ANY size.  Is that an error itself?
> > 
> > Finally: Should we consider ANY bool to be not a unique object representation?  It has 1 bit of data, but 8 bits of storage.  GCC's implementation of this trait accepts them, but I'm second guessing that at the moment...
> I did a bit more looking into this... In the 'TYPE b: X' case, 'X' is always the size of the field, so :
> 
>     struct Bitfield {
>       bool b: 16;
>     };
>     static_assert(sizeof(Bitfield) == 2);
> 
> The rest of my padding detection works correctly.
Sounds like you've found more GCC bugs. In C++ (unlike in C), there is no restriction on the maximum length of a bit-field. Specifically, [class.bit]p1 says: "The value of the integral constant expression may be larger than the number of bits in the object
representation (6.7) of the bit-field’s type; in such cases the extra bits are padding bits (6.7)."

For non-`bool` bit-fields, or `bool` bit-fields with more than 8 bits, the extra bits are padding, and the type does not have unique object representations. You'll need to check for this.

For `bool` bit-fields of length 1, we obviously have unique object representations for that one bit.

The interesting case is `bool` bit-fields with length between 2 and 8 inclusive. It would seem reasonable to assume they have unique representations, as we do for plain `bool` values, but this is really an ABI question (as, actually, is the plain `bool` case). The x86-64 psABI is not very clear on the bit-field question, but it does say "Booleans, when stored in a memory object, are stored as single byte objects the value of which is always 0 (false) or 1 (true).", so for at least x86-64, `bool`s of up to 8 bits should be considered to have unique object representations. The PPC64 psABI appears to say the same thing, but is also somewhat unclear about the bit-field case.


================
Comment at: lib/AST/ASTContext.cpp:2277
+
+  return false;
+}
----------------
More cases to handle here:

 * vectors (careful about, eg, vector of 3 foo)
 * `_Complex int` and friends
 * `_Atomic T`
 * Obj-C block pointers
 * Obj-C object pointers
 * and perhaps OpenCL's various builtin types (pipe, sampler_t, event_t, clk_event_t, queue_t, reserve_id_t)

It's fine to leave these for another change, but we shouldn't forget about them. (There're also Obj-C class types and the Obj-C selector type, but I think it makes sense for those to return `false` here.)


https://reviews.llvm.org/D39347