Re: [patch] Fix linux-ia64 on SIGILL for deleted breakpoint

[Pedro Alves <pedro at codesourcery dot com>]

On Sunday 25 July 2010 19:52:17, Jan Kratochvil wrote:
> On Sun, 25 Jul 2010 00:26:04 +0200, Pedro Alves wrote:
> > On Friday 23 July 2010 23:19:35, Jan Kratochvil wrote:
> > 
> > > This SIGTRAP->SIGILL case happens only on ia64 and ia64 does not use any
> > > set_gdbarch_decr_pc_after_break at all, PC stays on the breakpoint bundle+slot
> > > in both the SIGTRAP and SIGILL case.
> > > 
> > > You are right it is arch-specific.  On i386 I checked SIGILL is never
> > > generated (only in some fpu-emulated code).  So I checked s390x-linux-gnu::
> > > 	SIGILL on opcode 0xb29e
> > > 	si_addr = 0x800009a4
> > > 	.psw.addr = 0x800009a8
> > > 	instr at = 0x800009a4
> > > 	.psw.addr - instr == 4
> > 
> > Oh?  Does this mean the PC is left pointing _after_ the instruction
> > that caused the SIGILL?
> 
> Yes.  It probably corresponds to:
> 
> 390 - z/Architecture Principles of Operation (assembly manual)
> http://publibz.boulder.ibm.com/epubs/pdf/a2278325.pdf
> 6-6 Instruction-Length Code
> The instruction-length code (ILC) occupies two bit positions and provides the 
> length of the last instruction executed.  It permits identifying the
> instruction causing the interruption when the instruction address in the old
> PSW designates the next sequential instruction.
>                    ^^^^

I guess I'm surprised.  Doesn't s390 have a variable length encoding?  If
the processor just tried to execute an illegal instruction, I'm not seeing
what does exactly mean the length of the bad instruction, and that "next"
ends up pointing at a real instruction boundary.  Oh, well, whatever.
Good to know that we need to be careful with this.  Thanks for the pointer.

> 
> set_gdbarch_decr_pc_after_break = 2 is still OK there:

> GDB does not intentionally place SIGILL
> generating instruction on s390x so it does not need to recognize ILC.

Right.  decr_pc_after_break generally models the PC offset to the breakpoint
instruction, when the processor finishes executing a breakpoint trap, not
random exception modes.  If ever needed, we might pass the signal/event
as argument to gdbarch_decr_pc_after_break.  And let the callback consult
siginfo too...  We'll cross that bridge when we come to it.

> > I think you still need to audit other bits in linux-nat.c for SIGTRAP bkpts
> > handling.  E.g., see stop_wait_callback.  (For extra correctness,
> > count_events_callback, and the select_event_lwp_callback functions would
> > be relaxed too.)  I wasn't previously suggesting to make this ia64 arch
> > specific, which made fixing these other places too easier.  Notice how
> > gdbserver/linux-low.c also considers non-sigtrap bkpts (and it was a recent
> > change, needed for ARM thumb2 kernels that hadn't learned about the
> > breakpoint insns yet, IIRC).
> 
> Are we talking about?
> 	RFC: Updates support for breakpoints that generate SIGILL
> 	http://sourceware.org/ml/gdb-patches/2010-01/msg00619.html
> 
> As it is for arm*.c which does not use set_gdbarch_decr_pc_after_break I do
> not want to predict how SIGILL vs. gdbarch_decr_pc_after_break behaves.

It's for arm, but it's implemented in the arch-independent linux-low.c.
If your check on s390 is good, than that might have broken (rare ...) corner
cases on s390, like a SIGILL reported for an instruction set at the address
consecutive to an inserted software breakpoint.

> Which arch uses gdbarch_decr_pc_after_break && generates a non-SIGTRAP signal
> on a GDB-placed breakpoint?  It is probably none of the arches I target (i686,
> x86_64, ppc{,64}, s390{,x}, ia64, possibly arm).

I don't have a list.  Some archs may generate that only on some
situations (or used to and no longer do on more up to date versions).
I wouldn't know how all targets/archs behave in the call dummies on a
non-executable stack scenario, for example.  In such case, the target
doesn't actually execute the trap, and reports an exception (that results
in say, SIGSEGV), so gdbarch_decr_pc_after_break doesn't really apply.

From infrun.c:

  /* First, distinguish signals caused by the debugger from signals
     that have to do with the program's own actions.  Note that
     breakpoint insns may cause SIGTRAP or SIGILL or SIGEMT, depending
     on the operating system version.  Here we detect when a SIGILL or
     SIGEMT is really a breakpoint and change it to SIGTRAP.  We do
     something similar for SIGSEGV, since a SIGSEGV will be generated
     when we're trying to execute a breakpoint instruction on a
     non-executable stack.  This happens for call dummy breakpoints
     for architectures like SPARC that place call dummies on the
     stack.  */

According to <http://en.wikipedia.org/wiki/SIGEMT>
> On the PDP-11 architecture, EMT, TRAP, BPT and IOT were instructions used for
> program traps. EMT was for system software, TRAP for programmer use, BPT for 
> debugging, and IOT for I/O routines. Currently EMT does not have a standardised usage.

> > If this stays, please add a comment above this call mentioning that
> > we can safely call this function even for SIGILL, since
> > decr_pc_after_break is 0 on ia64.
> 
> Done.

Thanks.

> 
> 
> > (Alternatively, recode to avoid the assumption)
> 
> Sorry I do not try to code anything without an iron to test it.

What I meant was to have your new code not depend on decr_pc_after
break being 0, you don't need any other system to try that on.
It meant, to not have any code path in the SIGILL
case that calls gdbarch_decr_pc_after_break at all. Conceptually,
something like:

if (sig == SIGTRAP && software_breakpoint_inserted_at (PC - decr_pc_after_breakpoint))
 {
    rewind pc, discard signal
    return 1
 }
else if (sig == SIGILL && software_breakpoint_inserted_at (PC))
 {
    discard signal
    return 1
 }
return 0


> OK to check-in?

Okay.  Please fix the spelling of "alternative".

-- 
Pedro Alves