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

Jan