From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30584 invoked by alias); 26 Sep 2009 21:57:46 -0000 Received: (qmail 30563 invoked by uid 22791); 26 Sep 2009 21:57:44 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 26 Sep 2009 21:57:37 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8QLvD8H015924; Sat, 26 Sep 2009 17:57:13 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id n8QLvAfd013203 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 26 Sep 2009 17:57:13 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.3/8.14.3) with ESMTP id n8QLvARi029062; Sat, 26 Sep 2009 23:57:10 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.3/8.14.3/Submit) id n8QLv9rh029058; Sat, 26 Sep 2009 23:57:09 +0200 Date: Sat, 26 Sep 2009 21:57:00 -0000 From: Jan Kratochvil To: Joel Brobecker Cc: gdb-patches@sourceware.org Subject: Re: [RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1 Message-ID: <20090926215709.GA26221@host0.dyn.jankratochvil.net> References: <20090925204835.GF5077@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090925204835.GF5077@adacore.com> User-Agent: Mutt/1.5.20 (2009-08-17) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-09/txt/msg00840.txt.bz2 On Fri, 25 Sep 2009 22:48:36 +0200, Joel Brobecker wrote: > 2009-09-25 Joel Brobecker > > * 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 : [MLX] st8.rel [r2]=r14 0x4000000000000691 : movl r14=0x7fffffff;; -> 0x4000000000000690 : [MLX] data8 0x1fe8dc021c0 0x4000000000000691 : data8 0x0cfffefe3 Thanks, Jan gdb/ 2009-09-26 Jan Kratochvil 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 * 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 : 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 }