Re: [PATCH] radix-tree: don't left-shift negative values

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2025 22:46, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Andrew Cooper wrote:
>> On 13/02/2025 7:26 pm, Stefano Stabellini wrote:
>>> On Thu, 13 Feb 2025, Luca Fancellu wrote:
>>>>> On 13 Feb 2025, at 15:42, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> 
>>>>> wrote:
>>>>> On 2025-02-13 16:32, Nicola Vetrini wrote:
>>>>>> On 2025-02-13 16:01, Jan Beulich wrote:
>>>>>>> On 13.02.2025 15:52, Nicola Vetrini wrote:
>>>>>>>> On 2025-02-13 15:22, Jan Beulich wrote:
>>>>>>>>> Any (signed) integer is okay to pass into radix_tree_int_to_ptr(), yet
>>>>>>>>> left shifting negative values is UB. Use an unsigned intermediate 
>>>>>>>>> type,
>>>>>>>>> reducing the impact to implementation defined behavior (for the
>>>>>>>>> unsigned->signed conversion).
>>>>>>>>> Also please Misra C:2012 rule 7.3 by dropping the lower case numeric
>>>>>>>>> 'l'
>>>>>>>>> tag.
>>>>>>>>> No difference in generated code, at least on x86.
>>>>>>>>> Fixes: b004883e29bb ("Simplify and build-fix (for some gcc versions)
>>>>>>>>> radix_tree_int_to_ptr()")
>>>>>>>>> Reported-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> Bugseng: Why was the 7.3 violation not spotted by Eclair? According to
>>>>>>>>>         tagging.ecl the codebase is clean for this rule, aiui.
>>>>>>>> radix-tree.{c,h} is out of scope:
>>>>>>>> automation/eclair_analysis/ECLAIR/out_of_scope.ecl:32:-file_tag+={out_of_scope,"^xen/include/xen/radix-tree\\.h$"}
>>>>>>>> docs/misra/exclude-list.json:153:            "rel_path":
>>>>>>>> "common/radix-tree.c",
>>>>>>> Is there a record of why they are excluded? Is it further explainable
>>>>>>> why exclude-list.json mentions only the .c file and out_of_scope.ecl
>>>>>>> mentions only the .h one? Shouldn't different parts be in sync?
>>>>>> exclude-list.json is used to generate a configuration file for ECLAIR 
>>>>>> just before the analysis starts, so effectively both are excluded. It's 
>>>>>> a good point however to have only one file to handle exclusions, and use 
>>>>>> that file to generate the exclusion list dynamically, but then someone 
>>>>>> might want to exclude certain files only in some analyses and not 
>>>>>> others, which is not a good fit for exclude-list.json as it is now.
>>>>>> @Stefano, thoughts?
>>>>> I forgot to address the first question: the (vague) reasons are listed in 
>>>>> exclude-list.json as the "comment" field; in most cases, it's because the 
>>>>> files have been imported from Linux, but the full rationale is something 
>>>>> that should be asked to the original author, which is Luca Fancellu.
>>>> So IIRC the full rationale is that since some files are imported from 
>>>> Linux, we would like to maintain them as they are
>>>> in order to ease backports. Misra fixes can be done, but they need to be 
>>>> upstreamed to Linux and backported to Xen.
>>>>
>>>> Probably a re-evaluation could be done by the maintainers to see if some 
>>>> of these files could be removed from the exclusion
>>>> list.
>>> Yes, it is as Luca said. At the beginning of the project, we reviewed
>>> the codebase to define what was in scope and what was out of scope. One
>>> area of contention was the files imported from Linux. Many of these
>>> files were declared out of scope because we wanted to retain the ability
>>> to easily synchronize them with their corresponding files in Linux.  
>>>
>>> Now, years have passed, and we have gained significant experience from
>>> running this project. It is completely acceptable to redefine the scope,
>>> including making changes to exclude-list.json.
>>>
>>> However, we do not necessarily need to modify exclude-list.json to
>>> accept a single, clearly beneficial fix like this one. So, Jan, feel
>>> free to proceed and commit it.  
>>>
>>> I just wanted to provide some background. If you believe that removing
>>> common/radix-tree.c from docs/misra/exclude-list.json, and thereby
>>> including it in ECLAIR's regular scanning, would be the best approach, I
>>> am also fine with that.
>>
>> I agree with Jan that it's important that we have a single source of truth.
>>
>> Furthermore, it is critical that the justification of why things are in
>> certain categories are identified.  It only needs to be a single
>> sentence in a comment, but a developer needs to be able to look at the
>> file and figure out *why* a decision was taken...
>>
>> ... because as Stefano says, decisions change over time, opinions and
>> scope change, etc.
> 
> The single source of truth is supposed to be
> docs/misra/exclude-list.json, which has an entry for radix-tree with a
> simple explanation:
> 
>         {
>             "rel_path": "common/radix-tree.c",
>             "comment": "Imported from Linux, ignore for now"
>         },

At the risk of stating the obvious: That's radix-tree.c only, not
radix-tree.h.

Jan