Re: [XEN PATCH v2 12/13] xen/x86: fix violations of MISRA C:2012 Rule 7.2

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Jul 2023, Jan Beulich wrote:
> On 07.07.2023 10:04, Simone Ballarin wrote:
> > Il giorno ven 7 lug 2023 alle ore 09:04 Jan Beulich <jbeulich@xxxxxxxx> ha
> > scritto:
> > 
> >> On 07.07.2023 08:50, Simone Ballarin wrote:
> >>> Il giorno gio 6 lug 2023 alle ore 18:22 Jan Beulich <jbeulich@xxxxxxxx>
> >> ha
> >>> scritto:
> >>>
> >>>> On 06.07.2023 18:08, Simone Ballarin wrote:
> >>>>> Il giorno gio 6 lug 2023 alle ore 10:26 Jan Beulich <jbeulich@xxxxxxxx
> >>>
> >>>> ha
> >>>>> scritto:
> >>>>>
> >>>>>> On 05.07.2023 17:26, Simone Ballarin wrote:
> >>>>>>> --- a/xen/arch/x86/apic.c
> >>>>>>> +++ b/xen/arch/x86/apic.c
> >>>>>>> @@ -1211,7 +1211,7 @@ static void __init calibrate_APIC_clock(void)
> >>>>>>>       * Setup the APIC counter to maximum. There is no way the lapic
> >>>>>>>       * can underflow in the 100ms detection time frame.
> >>>>>>>       */
> >>>>>>> -    __setup_APIC_LVTT(0xffffffff);
> >>>>>>> +    __setup_APIC_LVTT(0xffffffffU);
> >>>>>>
> >>>>>> While making the change less mechanical, we want to consider to switch
> >>>>>> to ~0 in this and similar cases.
> >>>>>>
> >>>>>
> >>>>> Changing ~0U is more than not mechanical: it is possibly dangerous.
> >>>>> The resulting value could be different depending on the architecture,
> >>>>> I prefer to not make such kind of changes in a MISRA-related patch.
> >>>>
> >>>> What do you mean by "depending on the architecture", when this is
> >>>> x86-only code _and_ you can check what type parameter the called
> >>>> function has?
> >>>
> >>> Ok, I will change these literals in ~0U in the next submission.
> >>
> >> Except that I specifically meant ~0, not ~0U. We mean "maximum value"
> >> here, and at the call site it doesn't matter how wide the function
> >> parameter's type is. If it was 64-bit, ~0U would not do what is wanted.
> > 
> > ~0 is not a MISRA-compliant solution since bitwise operations on signed
> > integers have implementation-defined behavior. This solution definitively
> > violates Rule 10.1.
> 
> So if we adopted that rule (we didn't so far), we'd have to e.g. change
> all literal number shift counts to have U suffixes, no matter that
> without the suffix it is still entirely obvious that the numbers are
> unsigned? I'm afraid that'll face my opposition ...

Indeed we have not adopted Rule 10.1. However, may I suggest that we
don't make things potentially worse, just in case we end up deciding in
favor of 10.1? We might not adopt 10.1 at all, but still... The code is
already 0xffffffff, let's make things easier for all of us and just do
0xffffffffU ?

Let's put Rule 10.1 and the whole of MISRA C aside for a second.


Jan, let's say that you prefer ~0 or a different function parameter name
or something else on any of these patches. You do realize that you don't
need Simone or Federico or anyone else to make that change for you? You
can make the change, submit a patch, and in your case anyone can ack
it. Roger, Andrew, me, Bertrand, Julien, and almost anyone else could
ack it and it would go in. As I wrote yesterday, feel free to CC me and
I'll help you get in all the changes that you want.

If you submitted that patch to switch to ~0 it might already be
committed by now.

I am trying to highlight that suggesting changes on these mechanical
patches end up with more work for both the contributor and also the
maintainer compared to do the change yourself.

I think we should try to accept these patches as
mechanical-changes-only. This is the only way to scale up this effort.
If you spot something that you'd rather be done differently, do one of
the following:

a) Accept the patch as-is and submit a patch afterwards. Yes the line
   gets changed twice but it is the easiest solution.

b) Ask the contribitor to drop the single change you would rather do
   differently, or even better drop it yourself on commit. Then submit a
   patch with the change that you prefer.

c) For trivial things, like code style changes, do the change directly
   on commit.


I know emails encourage English replies, but to make this work we need
to do more code changes together and less English.