From patchwork Thu Mar 24 18:31:41 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ulrich Weigand X-Patchwork-Id: 772 Return-Path: Delivered-To: unknown Received: from imap.gmail.com (74.125.159.109) by localhost6.localdomain6 with IMAP4-SSL; 08 Jun 2011 14:45:41 -0000 Delivered-To: patches@linaro.org Received: by 10.42.161.68 with SMTP id s4cs125720icx; Thu, 24 Mar 2011 11:31:52 -0700 (PDT) Received: by 10.14.5.195 with SMTP id 43mr3221074eel.127.1300991510955; Thu, 24 Mar 2011 11:31:50 -0700 (PDT) Received: from mtagate2.uk.ibm.com (mtagate2.uk.ibm.com [194.196.100.162]) by mx.google.com with ESMTPS id r50si502699eeh.25.2011.03.24.11.31.49 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 24 Mar 2011 11:31:49 -0700 (PDT) Received-SPF: softfail (google.com: domain of transitioning uweigand@de.ibm.com does not designate 194.196.100.162 as permitted sender) client-ip=194.196.100.162; Authentication-Results: mx.google.com; spf=softfail (google.com: domain of transitioning uweigand@de.ibm.com does not designate 194.196.100.162 as permitted sender) smtp.mail=uweigand@de.ibm.com Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate2.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p2OIVgPK014267 for ; Thu, 24 Mar 2011 18:31:42 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1307.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2OIWB9a1482816 for ; Thu, 24 Mar 2011 18:32:11 GMT Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2OIVgWR025535 for ; Thu, 24 Mar 2011 12:31:42 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p2OIVfSm025531; Thu, 24 Mar 2011 12:31:41 -0600 Message-Id: <201103241831.p2OIVfSm025531@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 24 Mar 2011 19:31:41 +0100 Subject: [rfc, arm] Always use correct execution state for single-step breakpoints To: gdb-patches@sourceware.org Date: Thu, 24 Mar 2011 19:31:41 +0100 (CET) From: "Ulrich Weigand" Cc: matthew.gretton-dann@arm.com, patches@linaro.org X-Mailer: ELM [version 2.5 PL2] MIME-Version: 1.0 Hello, about a year ago, Matthew Gretton-Dann added improved heuristics in an attempt to ensure the breakpoints for software single-singlestepping are correct for the target execution mode: http://sourceware.org/ml/gdb-patches/2010-03/msg00168.html Now in general, GDB needs to make some assumptions (usually by consulting symbol tables) to determine the target execution mode for an arbitrary code address. However, when single-stepping, GDB detects the target address by decoding the branch instruction anyway, and can at the same time reliably detect the target execution mode. With Matthew's patch, this information is used whenever GDB does not have symbol information for the target address. When GDB *does* have symbol information, that still takes precedence. Now usually this is OK because the execution mode determined from symbol tables is still correct. However, we've had a bug report where symbols tables were present, but contained the *wrong* execution mode for a symbol (due to an error in hand-written assembler code). Now this is certainly a bug in the code (and led to the program crashing at run-time). However, you'd still expect a debugger to get this right -- bugs in the program are why you need a debugger in the first place :-) The following patch attempts to fix this test case by making sure that for single-step breakpoints, the symbol table is *never* consulted, but instead the known target execution mode is used. This is done by means of a new global "arm_override_mode" which is respected in arm_pc_is_thumb unconditionally (similar to arm_force_mode_string), and set by a new routine arm_insert_single_step_breakpoint which is then used whenever setting a single-step breakpoint. This routine operates on the execution mode encoded in the LSB of the address; to get at that, arm_get_next_pc is modified to return a PC in that form. (The global is a bit ugly, but the concept still seems right to me: if at some point in the future, we actually do use two different gdbarch's to represent Thumb vs. ARM, the single-step routines would determine the proper gdbarch to set the breakpoint in and simply use insert_single_step_breakpoint with that arch.) The old single-step heuristics in arm_pc_is_thumb introduced by Matthew is no longer needed, and removed by the patch. As a final part of the patch, there was one case where arm_linux_syscall_next_pc would not properly set the Thumb bit; this is fixed as well. Tested on armv7l-unknown-linux-gnueabi with no regressions; fixes the newly added test case. Any comments? I'm planning on committing this in a week or so. Bye, Ulrich ChangeLog: gdb/ * arm-tdep.h (arm_insert_single_step_breakpoint): Add prototype. * arm-tdep.c (arm_override_mode): New global. (arm_pc_is_thumb): Respect arm_override_mode. Remove single-step execution mode heuristics. (thumb_get_next_pc_raw): Remove INSERT_BKTP argument; always insert second single-step breakpoint if needed, using arm_insert_single_step_breakpoint. (arm_get_next_pc_raw): Remove INSERT_BKTP argument. Only handle ARM execution mode, do not call thumb_get_next_pc_raw. (arm_get_next_pc): Encode execution mode in return value. Call either arm_get_next_pc_raw or thumb_get_next_pc_raw. (arm_insert_single_step_breakpoint): New function. (arm_software_single_step): Call it. * arm-linux-tdep.c (arm_linux_sigreturn_return_addr): Add IS_THUMB argument to return execution mode of sigreturn target. (arm_linux_syscall_next_pc): Use it. (arm_linux_copy_svc): Update call. (arm_linux_software_single_step): Call arm_insert_single_step_breakpoint. gdb/testsuite/ * gdb.arch/thumb-singlestep.S: New file. * gdb.arch/thumb-singlestep.exp: Likewise. Index: gdb/arm-linux-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/arm-linux-tdep.c,v retrieving revision 1.84 diff -u -p -r1.84 arm-linux-tdep.c --- gdb/arm-linux-tdep.c 8 Mar 2011 01:04:35 -0000 1.84 +++ gdb/arm-linux-tdep.c 23 Mar 2011 18:36:17 -0000 @@ -669,18 +669,24 @@ arm_linux_regset_from_core_section (stru } /* Copy the value of next pc of sigreturn and rt_sigrturn into PC, - and return 1. Return 0 if it is not a rt_sigreturn/sigreturn - syscall. */ + return 1. In addition, set IS_THUMB depending on whether we + will return to ARM or Thumb code. Return 0 if it is not a + rt_sigreturn/sigreturn syscall. */ static int arm_linux_sigreturn_return_addr (struct frame_info *frame, unsigned long svc_number, - CORE_ADDR *pc) + CORE_ADDR *pc, int *is_thumb) { /* Is this a sigreturn or rt_sigreturn syscall? */ if (svc_number == 119 || svc_number == 173) { if (get_frame_type (frame) == SIGTRAMP_FRAME) { + ULONGEST t_bit = arm_psr_thumb_bit (frame_unwind_arch (frame)); + CORE_ADDR cpsr + = frame_unwind_register_unsigned (frame, ARM_PS_REGNUM); + + *is_thumb = (cpsr & t_bit) != 0; *pc = frame_unwind_caller_pc (frame); return 1; } @@ -698,11 +704,11 @@ arm_linux_syscall_next_pc (struct frame_ CORE_ADDR return_addr = 0; int is_thumb = arm_frame_is_thumb (frame); ULONGEST svc_number = 0; - int is_sigreturn = 0; if (is_thumb) { svc_number = get_frame_register_unsigned (frame, 7); + return_addr = pc + 2; } else { @@ -721,24 +727,15 @@ arm_linux_syscall_next_pc (struct frame_ { svc_number = get_frame_register_unsigned (frame, 7); } + + return_addr = pc + 4; } - is_sigreturn = arm_linux_sigreturn_return_addr (frame, svc_number, - &return_addr); + arm_linux_sigreturn_return_addr (frame, svc_number, &return_addr, &is_thumb); - if (is_sigreturn) - return return_addr; - + /* Addresses for calling Thumb functions have the bit 0 set. */ if (is_thumb) - { - return_addr = pc + 2; - /* Addresses for calling Thumb functions have the bit 0 set. */ - return_addr |= 1; - } - else - { - return_addr = pc + 4; - } + return_addr |= 1; return return_addr; } @@ -761,7 +758,7 @@ arm_linux_software_single_step (struct f if (next_pc > 0xffff0000) next_pc = get_frame_register_unsigned (frame, ARM_LR_REGNUM); - insert_single_step_breakpoint (gdbarch, aspace, next_pc); + arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc); return 1; } @@ -806,6 +803,7 @@ arm_linux_copy_svc (struct gdbarch *gdba struct frame_info *frame; unsigned int svc_number = displaced_read_reg (regs, dsc, 7); int is_sigreturn = 0; + int is_thumb; if (debug_displaced) fprintf_unfiltered (gdb_stdlog, "displaced: copying Linux svc insn %.8lx\n", @@ -814,7 +812,7 @@ arm_linux_copy_svc (struct gdbarch *gdba frame = get_current_frame (); is_sigreturn = arm_linux_sigreturn_return_addr(frame, svc_number, - &return_to); + &return_to, &is_thumb); if (is_sigreturn) { struct symtab_and_line sal; Index: gdb/arm-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/arm-tdep.c,v retrieving revision 1.338 diff -u -p -r1.338 arm-tdep.c --- gdb/arm-tdep.c 21 Mar 2011 17:28:04 -0000 1.338 +++ gdb/arm-tdep.c 23 Mar 2011 18:36:18 -0000 @@ -133,6 +133,13 @@ static const char *arm_mode_strings[] = static const char *arm_fallback_mode_string = "auto"; static const char *arm_force_mode_string = "auto"; +/* Internal override of the execution mode. -1 means no override, + 0 means override to ARM mode, 1 means override to Thumb mode. + The effect is the same as if arm_force_mode has been set by the + user (except the internal override has precedence over a user's + arm_force_mode override). */ +static int arm_override_mode = -1; + /* Number of different reg name sets (options). */ static int num_disassembly_options; @@ -356,9 +363,6 @@ arm_find_mapping_symbol (CORE_ADDR memad return 0; } -static CORE_ADDR arm_get_next_pc_raw (struct frame_info *frame, - CORE_ADDR pc, int insert_bkpt); - /* Determine if the program counter specified in MEMADDR is in a Thumb function. This function should be called for addresses unrelated to any executing frame; otherwise, prefer arm_frame_is_thumb. */ @@ -388,6 +392,10 @@ arm_pc_is_thumb (struct gdbarch *gdbarch if (IS_THUMB_ADDR (memaddr)) return 1; + /* Respect internal mode override if active. */ + if (arm_override_mode != -1) + return arm_override_mode; + /* If the user wants to override the symbol table, let him. */ if (strcmp (arm_force_mode_string, "arm") == 0) return 0; @@ -418,29 +426,9 @@ arm_pc_is_thumb (struct gdbarch *gdbarch target, then trust the current value of $cpsr. This lets "display/i $pc" always show the correct mode (though if there is a symbol table we will not reach here, so it still may not be - displayed in the mode it will be executed). - - As a further heuristic if we detect that we are doing a single-step we - see what state executing the current instruction ends up with us being - in. */ + displayed in the mode it will be executed). */ if (target_has_registers) - { - struct frame_info *current_frame = get_current_frame (); - CORE_ADDR current_pc = get_frame_pc (current_frame); - int is_thumb = arm_frame_is_thumb (current_frame); - CORE_ADDR next_pc; - if (memaddr == current_pc) - return is_thumb; - else - { - struct gdbarch *gdbarch = get_frame_arch (current_frame); - next_pc = arm_get_next_pc_raw (current_frame, current_pc, FALSE); - if (memaddr == gdbarch_addr_bits_remove (gdbarch, next_pc)) - return IS_THUMB_ADDR (next_pc); - else - return is_thumb; - } - } + return arm_frame_is_thumb (get_current_frame ()); /* Otherwise we're out of luck; we assume ARM. */ return 0; @@ -4216,7 +4204,7 @@ thumb_advance_itstate (unsigned int itst another breakpoint by our caller. */ static CORE_ADDR -thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc, int insert_bkpt) +thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc) { struct gdbarch *gdbarch = get_frame_arch (frame); struct address_space *aspace = get_frame_address_space (frame); @@ -4314,8 +4302,8 @@ thumb_get_next_pc_raw (struct frame_info /* Set a breakpoint on the following instruction. */ gdb_assert ((itstate & 0x0f) != 0); - if (insert_bkpt) - insert_single_step_breakpoint (gdbarch, aspace, pc); + arm_insert_single_step_breakpoint (gdbarch, aspace, + MAKE_THUMB_ADDR (pc)); cond_negated = (itstate >> 4) & 1; /* Skip all following instructions with the same @@ -4587,8 +4575,7 @@ thumb_get_next_pc_raw (struct frame_info } /* Get the raw next address. PC is the current program counter, in - FRAME. INSERT_BKPT should be TRUE if we want a breakpoint set on - the alternative next instruction if there are two options. + FRAME, which is assumed to be executing in ARM mode. The value returned has the execution state of the next instruction encoded in it. Use IS_THUMB_ADDR () to see whether the instruction is @@ -4596,7 +4583,7 @@ thumb_get_next_pc_raw (struct frame_info address. */ static CORE_ADDR -arm_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc, int insert_bkpt) +arm_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc) { struct gdbarch *gdbarch = get_frame_arch (frame); enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); @@ -4606,9 +4593,6 @@ arm_get_next_pc_raw (struct frame_info * unsigned long status; CORE_ADDR nextpc; - if (arm_frame_is_thumb (frame)) - return thumb_get_next_pc_raw (frame, pc, insert_bkpt); - pc_val = (unsigned long) pc; this_instr = read_memory_unsigned_integer (pc, 4, byte_order_for_code); @@ -4861,18 +4845,51 @@ arm_get_next_pc_raw (struct frame_info * return nextpc; } +/* Determine next PC after current instruction executes. Will call either + arm_get_next_pc_raw or thumb_get_next_pc_raw. Error out if infinite + loop is detected. */ + CORE_ADDR arm_get_next_pc (struct frame_info *frame, CORE_ADDR pc) { - struct gdbarch *gdbarch = get_frame_arch (frame); - CORE_ADDR nextpc = - gdbarch_addr_bits_remove (gdbarch, - arm_get_next_pc_raw (frame, pc, TRUE)); - if (nextpc == pc) - error (_("Infinite loop detected")); + CORE_ADDR nextpc; + + if (arm_frame_is_thumb (frame)) + { + nextpc = thumb_get_next_pc_raw (frame, pc); + if (nextpc == MAKE_THUMB_ADDR (pc)) + error (_("Infinite loop detected")); + } + else + { + nextpc = arm_get_next_pc_raw (frame, pc); + if (nextpc == pc) + error (_("Infinite loop detected")); + } + return nextpc; } +/* Like insert_single_step_breakpoint, but make sure we use a breakpoint + of the appropriate mode (as encoded in the PC value), even if this + differs from what would be expected according to the symbol tables. */ + +void +arm_insert_single_step_breakpoint (struct gdbarch *gdbarch, + struct address_space *aspace, + CORE_ADDR pc) +{ + struct cleanup *old_chain + = make_cleanup_restore_integer (&arm_override_mode); + + arm_override_mode = IS_THUMB_ADDR (pc); + pc = gdbarch_addr_bits_remove (gdbarch, pc); + + insert_single_step_breakpoint (gdbarch, aspace, pc); + + do_cleanups (old_chain); +} + /* single_step() is called just before we want to resume the inferior, if we want to single-step it but there is no hardware or kernel single-step support. We find the target of the coming instruction @@ -4883,13 +4900,9 @@ arm_software_single_step (struct frame_i { struct gdbarch *gdbarch = get_frame_arch (frame); struct address_space *aspace = get_frame_address_space (frame); - - /* NOTE: This may insert the wrong breakpoint instruction when - single-stepping over a mode-changing instruction, if the - CPSR heuristics are used. */ - CORE_ADDR next_pc = arm_get_next_pc (frame, get_frame_pc (frame)); - insert_single_step_breakpoint (gdbarch, aspace, next_pc); + + arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc); return 1; } Index: gdb/arm-tdep.h =================================================================== RCS file: /cvs/src/src/gdb/arm-tdep.h,v retrieving revision 1.49 diff -u -p -r1.49 arm-tdep.h --- gdb/arm-tdep.h 8 Mar 2011 01:04:35 -0000 1.49 +++ gdb/arm-tdep.h 23 Mar 2011 18:36:18 -0000 @@ -311,6 +311,8 @@ extern void CORE_ADDR arm_skip_stub (struct frame_info *, CORE_ADDR); CORE_ADDR arm_get_next_pc (struct frame_info *, CORE_ADDR); +void arm_insert_single_step_breakpoint (struct gdbarch *, + struct address_space *, CORE_ADDR); int arm_software_single_step (struct frame_info *); int arm_frame_is_thumb (struct frame_info *frame); --- /dev/null 2011-03-24 13:56:45.925176537 +0100 +++ gdb/testsuite/gdb.arch/thumb-singlestep.S 2011-03-24 19:13:29.000000000 +0100 @@ -0,0 +1,40 @@ +/* Test program with deliberately incorrect execution mode transition + + Copyright 2011 Free Software Foundation, Inc. + + This file is part of GDB. + + 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 . */ + + .text + .align 2 + .global foo + .thumb + /* .thumb_func deliberately omitted */ +foo: + mov r0,#42 + bx lr + + .text + .align 2 + .global main + .thumb + .thumb_func + .type main, %function +main: + push {r3, lr} + blx foo + pop {r3, pc} + .size main, .-main + --- /dev/null 2011-03-24 13:56:45.925176537 +0100 +++ gdb/testsuite/gdb.arch/thumb-singlestep.exp 2011-03-24 19:13:29.000000000 +0100 @@ -0,0 +1,43 @@ +# Copyright 2011 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 . + +# Test single-stepping into incorrectly marked Thumb routine + +if {![istarget arm*-*]} then { + verbose "Skipping ARM tests." + return +} + +set testfile "thumb-singlestep" +set srcfile ${testfile}.S +set binfile ${objdir}/${subdir}/${testfile} + +set additional_flags "additional_flags=-mthumb" +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } { + untested thumb-singlestep.exp + return -1 +} + +gdb_exit +gdb_start +gdb_reinitialize_dir $srcdir/$subdir +gdb_load ${binfile} + +if ![runto_main] then { + gdb_suppress_tests +} + +gdb_test "si" "foo \\(\\) at .*${srcfile}.*mov r0,#42.*" "step into foo" +