We are generating a lot of target-specific defines in the *-config-devices.h
and *-config-target.h files. Using them in common code is wrong and leads
to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there
as expected. To avoid these issues, we are already poisoning some of the
macros in include/exec/poison.h - but maintaining this list manually is
cumbersome. Thus let's generate the list of poisoned macros automatically
instead.
Note that CONFIG_TCG (which is also defined in config-host.h) and
CONFIG_USER_ONLY are special, so we have to filter these out.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
RFC since the shell stuff in "configure" is quite ugly ... maybe there's
a better way to do this via meson, but my meson-foo is still lacking...
Makefile | 2 +-
configure | 5 +++++
include/exec/poison.h | 2 ++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index bcbbec71a1..4cab10a2a4 100644
--- a/Makefile
+++ b/Makefile
@@ -213,7 +213,7 @@ qemu-%.tar.bz2:
distclean: clean
-$(quiet-@)test -f build.ninja && $(NINJA) $(NINJAFLAGS) -t clean -g || :
- rm -f config-host.mak config-host.h*
+ rm -f config-host.mak config-host.h* config-poison.h
rm -f tests/tcg/config-*.mak
rm -f config-all-disas.mak config.status
rm -f roms/seabios/config.mak roms/vgabios/config.mak
diff --git a/configure b/configure
index f7d022a5db..c7b5df3a5c 100755
--- a/configure
+++ b/configure
@@ -6441,6 +6441,11 @@ if test -n "${deprecated_features}"; then
echo " features: ${deprecated_features}"
fi
+cat *-config-devices.h *-config-target.h | grep '^#define ' \
+ | grep -v CONFIG_TCG | grep -v CONFIG_USER_ONLY \
+ | sed -e 's/#define //' -e 's/ .*//' | sort -u \
+ | sed -e 's/^/#pragma GCC poison /' > config-poison.h
+
# Save the configure command line for later reuse.
cat <<EOD >config.status
#!/bin/sh
diff --git a/include/exec/poison.h b/include/exec/poison.h
index 4cd3f8abb4..9e55d5aec2 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -4,6 +4,8 @@
#ifndef HW_POISON_H
#define HW_POISON_H
+#include "config-poison.h"
+
#pragma GCC poison TARGET_I386
#pragma GCC poison TARGET_X86_64
#pragma GCC poison TARGET_AARCH64
--
2.27.0
On 3/15/21 2:54 PM, Thomas Huth wrote:
> We are generating a lot of target-specific defines in the *-config-devices.h
> and *-config-target.h files. Using them in common code is wrong and leads
> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there
> as expected. To avoid these issues, we are already poisoning some of the
> macros in include/exec/poison.h - but maintaining this list manually is
> cumbersome. Thus let's generate the list of poisoned macros automatically
> instead.
> Note that CONFIG_TCG (which is also defined in config-host.h) and
> CONFIG_USER_ONLY are special, so we have to filter these out.
I have the impression that CONFIG_USER_ONLY should be poisoned too.
A lot of the
#ifndef CONFIG_USER_ONLY
end up currently doing the wrong thing in common modules includes,
especially due to the inverted nature of the check.
So the stuff that should be "hidden" for CONFIG_USER_ONLY, is actually processed even for CONFIG_USER_ONLY in the common modules,
while in "specific" modules it isn't, which remains a potential for trouble IMO.
Ciao,
CLaudio
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> RFC since the shell stuff in "configure" is quite ugly ... maybe there's
> a better way to do this via meson, but my meson-foo is still lacking...
>
> Makefile | 2 +-
> configure | 5 +++++
> include/exec/poison.h | 2 ++
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index bcbbec71a1..4cab10a2a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -213,7 +213,7 @@ qemu-%.tar.bz2:
>
> distclean: clean
> -$(quiet-@)test -f build.ninja && $(NINJA) $(NINJAFLAGS) -t clean -g || :
> - rm -f config-host.mak config-host.h*
> + rm -f config-host.mak config-host.h* config-poison.h
> rm -f tests/tcg/config-*.mak
> rm -f config-all-disas.mak config.status
> rm -f roms/seabios/config.mak roms/vgabios/config.mak
> diff --git a/configure b/configure
> index f7d022a5db..c7b5df3a5c 100755
> --- a/configure
> +++ b/configure
> @@ -6441,6 +6441,11 @@ if test -n "${deprecated_features}"; then
> echo " features: ${deprecated_features}"
> fi
>
> +cat *-config-devices.h *-config-target.h | grep '^#define ' \
> + | grep -v CONFIG_TCG | grep -v CONFIG_USER_ONLY \
> + | sed -e 's/#define //' -e 's/ .*//' | sort -u \
> + | sed -e 's/^/#pragma GCC poison /' > config-poison.h
> +
> # Save the configure command line for later reuse.
> cat <<EOD >config.status
> #!/bin/sh
> diff --git a/include/exec/poison.h b/include/exec/poison.h
> index 4cd3f8abb4..9e55d5aec2 100644
> --- a/include/exec/poison.h
> +++ b/include/exec/poison.h
> @@ -4,6 +4,8 @@
> #ifndef HW_POISON_H
> #define HW_POISON_H
>
> +#include "config-poison.h"
> +
> #pragma GCC poison TARGET_I386
> #pragma GCC poison TARGET_X86_64
> #pragma GCC poison TARGET_AARCH64
>
On 15/03/2021 15.07, Claudio Fontana wrote: > On 3/15/21 2:54 PM, Thomas Huth wrote: >> We are generating a lot of target-specific defines in the *-config-devices.h >> and *-config-target.h files. Using them in common code is wrong and leads >> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there >> as expected. To avoid these issues, we are already poisoning some of the >> macros in include/exec/poison.h - but maintaining this list manually is >> cumbersome. Thus let's generate the list of poisoned macros automatically >> instead. >> Note that CONFIG_TCG (which is also defined in config-host.h) and >> CONFIG_USER_ONLY are special, so we have to filter these out. > > > > I have the impression that CONFIG_USER_ONLY should be poisoned too. > > A lot of the > > #ifndef CONFIG_USER_ONLY > > end up currently doing the wrong thing in common modules includes, > especially due to the inverted nature of the check. Not sure about that ... do you have an example at hand? Anyway, one thing is sure, if we want to poison CONFIG_USER_ONLY, this will certainly cause a lot of clean up work first, since it is used all over the place... Thomas
On 3/15/21 4:08 PM, Thomas Huth wrote: > On 15/03/2021 15.07, Claudio Fontana wrote: >> On 3/15/21 2:54 PM, Thomas Huth wrote: >>> We are generating a lot of target-specific defines in the *-config-devices.h >>> and *-config-target.h files. Using them in common code is wrong and leads >>> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there >>> as expected. To avoid these issues, we are already poisoning some of the >>> macros in include/exec/poison.h - but maintaining this list manually is >>> cumbersome. Thus let's generate the list of poisoned macros automatically >>> instead. >>> Note that CONFIG_TCG (which is also defined in config-host.h) and >>> CONFIG_USER_ONLY are special, so we have to filter these out. >> >> >> >> I have the impression that CONFIG_USER_ONLY should be poisoned too. >> >> A lot of the >> >> #ifndef CONFIG_USER_ONLY >> >> end up currently doing the wrong thing in common modules includes, >> especially due to the inverted nature of the check. > > Not sure about that ... do you have an example at hand? it was the whole story around hw/core/cpu.h . It contains CONFIG_USER_ONLY, with the unwanted behavior mentioned, and seeing its existing use, I stopped short of introducing a bug: https://www.mail-archive.com/qemu-devel@nongnu.org/msg768318.html Other code in hw/core/cpu.c also uses CONFIG_USER_ONLY (See the XXX), and the hw/core/cpu.h continues to carry CONFIG_USER_ONLY, with the potential to lead other people to misuse it (putting in an extra prototype is harmless, but an extra field isn't). > > Anyway, one thing is sure, if we want to poison CONFIG_USER_ONLY, this will > certainly cause a lot of clean up work first, since it is used all over the > place... > > Thomas Yes, and probably a good idea. Thanks, Claudio
On 3/15/21 2:54 PM, Thomas Huth wrote:
> We are generating a lot of target-specific defines in the *-config-devices.h
> and *-config-target.h files. Using them in common code is wrong and leads
> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there
> as expected. To avoid these issues, we are already poisoning some of the
> macros in include/exec/poison.h - but maintaining this list manually is
> cumbersome. Thus let's generate the list of poisoned macros automatically
> instead.
> Note that CONFIG_TCG (which is also defined in config-host.h) and
IIRC we can't poison CONFIG_XEN / CONFIG_HAX because they are
pulled in via "sysemu/hw_accel.h".
> CONFIG_USER_ONLY are special, so we have to filter these out.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> RFC since the shell stuff in "configure" is quite ugly ... maybe there's
> a better way to do this via meson, but my meson-foo is still lacking...
>
> Makefile | 2 +-
> configure | 5 +++++
> include/exec/poison.h | 2 ++
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index bcbbec71a1..4cab10a2a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -213,7 +213,7 @@ qemu-%.tar.bz2:
>
> distclean: clean
> -$(quiet-@)test -f build.ninja && $(NINJA) $(NINJAFLAGS) -t clean -g || :
> - rm -f config-host.mak config-host.h*
> + rm -f config-host.mak config-host.h* config-poison.h
> rm -f tests/tcg/config-*.mak
> rm -f config-all-disas.mak config.status
> rm -f roms/seabios/config.mak roms/vgabios/config.mak
> diff --git a/configure b/configure
> index f7d022a5db..c7b5df3a5c 100755
> --- a/configure
> +++ b/configure
> @@ -6441,6 +6441,11 @@ if test -n "${deprecated_features}"; then
> echo " features: ${deprecated_features}"
> fi
>
> +cat *-config-devices.h *-config-target.h | grep '^#define ' \
> + | grep -v CONFIG_TCG | grep -v CONFIG_USER_ONLY \
> + | sed -e 's/#define //' -e 's/ .*//' | sort -u \
> + | sed -e 's/^/#pragma GCC poison /' > config-poison.h
> +
> # Save the configure command line for later reuse.
> cat <<EOD >config.status
> #!/bin/sh
> diff --git a/include/exec/poison.h b/include/exec/poison.h
> index 4cd3f8abb4..9e55d5aec2 100644
> --- a/include/exec/poison.h
> +++ b/include/exec/poison.h
> @@ -4,6 +4,8 @@
> #ifndef HW_POISON_H
> #define HW_POISON_H
>
> +#include "config-poison.h"
> +
> #pragma GCC poison TARGET_I386
> #pragma GCC poison TARGET_X86_64
> #pragma GCC poison TARGET_AARCH64
>
On 15/03/2021 15.52, Philippe Mathieu-Daudé wrote:
> On 3/15/21 2:54 PM, Thomas Huth wrote:
>> We are generating a lot of target-specific defines in the *-config-devices.h
>> and *-config-target.h files. Using them in common code is wrong and leads
>> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there
>> as expected. To avoid these issues, we are already poisoning some of the
>> macros in include/exec/poison.h - but maintaining this list manually is
>> cumbersome. Thus let's generate the list of poisoned macros automatically
>> instead.
>> Note that CONFIG_TCG (which is also defined in config-host.h) and
>
> IIRC we can't poison CONFIG_XEN / CONFIG_HAX because they are
> pulled in via "sysemu/hw_accel.h".
That's a good hint ... but I think it can be fixed with a patch like this:
diff a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
--- a/include/sysemu/hw_accel.h
+++ b/include/sysemu/hw_accel.h
@@ -12,19 +12,24 @@
#define QEMU_HW_ACCEL_H
#include "hw/core/cpu.h"
+
+#ifdef NEED_CPU_H
+
#include "sysemu/hax.h"
#include "sysemu/kvm.h"
#include "sysemu/hvf.h"
#include "sysemu/whpx.h"
-void cpu_synchronize_state(CPUState *cpu);
-void cpu_synchronize_post_reset(CPUState *cpu);
-void cpu_synchronize_post_init(CPUState *cpu);
-void cpu_synchronize_pre_loadvm(CPUState *cpu);
-
static inline bool cpu_check_are_resettable(void)
{
return kvm_enabled() ? kvm_cpu_check_are_resettable() : true;
}
+#endif
+
+void cpu_synchronize_state(CPUState *cpu);
+void cpu_synchronize_post_reset(CPUState *cpu);
+void cpu_synchronize_post_init(CPUState *cpu);
+void cpu_synchronize_pre_loadvm(CPUState *cpu);
+
#endif /* QEMU_HW_ACCEL_H */
Thomas
On Mon, 15 Mar 2021 at 15:26, Thomas Huth <thuth@redhat.com> wrote: > > On 15/03/2021 15.52, Philippe Mathieu-Daudé wrote: > > On 3/15/21 2:54 PM, Thomas Huth wrote: > >> We are generating a lot of target-specific defines in the *-config-devices.h > >> and *-config-target.h files. Using them in common code is wrong and leads > >> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there > >> as expected. To avoid these issues, we are already poisoning some of the > >> macros in include/exec/poison.h - but maintaining this list manually is > >> cumbersome. Thus let's generate the list of poisoned macros automatically > >> instead. > >> Note that CONFIG_TCG (which is also defined in config-host.h) and > > > > IIRC we can't poison CONFIG_XEN / CONFIG_HAX because they are > > pulled in via "sysemu/hw_accel.h". > > That's a good hint ... but I think it can be fixed with a patch like this: > > diff a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h > --- a/include/sysemu/hw_accel.h > +++ b/include/sysemu/hw_accel.h > @@ -12,19 +12,24 @@ > #define QEMU_HW_ACCEL_H > > #include "hw/core/cpu.h" > + > +#ifdef NEED_CPU_H > + > #include "sysemu/hax.h" > #include "sysemu/kvm.h" > #include "sysemu/hvf.h" > #include "sysemu/whpx.h" This doesn't look right, because sysemu/kvm.h itself contains a NEED_CPU_H check, which implies that there are situations where NEED_CPU_H is not defined and we need to pull in the header. thanks -- PMM
On 15/03/2021 16.37, Peter Maydell wrote: > On Mon, 15 Mar 2021 at 15:26, Thomas Huth <thuth@redhat.com> wrote: >> >> On 15/03/2021 15.52, Philippe Mathieu-Daudé wrote: >>> On 3/15/21 2:54 PM, Thomas Huth wrote: >>>> We are generating a lot of target-specific defines in the *-config-devices.h >>>> and *-config-target.h files. Using them in common code is wrong and leads >>>> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there >>>> as expected. To avoid these issues, we are already poisoning some of the >>>> macros in include/exec/poison.h - but maintaining this list manually is >>>> cumbersome. Thus let's generate the list of poisoned macros automatically >>>> instead. >>>> Note that CONFIG_TCG (which is also defined in config-host.h) and >>> >>> IIRC we can't poison CONFIG_XEN / CONFIG_HAX because they are >>> pulled in via "sysemu/hw_accel.h". >> >> That's a good hint ... but I think it can be fixed with a patch like this: >> >> diff a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h >> --- a/include/sysemu/hw_accel.h >> +++ b/include/sysemu/hw_accel.h >> @@ -12,19 +12,24 @@ >> #define QEMU_HW_ACCEL_H >> >> #include "hw/core/cpu.h" >> + >> +#ifdef NEED_CPU_H >> + >> #include "sysemu/hax.h" >> #include "sysemu/kvm.h" >> #include "sysemu/hvf.h" >> #include "sysemu/whpx.h" > > This doesn't look right, because sysemu/kvm.h itself contains > a NEED_CPU_H check, which implies that there are situations where > NEED_CPU_H is not defined and we need to pull in the header. I just tried, and QEMU seems to compile fine with my patch... I guess the check for NEED_CPU_H is mainly required for files that include "sysemu/kvm.h" directly, without the detour via hw_accel.h. Anyway, I can also push the check rather into the hax.h, hvf.h and whpx.h files themselves, to make them more similar to kvm.h. Thomas
On 3/15/21 8:54 AM, Thomas Huth wrote:
> We are generating a lot of target-specific defines in the *-config-devices.h
> and *-config-target.h files. Using them in common code is wrong and leads
> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there
> as expected. To avoid these issues, we are already poisoning some of the
> macros in include/exec/poison.h - but maintaining this list manually is
> cumbersome. Thus let's generate the list of poisoned macros automatically
> instead.
> Note that CONFIG_TCG (which is also defined in config-host.h) and
> CONFIG_USER_ONLY are special, so we have to filter these out.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> RFC since the shell stuff in "configure" is quite ugly ... maybe there's
> a better way to do this via meson, but my meson-foo is still lacking...
>
> +++ b/configure
> @@ -6441,6 +6441,11 @@ if test -n "${deprecated_features}"; then
> echo " features: ${deprecated_features}"
> fi
>
> +cat *-config-devices.h *-config-target.h | grep '^#define ' \
> + | grep -v CONFIG_TCG | grep -v CONFIG_USER_ONLY \
> + | sed -e 's/#define //' -e 's/ .*//' | sort -u \
> + | sed -e 's/^/#pragma GCC poison /' > config-poison.h
Most times, a 'grep | sed' pipeline can be rewritten in pure sed. In
this case:
cat *-config-devices.h *-config-target.h | \
sed -n -e '/^#define / { s///; /CONFIG_TCG/d; /CONFIG_USER_ONLY/d;' \
-e 's/ .*//; s/^/#pragma GCC poison /p; }' | \
sort -u > config-poison.h
But as you say, doing it in meson might be even nicer (and that is also
beyond my meson-foo)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 15/03/2021 19.24, Eric Blake wrote:
> On 3/15/21 8:54 AM, Thomas Huth wrote:
>> We are generating a lot of target-specific defines in the *-config-devices.h
>> and *-config-target.h files. Using them in common code is wrong and leads
>> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there
>> as expected. To avoid these issues, we are already poisoning some of the
>> macros in include/exec/poison.h - but maintaining this list manually is
>> cumbersome. Thus let's generate the list of poisoned macros automatically
>> instead.
>> Note that CONFIG_TCG (which is also defined in config-host.h) and
>> CONFIG_USER_ONLY are special, so we have to filter these out.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> RFC since the shell stuff in "configure" is quite ugly ... maybe there's
>> a better way to do this via meson, but my meson-foo is still lacking...
>>
>
>> +++ b/configure
>> @@ -6441,6 +6441,11 @@ if test -n "${deprecated_features}"; then
>> echo " features: ${deprecated_features}"
>> fi
>>
>> +cat *-config-devices.h *-config-target.h | grep '^#define ' \
>> + | grep -v CONFIG_TCG | grep -v CONFIG_USER_ONLY \
>> + | sed -e 's/#define //' -e 's/ .*//' | sort -u \
>> + | sed -e 's/^/#pragma GCC poison /' > config-poison.h
>
> Most times, a 'grep | sed' pipeline can be rewritten in pure sed. In
> this case:
>
> cat *-config-devices.h *-config-target.h | \
> sed -n -e '/^#define / { s///; /CONFIG_TCG/d; /CONFIG_USER_ONLY/d;' \
> -e 's/ .*//; s/^/#pragma GCC poison /p; }' | \
> sort -u > config-poison.h
Thanks! I'll update my patch and use your suggestion (unless someone comes
up with some Meson magic to do it there instead)
Thomas
© 2016 - 2024 Red Hat, Inc.