Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

["Maciej W. Rozycki" <macro at imgtec dot com>]

On Wed, 24 Feb 2016, Pedro Alves wrote:

> >> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
> >> +   of TRAP_* to abstract out these peculiarities.  */
> >>  #if defined __i386__ || defined __x86_64__
> >>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> >> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> >>  #elif defined __powerpc__
> >>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
> >> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> >> +#elif defined __mips__
> >> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> >> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
> > 
> >  Shall I add the TRAP_BRKPT and TRAP_HWBKPT codes to the MIPS Linux kernel 
> > then or not?  
> 
> The higher order issue was having a way to distinguish the possible
> traps, for correctness.  Since we found a way, it's no longer a
> pressing issue and we could leave it be.
> 
> > If anything, this looks like a performance win to me, at no 
> > significant cost (any possible kernel overhead will be in the range of a 
> > couple processor instructions, which is nothing compared to an extra 
> > ptrace(2) call and all the associated processing).
> 
> Yeah.  We usually need several other ptrace calls so it may not
> be noticeable.  But if you do teach the kernel about TRAP_BRKPT, and want to
> avoid the watchpoints check, I think gdb/gdbserver could be made to auto detect
> at run time what does the kernel report.  E.g,. have gdb fork itself, set a
> sw bp at the current PC in the child and continue it.  That triggers the bp
> immediately.  Then the parent can check what is in the child's si_code.  We
> do similar things already in linux_check_ptrace_features (gdb/nat/linux-ptrace.c).

 That would be the big-hammer approach.  However from my understanding of 
your code the benefit from such probing would only matter for speeding up 
random SIGTRAP reporting, a corner case not worth it IMO.

 Whereas for real breakpoint hits if we report TRAP_BRKPT for software 
breakpoints and TRAP_HWBKPT for hardware data or instruction breakpoints 
(the latters once implemented), then the:

	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))

condition will never be true for legitimate SIGTRAP events and the slow 
path won't trigger.  But then the MIPS variant will need:

# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL || (X) == TRAP_HWBKPT)

to actually recognise these events at all in the first place.  So we 
better have it right away or updated kernels will break GDB for a change.  
But you haven't put it in your proposal despite my earlier suggestion, 
hence my question whether you want this kernel API correction and the 
resulting optimisation or not (I do).

 Given my observations above it looks to me that we only really need the 
two macros updated with my proposal on the GDB side and a corresponding 
rather trivial kernel update to have the codes set in the first place, and 
we can get away without complicating code with an extra run-time probe.

 Have I missed anything?

  Maciej