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
