Re: [PATCH 05/30] bsd-user/arm/arget_arch_cpu.h: Move EXCP_DEBUG and EXCP_BKPT together

[Warner Losh <imp@bsdimp.com>]

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

On Thu, Jan 13, 2022 at 10:13 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Sun, 9 Jan 2022 at 16:26, Warner Losh <imp@bsdimp.com> wrote:
> >
> > Implement EXCP_DEBUG and EXCP_BKPT the same, as is done in
> > linux-user. The prior adjustment of register 15 isn't needed, so remove
> > that. Remove a redunant comment (that code in FreeBSD never handled
> > break points).
> >
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >  bsd-user/arm/target_arch_cpu.h | 23 +++--------------------
> >  1 file changed, 3 insertions(+), 20 deletions(-)
> >
> > diff --git a/bsd-user/arm/target_arch_cpu.h
> b/bsd-user/arm/target_arch_cpu.h
> > index c526fc73502..05b19ce6119 100644
> > --- a/bsd-user/arm/target_arch_cpu.h
> > +++ b/bsd-user/arm/target_arch_cpu.h
> > @@ -21,6 +21,7 @@
> >  #define _TARGET_ARCH_CPU_H_
> >
> >  #include "target_arch.h"
> > +#include "signal-common.h"
> >
> >  #define TARGET_DEFAULT_CPU_MODEL "any"
> >
> > @@ -64,19 +65,7 @@ static inline void target_cpu_loop(CPUARMState *env)
> >              }
> >              break;
> >          case EXCP_SWI:
> > -        case EXCP_BKPT:
> >              {
> > -                /*
> > -                 * system call
> > -                 * See arm/arm/trap.c cpu_fetch_syscall_args()
> > -                 */
> > -                if (trapnr == EXCP_BKPT) {
> > -                    if (env->thumb) {
> > -                        env->regs[15] += 2;
> > -                    } else {
> > -                        env->regs[15] += 4;
> > -                    }
> > -                }
>
> So the previous code was implementing BKPT as a way to do
> a syscall (added in commit 8d450c9a30). Was that just a mistake ?
>

I did some digging and I'm at a loss for why this code was ever here.


> >                  n = env->regs[7];
> >                  if (bsd_type == target_freebsd) {
> >                      int ret;
> > @@ -171,14 +160,8 @@ static inline void target_cpu_loop(CPUARMState *env)
> >              queue_signal(env, info.si_signo, &info);
> >              break;
> >          case EXCP_DEBUG:
> > -            {
> > -
> > -                info.si_signo = TARGET_SIGTRAP;
> > -                info.si_errno = 0;
> > -                info.si_code = TARGET_TRAP_BRKPT;
> > -                info.si_addr = env->exception.vaddress;
> > -                queue_signal(env, info.si_signo, &info);
> > -            }
> > +        case EXCP_BKPT:
> > +            force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT,
> env->regs[15]);
> >              break;
> >          case EXCP_YIELD:
> >              /* nothing to do here for user-mode, just resume guest code
> */
>
> Looks like it now matches the freebsd kernel behaviour, anyway.
>

Yea. That's why I went ahead and made the change rather than slavishly
carry it
over for something weird I couldn't find out about... I think it's an old
mistake...
I'll update the commit message to specifically note it.


> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM
>

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