Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

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

On Mon, 29 Sep 2014, Maciej W. Rozycki wrote:

> > I'm confused on this part of your original description:
> > 
> > >  The issue is `insert_bp_location' overwrites the previously adjusted 
> > > value in `placed_address' with the original address, that is only replaced 
> > > back with the correct adjusted address later on when 
> > > `gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
> > > the value in `placed_address' does not correspond to data stored in 
> > > `shadow_contents', leading to incorrect instruction bytes being supplied 
> > > when `one_breakpoint_xfer_memory' is called to supply the instruction 
> > > overlaid by the breakpoint.
> > 
> > It doesn't look like to me that this is really the problem, since
> > default_memory_insert_breakpoint adjusts bp_tgt->placed_address
> > before reading memory.
> 
>  Not true (from `mips_breakpoint_from_pc'):
> 
> 	  insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
> 	  size = status ? 2
> 			: mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
>           *pcptr = unmake_compact_addr (pc);
> 
> (hmm, weird indentation here, will have to fix) -- as you can see 
> `mips_fetch_instruction' (that reads the instruction under `pc') is called 
> before the ISA bit is stripped as `pc' is written back to `*pcptr', and 
> `pc' has to have the ISA bit set for the reasons I stated in the last 
> mail.
> 
>  Maybe I could work it around by writing `*pcptr' back first (and still 
> calling `mips_fetch_instruction' with the original `pc'), but that looks 
> hackish to me; first of all there is no contract in the API between the 
> implementation of `gdbarch_breakpoint_from_pc' and its callers that memory 
> behind `pcptr' is the address used for breakpoint shadowing.  I think the 
> data structures used for shadowing should simply be consistent all the 
> time.
> 
> > Instead, the issue is that because the breakpoint is supposed to be
> > inserted (we're re-inserting it), one_breakpoint_xfer_memory needs
> > to store the breakpoint instruction on top of the memory we're
> > about to write.  And then one_breakpoint_xfer_memory gets the
> > breakpoint instruction wrong exactly because it lost the ISA bit.

 As a further note -- as long as symbol information is available the 
instruction used for the breakpoint is always a valid encoding for the ISA 
being used at the breakpoint location even if the ISA bit has been lost, 
because the ISA bit is only used to determine the encoding as the last 
resort.  The ELF `st_other' symbol flags are a preferred way to determine 
the ISA and are available for the majority of failures I see because of 
this defect.  Only cases like software watchpoint tests that single-step 
through possibly stripped system library code will trip on the lost ISA 
bit.

 Of course getting the ISA right does not guarantee the right size of the 
breakpoint instruction, which is the matter of this bug.

> > I could be missing something else, of course.
> > 
> > The patch below is what I'd like to push on top of the software single-step
> > rework (which I've meanwhile slit and posted here
> > https://sourceware.org/ml/gdb-patches/2014-09/msg00755.html)
> > 
> > I've pushed that series with this patch on top here, for convenience:
> > 
> >   git@github.com:palves/gdb.git palves/mips_instruction_shadow_inconsistency
> > 
> > Obviously, the mips-linux-gnu testing mentioned in the log is tentative. :-)
> 
>  I'll push it through testing, although given what I wrote above I have 
> little hope, so it'll be just a smoke test with a microMIPS multilib and 
> the offending test case first.

 So I smoke-tested gdb.base/break.exp that fails horribly with our current 
trunk and the `-EL -mmicromips' multilib and it still fails the same way 
with your tree:

(gdb) PASS: gdb.base/break.exp: run until file:function(1) breakpoint
continue
Continuing.
720

Breakpoint 2, marker2 (a=43) at /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c:63
63	int marker2 (a) int a; { return (1); }	/* set breakpoint 9 here */
(gdb) PASS: gdb.base/break.exp: run until quoted breakpoint
continue
Continuing.

Program received signal SIGILL, Illegal instruction.
0x0040098d in marker2 (a=43) at /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c:63
63	int marker2 (a) int a; { return (1); }	/* set breakpoint 9 here */
(gdb) FAIL: gdb.base/break.exp: run until file:linenum breakpoint
break +1
Breakpoint 10 at 0x4009a3: file /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c, 
line 64.
(gdb) FAIL: gdb.base/break.exp: breakpoint offset +1
step

Program terminated with signal SIGILL, Illegal instruction.
The program no longer exists.
(gdb) FAIL: gdb.base/break.exp: step onto breakpoint

Child terminated with signal = 0x4 (SIGILL)
GDBserver exiting
[...]

 Sorry.  This test script scores "all passed" with my change.

  Maciej