Re: [PATCH v2] MIPS: Add basic support for ptrace single step

Re: [PATCH v2] MIPS: Add basic support for ptrace single step

[Thomas Bogendoerfer]

On Wed, Feb 10, 2021 at 02:52:41PM +0800, Tiezhu Yang wrote:
> In the current code, arch_has_single_step() is not defined on MIPS,
> that means MIPS does not support instruction single-step for user mode.
> 
> Delve is a debugger for the Go programming language, the ptrace syscall
> PtraceSingleStep() failed [1] on MIPS and then the single step function
> can not work well, we can see that PtraceSingleStep() definition returns
> ptrace(PTRACE_SINGLESTEP) [2].
> 
> So it is necessary to support ptrace single step on MIPS.

one way to see it. The other way is, that the Go debugger needs to learn
single stepping without ptrace single step support. IMHO gdb is already
able to do that.

IMHO ptrace single step is for CPUs supporting single stepping and not
for emulating it in the kernel.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2] MIPS: Add basic support for ptrace single step

[Thomas Bogendoerfer]

On Thu, Feb 11, 2021 at 11:29:05AM +0100, Thomas Bogendoerfer wrote:
> IMHO ptrace single step is for CPUs supporting single stepping and not
> for emulating it in the kernel.

I've checked other arch how they implement single step, and looks like
I'm wrong. So I'm ok with applying your patch. Can you resend it again,
so I'll get the latest version in patchwork ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2] MIPS: Add basic support for ptrace single step

[Maciej W. Rozycki]

On Fri, 12 Feb 2021, Thomas Bogendoerfer wrote:

> > IMHO ptrace single step is for CPUs supporting single stepping and not
> > for emulating it in the kernel.
> 
> I've checked other arch how they implement single step, and looks like
> I'm wrong. So I'm ok with applying your patch. Can you resend it again,
> so I'll get the latest version in patchwork ?

 Huh?  How is that supposed to work?  Skimming over the code it hardcodes 
the breakpoint instruction, which is ISA-dependent and relies on branches 
or jumps to have a delay slot, which is not universally true.  The kernel 
does not know all the exotic branches the MIPS ISA has (BC1ANY4F anyone?) 
either and IMHO should not.

 This is broken and belongs to the userland anyway.  See how complex the 
handling is in GDB, specifically `mips16_next_pc', `micromips_next_pc' and 
`mips32_next_pc' in gdb/mips-tdep.c.

 We do have branch emulation code, but it was intended for a different 
purpose and is therefore not complete enough for single-stepping 
emulation.

 And I find it regrettable that the kernel has become so bloated here and 
attempts are made to make it even more bloated.  All under the original 
excuse made by FP emulation code, which also should have been made in the 
userland.  It all really does not belong to the kernel with its elevated 
privilege.  It does not require the privilege.

 We do need a ptrace(2) request to stop on signal handler invocation 
though, which is something we have been missing and never got to 
implementing.

  Maciej

Re: [PATCH v2] MIPS: Add basic support for ptrace single step

[Thomas Bogendoerfer]

On Tue, Feb 16, 2021 at 02:55:36PM +0100, Maciej W. Rozycki wrote:
> On Fri, 12 Feb 2021, Thomas Bogendoerfer wrote:
> 
> > > IMHO ptrace single step is for CPUs supporting single stepping and not
> > > for emulating it in the kernel.
> > 
> > I've checked other arch how they implement single step, and looks like
> > I'm wrong. So I'm ok with applying your patch. Can you resend it again,
> > so I'll get the latest version in patchwork ?
> 
>  [..]
>  This is broken and belongs to the userland anyway.  See how complex the 
> handling is in GDB, specifically `mips16_next_pc', `micromips_next_pc' and 
> `mips32_next_pc' in gdb/mips-tdep.c.

I should have looked closer how other archs are implementing single
stepping. It's only alpha, which emulates it by setting breakpoints. All 
others have some type of cpu cupport for getting traps after a single
intstruction has been issued. So I'm reverting the commit and supporting
Maciej's statement about putting that stuff into userland (again).

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]