[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D122248#3403636 <https://reviews.llvm.org/D122248#3403636>, @yihanaa wrote:

> In D122248#3403518 <https://reviews.llvm.org/D122248#3403518>, @aaron.ballman wrote:
>
>> In D122248#3403478 <https://reviews.llvm.org/D122248#3403478>, @erichkeane wrote:
>>
>>> If it is ok, I think we should probably change the format of the 'dump' for fields.  Using the colon to split up the field from the value is unfortunate, may I suggest replacing it with '=' instead?  As well as printing the size after a colon.  So for:
>>>
>>>   void foo(void) {
>>>     struct Bar {
>>>       unsigned c : 1;
>>>       unsigned : 3;
>>>       unsigned : 0;
>>>       unsigned b;
>>>     };
>>>   
>>>     struct Bar a = {
>>>       .c = 1,
>>>       .b = 2022,
>>>     };
>>>   
>>>     __builtin_dump_struct(&a, &printf);
>>>   }
>>>
>>> Output:
>>>
>>>   struct Bar {
>>>   unsigned int c : 1 = 1
>>>   unsigned int : 3  = 0
>>>   unsigned int : 0 = 
>>>   unsigned int b = 2022
>>>   }
>>>
>>> What do you all think?
>>
>> I think that's a good idea for clarity. For the case where we have no value, I wonder if we want to do something like: `unsigned int : 0 = <uninitialized>` (or something else to make it exceptionally clear that there's nothing missing after the `=`)?
>
> how
>
> In D122248#3403518 <https://reviews.llvm.org/D122248#3403518>, @aaron.ballman wrote:
>
>> In D122248#3403478 <https://reviews.llvm.org/D122248#3403478>, @erichkeane wrote:
>>
>>> If it is ok, I think we should probably change the format of the 'dump' for fields.  Using the colon to split up the field from the value is unfortunate, may I suggest replacing it with '=' instead?  As well as printing the size after a colon.  So for:
>>>
>>>   void foo(void) {
>>>     struct Bar {
>>>       unsigned c : 1;
>>>       unsigned : 3;
>>>       unsigned : 0;
>>>       unsigned b;
>>>     };
>>>   
>>>     struct Bar a = {
>>>       .c = 1,
>>>       .b = 2022,
>>>     };
>>>   
>>>     __builtin_dump_struct(&a, &printf);
>>>   }
>>>
>>> Output:
>>>
>>>   struct Bar {
>>>   unsigned int c : 1 = 1
>>>   unsigned int : 3  = 0
>>>   unsigned int : 0 = 
>>>   unsigned int b = 2022
>>>   }
>>>
>>> What do you all think?
>>
>> I think that's a good idea for clarity. For the case where we have no value, I wonder if we want to do something like: `unsigned int : 0 = <uninitialized>` (or something else to make it exceptionally clear that there's nothing missing after the `=`)?
>
> How to judge whether this field is initialized? Maybe this memory has been initialized by memset

He means a special-case for the 0-size bitfield, which HAS no value (actually, wonder if this is a problem with the no-unique-address types as well?).  I might suggest `N/A` instead of `uninitialized`, but am open to bikeshedding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122248