* [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1
@ 2009-09-25 20:48 Joel Brobecker
2009-09-26 21:57 ` Jan Kratochvil
2009-09-29 0:00 ` Joel Brobecker
0 siblings, 2 replies; 8+ messages in thread
From: Joel Brobecker @ 2009-09-25 20:48 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 3442 bytes --]
Hello,
We have a small testcase at AdaCore that just puts a breakpoint
on a line of code, runs to it, and then continues past it. I am
pasting the code at the end of this email. The initial purpose
of that testcase was to stress GDB with a very large number of
Ada tasks, but this is not relevant to the issue at hand, so I reduced
the number of tasks to 2.
To reproduce the problem, compile the program pasted at the end of
this message, and build it with the following trivial gnatmake:
% gnatmake -g ct
Then, insert a breakpoint at line 51, run to this breakpoint, and
then try to continue from that breakpoint:
(gdb) b 51
Breakpoint 1 at 0x4000000000004bd1: file ct.adb, line 51.
(gdb) run
[...]
Breakpoint 1, ct () at ct.adb:51
51 delay 1.0; -- BREAK
(gdb) cont
Continuing.
Program received signal SIGILL, Illegal instruction.
ct () at ct.adb:51
51 delay 1.0; -- BREAK
The program should just run until it completes.
The problem comes from the fact that the instruction where we
insert the breakpoint is in a slot 1 and is an L type instruction.
However, we save the wrong portion of the instruction bundle before
we realize this fact, and then insert the breakpoint in slot 2:
First we store the piece of bundle assuming slot 1, then reread
the bundle this time with breakpoints inserted, realize that we
have an L-type instruction at slot 1 and thus decide to put the
breakpoint on slot 2. At this point, the wrong portion of the bundle
has been saved in the shadow contents buffer, and breakpoint removal
is broken.
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? I would
normally apply immediately, but since we're close to release, a second
pair of eyes is always nice.
Thanks,
--
Joel
with Ada.Text_IO;
procedure Ct is
Number_Of_Tasks_To_Create : constant := 2;
task type T (Index : Natural; Last : Boolean) is
entry Finish;
end T;
task body T is
begin
-- Signal the creation of the task.
if Last then
Ada.Text_IO.Put_Line
("Starting task number:" & Natural'Image (Index) & " (last)");
else
Ada.Text_IO.Put_Line
("Starting task number:" & Natural'Image (Index));
end if;
-- Wait until told to finish the task. Then print a message
-- showing that the task is completing.
accept Finish do
if Last then
Ada.Text_IO.Put_Line
("End of task number:" & Natural'Image (Index) & " (last)");
else
Ada.Text_IO.Put_Line
("End of task number:" & Natural'Image (Index));
end if;
end Finish;
end T;
type TA is access T;
All_Tasks : array (Positive range 1 .. Number_Of_Tasks_To_Create) of TA;
begin
for Loop_Index in All_Tasks'Range loop
All_Tasks (Loop_Index) :=
new T (Loop_Index, Loop_Index = All_Tasks'Last);
end loop;
-- Add a delay to make sure all "Starting task ..." messages are
-- printed before Finish'ing thee tasks.
delay 1.0;
delay 1.0; -- BREAK
for Loop_Index in All_Tasks'Range loop
All_Tasks (Loop_Index).Finish;
end loop;
end Ct;
[-- Attachment #2: ia64-bp.diff --]
[-- Type: text/x-diff, Size: 1382 bytes --]
commit 1491fbdd98bd0bd63717f03f64c24bdda3917029
Author: Joel Brobecker <brobecker@adacore.com>
Date: Fri Sep 25 14:51:45 2009 -0400
Check instrunction slot num and type before deciding which slot to save.
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 674204a..bbde5f6 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -635,6 +635,12 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
return val;
}
+ /* 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;
@@ -657,12 +663,6 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
return val;
}
- /* 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;
-
/* Breakpoints already present in the code will get deteacted and not get
reinserted by bp_loc_is_permanent. Multiple breakpoints at the same
location cannot induce the internal error as they are optimized into
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-25 20:48 [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 Joel Brobecker @ 2009-09-26 21:57 ` Jan Kratochvil 2009-09-29 0:16 ` Joel Brobecker 2009-09-29 0:00 ` Joel Brobecker 1 sibling, 1 reply; 8+ messages in thread From: Jan Kratochvil @ 2009-09-26 21:57 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches 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 } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-26 21:57 ` Jan Kratochvil @ 2009-09-29 0:16 ` Joel Brobecker 2009-09-29 1:31 ` Joel Brobecker 2009-09-29 18:55 ` Jan Kratochvil 0 siblings, 2 replies; 8+ messages in thread From: Joel Brobecker @ 2009-09-29 0:16 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-29 0:16 ` Joel Brobecker @ 2009-09-29 1:31 ` Joel Brobecker 2009-09-29 18:55 ` Jan Kratochvil 1 sibling, 0 replies; 8+ messages in thread From: Joel Brobecker @ 2009-09-29 1:31 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 463 bytes --] > So I checked it in both HEAD and branch (I want to add some comments > as discussed on IRC). Here is what I ended up checking in (HEAD + branch): 2009-09-29 Joel Brobecker <brobecker@adacore.com> * ia64-tdep.c: Update the comments on how we insert/remove breakpoints for L-X instructions. (ia64_memory_insert_breakpoint, ia64_memory_remove_breakpoint): Update the comments inside these functions. No code change. -- Joel [-- Attachment #2: ia64-comments.diff --] [-- Type: text/x-diff, Size: 5800 bytes --] Index: ia64-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/ia64-tdep.c,v retrieving revision 1.202 diff -u -p -r1.202 ia64-tdep.c --- ia64-tdep.c 28 Sep 2009 23:54:01 -0000 1.202 +++ ia64-tdep.c 29 Sep 2009 01:19:52 -0000 @@ -586,6 +586,23 @@ fetch_instruction (CORE_ADDR addr, instr SLOTNUM (`adress & 0x0f', value in the range <0..2>). We need to know SLOTNUM in ia64_memory_remove_breakpoint. + There is one special case where we need to be extra careful: + L-X instructions, which are instructions that occupy 2 slots + (The L part is always in slot 1, and the X part is always in + slot 2). We must refuse to insert breakpoints for an address + that points at slot 2 of a bundle where an L-X instruction is + present, since there is logically no instruction at that address. + However, to make things more interesting, the opcode of L-X + instructions is located in slot 2. This means that, to insert + a breakpoint at an address that points to slot 1, we actually + need to write the breakpoint in slot 2! Slot 1 is actually + the extended operand, so writing the breakpoint there would not + have the desired effect. Another side-effect of this issue + is that we need to make sure that the shadow contents buffer + does save byte 15 of our instruction bundle (this is the tail + end of slot 2, which wouldn't be saved if we were to insert + the breakpoint in slot 1). + ia64 16-byte bundle layout: | 5 bits | slot 0 with 41 bits | slot 1 with 41 bits | slot 2 with 41 bits | @@ -594,12 +611,11 @@ fetch_instruction (CORE_ADDR addr, instr == bp_tgt->shadow_len reqd \subset covered 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 that last byte at 0xF). 0xABCDE2 0xABCDE2 0xE <0xA...0xF> <0x2..0xF> - + + L-X instructions are treated a little specially, as explained above: + 0xABCDE1 0xABCDE1 0xF <0xA...0xF> <0x1..0xF> + `objdump -d' and some other tools show a bit unjustified offsets: original PC byte where starts the instruction objdump offset 0xABCDE0 0xABCDE0 0xABCDE0 @@ -643,19 +659,25 @@ ia64_memory_insert_breakpoint (struct gd for addressing the SHADOW_CONTENTS placement. */ shadow_slotnum = slotnum; - /* Cover always the last byte of the bundle for the L-X slot case. */ + /* Always cover the last byte of the bundle in case we are inserting + a breakpoint on an L-X instruction. */ 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 (template_encoding_table[template][slotnum] == X) { + /* X unit types can only be used in slot 2, and are actually + part of a 2-slot L-X instruction. We cannot break at this + address, as this is the second half of an instruction that + lives in slot 1 of that bundle. */ gdb_assert (slotnum == 2); error (_("Can't insert breakpoint for non-existing slot X")); } if (template_encoding_table[template][slotnum] == L) { + /* L unit types can only be used in slot 1. But the associated + opcode for that instruction is in slot 2, so bump the slot number + accordingly. */ gdb_assert (slotnum == 1); slotnum = 2; } @@ -730,20 +752,26 @@ ia64_memory_remove_breakpoint (struct gd 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 (template_encoding_table[template][slotnum] == X) { + /* X unit types can only be used in slot 2, and are actually + part of a 2-slot L-X instruction. We refuse to insert + breakpoints at this address, so there should be no reason + for us attempting to remove one there, except if the program's + code somehow got modified in memory. */ gdb_assert (slotnum == 2); - warning (_("Cannot remove breakpoint at address %s " - "from non-existing slot X, memory has changed underneath"), + warning (_("Cannot remove breakpoint at address %s from non-existing " + "X-type slot, memory has changed underneath"), paddress (gdbarch, bp_tgt->placed_address)); do_cleanups (cleanup); return -1; } if (template_encoding_table[template][slotnum] == L) { + /* L unit types can only be used in slot 1. But the breakpoint + was actually saved using slot 2, so update the slot number + accordingly. */ gdb_assert (slotnum == 1); slotnum = 2; } @@ -768,8 +796,8 @@ ia64_memory_remove_breakpoint (struct gd 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 - never any other possibly also stored in SHADOW_CONTENTS. */ + /* In BUNDLE_MEM, be careful to modify only the bits belonging to SLOTNUM + and not any of the other ones that are stored in SHADOW_CONTENTS. */ replace_slotN_contents (bundle_mem, instr_saved, slotnum); val = target_write_memory (addr, bundle_mem, BUNDLE_LEN); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-29 0:16 ` Joel Brobecker 2009-09-29 1:31 ` Joel Brobecker @ 2009-09-29 18:55 ` Jan Kratochvil 2009-09-29 19:14 ` Joel Brobecker 1 sibling, 1 reply; 8+ messages in thread From: Jan Kratochvil @ 2009-09-29 18:55 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches On Tue, 29 Sep 2009 02:16:41 +0200, Joel Brobecker wrote: > So I checked it in both HEAD and branch (I want to add some comments > as discussed on IRC). Yes, I agree, thanks for all the work. > 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, Yes, I agree the file gdb.base/breakpoint-shadow.exp now became the same as its former ia64-less revision 1.1. > If we go for the gdb.arch approach, we can then remove the who section > specific to ia64 from breakpoint-shadow.exp. Done. OK to check-in? Even on branch? Thanks, Jan gdb/testsuite/ 2009-09-29 Jan Kratochvil <jan.kratochvil@redhat.com> * gdb.base/breakpoint-shadow.exp: Move the ia64 part into ... * gdb.arch/ia64-breakpoint-shadow.exp, ... a new file, with new tests. * gdb.arch/ia64-breakpoint-shadow.S: New file. --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ gdb/testsuite/gdb.arch/ia64-breakpoint-shadow.S 29 Sep 2009 18:50:29 -0000 @@ -0,0 +1,44 @@ +/* Copyright 2009 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. + + This file is part of the gdb testsuite. + It tests displaced stepping over various insns that require special + handling. */ + + .text + .align 16 + .global main + .proc main +main: + mov r2=r12 + mov r14=1 + ;; +bundle: + /* Store value 1 into `long' variable on stack. */ + st8.rel [r2]=r14 + /* This long constant requires L-X slot in this bundle. */ + movl r14=0x7fffffff + ;; + /* Store value 0x7fffffff into `long' variable on stack. */ + st8.rel [r2]=r14 + mov r14=r0 + ;; + mov r8=r14 + mov r12=r2 + br.ret.sptk.many b0 + + .endp main + + .section .note.GNU-stack,"",@progbits --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ gdb/testsuite/gdb.arch/ia64-breakpoint-shadow.exp 29 Sep 2009 18:50:29 -0000 @@ -0,0 +1,80 @@ +# Copyright 2009 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +if ![istarget "ia64-*-*"] { + verbose "Skipping ia64-breakpoint-shadow test." + return +} + +set testfile ia64-breakpoint-shadow +set srcfile ${testfile}.S +set binfile ${objdir}/${subdir}/${testfile} +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { + untested "Couldn't compile test program" + return -1 +} + +gdb_exit +gdb_start +gdb_reinitialize_dir $srcdir/$subdir +gdb_load ${binfile} + +# We need to start the inferior to place the breakpoints in the memory at all. +if ![runto_main] { + untested start + return -1 +} + +# The default "auto" mode removes all the breakpoints when we stop (and not +# running the nonstop mode). We would not be able to test the shadow. +gdb_test "set breakpoint always-inserted on" +gdb_test "show breakpoint always-inserted" "Always inserted breakpoint mode is on." + +set match "\nDump of assembler code for function main:\r\n(.*)End of assembler dump.\r\n$gdb_prompt $" + +set test "disassembly without breakpoints" +gdb_test_multiple "disass main" $test { + -re $match { + set orig $expect_out(1,string) + pass $test + } +} + +set test "slot 0 breakpoint placed" +gdb_test_multiple "b bundle" $test { + -re "Breakpoint \[0-9\] at (0x\[0-9a-f\]*0):.*$gdb_prompt $" { + pass $test + set bpt2address $expect_out(1,string) + } +} + +if ![info exists bpt2address] { + return -1 +} + +gdb_test "b *($bpt2address + 1)" "Breakpoint \[0-9\] at .*" "slot 1 breakpoint placed" +gdb_test "b *($bpt2address + 2)" "Can't insert breakpoint for non-existing slot X" "slot 2 (slot X) breakpoint refusal" + +set test "disassembly with breakpoints" +gdb_test_multiple "disass main" $test { + -re $match { + set got $expect_out(1,string) + if [string equal -nocase $orig $got] { + pass $test + } else { + fail $test + } + } +} --- gdb/testsuite/gdb.base/breakpoint-shadow.exp 10 Sep 2009 22:26:51 -0000 1.4 +++ gdb/testsuite/gdb.base/breakpoint-shadow.exp 29 Sep 2009 18:50:29 -0000 @@ -48,29 +48,7 @@ gdb_test_multiple "disass main" $test { } gdb_test "b [gdb_get_line_number "break-first"]" "Breakpoint \[0-9\] at .*" "First breakpoint placed" -set test "Second breakpoint placed" -gdb_test_multiple "b [gdb_get_line_number "break-second"]" $test { - -re "Breakpoint \[0-9\] at (0x\[0-9a-f\]*):.*$gdb_prompt $" { - pass $test - set bpt2address $expect_out(1,string) - } -} - -if [istarget "ia64-*-*"] then { - # 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] { - pass $test - - gdb_test "b *($bpt2address + 1)" "Breakpoint \[0-9\] at .*" "Third breakpoint on ia64 in the Second breakpoint's bundle" - } else { - unresolved $test - } -} +gdb_test "b [gdb_get_line_number "break-second"]" "Breakpoint \[0-9\] at .*" "Second breakpoint placed" set test "disassembly with breakpoints" gdb_test_multiple "disass main" $test { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-29 18:55 ` Jan Kratochvil @ 2009-09-29 19:14 ` Joel Brobecker 2009-09-29 19:29 ` Jan Kratochvil 0 siblings, 1 reply; 8+ messages in thread From: Joel Brobecker @ 2009-09-29 19:14 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches This is great work, thanks! > gdb/testsuite/ > 2009-09-29 Jan Kratochvil <jan.kratochvil@redhat.com> > > * gdb.base/breakpoint-shadow.exp: Move the ia64 part into ... > * gdb.arch/ia64-breakpoint-shadow.exp, ... a new file, with new tests. > * gdb.arch/ia64-breakpoint-shadow.S: New file. Just one nit, I think. Shouldn't we disassemble bundle, rather than main? Pre-approved, HEAD and branch. -- Joel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-29 19:14 ` Joel Brobecker @ 2009-09-29 19:29 ` Jan Kratochvil 0 siblings, 0 replies; 8+ messages in thread From: Jan Kratochvil @ 2009-09-29 19:29 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches On Tue, 29 Sep 2009 21:14:01 +0200, Joel Brobecker wrote: > > gdb/testsuite/ > > 2009-09-29 Jan Kratochvil <jan.kratochvil@redhat.com> > > > > * gdb.base/breakpoint-shadow.exp: Move the ia64 part into ... > > * gdb.arch/ia64-breakpoint-shadow.exp, ... a new file, with new tests. > > * gdb.arch/ia64-breakpoint-shadow.S: New file. > > Just one nit, I think. Shouldn't we disassemble bundle, rather than > main? I do not find any difference, whole `main' is supplied by the testcase, I do not see any pros/cons of either of two. Thanks for the review. > Pre-approved, HEAD and branch. Checked-in in the posted form: http://sourceware.org/ml/gdb-cvs/2009-09/msg00228.html http://sourceware.org/ml/gdb-cvs/2009-09/msg00229.html (gdb_7_0-branch) Thanks, Jan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 2009-09-25 20:48 [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 Joel Brobecker 2009-09-26 21:57 ` Jan Kratochvil @ 2009-09-29 0:00 ` Joel Brobecker 1 sibling, 0 replies; 8+ messages in thread From: Joel Brobecker @ 2009-09-29 0:00 UTC (permalink / raw) To: gdb-patches > 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. Now checked in HEAD and branch. -- Joel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-09-29 19:29 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-09-25 20:48 [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 Joel Brobecker 2009-09-26 21:57 ` Jan Kratochvil 2009-09-29 0:16 ` Joel Brobecker 2009-09-29 1:31 ` Joel Brobecker 2009-09-29 18:55 ` Jan Kratochvil 2009-09-29 19:14 ` Joel Brobecker 2009-09-29 19:29 ` Jan Kratochvil 2009-09-29 0:00 ` Joel Brobecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox