Re: [PATCH] target/i386: Re-introduce few KVM stubs for Clang debug builds

[Philippe Mathieu-Daudé]

On 11/9/23 14:32, Kevin Wolf wrote:
> Am 11.09.2023 um 13:15 hat Stefan Hajnoczi geschrieben:
> > On Mon, 11 Sept 2023 at 06:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > Since commits 3adce820cf..ef1cf6890f, When building on
> > > a x86 host configured as:
> > >
> > >    $ ./configure --cc=clang \
> > >      --target-list=x86_64-linux-user,x86_64-softmmu \
> > >      --enable-debug
> > >
> > > we get:
> > >
> > >    [71/71] Linking target qemu-x86_64
> > >    FAILED: qemu-x86_64
> > >    /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function 
> > > `cpu_x86_cpuid':
> > >    cpu.c:(.text+0x1374): undefined reference to `kvm_arch_get_supported_cpuid'
> > >    /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function 
> > > `x86_cpu_filter_features':
> > >    cpu.c:(.text+0x81c2): undefined reference to `kvm_arch_get_supported_cpuid'
> > >    /usr/bin/ld: cpu.c:(.text+0x81da): undefined reference to 
> > > `kvm_arch_get_supported_cpuid'
> > >    /usr/bin/ld: cpu.c:(.text+0x81f2): undefined reference to 
> > > `kvm_arch_get_supported_cpuid'
> > >    /usr/bin/ld: cpu.c:(.text+0x820a): undefined reference to 
> > > `kvm_arch_get_supported_cpuid'
> > >    /usr/bin/ld: 
> > > libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o:cpu.c:(.text+0x8225): more 
> > > undefined references to `kvm_arch_get_supported_cpuid' follow
> > >    clang: error: linker command failed with exit code 1 (use -v to see 
> > > invocation)
> > >    ninja: build stopped: subcommand failed.
> > >
> > > '--enable-debug' disables optimizations (CFLAGS=-O0).
> > >
> > > While at this (un)optimization level GCC eliminate the
> > > following dead code:
> > >
> > >    if (0 && foo()) {
> > >        ...
> > >    }
> > >
> > > Clang does not. Therefore restore a pair of stubs for
> > > unoptimized Clang builds.
> > >
> > > Reported-by: Kevin Wolf <kwolf@redhat.com>
> > > Fixes: 3adce820cf ("target/i386: Remove unused KVM stubs")
> > > Fixes: ef1cf6890f ("target/i386: Allow elision of kvm_hv_vpindex_settable()")
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   target/i386/kvm/kvm_i386.h | 21 ++++++++++++++++++---
> > >   1 file changed, 18 insertions(+), 3 deletions(-)


> > > +#elif defined(__clang__) && !defined(__OPTIMIZE__)
> >
> > Another approach is a static library with a .o file containing the
> > stubs so the linker only includes it in the executable if the compiler
> > emitted the symbols. That way there is no need for defined(__clang__)
> > && !defined(__OPTIMIZE__) and it will work with other
> > compilers/optimization levels. It's more work to set up though.
>
> Isn't that exactly how it was before the stubs were removed? It would be
> a simple revert of that commit.
>
> The approach with static inline functions defined only for a very
> specific configuration looks a lot more fragile to me. In fact, I'm
> surprised that it works because I think it requires that the header
> isn't used in any files that are shared between user space and system
> emulation - and naively cpu.c sounded like something that could be
> shared. Looks like this patch only works because the linux-user target
> uses a separate build of the same CPU emulation source file.
>
> So I think reverting the commit that removed the stubs would be much
> more obvious.

Yes, v2 reverts:
20230911131507.24943-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20230911131507.24943-1-philmd@linaro.org/