Re: [XEN PATCH] xen/types: address Rule 10.1 for macro BITS_TO_LONGS

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

On 08/09/2023 21:37, Stefano Stabellini wrote:
> On Fri, 8 Sep 2023, Nicola Vetrini wrote:
>> On 08/09/2023 13:59, Jan Beulich wrote:
>> > On 08.09.2023 13:57, Jan Beulich wrote:
>> > > On 08.09.2023 10:48, Nicola Vetrini wrote:
>> > > > There is a build error due to -Werror because of a pointer comparison at
>> > > > line 469 of common/numa.c:
>> > > > i = min(PADDR_BITS, BITS_PER_LONG - 1);
>> > > > where
>> > > > #define PADDR_BITS              52
>> > > >
>> > > > I guess PADDR_BITS can become unsigned or gain a cast
>> > >
>> > > While generally converting constants to unsigned comes with a certain
>> > > risk, I think for this (and its siblings) this ought to be okay. As to
>> > > the alternative of a cast - before considering that, please consider
>> > > e.g. adding 0u (as we do elsewhere in the code base to deal with such
>> > > cases).
>> >
>> > And just after sending I realized that this would still be disliked by
>> > Misra's type system. (Much like then aiui the 1 above will need to
>> > become 1u. Which is all pretty horrible.)
>> >
>> > Jan
>> 
>> I have a proposal: in our tests we enabled an ECLAIR configuration 
>> that allows
>> to bypass the
>> constraint imposed by Rule 10.4 that warrants the 1U iff the value is 
>> constant
>> and both types
>> can represent it correctly (in this case BITS_PER_LONG -1). This would 
>> allow
>> using the proposed
>> solution and documenting why it's ok not to respect R10.4. What do you 
>> think?
> 
> I think that would be OK. I think we would want to document this in
> rules.rst. Please send a patch.

I checked: that configuration is already enabled in current staging, so 
perhaps the only
action in that regard would be to send a patch documenting it in 
rules.rst.

I just noticed one further issue with making BYTES_PER_LONG unsigned, in 
that causes
several instances of (1U << 3) to appear inside the file 
xen/arch/x86/xen.lds
produced by the build, which in turn causes ld to fail on that 'U'. For 
reference, the version of ld used by the build is the following:
GNU ld (GNU Binutils for Ubuntu) 2.38

The following is a snippet of the output:

        . = ALIGN((1 << 12));
        __ro_after_init_end = .;
        __start_bug_frames = .;
        *(.bug_frames.0)
        __stop_bug_frames_0 = .;
        *(.bug_frames.1)
        __stop_bug_frames_1 = .;
        *(.bug_frames.2)
        __stop_bug_frames_2 = .;
        *(.bug_frames.3)
        __stop_bug_frames_3 = .;
        *(.rodata)
        *(.rodata.*)
        *(.data.rel.ro)
        *(.data.rel.ro.*)
        . = ALIGN((1U << 3)); __start_vpci_array = .; 
*(SORT(.data.vpci.*)) __end_vpci_array = .;
   } :text
   .note.gnu.build-id : AT(ADDR(".note.gnu.build-id") - (((((((261 >> 8) 
* 0xffff000000000000) | (261 << 39))) + ((1 << 39) / 2)) + (64 << 30)) + 
(1 << 30))) {
        __note_gnu_build_id_start = .;
        *(.note.gnu.build-id)
        __note_gnu_build_id_end = .;


-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)