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

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

[Philippe Mathieu-Daudé]

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(-)

diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
index 55d4e68c34..0b62ac628f 100644
--- a/target/i386/kvm/kvm_i386.h
+++ b/target/i386/kvm/kvm_i386.h
@@ -32,7 +32,6 @@
 
 bool kvm_has_smm(void);
 bool kvm_enable_x2apic(void);
-bool kvm_hv_vpindex_settable(void);
 bool kvm_has_pit_state2(void);
 
 bool kvm_enable_sgx_provisioning(KVMState *s);
@@ -41,8 +40,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_after_reset_vcpu(X86CPU *cpu);
 void kvm_arch_do_init_vcpu(X86CPU *cs);
-uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
-                                      uint32_t index, int reg);
 uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
 
 void kvm_set_max_apic_id(uint32_t max_apic_id);
@@ -60,6 +57,10 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
 
 bool kvm_has_x2apic_api(void);
 bool kvm_has_waitpkg(void);
+bool kvm_hv_vpindex_settable(void);
+
+uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
+                                      uint32_t index, int reg);
 
 uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
 void kvm_update_msi_routes_all(void *private, bool global,
@@ -76,6 +77,20 @@ typedef struct kvm_msr_handlers {
 bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
                     QEMUWRMSRHandler *wrmsr);
 
+#elif defined(__clang__) && !defined(__OPTIMIZE__)
+
+static inline bool kvm_hv_vpindex_settable(void)
+{
+    g_assert_not_reached();
+}
+
+static inline uint32_t kvm_arch_get_supported_cpuid(KVMState *env,
+                                                    uint32_t function,
+                                                    uint32_t index, int reg)
+{
+    g_assert_not_reached();
+}
+
 #endif /* CONFIG_KVM */
 
 void kvm_pc_setup_irq_routing(bool pci_enabled);
-- 
2.41.0

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

[Stefan Hajnoczi]

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(-)
>
> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
> index 55d4e68c34..0b62ac628f 100644
> --- a/target/i386/kvm/kvm_i386.h
> +++ b/target/i386/kvm/kvm_i386.h
> @@ -32,7 +32,6 @@
>
>  bool kvm_has_smm(void);
>  bool kvm_enable_x2apic(void);
> -bool kvm_hv_vpindex_settable(void);
>  bool kvm_has_pit_state2(void);
>
>  bool kvm_enable_sgx_provisioning(KVMState *s);
> @@ -41,8 +40,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
>  void kvm_arch_reset_vcpu(X86CPU *cs);
>  void kvm_arch_after_reset_vcpu(X86CPU *cpu);
>  void kvm_arch_do_init_vcpu(X86CPU *cs);
> -uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> -                                      uint32_t index, int reg);
>  uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
>
>  void kvm_set_max_apic_id(uint32_t max_apic_id);
> @@ -60,6 +57,10 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
>
>  bool kvm_has_x2apic_api(void);
>  bool kvm_has_waitpkg(void);
> +bool kvm_hv_vpindex_settable(void);
> +
> +uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> +                                      uint32_t index, int reg);
>
>  uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
>  void kvm_update_msi_routes_all(void *private, bool global,
> @@ -76,6 +77,20 @@ typedef struct kvm_msr_handlers {
>  bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
>                      QEMUWRMSRHandler *wrmsr);
>
> +#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.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Daniel P. Berrangé]

On Mon, Sep 11, 2023 at 12:38:32PM +0200, Philippe Mathieu-Daudé 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()")

Why not just revert those two commits, since we now learned the rationale
for them was incorrect ?

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/i386/kvm/kvm_i386.h | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4494 bytes --]

Or instead of using linker behavior, maybe just change the #ifdef so it
only applies when KVM is disabled. I didn't look at the code to see if this
is possible, but it would be nice to avoid the very specific #ifdef
condition in this patch.

Stefan

On Mon, Sep 11, 2023, 07:15 Stefan Hajnoczi <stefanha@gmail.com> wrote:

