From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8504 invoked by alias); 23 Mar 2011 21:11:07 -0000 Received: (qmail 8494 invoked by uid 22791); 23 Mar 2011 21:11:05 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,TW_CP,TW_EG,TW_RB,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 23 Mar 2011 21:10:58 +0000 Received: (qmail 16112 invoked from network); 23 Mar 2011 21:10:57 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 23 Mar 2011 21:10:57 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: mark raw registers the target doesn't have access to as unavailable Date: Wed, 23 Mar 2011 22:14:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.35-28-generic; KDE/4.6.1; x86_64; ; ) Cc: Andreas Tobler MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201103232110.52701.pedro@codesourcery.com> 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: 2011-03/txt/msg01042.txt.bz2 This change: : 2011-03-18 Pedro Alves ... * regcache.c: ... (regcache_save): Adjust to handle REG_UNAVAILABLE registers. ... Broke amd64 and ppc freebsd gdbs, and probably other targets. They're hitting this new assertion: void regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read, void *src) { ... for (regnum = 0; regnum < dst->descr->nr_cooked_registers; regnum++) { if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup)) { enum register_status status = cooked_read (src, regnum, buf); if (status == REG_VALID) memcpy (register_buffer (dst, regnum), buf, register_size (gdbarch, regnum)); else { gdb_assert (status != REG_UNKNOWN); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memset (register_buffer (dst, regnum), 0, register_size (gdbarch, regnum)); } dst->register_status[regnum] = status; } } In fact, the only thing relevant that I changed was adding the assert. The idea was, if we've just read a register, we should know whether its value is valid, or unavailable. Shouldn't be "unknown" any more. Now, what I missed is that some targets have debug apis that don't give debugger access to all the raw registers the architecture supports. E.g., from amd64fbsd-tdep.c: /* Mapping between the general-purpose registers in `struct reg' format and GDB's register cache layout. ... static int amd64fbsd_r_reg_offset[] = { 14 * 8, /* %rax */ ... 21 * 8, /* %ss */ -1, /* %ds */ -1, /* %es */ -1, /* %fs */ -1 /* %gs */ }; All those -1's mean that those registers aren't retrievable from fbsd's `struct reg'. I see amd64-darwin-tdep.c:amd64_darwin_thread_state_reg_offset also doesn't expose a few of the segment registers. I think this calls for marking these registers as "unavailable". That is, they exist in the target machine, so it's correct for the target's description to include them, but, GDB just doesn't know their value. That's what the patch below does. Here's the result on amd64-fbsd: (gdb) info registers rax 0xffffffffffffffff -1 rbx 0x1 1 rcx 0x800da59d0 34374048208 rdx 0x7fffffffda48 140737488345672 rsi 0x7fffffffda38 140737488345656 rdi 0x1 1 rbp 0x7fffffffd9e0 0x7fffffffd9e0 rsp 0x7fffffffd9c0 0x7fffffffd9c0 r8 0x3a 58 r9 0x7fffffffef42 140737488351042 r10 0x18 24 r11 0x0 0 r12 0x7fffffffda48 140737488345672 r13 0x7fffffffda38 140737488345656 r14 0x0 0 r15 0x0 0 rip 0x400733 0x400733 eflags 0x293 [ CF AF SF IF ] cs 0x43 67 ss 0x3b 59 ds *value not available* es *value not available* fs *value not available* gs *value not available* Notice the "*value not available*". Working with that register will yield: (gdb) p $ds $1 = (gdb) p $ds + 1 value is not available I think this is appropriate. The code I'm touching had a comment in place around a similar, but #if 0'd out assertion, suggesting that the targets should themselves always indicate the state of all registers. That would mean going over all code that looks like this: for (i = 0; i < ARRAY_SIZE (amd64fbsd_jmp_buf_reg_offset); i++) { if (amd64fbsd_jmp_buf_reg_offset[i] != -1) { ... regcache_raw_supply (regcache, i, buf); } } and add an else clause that does regcache_raw_supply (regcache, i, NULL); NULL means "unavailable" already. I think that's overkill: we can just handle this in a centralized place, like the patch below. Before the state, this was a bit dangerous, because all you would see is a register with value 0. Now, it's clearly visible and distinct. Any objection? I tested this on amd64-linux, and Andreas Tobler tested this on both amd64- and powerpc-freebsd, all with no regressions. I think (though I haven't tried it) this bit in corelow.c:get_core_registers: /* Mark all registers not found in the core as unavailable. */ for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++) if (regcache_register_status (regcache, i) == REG_UNKNOWN) regcache_raw_supply (regcache, i, NULL); can go away afterwards as now redundant. -- Pedro Alves 2011-03-23 Pedro Alves gdb/ * regcache.c (regcache_raw_read): If the target didn't supply an given raw register, mark it as unavailable. --- gdb/regcache.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) Index: src/gdb/regcache.c =================================================================== --- src.orig/gdb/regcache.c 2011-03-23 14:05:54.000000000 +0000 +++ src/gdb/regcache.c 2011-03-23 14:27:34.892156993 +0000 @@ -586,25 +586,20 @@ regcache_raw_read (struct regcache *regc to the current thread. This switching shouldn't be necessary only there is still only one target side register cache. Sigh! On the bright side, at least there is a regcache object. */ - if (!regcache->readonly_p) + if (!regcache->readonly_p + && regcache_register_status (regcache, regnum) == REG_UNKNOWN) { - if (regcache_register_status (regcache, regnum) == REG_UNKNOWN) - { - struct cleanup *old_chain = save_inferior_ptid (); + struct cleanup *old_chain = save_inferior_ptid (); - inferior_ptid = regcache->ptid; - target_fetch_registers (regcache, regnum); - do_cleanups (old_chain); - } -#if 0 - /* FIXME: cagney/2004-08-07: At present a number of targets - forget (or didn't know that they needed) to set this leading to - panics. Also is the problem that targets need to indicate - that a register is in one of the possible states: valid, - undefined, unknown. The last of which isn't yet - possible. */ - gdb_assert (regcache_register_status (regcache, regnum) == REG_VALID); -#endif + inferior_ptid = regcache->ptid; + target_fetch_registers (regcache, regnum); + do_cleanups (old_chain); + + /* A number of targets can't access the whole set of raw + registers (because the debug API provides no means to get at + them). */ + if (regcache->register_status[regnum] == REG_UNKNOWN) + regcache->register_status[regnum] = REG_UNAVAILABLE; } if (regcache->register_status[regnum] != REG_VALID)