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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@vates.tech>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> 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. Over the past months, I made small edits upon receiving 
feedback from the community (e.g., excluding gdbsx.c), but there's the 
possibility that the content should be re-evaulated in its entirety 
(which will likely lead to additional MISRA violations being generated, 
even for clean rules, as you correctly pointed out) and possibly lead to 
different sets of excluded files depending on the type of analysis 
(i.e., a restricted "safety" configuration and a wider "community" 
configuration).

> Thanks,
>  Nicola
> 
>>> We are in the process of setting up a wider analysis (i.e. with a
>>> different exclusion set) with a broader configuration that may catch
>>> these issues.
>> 
>> Good.
>> 
>> Jan

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253