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

[Julien Grall <julien@xxxxxxx>]

Hi Stewart,

On 14/10/2022 20:23, Stewart Hildebrand wrote:
> On 10/14/22 04:22, Julien Grall wrote:
> > Hi,
> >
> > On 14/10/2022 08:16, Jan Beulich wrote:
> > > On 14.10.2022 04:53, Stewart Hildebrand wrote:
> > > > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> > > I guess a non-empty description and a Fixes: tag would be nice.
> Okay, I will send a v2 with the following description:
> All functions in domain_build.c should be marked __init. This was 
> spotted when building the hypervisor with -Og.
> Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>
> > +1. I am actually quite interested to understand how this was spotted.
> >
> > The build system should check that any function/data in domain_build.c
> > are part of the __init section. So I guess the compiler you are using
> > doesn't inline the function?
> >
> > If so, I am actually surprised you are the first one spotted this... We
> > are building on various distribution without any issues (?). I would be
> > interested to know the compiler version and maybe we could add it in the
> > CI.
> I added -Og to the make command line so it takes precedence over the 
> default -O1/-O2:
> $ make EXTRA_CFLAGS_XEN_CORE="-Og" XEN_TARGET_ARCH=arm64 
> CROSS_COMPILE=aarch64-none-linux-gnu- dist-xen -j $(nproc)
> Indeed, I did observe the build error:
> Error: size of arch/arm/domain_build.o:.text is 0x00000008
>
> I used this rune to reveal the culprit:
>
> $ aarch64-none-linux-gnu-objdump -d xen/arch/arm/domain_build.o | head
>
> xen/arch/arm/domain_build.o:     file format elf64-littleaarch64
>
>
> Disassembly of section .text:
>
> 0000000000000000 <handle_linux_pci_domain>:
>      0:   52800000        mov     w0, #0x0                        // #0
>      4:   d65f03c0        ret
>
> I am using this toolchain: 
> https://developer.arm.com/-/media/Files/downloads/gnu/11.3.rel1/binrel/arm-gnu-toolchain-11.3.rel1-x86_64-aarch64-none-linux-gnu.tar.xz
Thanks for the details. I guess the '-Og' is the culprint.

> Further, there were two more build errors observed when building with -Og:
> arch/arm/domain_build.c: In function ‘make_cpus_node’:
> arch/arm/domain_build.c:2013:12: error: ‘clock_valid’ may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>   2013 |         if ( clock_valid )
>        |            ^
I think this is a false positive because 'clock_valid' is set at the 
same time as 'compatible'. The latter is check that is not NULL just 
after it is set.
In general, I tend to prefer if variable are not initialized (unless 
strictly necessary) because we can take advantage of the compiler to 
spot any issue.
In this case, it should not be a big problem because the default value 
(false) would be sensible here.
> 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.
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). 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.
Cheers,

--
Julien Grall