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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

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