[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:420
+    auto *FD = IFD ? IFD->getAnonField() : dyn_cast<FieldDecl>(D);
+    if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion()))
+      continue;
----------------
rsmith wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > rsmith wrote:
> > > > erichkeane wrote:
> > > > > rsmith wrote:
> > > > > > erichkeane wrote:
> > > > > > > rsmith wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > Losing the unnamed bitfield is unfortunate, and I the 'dump struct' handles.  As is losing the anonymous struct/union.
> > > > > > > > > 
> > > > > > > > > Also, how does this handle 'record type' members?  Does the user have to know the inner type already?  If they already know that much information about the type, they wouldn't really need this, right?
> > > > > > > > If `__builtin_dump_struct` includes unnamed bitfields, that's a bug in that builtin.
> > > > > > > > 
> > > > > > > > In general, the user function gets given the bases and members with the right types, so they can use an overload set or a template to dispatch based on that type. See the SemaCXX test for a basic example of that.
> > > > > > > I thought it did?  For the use case I see `__builtin_dump_struct` having, I would think printing the existence of unnamed bitfields to be quite useful.
> > > > > > > 
> > > > > > > 
> > > > > > Why? They're not part of the value, they're just padding.
> > > > > They are lexically part of the type and are part of what makes up the 'size' of the thing. I would expect something dumping the type to be as consistent lexically as possible.
> > > > Looks like `__builtin_dump_struct` currently includes them *and* prints a value (whatever garbage happens to be in those padding bits). That's obviously a bug.
> > > O.o! Yeah, printing the value is nonsense.
> > I think the user should be informed somehow that there is an unnamed bitfield here
> It seems to me that `__builtin_dump_struct` is displaying the value of a struct object, not the representation of the object, and unnamed bitfields are not a part of the value, so should not be included. And I think it makes sense for that builtin to be value-oriented: someone using it presumably already knows whatever they want to know about the representation, to the extent that it matters, or can look it up; what they don't know is the information that varies from one instance to another. If we want a builtin that's more oriented around showing the struct's memory representation, I think we'd want quite a different output format, including offsets for fields -- and even then I don't think it makes sense to include unnamed bit-fields because they're not different from any other kind of padding in the object. If we still want to print unnamed bit-fields for some reason, presumably we should also print alignment and packing attributes and pragmas, because they too can change the padding within a struct.
I tend to agree that `__builtin_dump_struct` should be focused on the values in the structure and not the representation.

However, keeping in mind that the original builtin was for kernel folks and the kernel tends to do some pretty interesting things (that aren't always supported by the standard), there's a part of me that wonders if they have a use case for unnamed bit-fields where they expect data to be squirreled away in those bits (through `memcpy()` or some other means) that they'd like to be able to read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221