This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sat, 26 Sep 2009 23:57:09 +0200
- Subject: Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1
- References: <20090925204835.GF5077@adacore.com>
On Fri, 25 Sep 2009 22:48:36 +0200, Joel Brobecker wrote:
> 2009-09-25 Joel Brobecker <brobecker@adacore.com>
>
> * ia64-tdep.c (ia64_memory_insert_breakpoint): Check the slotnum
> and the type of instruction before deciding which slot to save
> in the breakpoint shadown contents.
>
> Tested on ia64-linux. Jan, does this look right to you too?
Yes. This is a recent regression from me, sorry and thanks for the fix.
Just when writing a testcase for your fix I found that these L-X slots were
still not behaving right. Additional fixes on top of your patch are needed,
attached.
No regressions on ia64-rhel54-linux-gnu.
> I would normally apply immediately, but since we're close to release,
Without the fix below the attached patch will:
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
Thanks,
Jan
gdb/
2009-09-26 Jan Kratochvil <jan.kratochvil@redhat.com>
Fix ia64 breakpoints in the L-X slot.
* ia64-tdep.c (ia64_memory_insert_breakpoint): Extend the comment.
New variable shadow_slotnum, use it appropriately instead of slotnum.
Move shadow_len initialization before SLOTNUM adjustment, cover now the
whole remaining bundle. Error now on breakpoints requested for the
slot 2 of L-X bundles. Better sanity check the requested slot 1 of L-X
bundles.
(ia64_memory_remove_breakpoint): New variable shadow_slotnum, use it
appropriately instead of slotnum. Warn now on breakpoints requested
for the slot 2 of L-X bundles. Better sanity check the requested slot
1 of L-X bundles. Update the assertio check of PLACED_SIZE.
(ia64_breakpoint_from_pc): New variable shadow_slotnum, use it
appropriately instead of slotnum. Move *lenptr initialization before
SLOTNUM adjustment, cover now the whole remaining bundle. Error now
on breakpoints requested for the slot 2 of L-X bundles. Better sanity
check the requested slot 1 of L-X bundles. Simplify the returned
expression.
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'.
--- gdb/ia64-tdep.c-reorder 2009-09-26 23:52:33.000000000 +0200
+++ gdb/ia64-tdep.c 2009-09-26 23:52:49.000000000 +0200
@@ -592,8 +592,12 @@ fetch_instruction (CORE_ADDR addr, instr
The current addressing used by the code below:
original PC placed_address placed_size required covered
== bp_tgt->shadow_len reqd \subset covered
- 0xABCDE0 0xABCDE0 0xE <0x0...0x5> <0x0..0xD>
- 0xABCDE1 0xABCDE1 0xE <0x5...0xA> <0x1..0xE>
+ 0xABCDE0 0xABCDE0 0x10 <0x0...0x5> <0x0..0xF>
+ 0xABCDE1 0xABCDE1 0xF <0x5...0xA> <0x1..0xF>
+ 0xABCDE1 L-X 0xABCDE1 L-X 0xF <0xA...0xF> <0x1..0xF>
+ L is always in slot 1 and X is always in slot 2, while the address is
+ using slot 1 the breakpoint instruction must be placed
+ to the slot 2 (requiring to shadow tha last byte 0xF).
0xABCDE2 0xABCDE2 0xE <0xA...0xF> <0x2..0xF>
`objdump -d' and some other tools show a bit unjustified offsets:
@@ -611,7 +615,7 @@ ia64_memory_insert_breakpoint (struct gd
{
CORE_ADDR addr = bp_tgt->placed_address;
gdb_byte bundle[BUNDLE_LEN];
- int slotnum = (int) (addr & 0x0f) / SLOT_MULTIPLIER;
+ int slotnum = (int) (addr & 0x0f) / SLOT_MULTIPLIER, shadow_slotnum;
long long instr_breakpoint;
int val;
int template;
@@ -635,18 +639,30 @@ ia64_memory_insert_breakpoint (struct gd
return val;
}
+ /* SHADOW_SLOTNUM saves the original slot number as expected by the caller
+ for addressing the SHADOW_CONTENTS placement. */
+ shadow_slotnum = slotnum;
+
+ /* Cover always the last byte of the bundle for the L-X slot case. */
+ bp_tgt->shadow_len = BUNDLE_LEN - shadow_slotnum;
+
/* Check for L type instruction in slot 1, if present then bump up the slot
number to the slot 2. */
template = extract_bit_field (bundle, 0, 5);
- if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
- slotnum = 2;
-
- /* Slot number 2 may skip at most 2 bytes at the beginning. */
- bp_tgt->shadow_len = BUNDLE_LEN - 2;
+ if (template_encoding_table[template][slotnum] == X)
+ {
+ gdb_assert (slotnum == 2);
+ error (_("Can't insert breakpoint for non-existing slot X"));
+ }
+ if (template_encoding_table[template][slotnum] == L)
+ {
+ gdb_assert (slotnum == 1);
+ slotnum = 2;
+ }
/* Store the whole bundle, except for the initial skipped bytes by the slot
number interpreted as bytes offset in PLACED_ADDRESS. */
- memcpy (bp_tgt->shadow_contents, bundle + slotnum, bp_tgt->shadow_len);
+ memcpy (bp_tgt->shadow_contents, bundle + shadow_slotnum, bp_tgt->shadow_len);
/* Re-read the same bundle as above except that, this time, read it in order
to compute the new bundle inside which we will be inserting the
@@ -676,7 +692,7 @@ ia64_memory_insert_breakpoint (struct gd
bp_tgt->placed_size = bp_tgt->shadow_len;
- val = target_write_memory (addr + slotnum, bundle + slotnum,
+ val = target_write_memory (addr + shadow_slotnum, bundle + shadow_slotnum,
bp_tgt->shadow_len);
do_cleanups (cleanup);
@@ -689,7 +705,7 @@ ia64_memory_remove_breakpoint (struct gd
{
CORE_ADDR addr = bp_tgt->placed_address;
gdb_byte bundle_mem[BUNDLE_LEN], bundle_saved[BUNDLE_LEN];
- int slotnum = (addr & 0x0f) / SLOT_MULTIPLIER;
+ int slotnum = (addr & 0x0f) / SLOT_MULTIPLIER, shadow_slotnum;
long long instr_breakpoint, instr_saved;
int val;
int template;
@@ -710,13 +726,29 @@ ia64_memory_remove_breakpoint (struct gd
return val;
}
+ /* SHADOW_SLOTNUM saves the original slot number as expected by the caller
+ for addressing the SHADOW_CONTENTS placement. */
+ shadow_slotnum = slotnum;
+
/* Check for L type instruction in slot 1, if present then bump up the slot
number to the slot 2. */
template = extract_bit_field (bundle_mem, 0, 5);
- if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
- slotnum = 2;
+ if (template_encoding_table[template][slotnum] == X)
+ {
+ gdb_assert (slotnum == 2);
+ warning (_("Cannot remove breakpoint at address %s "
+ "from non-existing slot X, memory has changed underneath"),
+ paddress (gdbarch, bp_tgt->placed_address));
+ do_cleanups (cleanup);
+ return -1;
+ }
+ if (template_encoding_table[template][slotnum] == L)
+ {
+ gdb_assert (slotnum == 1);
+ slotnum = 2;
+ }
- gdb_assert (bp_tgt->placed_size == BUNDLE_LEN - 2);
+ gdb_assert (bp_tgt->placed_size == BUNDLE_LEN - shadow_slotnum);
gdb_assert (bp_tgt->placed_size == bp_tgt->shadow_len);
instr_breakpoint = slotN_contents (bundle_mem, slotnum);
@@ -732,7 +764,8 @@ ia64_memory_remove_breakpoint (struct gd
/* Extract the original saved instruction from SLOTNUM normalizing its
bit-shift for INSTR_SAVED. */
memcpy (bundle_saved, bundle_mem, BUNDLE_LEN);
- memcpy (bundle_saved + slotnum, bp_tgt->shadow_contents, bp_tgt->shadow_len);
+ memcpy (bundle_saved + shadow_slotnum, bp_tgt->shadow_contents,
+ bp_tgt->shadow_len);
instr_saved = slotN_contents (bundle_saved, slotnum);
/* In BUNDLE_MEM be careful to modify only the bits belonging to SLOTNUM and
@@ -755,7 +788,7 @@ ia64_breakpoint_from_pc (struct gdbarch
{
CORE_ADDR addr = *pcptr;
static gdb_byte bundle[BUNDLE_LEN];
- int slotnum = (int) (*pcptr & 0x0f) / SLOT_MULTIPLIER;
+ int slotnum = (int) (*pcptr & 0x0f) / SLOT_MULTIPLIER, shadow_slotnum;
long long instr_fetched;
int val;
int template;
@@ -777,11 +810,26 @@ ia64_breakpoint_from_pc (struct gdbarch
if (val != 0)
return NULL;
+ /* SHADOW_SLOTNUM saves the original slot number as expected by the caller
+ for addressing the SHADOW_CONTENTS placement. */
+ shadow_slotnum = slotnum;
+
+ /* Cover always the last byte of the bundle for the L-X slot case. */
+ *lenptr = BUNDLE_LEN - shadow_slotnum;
+
/* Check for L type instruction in slot 1, if present then bump up the slot
number to the slot 2. */
template = extract_bit_field (bundle, 0, 5);
- if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
- slotnum = 2;
+ if (template_encoding_table[template][slotnum] == X)
+ {
+ gdb_assert (slotnum == 2);
+ error (_("Can't insert breakpoint for non-existing slot X"));
+ }
+ if (template_encoding_table[template][slotnum] == L)
+ {
+ gdb_assert (slotnum == 1);
+ slotnum = 2;
+ }
/* A break instruction has its all its opcode bits cleared except for
the parameter value. For L+X slot pair we are at the X slot (slot 2) so
@@ -790,10 +838,7 @@ ia64_breakpoint_from_pc (struct gdbarch
instr_fetched &= 0x1003ffffc0LL;
replace_slotN_contents (bundle, instr_fetched, slotnum);
- *lenptr = BUNDLE_LEN - 2;
-
- /* SLOTNUM is possibly already locally modified - use caller's *PCPTR. */
- return bundle + (*pcptr & 0x0f);
+ return bundle + shadow_slotnum;
}
static CORE_ADDR
--- gdb/testsuite/gdb.base/breakpoint-shadow.c 3 Jan 2009 05:58:03 -0000 1.2
+++ gdb/testsuite/gdb.base/breakpoint-shadow.c 26 Sep 2009 21:51:20 -0000
@@ -18,10 +18,14 @@
int
main (void)
{
- volatile int i;
+ volatile long l;
- i = 1; /* break-first */
- i = 2; /* break-second */
+ l = 1; /* break-first */
+
+ /* This value already requires L-X slot on ia64 while it even fits in L on
+ arches with 32bit long. Ensure its lower part does not have much zeroes
+ accidentally matching ia64 `break' opcode. */
+ l = (1U << 31) - 1; /* break-second */
return 0;
}
--- gdb/testsuite/gdb.base/breakpoint-shadow.exp 10 Sep 2009 22:26:51 -0000 1.4
+++ gdb/testsuite/gdb.base/breakpoint-shadow.exp 26 Sep 2009 21:51:20 -0000
@@ -56,17 +56,18 @@ gdb_test_multiple "b [gdb_get_line_numbe
}
}
-if [istarget "ia64-*-*"] then {
+if {[istarget "ia64-*-*"] && [info exists bpt2address]} {
# 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.
set test "Second breakpoint address is valid on ia64"
- if [string match "*\[01\]" $bpt2address] {
+ if [string match "*1" $bpt2address] {
pass $test
- gdb_test "b *($bpt2address + 1)" "Breakpoint \[0-9\] at .*" "Third breakpoint on ia64 in the Second breakpoint's bundle"
+ gdb_test "b *($bpt2address - 1)" "Breakpoint \[0-9\] at .*" "Third breakpoint on ia64 in the Second breakpoint's bundle"
+ gdb_test "b *($bpt2address + 1)" "Can't insert breakpoint for non-existing slot X" "Slot X breakpoint refusal"
} else {
unresolved $test
}