| Message ID | 20230628164821.16771-1-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: gdbstub: Guard M-profile code with CONFIG_TCG | expand |
On 28/6/23 18:48, Fabiano Rosas wrote: > This code is only relevant when TCG is present in the build. Building > with --disable-tcg --enable-xen on an x86 host we get: > > $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen > $ make -j$(nproc) > ... > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': > ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' > ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' > > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': > ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' I'm a bit confused, isn't this covered by the cross-arm64-xen-only job? > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > This is a respin of: > https://lore.kernel.org/r/20230313151058.19645-5-farosas@suse.de > --- > target/arm/gdbstub.c | 4 ++++ > 1 file changed, 4 insertions(+)
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 28/6/23 18:48, Fabiano Rosas wrote: >> This code is only relevant when TCG is present in the build. Building >> with --disable-tcg --enable-xen on an x86 host we get: >> >> $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen >> $ make -j$(nproc) >> ... >> libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': >> ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' >> ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' >> >> libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': >> ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' > > I'm a bit confused, isn't this covered by the cross-arm64-xen-only > job? It should be. Perhaps the CI is using different optimization flags. I'll try to figure it out.
Fabiano Rosas <farosas@suse.de> writes: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> On 28/6/23 18:48, Fabiano Rosas wrote: >>> This code is only relevant when TCG is present in the build. Building >>> with --disable-tcg --enable-xen on an x86 host we get: >>> >>> $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen >>> $ make -j$(nproc) >>> ... >>> libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': >>> ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' >>> ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' >>> >>> libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': >>> ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' >> >> I'm a bit confused, isn't this covered by the cross-arm64-xen-only >> job? > > It should be. Perhaps the CI is using different optimization flags. I'll > try to figure it out. Yep. The CI has -O2 while I am using --enable-debug.
On Wed, 28 Jun 2023 at 17:48, Fabiano Rosas <farosas@suse.de> wrote: > > This code is only relevant when TCG is present in the build. Building > with --disable-tcg --enable-xen on an x86 host we get: > > $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen > $ make -j$(nproc) > ... > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': > ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' > ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' > > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': > ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' > > Signed-off-by: Fabiano Rosas <farosas@suse.de> Applied to target-arm.next, thanks. -- PMM
On 28/6/23 18:48, Fabiano Rosas wrote: > This code is only relevant when TCG is present in the build. Building > with --disable-tcg --enable-xen on an x86 host we get: > > $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen > $ make -j$(nproc) > ... > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': > ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' > ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' > > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': > ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > This is a respin of: > https://lore.kernel.org/r/20230313151058.19645-5-farosas@suse.de > --- > target/arm/gdbstub.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c > index 03b17c814f..f421c5d041 100644 > --- a/target/arm/gdbstub.c > +++ b/target/arm/gdbstub.c > @@ -324,6 +324,7 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg) > return cpu->dyn_sysreg_xml.num; > } > > +#ifdef CONFIG_TCG OK. > typedef enum { > M_SYSREG_MSP, > M_SYSREG_PSP, > @@ -481,6 +482,7 @@ static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg) > return cpu->dyn_m_secextreg_xml.num; > } > #endif > +#endif /* CONFIG_TCG */ > > const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) > { > @@ -561,6 +563,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) > arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), > "system-registers.xml", 0); > > +#ifdef CONFIG_TCG IIUC tcg_enabled(), this guard shouldn't be necessary; if CONFIG_TCG is not defined, tcg_enabled() evaluates to 0, and the compiler should elide the whole block. > if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) { > gdb_register_coprocessor(cs, > arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg, > @@ -575,4 +578,5 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) > } > #endif > } > +#endif /* CONFIG_TCG */ > }
On Tue, 4 Jul 2023 at 16:21, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 28/6/23 18:48, Fabiano Rosas wrote: > > This code is only relevant when TCG is present in the build. Building > > with --disable-tcg --enable-xen on an x86 host we get: > > > > $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen > > $ make -j$(nproc) > > ... > > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': > > ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' > > ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' > > > > libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': > > ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' > > > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > > --- > > This is a respin of: > > https://lore.kernel.org/r/20230313151058.19645-5-farosas@suse.de > > --- > > target/arm/gdbstub.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c > > index 03b17c814f..f421c5d041 100644 > > --- a/target/arm/gdbstub.c > > +++ b/target/arm/gdbstub.c > > @@ -324,6 +324,7 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg) > > return cpu->dyn_sysreg_xml.num; > > } > > > > +#ifdef CONFIG_TCG > > OK. > > > typedef enum { > > M_SYSREG_MSP, > > M_SYSREG_PSP, > > @@ -481,6 +482,7 @@ static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg) > > return cpu->dyn_m_secextreg_xml.num; > > } > > #endif > > +#endif /* CONFIG_TCG */ > > > > const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) > > { > > @@ -561,6 +563,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) > > arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), > > "system-registers.xml", 0); > > > > +#ifdef CONFIG_TCG > > IIUC tcg_enabled(), this guard shouldn't be necessary; if CONFIG_TCG > is not defined, tcg_enabled() evaluates to 0, and the compiler should > elide the whole block. IME it's a bit optimistic to assume that the compiler will always do that, especially with no optimisation enabled. thanks -- PMM
On 4/7/23 17:44, Peter Maydell wrote: > On Tue, 4 Jul 2023 at 16:21, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> On 28/6/23 18:48, Fabiano Rosas wrote: >>> This code is only relevant when TCG is present in the build. Building >>> with --disable-tcg --enable-xen on an x86 host we get: >>> >>> $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen >>> $ make -j$(nproc) >>> ... >>> libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': >>> ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' >>> ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' >>> >>> libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': >>> ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' >>> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de> >>> --- >>> This is a respin of: >>> https://lore.kernel.org/r/20230313151058.19645-5-farosas@suse.de >>> --- >>> target/arm/gdbstub.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c >>> index 03b17c814f..f421c5d041 100644 >>> --- a/target/arm/gdbstub.c >>> +++ b/target/arm/gdbstub.c >>> @@ -324,6 +324,7 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg) >>> return cpu->dyn_sysreg_xml.num; >>> } >>> >>> +#ifdef CONFIG_TCG >> >> OK. >> >>> typedef enum { >>> M_SYSREG_MSP, >>> M_SYSREG_PSP, >>> @@ -481,6 +482,7 @@ static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg) >>> return cpu->dyn_m_secextreg_xml.num; >>> } >>> #endif >>> +#endif /* CONFIG_TCG */ >>> >>> const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) >>> { >>> @@ -561,6 +563,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) >>> arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), >>> "system-registers.xml", 0); >>> >>> +#ifdef CONFIG_TCG >> >> IIUC tcg_enabled(), this guard shouldn't be necessary; if CONFIG_TCG >> is not defined, tcg_enabled() evaluates to 0, and the compiler should >> elide the whole block. > > IME it's a bit optimistic to assume that the compiler will always > do that, especially with no optimisation enabled. OK I see, thanks.
On 7/4/23 17:44, Peter Maydell wrote: >> IIUC tcg_enabled(), this guard shouldn't be necessary; if CONFIG_TCG >> is not defined, tcg_enabled() evaluates to 0, and the compiler should >> elide the whole block. > > IME it's a bit optimistic to assume that the compiler will always > do that, especially with no optimisation enabled. There's plenty of other places that we do. The compiler is usually pretty good with "if (0)". My question is if > if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) { needs to be written if (tcg_enabled()) { if (arm_feature(..., M) { ... } } r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 7/4/23 17:44, Peter Maydell wrote: >>> IIUC tcg_enabled(), this guard shouldn't be necessary; if CONFIG_TCG >>> is not defined, tcg_enabled() evaluates to 0, and the compiler should >>> elide the whole block. >> >> IME it's a bit optimistic to assume that the compiler will always >> do that, especially with no optimisation enabled. > > There's plenty of other places that we do. > The compiler is usually pretty good with "if (0)". > > My question is if > >> if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) { > > needs to be written > > if (tcg_enabled()) { > if (arm_feature(..., M) { > ... > } > } Yeah, that doesn't work either. I don't understand why in this particular case the compiler seems unable to remove that code. Can anyone else reproduce this or is it just happening on my setup? Maybe something is broken on my side...
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index 03b17c814f..f421c5d041 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -324,6 +324,7 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg) return cpu->dyn_sysreg_xml.num; } +#ifdef CONFIG_TCG typedef enum { M_SYSREG_MSP, M_SYSREG_PSP, @@ -481,6 +482,7 @@ static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg) return cpu->dyn_m_secextreg_xml.num; } #endif +#endif /* CONFIG_TCG */ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) { @@ -561,6 +563,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), "system-registers.xml", 0); +#ifdef CONFIG_TCG if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) { gdb_register_coprocessor(cs, arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg, @@ -575,4 +578,5 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) } #endif } +#endif /* CONFIG_TCG */ }
This code is only relevant when TCG is present in the build. Building with --disable-tcg --enable-xen on an x86 host we get: $ ../configure --target-list=x86_64-softmmu,aarch64-softmmu --disable-tcg --enable-xen $ make -j$(nproc) ... libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `m_sysreg_ptr': ../target/arm/gdbstub.c:358: undefined reference to `arm_v7m_get_sp_ptr' ../target/arm/gdbstub.c:361: undefined reference to `arm_v7m_get_sp_ptr' libqemu-aarch64-softmmu.fa.p/target_arm_gdbstub.c.o: in function `arm_gdb_get_m_systemreg': ../target/arm/gdbstub.c:405: undefined reference to `arm_v7m_mrs_control' Signed-off-by: Fabiano Rosas <farosas@suse.de> --- This is a respin of: https://lore.kernel.org/r/20230313151058.19645-5-farosas@suse.de --- target/arm/gdbstub.c | 4 ++++ 1 file changed, 4 insertions(+)