Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1

[Joel Brobecker <brobecker at adacore dot com>]

> b *(0x4000000000000691 + 1)
> Breakpoint 5 at 0x4000000000000692: file ./gdb.base/breakpoint-shadow.c, line 28.
> ia64-tdep.c:672: internal-error: Address 0x4000000000000692 already contains a breakpoint.
> +
> 0x4000000000000690 <main+16>:   [MLX]       st8.rel [r2]=r14
> 0x4000000000000691 <main+17>:               movl r14=0x7fffffff;;
> ->
> 0x4000000000000690 <main+16>:   [MLX]       data8 0x1fe8dc021c0
> 0x4000000000000691 <main+17>:               data8 0x0cfffefe3

Yeah, good catch.  The only comment I had on the patch is that
it doesn't error out while trying to insert the breakpoint in
the normal case where "breakpoint always-inserted" is off.  Instead,
it errors out while trying to resume the execution of the inferior,
and the breakpoint prevents us from continuing until the breakpoint
is either removed or deleted.  I don't think we really have any 
mechanism in place at the moment that would allow us to catch
the problem early. But your patch already improves the situation.

So I checked it in both HEAD and branch (I want to add some comments
as discussed on IRC).

Given that this only occurs when either: debugging info points
to the wrong address, or when the user specifically indicates slot
2 of an L+X instruction as the address of the breakpoint, I consider
this as a non-problem.

> gdb/testsuite/
> 2009-09-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/breakpoint-shadow.c (main): Change `int i' type to `long l'.
> 	Use wider value for the second initialization.
> 	* gdb.base/breakpoint-shadow.exp <ia64>: Fix TCL error on missing
> 	bpt2address value.  Require now $bpt2address to be at slot 1.  Move
> 	"Third breakpoint" from slot 2 to slot 0.  New test `Slot X breakpoint
> 	refusal'.

You found an astute solution to provoking the situation.

However, even if the logical way of storing the new value for our long
is to use an L+X instruction on ia64, I am wondering if we wouldn't be
better off building our executable from assembly instead of from source,
the way we do in gdb.arch.  That way, we would know for sure in which
slot each breakpoint is inserted and what type of instruction we have
underneath.  We can also make sure we test all sort of combinations.
I wanted to write a testcase when I initially submitted my own patch,
but I couldn't work from my Ada example, because Ada programs usually
require the Ada Runtime.

What do you think? I've held the testsuite patch for now, because
the comment in breakpoint-shadow.c is a little confusing, and
the following comment in breakpoint-shadow.exp becomes a little
outdated:

    # Unoptimized code should not use the 3rd slot for the first instruction of
    # a source line.  This is important for our test, because we want both
    # breakpoints ("Second breakpoint" and the following one) to be in the same
    # bundle.

If we go for the gdb.arch approach, we can then remove the who section
specific to ia64 from breakpoint-shadow.exp.

-- 
Joel