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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
