Re: [PATCH] xen/arm: mark handle_linux_pci_domain() __init

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.10.2022 22:15, Julien Grall wrote:
> On 14/10/2022 20:23, Stewart Hildebrand wrote:
>> arch/arm/efi/boot.c: In function ‘efi_start’:
>> arch/arm/efi/boot.c:1464:9: error: ‘argc’ may be used uninitialized in 
>> this function [-Werror=maybe-uninitialized]
>>   1464 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, 
>> name.s);
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I am a bit puzzled why it warn on this line but not few lines above 
> where it is already used.

Same here. Plus it ought to warn for argv then too, I think.

> This function is a bit difficult to read. AFAIU, the code look like:
> 
> if ( use_cfg_file )
> {
>     argc = ...
> }
> 
> /* do something common */
> 
> if ( use_cfg_file )
>    efi_arch_handle_cmd(argc, ...);
> 
> The GCC with -Og is probably not capable to detect that argc will always 
> be used when 'use_cfg_file'.
> 
> The "do something common" is two lines. So I am tempted to suggest to 
> just duplicate those two lines. This could also allow us to move all the 
> code in the ifs (nearly 100 lines over the two ifs!) in a separate function.
> 
> But I think Jan (the maintainer of the code) may not be happy with 
> that).

Indeed. Even if it's only two statements now, my view is that we ought to
avoid such code duplication.

Further I wonder whether the call to efi_check_dt_boot() shouldn't
actually live in an "else" to the 2nd if(). It would be at least
questionable if parts of the modules were described by the .cfg file and
other parts by DT (which in turn makes assumptions about the relative
placement of those modules wrt xen.efi on the EFI partition, when I'd
expect it to be self-contained).

> So short of a second better suggestion, initializing 'argc' to 0 
> (?) and a comment explaining this is to silence the compiler may be the 
> way to go.

I'd prefer if we avoided that, not the least because this could then trip
(good) static checkers to report written-but-never-used instances. What I
might accept (albeit that doesn't address said concern) is an "else" to
the first if() setting argc and argv (accompanied by a suitable comment,
down the road perhaps including a SAF-<nnn>-false-positive marker).

Jan