Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2024 10:13, Luca Fancellu wrote:
> 
> 
>> On 2 May 2024, at 07:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 02.05.2024 08:33, Luca Fancellu wrote:
>>>
>>>
>>>> On 2 May 2024, at 07:14, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>
>>>> On 01.05.2024 08:57, Luca Fancellu wrote:
>>>>> Hi Jan,
>>>>>
>>>>>> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>
>>>>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>>>>> --- a/xen/arch/arm/include/asm/setup.h
>>>>>>> +++ b/xen/arch/arm/include/asm/setup.h
>>>>>>> @@ -64,18 +64,20 @@ struct membank {
>>>>>>> };
>>>>>>>
>>>>>>> struct membanks {
>>>>>>> -    unsigned int nr_banks;
>>>>>>> -    unsigned int max_banks;
>>>>>>> +    __struct_group(membanks_hdr, common, ,
>>>>>>> +        unsigned int nr_banks;
>>>>>>> +        unsigned int max_banks;
>>>>>>> +    );
>>>>>>>   struct membank bank[];
>>>>>>> };
>>>>>>
>>>>>> I'm afraid I can't spot why __struct_group() is needed here. Why would 
>>>>>> just
>>>>>> one of the two more straightforward
>>>>>>
>>>>>> struct membanks {
>>>>>>  struct membanks_hdr {
>>>>>>      unsigned int nr_banks;
>>>>>>      unsigned int max_banks;
>>>>>>  );
>>>>>>  struct membank bank[];
>>>>>> };
>>>>>>
>>>>>
>>>>> At the first sight I thought this solution could have worked, however GCC 
>>>>> brought me back down to earth
>>>>> remembering me that flexible array members can’t be left alone in an 
>>>>> empty structure:
>>>>>
>>>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6:
>>>>>  error: declaration does not declare anything [-Werror]
>>>>> 70 | };
>>>>> | ^
>>>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20:
>>>>>  error: flexible array member in a struct with no named members
>>>>> 71 | struct membank bank[];
>>>>> | ^~~~
>>>>> [...]
>>>>
>>>> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution
>>>> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of
>>>> borrowing __struct_group(), we could consider borrowing this as well. Or
>>>> open-code it just here, for the time being (perhaps my preference). Yet
>>>> it's not clear to me that doing so will actually be enough to make things
>>>> work for you.
>>>
>>> I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group()
>>> was enough for my purpose, can I ask the technical reasons why it would be 
>>> your
>>> preference? Is there something in that construct that is a concern for you?
>>
>> I don't like either construct very much, but of the two 
>> __DECLARE_FLEX_ARRAY()
>> looks slightly more "natural" for what is wanted and how it's done.
>> __struct_group() introducing twice the (effectively) same structure feels
>> pretty odd, for now at least. It's not even entirely clear to me whether 
>> there
>> aren't pitfalls, seeing that the C spec differentiates named and unnamed
>> struct fields in a few cases. For __DECLARE_FLEX_ARRAY(), otoh, I can't
>> presently see any reason to suspect possible corner cases.
>>
>> Yet as said before - I'm not sure __DECLARE_FLEX_ARRAY() alone would be 
>> enough
>> for what you want to achieve.
> 
> Mmm yes, I think I would still have problems because container_of wants a 
> named member,
> so __DECLARE_FLEX_ARRAY() doesn’t help much alone, if I’m not missing 
> something obvious.
> If you believe however that importing __struct_group() only for this instance 
> is not enough to justify
> its presence in the codebase, I could open-code it, provided that maintainers 
> are ok with that.

I'm afraid I've even more strongly against open-coding. If you can get an
ack from another maintainer for the introduction of struct_group(), that
would still allow it to go in. I didn't NAK the change, I merely have
reservations.

> In any case it would be used soon also for other architectures using bootinfo.

Oh, would it?

Jan