Re: [PATCH v2 3/8] xen/ppc: Implement atomic.h

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 8/31/23 12:47 PM, Shawn Anastasio wrote:
> On 8/29/23 8:43 AM, Jan Beulich wrote:
>> On 23.08.2023 22:07, Shawn Anastasio wrote:
>>> +#define read_atomic(p)                                                     
>>>     \
>>> +    ({                                                                     
>>>     \
>>> +        union {                                                            
>>>     \
>>> +            typeof(*(p)) val;                                              
>>>     \
>>> +            char c[0];                                                     
>>>     \
>>
>> Using [0] here is likely to set us up for compiler complaints ...
>>
> 
> AIUI zero-length members are explicitly permitted as a GNU extension,
> but their usage here wasn't an explicit choice on my part as this macro
> was inherited from arm's atomic.h. See below.
> 
>>> +        } x_;                                                              
>>>     \
>>> +        read_atomic_size(p, x_.c, sizeof(*(p)));                           
>>>     \
>>
>> ... here. Can't this simply be c[sizeof(*(val))]? And do you need
>> a union here in the first place, when read_atomic() takes void* as
>> its 2nd parameter?
>>
> 
> Yes, I should have taken a closer look at this before importing it from
> arm. The type punning does seem entirely redundant given that
> read_atomic_size takes a void* -- I'm not sure why it was written this
> way to begin with.
> 
> In any case, I'll do away with the unnecessary union.

Quick follow up -- I think I now understand why it was written this way.
Callers are allowed to pass const pointers to read_atomic, but if the
macro just declared a local variable using typeof(*(p)), the resulting
variable would keep the const qualifier and thus be unpassable to
read_atomic_size.

It also appears that using standard C flexible array members inside of
unions isn't permitted, but the GNU 0-length array construct is. Of
course, declaring c's length as sizeof(*(p)) as you suggested works too.

As for getting rid of the union entirely then, it still ensures that
that the resulting variable has the correct alignment. We could
alternatively add an __aligned(__alignof__(*(p))) or similar to a bare
array declaration to achieve the same effect.

I think for now, I'll just leave it as-is with the union but replace the
0-length array with an array of the correct size.

Thanks,
Shawn