| Message ID | 20221014025354.30248-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: mark handle_linux_pci_domain() __init | expand |
On 14.10.2022 04:53, Stewart Hildebrand wrote:
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
I guess a non-empty description and a Fixes: tag would be nice.
Jan
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@amd.com> > > I guess a non-empty description and a Fixes: tag would be nice. +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. Cheers,
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@amd.com> >> >> 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@amd.com> > +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 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 ) | ^ 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 assume these uninitialized use errors can simply be fixed by initializing the respective variables to false/0, but a second opinion would certainly be helpful.
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@amd.com> >>> >>> 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@amd.com> > >> +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,
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
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index b975d0b309..9fa283d694 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1051,8 +1051,8 @@ static void __init assign_static_memory_11(struct domain *d, * The current heuristic assumes that a device is a host bridge * if the type is "pci" and then parent type is not "pci". */ -static int handle_linux_pci_domain(struct kernel_info *kinfo, - const struct dt_device_node *node) +static int __init handle_linux_pci_domain(struct kernel_info *kinfo, + const struct dt_device_node *node) { uint16_t segment; int res;
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- xen/arch/arm/domain_build.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)