Re: [RFC PATCH] xen: Remove -Wdeclaration-after-statement

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

On Fri Aug 9, 2024 at 8:25 PM BST, Stefano Stabellini wrote:
> Adding Roberto
>
> Does MISRA have a view on this? I seem to remember this is discouraged?
>

I'd be surprised if MISRA didn't promote declaring close to first use to avoid
use-before-init, but you very clearly have a lot more exposure to it than I do.

I'm quite curious about what its preference is and the rationale for it.

>
> On Fri, 9 Aug 2024, Alejandro Vallejo wrote:
> > This warning only makes sense when developing using a compiler with C99
> > support on a codebase meant to be built with C89 compilers too, and
> > that's no longer the case (nor should it be, as it's been 25 years since
> > C99 came out already).
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> > Yes, I'm opening this can of worms. I'd like to hear others people's
> > thoughts on this and whether this is something MISRA has views on. If
> > there's an ulterior non-obvious reason besides stylistic preference I
> > think it should be documented somewhere, but I haven't seen such an
> > explanation.
> > 
> > IMO, the presence of this warning causes several undesirable effects:
> > 
> >   1. Small functions are hampered by the preclusion of check+declare
> >      patterns that improve readability via concision. e.g: Consider a
> >      silly example like:
> > 
> >      /* with warning */                     /* without warning */
> >      void foo(uint8_t *p)                   void foo(uint8_t *p)
> >      {                                      {
> >          uint8_t  tmp1;                         if ( !p )
> >          uint16_t tmp2;                             return;
> >          uint32_t tmp3;
> >                                                 uint8_t  tmp1 = OFFSET1 + *p;
> >          if ( !p )                              uint16_t tmp2 = OFFSET2 + *p;
> >              return;                            uint32_t tmp3 = OFFSET3 + *p;
> > 
> >          tmp1 = OFFSET1 + *p;                   /* Lots of uses of `tmpX` */
> >          tmp2 = OFFSET2 + *p;               }
> >          tmp2 = OFFSET2 + *p;
> > 
> >          /* Lots of uses of tmpX */
> >      }
> > 
> >   2. Promotes scope-creep. On small functions it doesn't matter much,
> >      but on bigger ones to prevent declaring halfway through the body
> >      needlessly increases variable scope to the full scope in which they
> >      are defined rather than the subscope of point-of-declaration to
> >      end-of-current-scope. In cases in which they can be trivially
> >      defined at that point, it also means they can be trivially misused
> >      before they are meant to. i.e: On the example in (1) assume the
> >      conditional in "with warning" is actually a large switch statement.
> > 
> >   3. It facilitates a disconnect between time-of-declaration and
> >      time-of-use that lead very easily to "use-before-init" bugs.
> >      While a modern compiler can alleviate the most egregious cases of
> >      this, there's cases it simply cannot cover. A conditional
> >      initialization on anything with external linkage combined with a
> >      conditional use on something else with external linkage will squash
> >      the warning of using an uninitialised variable. Things are worse
> >      where the variable in question is preinitialised to something
> >      credible (e.g: a pointer to NULL), as then that can be misused
> >      between its declaration and its original point of intended use.
> > 
> > So... thoughts? yay or nay?
>
> In my opinion, there are some instances where mixing declarations and
> statements would enhance the code, but these are uncommon. Given the
> choice between:
>
> 1) declarations always first
> 2) declarations always mixed with statements
>
> I would choose 1).

FWIW, so would I under those contraints. But let me at least try to persuade
you. There's at least two more arguments to weigh:

  1. It wasn't that long ago that we had to resort to GNU extensions to work
     around this restriction. It's mildly annoying having to play games with
     compiler extensions because we're purposely restricting our use of the
     language.

     See the clang codegen workaround:
        https://lore.kernel.org/xen-devel/D2ZM0D609TOQ.2GQQWR1QALUPA@cloud.com/

  2. There's an existing divide between toolstack and hypervisor. Toolstack
     already allows this kind of mixing, and it's hard not-to due to external
     dependencies. While style doesn't have to match 1:1, it'd be (imo)
     preferrable to try and remove avoidable differences.

     See (40be6307ec00: "Only compile the hypervisor with -Wdeclaration-after-statement")
         https://lore.kernel.org/xen-devel/20231205183226.26636-1-julien@xen.org/

With this in mind, ...

>
> One could say that mixing declarations with statements should be done
> only when appropriate, but the challenge lies in the subjective nature
> of "when appropriate." Different individuals have varying
> interpretations of this, which could lead to unnecessary bikeshedding.

... I do agree that whatever changes we introduce, considering the usual
complaints about the review process, should be in a direction of measurable
objectivity so as to minimize unproductive nitpicking.

In that sense, a differently worded choice is:

  1. Declarations always at the beginning of the scope closest to first use.
  2. Declarations always closest to first use.

They really aren't that different, and we do already make reviews asking for
declarations to be moved to the closest scope. Given this choice I definitely
prefer (2), in part because it removes the uncertainty from review that true
freeform declarations would allow, better aligns our dialect of C with the
current standard and aligns the hypervisor style (slightly) with the tools it
requires.

Plus the advantages I already mentioned in the original email.

>
> For these reasons, I do not support mixing declarations and statements.

Pending whatever MISRA has to say on the matter, I hope these arguments can
steer your view.

Cheers,
Alejandrq