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

[Simone Ballarin <simone.ballarin@bugseng.com>]

[-- Attachment #1: Type: text/plain, Size: 4537 bytes --]

Il giorno ven 7 lug 2023 alle ore 23:53 Stefano Stabellini <
sstabellini@kernel.org> ha scritto:

> 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@suse.com>
> ha
> > > scritto:
> > >
> > >> On 07.07.2023 08:50, Simone Ballarin wrote:
> > >>> Il giorno gio 6 lug 2023 alle ore 18:22 Jan Beulich <
> jbeulich@suse.com>
> > >> ha
> > >>> scritto:
> > >>>
> > >>>> On 06.07.2023 18:08, Simone Ballarin wrote:
> > >>>>> Il giorno gio 6 lug 2023 alle ore 10:26 Jan Beulich <
> jbeulich@suse.com
> > >>>
> > >>>> 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.
>


I agree.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com
<http://bugseng.com>)

[-- Attachment #2: Type: text/html, Size: 6319 bytes --]