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

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122248#3403637 <https://reviews.llvm.org/D122248#3403637>, @erichkeane wrote:

> 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.

I'm sorry, I misunderstood @aaron.ballman.

In D122248#3403637 <https://reviews.llvm.org/D122248#3403637>, @erichkeane wrote:

> 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.

I'm sorry I misunderstood what you meant @aaron.ballman.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122248