> 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(-)
> >
> > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
> > index 55d4e68c34..0b62ac628f 100644
> > --- a/target/i386/kvm/kvm_i386.h
> > +++ b/target/i386/kvm/kvm_i386.h
> > @@ -32,7 +32,6 @@
> >
> >  bool kvm_has_smm(void);
> >  bool kvm_enable_x2apic(void);
> > -bool kvm_hv_vpindex_settable(void);
> >  bool kvm_has_pit_state2(void);
> >
> >  bool kvm_enable_sgx_provisioning(KVMState *s);
> > @@ -41,8 +40,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error
> **errp);
> >  void kvm_arch_reset_vcpu(X86CPU *cs);
> >  void kvm_arch_after_reset_vcpu(X86CPU *cpu);
> >  void kvm_arch_do_init_vcpu(X86CPU *cs);
> > -uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> > -                                      uint32_t index, int reg);
> >  uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t
> index);
> >
> >  void kvm_set_max_apic_id(uint32_t max_apic_id);
> > @@ -60,6 +57,10 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
> >
> >  bool kvm_has_x2apic_api(void);
> >  bool kvm_has_waitpkg(void);
> > +bool kvm_hv_vpindex_settable(void);
> > +
> > +uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> > +                                      uint32_t index, int reg);
> >
> >  uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
> >  void kvm_update_msi_routes_all(void *private, bool global,
> > @@ -76,6 +77,20 @@ typedef struct kvm_msr_handlers {
> >  bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
> >                      QEMUWRMSRHandler *wrmsr);
> >
> > +#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.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>

[-- Attachment #2: Type: text/html, Size: 5676 bytes --]

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

[Michael Tokarev]

11.09.2023 14:56, Stefan Hajnoczi:
> Or instead of using linker behavior, maybe just change the #ifdef so it only applies when KVM is disabled. I didn't look at the code to see if this is 
> possible, but it would be nice to avoid the very specific #ifdef condition in this patch.

Yeah, this is definitely preferrable to define it based on !kvm instead of !optimize.

I mean the same thing when replied in the pullreq which broke things, - to base
it on !kvm.  But use something very similar to what Philippe did with the functions
themselves (not with the condition).

/mjt

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

[Kevin Wolf]

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(-)
> >
> > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
> > index 55d4e68c34..0b62ac628f 100644
> > --- a/target/i386/kvm/kvm_i386.h
> > +++ b/target/i386/kvm/kvm_i386.h
> > @@ -32,7 +32,6 @@
> >
> >  bool kvm_has_smm(void);
> >  bool kvm_enable_x2apic(void);
> > -bool kvm_hv_vpindex_settable(void);
> >  bool kvm_has_pit_state2(void);
> >
> >  bool kvm_enable_sgx_provisioning(KVMState *s);
> > @@ -41,8 +40,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
> >  void kvm_arch_reset_vcpu(X86CPU *cs);
> >  void kvm_arch_after_reset_vcpu(X86CPU *cpu);
> >  void kvm_arch_do_init_vcpu(X86CPU *cs);
> > -uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> > -                                      uint32_t index, int reg);
> >  uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
> >
> >  void kvm_set_max_apic_id(uint32_t max_apic_id);
> > @@ -60,6 +57,10 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
> >
> >  bool kvm_has_x2apic_api(void);
> >  bool kvm_has_waitpkg(void);
> > +bool kvm_hv_vpindex_settable(void);
> > +
> > +uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> > +                                      uint32_t index, int reg);
> >
> >  uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
> >  void kvm_update_msi_routes_all(void *private, bool global,
> > @@ -76,6 +77,20 @@ typedef struct kvm_msr_handlers {
> >  bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
> >                      QEMUWRMSRHandler *wrmsr);
> >
> > +#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.

Kevin

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:
https://lore.kernel.org/qemu-devel/20230911131507.24943-1-philmd@linaro.org/

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

[Michael Tokarev]

11.09.2023 15:32, Kevin Wolf wrote:
..
> 
> 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.

That's why both I and Stephan disliked the #ifdef condition, not the
approach.  If it were me, I'd change the condition to be !KVM and keep
the inline functions (or #defines for that matter), instead of re-
introducing stubs in a separate .c file.

/mjt