From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9940 invoked by alias); 24 Sep 2013 12:06:52 -0000 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 Received: (qmail 9931 invoked by uid 89); 24 Sep 2013 12:06:52 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 24 Sep 2013 12:06:52 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8OC6ltJ017453 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 24 Sep 2013 08:06:47 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r8OC6j5p032027; Tue, 24 Sep 2013 08:06:45 -0400 Message-ID: <52418054.7040805@redhat.com> Date: Tue, 24 Sep 2013 12:06:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Mark Kettenis CC: gdb-patches@sourceware.org, eliz@gnu.org, aburgess@broadcom.com Subject: Re: [PATCH] Always print call-clobbered registers in outer frames. References: <5200F55E.2050308@broadcom.com> <201308061318.r76DIMdd016369@glazunov.sibelius.xs4all.nl> <5200FECF.7030304@broadcom.com> <201308061541.r76FfYQN022875@glazunov.sibelius.xs4all.nl> <520142D9.4030304@redhat.com> <5208E3C8.7060107@broadcom.com> <5208E938.3080305@redhat.com> <201308122001.r7CK1862007934@glazunov.sibelius.xs4all.nl> <520E7255.7080206@redhat.com> <5211F25A.5070907@broadcom.com> <5228B15F.7060108@redhat.com> <5228B2D8.7060604@broadcom.com> <5237567C.8050406@redhat.com> <5239B2D8.4030403@broadcom.com> <5239CCB3.605@redhat.com> <83zjram6sw.fsf@gnu.org> <201309182047.r8IKlOGA010471@glazunov.sibelius.xs4all.nl> <83fvt1mems.fsf@gnu.org> <523B2D39.8060303@redhat.com> <523B4D48.3050206@redhat.com> <201309201227.r8KCRfgQ006644@glazunov.sibelius.xs4all.nl> In-Reply-To: <201309201227.r8KCRfgQ006644@glazunov.sibelius.xs4all.nl> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-09/txt/msg00848.txt.bz2 On 09/20/2013 01:27 PM, Mark Kettenis wrote: > I strongly disagree with this change. Ack. My thought process was that it was not that much different from defining the value of PC in frames other than #0 to be the return address, not the value the PC had _before_ the call. If we went further and defined all register values in that direction, things would all be consistent. Something like: The values of registers in the outer frame, are defined as being the values the registers would have if the inner frame was to return immediately, respecting relevant ABI rules. IOW, call-clobbered registers would show whatever value's current in the inner frame, and callee-saved registers would show the saved values. I think _both_ definitions are reasonable, though this one seemed a little more consistent to me. > I think it is useful to show that rgeisters have not been saved. Right, but that could also be done in some other way. I toyed with something like Doug suggested. That is, make "info reg" indicate whether the register was actually saved or GDB assumed same_value. Something like: (gdb) i reg rax 0x1 1 [not saved] rbx 0x0 0 rcx 0x2 2 [not saved] rdx 0x3 3 [not saved] rsi 0x4 4 [not saved] rdi 0x5 5 [not saved] rbp 0x7fffffffda70 0x7fffffffda70 rsp 0x7fffffffda40 0x7fffffffda40 So GDB would still "flatten" the registers, and "p $rax" would still print some number, and the user could tell that the registers' values might no longer be the values the frame had when it called out the inner frame. > If you want to look at their > value anyway, you can always go down (or up) the stack and chase the > value of the register yourself. Of course. Though not very convenient. If that's the direction we're heading, and if GDB gets the saved/not-saved wrong, that may be a nuisance. It might be useful to have shorthand for that (though admittedly the use cases will probably trigger so seldom that we may never hear requests for it). > > I also think it doesn't make sense for the varobj subsystem to > reimplement bits of the unwinder subsystem. You mean value machinery, I guess. I wouldn't say reimplement. The idea was to still leave the unwinder treating not-saved as is, and only hide that at the user-facing presentation layer, so that anything that might need to rely on the unwinder machinery getting info about registers at inner frame call time, wouldn't get possibly no-longer-correct register values unless it wanted to. (IOW, we could move the value_of_register function to stack.c and call it part of the unwinder machinery as well.) > If newer GCC versions are even less explicit about saving or not > saving registers than older versions, we should probably improve the > _dwarf2_frame_init_reg() implementations to indicate properly which > registers are caller-saved and which registers are callee-saved. So... I tried that out for AMD64, on top of the patch. (top-gdb) i reg rax 0x5e60ff 6185215 rbx 0x0 0 rcx 0x323d435bf0 215776189424 rdx 0x7fffffffda50 140737488345680 rsi 0x0 0 rdi 0x7fffffffda50 140737488345680 rbp 0x7fffffffd9a0 0x7fffffffd9a0 rsp 0x7fffffffd860 0x7fffffffd860 r8 0x70 112 r9 0x100000 1048576 r10 0x8 8 r11 0x246 582 r12 0x45aed0 4566736 r13 0x7fffffffdb50 140737488345936 r14 0x0 0 r15 0x0 0 rip 0x5e6114 0x5e6114 eflags 0x206 [ PF IF ] cs 0x33 51 ss 0x2b 43 ds 0x0 0 es 0x0 0 fs 0x0 0 gs 0x0 0 (top-gdb) up #1 0x00000000005e3876 in catch_errors (func=0x5e60ff , func_args=0x7fffffffda50, errstring=0x8a6194 "", mask=RETURN_MASK_ALL) at ../../src/gdb/exceptions.c:524 524 val = func (func_args); (top-gdb) i reg rax rbx 0x0 0 rcx rdx rsi rdi rbp 0x7fffffffda10 0x7fffffffda10 rsp 0x7fffffffd9b0 0x7fffffffd9b0 r8 r9 r10 r11 r12 0x45aed0 4566736 r13 0x7fffffffdb50 140737488345936 r14 0x0 0 r15 0x0 0 rip 0x5e3876 0x5e3876 eflags cs ss ds es fs gs The patch also teaches the heuristic unwinder about call-clobbered registers, though I wonder whether we should be assuming much about the ABI around assembly or no-debug-info functions. Hmm. Argh, this thread is getting quite confusing with the back and forth. Sorry about that. --- gdb/amd64-tdep.c | 54 +++++++++++++++++++++++++- gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp | 31 +++++++++++++-- gdb/testsuite/gdb.mi/mi-reg-undefined.exp | 43 ++++++++++++++++---- 3 files changed, 115 insertions(+), 13 deletions(-) diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index 3ab74f0..3ab6058 100644 --- a/gdb/amd64-tdep.c +++ b/gdb/amd64-tdep.c @@ -40,6 +40,7 @@ #include "exceptions.h" #include "amd64-tdep.h" #include "i387-tdep.h" +#include "dwarf2-frame.h" #include "features/i386/amd64.c" #include "features/i386/amd64-avx.c" @@ -1744,6 +1745,28 @@ amd64_alloc_frame_cache (void) return cache; } +/* Return true if the ABI specifies REGNUM as a callee-saved + register. */ + +static int +amd64_register_callee_saved (int regnum) +{ + if (regnum == AMD64_RBP_REGNUM + || regnum == AMD64_RBX_REGNUM + || (AMD64_R12_REGNUM <= regnum && regnum <= AMD64_R15_REGNUM)) + return 1; + + /* The control bits of the MXCSR register are callee-saved. */ + if (regnum == AMD64_MXCSR_REGNUM) + return 1; + + /* The x87 control word is callee-saved. */ + if (regnum == AMD64_FCTRL_REGNUM) + return 1; + + return 0; +} + /* GCC 4.4 and later, can put code in the prologue to realign the stack pointer. Check whether PC points to such code, and update CACHE accordingly. Return the first instruction after the code @@ -2423,7 +2446,10 @@ amd64_frame_prev_register (struct frame_info *this_frame, void **this_cache, return frame_unwind_got_memory (this_frame, regnum, cache->saved_regs[regnum]); - return frame_unwind_got_register (this_frame, regnum, regnum); + if (amd64_register_callee_saved (regnum)) + return frame_unwind_got_register (this_frame, regnum, regnum); + else + return frame_unwind_got_optimized (this_frame, regnum); } static const struct frame_unwind amd64_frame_unwind = @@ -2846,6 +2872,30 @@ static const int amd64_record_regmap[] = AMD64_DS_REGNUM, AMD64_ES_REGNUM, AMD64_FS_REGNUM, AMD64_GS_REGNUM }; +/* Implement the "init_reg" dwarf2_frame_ops method. */ + +static void +amd64_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, + struct dwarf2_frame_state_reg *reg, + struct frame_info *this_frame) +{ + /* Mark the PC as the destination for the return address. */ + if (regnum == gdbarch_pc_regnum (gdbarch)) + reg->how = DWARF2_FRAME_REG_RA; + + /* Mark the stack pointer as the call frame address. */ + else if (regnum == gdbarch_sp_regnum (gdbarch)) + reg->how = DWARF2_FRAME_REG_CFA; + + /* The above was taken from the default init_reg in dwarf2-frame.c + while the below is AMD64 specific. */ + + else if (amd64_register_callee_saved (regnum)) + reg->how = DWARF2_FRAME_REG_SAME_VALUE; + else + reg->how = DWARF2_FRAME_REG_UNDEFINED; +} + void amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) { @@ -2948,6 +2998,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) frame_unwind_append_unwinder (gdbarch, &amd64_frame_unwind); frame_base_set_default (gdbarch, &amd64_frame_base); + dwarf2_frame_set_init_reg (gdbarch, amd64_dwarf2_frame_init_reg); + /* If we have a register mapping, enable the generic core file support. */ if (tdep->gregset_reg_offset) set_gdbarch_regset_from_core_section (gdbarch, diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp index 44bf8e9..c11b007 100644 --- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp +++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp @@ -44,11 +44,24 @@ for {set f 0} {$f < 3} {incr f} { set pattern_rax_rbx_rcx_info "$hex\\s+$decimal" set pattern_r8_r9_print "$hex" set pattern_r8_r9_info "$hex\\s+$decimal" + set pattern_r12_r15_print "$hex" + set pattern_r12_r15_info "$hex\\s+$decimal" } else { + # Even though rbx is callee-saved, these are all explicitly + # marked undefined in the debug info. set pattern_rax_rbx_rcx_print "" set pattern_rax_rbx_rcx_info "" - set pattern_r8_r9_print "$hex" - set pattern_r8_r9_info "$hex\\s+$decimal" + + # ABI specifies these as caller-saved. GDB should be + # implicitly aware they're not saved, even if the debug info + # doesn't explicitly say so (and also doesn't say they were + # actually saved). + set pattern_r8_r9_print "" + set pattern_r8_r9_info "" + + # ABI specifies these as callee-saved. + set pattern_r12_r15_print "$hex" + set pattern_r12_r15_info "$hex\\s+$decimal" } # Select frame. @@ -64,8 +77,18 @@ for {set f 0} {$f < 3} {incr f} { gdb_test "p/x \$r8" "$pattern_r8_r9_print" "print \$r8 in frame ${f}" gdb_test "p/x \$r9" "$pattern_r8_r9_print" "print \$r9 in frame ${f}" + gdb_test "p/x \$r12" "$pattern_r12_r15_print" "print \$r12 in frame ${f}" + gdb_test "p/x \$r15" "$pattern_r12_r15_print" "print \$r15 in frame ${f}" # Display register values. - gdb_test "info registers rax rbx rcx r8 r9" "rax\\s+${pattern_rax_rbx_rcx_info}\\s*\r\nrbx\\s+${pattern_rax_rbx_rcx_info}\\s*\r\nrcx\\s+${pattern_rax_rbx_rcx_info}\\s*\r\nr8\\s+${pattern_r8_r9_info}\\s*\r\nr9\\s+${pattern_r8_r9_info}\\s*" \ - "Check values of rax, rbx, rcx, r8, r9 in frame ${f}" + send_gdb "info registers rax rbx rcx r8 r9 r12 r15\n" + gdb_expect_list "info registers in frame ${f}" "$gdb_prompt $" \ + [list \ + "rax\\s+$pattern_rax_rbx_rcx_info\r\n" \ + "rbx\\s+$pattern_rax_rbx_rcx_info\r\n" \ + "rcx\\s+$pattern_rax_rbx_rcx_info\r\n" \ + "r8\\s+$pattern_r8_r9_info\r\n" \ + "r9\\s+$pattern_r8_r9_info\r\n" \ + "r12\\s+$pattern_r12_r15_info\r\n" \ + "r15\\s+$pattern_r12_r15_info\r\n" ] } diff --git a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp index cb3a716..64fa581 100644 --- a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp +++ b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp @@ -52,20 +52,47 @@ mi_gdb_test "111-stack-list-frames" \ "111\\^done,stack=\\\[frame=\{level=\"0\",addr=\"$hex\",func=\"stop_frame\",.*\},frame=\{level=\"1\",addr=\"$hex\",func=\"first_frame\",.*\},frame=\{level=\"2\",addr=\"$hex\",func=\"main\",.*\}\\\]" \ "stack frame listing" -set not_saved_pattern "" - for {set f 0} {$f < 3} {incr f} { if {${f} == 0} { - set pattern_0_1_2 ${hex} + set pattern ${hex} } else { - set pattern_0_1_2 ${not_saved_pattern} + set pattern "" } - mi_gdb_test "22${f}-data-list-register-values --thread 1 --frame ${f} x 0 1 2 8 9" \ - "22${f}\\^done,register-values=\\\[\{number=\"0\",value=\"${pattern_0_1_2}\"\},\{number=\"1\",value=\"${pattern_0_1_2}\"\},\{number=\"2\",value=\"${pattern_0_1_2}\"\},\{number=\"8\",value=\"$hex\"\},\{number=\"9\",value=\"$hex\"\}\\\]" \ + # - rax,rbx,rcx are explicitly marked undefined in the debug info, + # so GDB should display them all as not saved, even though rbx + # is callee-saved. + # + # - r8-r9 are caller-saved. GDB should be implicitly aware + # they're not saved, even if the debug info doesn't explicitly + # say so (and also doesn't say they were actually saved). + # + # - r12-r15 are callee-saved. GDB should be able to find their + # values. + + mi_gdb_test "22${f}-data-list-register-values --thread 1 --frame ${f} x 0 1 2 8 9 12 15" \ + [join [list \ + "22${f}\\^done,register-values=\\\["\ + "\{number=\"0\",value=\"${pattern}\"\}," \ + "\{number=\"1\",value=\"${pattern}\"\}," \ + "\{number=\"2\",value=\"${pattern}\"\}," \ + "\{number=\"8\",value=\"${pattern}\"\}," \ + "\{number=\"9\",value=\"${pattern}\"\}," \ + "\{number=\"12\",value=\"$hex\"\}," \ + "\{number=\"15\",value=\"$hex\"\}" \ + "\\\]" ] "" ] \ "register values, format x, frame ${f}" - mi_gdb_test "33${f}-data-list-register-values --thread 1 --frame ${f} r 0 1 2 8 9" \ - "33${f}\\^done,register-values=\\\[\{number=\"0\",value=\"${pattern_0_1_2}\"\},\{number=\"1\",value=\"${pattern_0_1_2}\"\},\{number=\"2\",value=\"${pattern_0_1_2}\"\},\{number=\"8\",value=\"$hex\"\},\{number=\"9\",value=\"$hex\"\}\\\]" \ + mi_gdb_test "33${f}-data-list-register-values --thread 1 --frame ${f} r 0 1 2 8 9 12 15" \ + [join [list \ + "33${f}\\^done,register-values=\\\["\ + "\{number=\"0\",value=\"${pattern}\"\}," \ + "\{number=\"1\",value=\"${pattern}\"\}," \ + "\{number=\"2\",value=\"${pattern}\"\}," \ + "\{number=\"8\",value=\"${pattern}\"\}," \ + "\{number=\"9\",value=\"${pattern}\"\}," \ + "\{number=\"12\",value=\"$hex\"\}," \ + "\{number=\"15\",value=\"$hex\"\}" \ + "\\\]" ] "" ] \ "register values, format r, frame ${f}" } -- 1.7.11.7