[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] radix-tree: don't left-shift negative values
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |