From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2292 invoked by alias); 18 Aug 2010 18:28:58 -0000 Received: (qmail 2152 invoked by uid 22791); 18 Aug 2010 18:28:56 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,TW_GJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate4.de.ibm.com (HELO mtagate4.de.ibm.com) (195.212.17.164) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 18 Aug 2010 18:28:50 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate4.de.ibm.com (8.13.1/8.13.1) with ESMTP id o7IISlYt004801 for ; Wed, 18 Aug 2010 18:28:47 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o7IISlSa3735696 for ; Wed, 18 Aug 2010 20:28:47 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id o7IISlu7004598 for ; Wed, 18 Aug 2010 20:28:47 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id o7IISkgd004584; Wed, 18 Aug 2010 20:28:46 +0200 Message-Id: <201008181828.o7IISkgd004584@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Wed, 18 Aug 2010 20:28:46 +0200 Subject: Re: [rfc] Strip Thumb bit from PC returned by arm_get_longjmp_target To: matthew.gretton-dann@arm.com (Matthew Gretton-Dann) Date: Wed, 18 Aug 2010 18:28:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <1282122047.6505.28.camel@e102319-lin.cambridge.arm.com> from "Matthew Gretton-Dann" at Aug 18, 2010 10:00:47 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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: 2010-08/txt/msg00312.txt.bz2 Matthew Gretton-Dann wrote: > The problem with the patch is it removes what may be the only way we > have of telling the instruction state of the longjmp target. If you > have debugging information (mapping symbols at the very least) > everything is fine, but if you don't then how do you tell what the state > is? arm_pc_is_thumb does use this bit to detect the instruction state > (and arm_breakpoint_from_pc then uses this result to determine the > breakpoint type). Ah, I see. I was confused about just where the Thumb bit was supposed to be present and where not, sorry ... > In the case above I think the correct fix is to make > arm_adjust_breakpoint_address not strip out the address bits (which it > does when trying to work out whether we are single stepping through an > IT block). Does the patch below seem reasonable to you? > However, I think there is a more fundamental issue to be solved in the > ARM backend. For ARM (and possibly other architectures) you do not have > enough information in the PC to be able to accurately set a breakpoint, > you also need to know the instruction set in use at that address. > > Currently this is being done in a fairly ad hoc and inconsistent fashion > by using bit-0 to indicate Thumb or ARM state. But this troublesome to > maintain, and can make the debugger behave in a non-obvious manner to > the user. I think it would be cleaner to change the interfaces to > various functions (for example breakpoint_from_pc) to take an > 'instruction set state' parameter as well as the pc (or change the pc > from a CORE_ADDR to a { CORE_ADDR pc; enum instruction_set isa; } pair), > but this would be a large scale change with impacts throughout the code > base - which probably makes it impractical, and certainly in need of > discussion. Good point. I think there are two potential longer-term options. On the one hand, we've been planning to add support for multiple address spaces within a single process; this would imply that all gdbarch callbacks that get an address today also receive an address space identifier. If we have that, we could create two address spaces to represent the Thumb vs. ARM instruction space ... Another option would be to actually have two separate *gdbarch* implementations for Thumb vs. ARM, and use the multi-architecture support to choose the proper architecture to use. For frames, this would already be possible today using the frame-arch unwinder I've added for Cell support. There is even a breakpoint-location architecture, but no way yet for the back-end to influence how this is selected ... that can be added, though. (There may be other places where support is currently insufficient, of course.) In fact, the latter option seems nice anyway since in several gdbarch callback we already basically have a big "if ARM vs. Thumb" switching between two quite different implementations. Just having two different gdbarch's in the first place might actually simplify this ... Bye, Ulrich ChangeLog: * arm-tdep.c (arm_adjust_breakpoint_address): Do not strip out Thumb bit when returning incoming address unchanged. Set the Thumb bit when actually adjusting address. Index: gdb/arm-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/arm-tdep.c,v retrieving revision 1.304 diff -u -p -r1.304 arm-tdep.c --- gdb/arm-tdep.c 27 May 2010 19:06:12 -0000 1.304 +++ gdb/arm-tdep.c 18 Aug 2010 17:55:54 -0000 @@ -3346,6 +3346,7 @@ arm_adjust_breakpoint_address (struct gd int buf_len, buf2_len; enum bfd_endian order = gdbarch_byte_order_for_code (gdbarch); int i, any, last_it, last_it_count; + CORE_ADDR orig_bpaddr; /* If we are using BKPT breakpoints, none of this is necessary. */ if (gdbarch_tdep (gdbarch)->thumb2_breakpoint == NULL) @@ -3364,7 +3365,14 @@ arm_adjust_breakpoint_address (struct gd /* Thumb-2 code must have mapping symbols to have a chance. */ return bpaddr; - bpaddr = gdbarch_addr_bits_remove (gdbarch, bpaddr); + /* From here on, we always should return addresses with the Thumb + bit set. However, the incoming address may not consistently + have that bit set -- if it is clear, and we set the bit when + returning the original address otherwise unchanged, common code + would erroneously assume we adjusted the breakpoint. Just return + the original address as-is in those cases. */ + orig_bpaddr = bpaddr; + bpaddr = gdbarch_addr_bits_remove (gdbarch, orig_bpaddr); if (find_pc_partial_function (bpaddr, NULL, &func_start, NULL) && func_start > boundary) @@ -3377,11 +3385,11 @@ arm_adjust_breakpoint_address (struct gd buf_len = min (bpaddr - boundary, MAX_IT_BLOCK_PREFIX); if (buf_len == 0) /* No room for an IT instruction. */ - return bpaddr; + return orig_bpaddr; buf = xmalloc (buf_len); if (target_read_memory (bpaddr - buf_len, buf, buf_len) != 0) - return bpaddr; + return orig_bpaddr; any = 0; for (i = 0; i < buf_len; i += 2) { @@ -3395,7 +3403,7 @@ arm_adjust_breakpoint_address (struct gd if (any == 0) { xfree (buf); - return bpaddr; + return orig_bpaddr; } /* OK, the code bytes before this instruction contain at least one @@ -3437,7 +3445,7 @@ arm_adjust_breakpoint_address (struct gd { buf = extend_buffer_earlier (buf, bpaddr, buf_len, bpaddr - boundary); if (buf == NULL) - return bpaddr; + return orig_bpaddr; buf_len = bpaddr - boundary; i = 0; } @@ -3446,7 +3454,7 @@ arm_adjust_breakpoint_address (struct gd { buf = extend_buffer_earlier (buf, bpaddr, buf_len, bpaddr - boundary); if (buf == NULL) - return bpaddr; + return orig_bpaddr; buf_len = bpaddr - boundary; i = 0; } @@ -3477,15 +3485,15 @@ arm_adjust_breakpoint_address (struct gd if (last_it == -1) /* There wasn't really an IT instruction after all. */ - return bpaddr; + return orig_bpaddr; if (last_it_count < 1) /* It was too far away. */ - return bpaddr; + return orig_bpaddr; /* This really is a trouble spot. Move the breakpoint to the IT instruction. */ - return bpaddr - buf_len + last_it; + return MAKE_THUMB_ADDR (bpaddr - buf_len + last_it); } /* ARM displaced stepping support. -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com