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.hildebr...@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.hildebr...@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,
--
Julien Grall
