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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:2213
+        Field->isBitField()
+            ? Field->getBitWidthValue(Context)
+            : Context.toBits(Context.getTypeSizeInChars(Field->getType()));
----------------
erichkeane wrote:
> rsmith wrote:
> > 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.
> Umm... so holy crap.  We actually reserve the object spece for the whole thing, but ONLY the first sizeof(field) fields are actually stored to!  Everything else is truncated!
> 
> I think the correct solution is to do:
>    // too large of a bitfield size causes truncation, and thus 'padding' bits.
>    if (FieldSizeInBits > Context.getTypeSizeInBits(Field->getType()) return false;
I suspect that the 'bool' 2-8 condition is a case where we can let the sleeping dogs lie here :)  Incoming patch has the other condition, where the bitfield size is greater than the type size.


================
Comment at: lib/AST/ASTContext.cpp:2277
+
+  return false;
+}
----------------
rsmith wrote:
> 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.)
I'm not sure what the correct answer is in any of those cases, since i'm not particularly familiar with any of them.  I'll put a 'fixme' with the contents of your comment in the code, and revisit it (or hope someone more knowledgable can).


https://reviews.llvm.org/D39347