Re: [XEN PATCH] automation/eclair: reorganize pipelines

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 25 Apr 2024, at 14:32, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
> 
> On 2024-04-25 11:40, Federico Serafini wrote:
>> On 25/04/24 02:06, Stefano Stabellini wrote:
>>> On Tue, 23 Apr 2024, Federico Serafini wrote:
>>>> From: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
>>>> Introduce accepted_guidelines.sh: a script to autogenerate the
>>>> configuration file accepted.ecl from docs/misra/rules.rst which enables
>>>> all accepted guidelines.
>>>> Introduce monitored.ecl: a manual selection of accepted guidelines
>>>> which are clean or almost clean, it is intended to be used for the
>>>> analyses triggered by commits.
>>>> Reorganize tagging.ecl:
>>>>   -Remove "accepted" tags: keeping track of accepted guidelines tagging
>>>>    them as "accepted" in the configuration file tagging.ecl is no
>>>>    longer needed since docs/rules.rst is keeping track of them.
>>>>   -Tag more guidelines as clean.
>>>> Reorganize eclair pipelines:
>>>>   - Set1, Set2, Set3 are now obsolete: remove the corresponding
>>>>     pipelines and ecl files.
>>>>   - Amend scheduled eclair pipeline to use accepted.ecl.
>>>>   - Amend triggered eclair pipeline to use monitored.ecl.
>>>> Rename and improve action_check_clean_regressions.sh to print a
>>>> diagnostic in case a commit introduces a violation of a clean guideline.
>>>> An example of diagnostic is the following:
>>>> Failure: 13 regressions found for clean guidelines
>>>>   service MC3R1.R8.2: (required) Function types shall be in prototype form 
>>>> with named parameters:
>>>>    violation: 13
>>>> Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
>>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>>>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
>>> Fantastic work, thank you!
>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> Is this patch safe to commit now? Or would it cause gitlab-ci breakage?
>> Yes, it is safe because the ECLAIR analysis is still allowed to fail.
>> Committing this patch wouldn't break the CI but it will highlight some 
>> regressions with the orange badge and the following messages:
>> arm:
>> Failure: 5 regressions found for clean guidelines
>>  service MC3R1.R1.1: (required) The program shall contain no violations of 
>> the standard C syntax and constraints, and shall not exceed the 
>> implementation's translation limits:
>>   violation: 5
> 

Hi Nicola,

> +Cc ARM maintainers, Luca Fancellu
> 
> after some investigation on these regressions on R1.1, there are some 
> concerns with the current code:
> 2209c1e35b479dff8ed3d3161001ffdefa0a704e introduced the struct
> 
> struct membanks {
>    unsigned int nr_banks;
>    unsigned int max_banks;
>    struct membank bank[];
> };
> 
> with a flexible array member at the end; this is well-defined in C99, but 
> what is non-standard (a GNU extension) is having this struct as a member to 
> another struct/union in a position other than the last.
> 
> This is still supported by GNU (albeit reliance on this is undocumented for 
> Xen), but what is concerning here is the following (taken from [1]):
> 
> "A structure containing a C99 flexible array member, or a union containing 
> such a structure, is not the last field of another structure, for example:
> 
> struct flex  { int length; char data[]; };
> 
> struct mid_flex { int m; struct flex flex_data; int n; };
> 
> In the above, accessing a member of the array mid_flex.flex_data.data[] might 
> have undefined behavior. Compilers do not handle such a case consistently. 
> Any code relying on this case should be modified to ensure that flexible 
> array members only end up at the ends of structures.
> Please use the warning option -Wflex-array-member-not-at-end to identify all 
> such cases in the source code and modify them. This extension is now 
> deprecated."
> 
> It looks like this option, and the corresponding deprecation of the 
> extension, will be available starting from GCC 14, so this might impact 
> future compiler updates for Xen builds.
> 
> Linux is also concerned about this (see [2]), so I think the changes in 
> struct layout introduced by the series [3] should be reviewed to determine 
> whether this change was deliberate or happened as a byproduct. If this is 
> determined not to be a legitimate concern, then this can be documented as an 
> use of the GNU extension.

Thanks for letting us know, so the change was deliberate, the effect probably 
not, I guess we need to find a way to fix this in order to use this interface.

Cheers,
Luca