[PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Fabiano Rosas]

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
 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 */
 }
-- 
2.35.3

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Philippe Mathieu-Daudé]

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

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Fabiano Rosas]

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.

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Fabiano Rosas]

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.

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Peter Maydell]

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

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Philippe Mathieu-Daudé]

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 */
>   }

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Peter Maydell]

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

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Philippe Mathieu-Daudé]

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.

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Richard Henderson]

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~

Re: [PATCH] target/arm: gdbstub: Guard M-profile code with CONFIG_TCG

[Fabiano Rosas]

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...