From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31427 invoked by alias); 15 Mar 2004 17:12:15 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 31390 invoked from network); 15 Mar 2004 17:12:10 -0000 Received: from unknown (HELO localhost.redhat.com) (66.30.197.194) by sources.redhat.com with SMTP; 15 Mar 2004 17:12:10 -0000 Received: from gnu.org (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 94C9C2B92; Mon, 15 Mar 2004 12:12:09 -0500 (EST) Message-ID: <4055E3E9.5020900@gnu.org> Date: Fri, 19 Mar 2004 00:09:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-GB; rv:1.4.1) Gecko/20040217 MIME-Version: 1.0 To: Ulrich Weigand Cc: gdb-patches@sources.redhat.com Subject: [commit] Don't use STEP_FRAME_ID; Was: [PATCH] Fix signals.exp test case on S/390 References: <200403111711.SAA06213@faui1d.informatik.uni-erlangen.de> In-Reply-To: <200403111711.SAA06213@faui1d.informatik.uni-erlangen.de> Content-Type: multipart/mixed; boundary="------------090407020308050409060100" X-SW-Source: 2004-03/txt/msg00313.txt.bz2 Message-ID: <20040319000900.D7xa71CcW6aQsdQGahm7F8rwBc5tqIYG8y5pVfd9pC4@z> This is a multi-part message in MIME format. --------------090407020308050409060100 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3136 I've committted the attached. As Ulrich explains in the below, trying to use STEP_FRAME_ID was creating a big nasty rat hole. The patch does preserve the old behavior when legacy_frame_p (hmm, which must be on its last legs :-). > Andrew Cagney wrote: > >>>> > It will run into the first if, and simply use step_frame_id, >>>> > which is wrong in this case. That's why my patch add another >>>> > condition to the first if, to make it not taken and actually >>>> > use the (correct) get_prev_frame case. >> >>> >>> Where is step_frame_id pointing? > > > To the function that was interrupted by the signal (i.e. the > function where I entered 'next'). Good. >>> Anyway, I think this code: >>> > if (frame_id_p (step_frame_id) >>> > && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc)) >>> > /* NOTE: cagney/2004-02-27: Use the global state's idea of the >>> > stepping frame ID. I suspect this is done as it is lighter >>> > weight than a call to get_prev_frame. */ >>> > sr_id = step_frame_id; >>> should simply be deleted. I wondered about it and you've just confirmed >>> my suspicions. With that code gone is half the problem solved? > > > Yes, deleting this works just fine for me, in fact ... Good. >>> That leaves the other problem, which is much harder :-( > > > ... it even solves the other problem as well! Yow! > The reason for this is that the whole problematic if > that uses frame_id_inner becomes irrelevant: > > if (pc_in_sigtramp (stop_pc) > && frame_id_inner (step_frame_id, > frame_id_build (read_sp (), 0))) > /* We stepped out of a signal handler, and into its > calling trampoline. This is misdetected as a > subroutine call, but stepping over the signal > trampoline isn't such a bad idea. In order to do that, > we have to ignore the value in step_frame_id, since > that doesn't represent the frame that'll reach when we > return from the signal trampoline. Otherwise we'll > probably continue to the end of the program. */ > step_frame_id = null_frame_id; > > step_over_function (ecs); > > With those lines in step_over_function deleted, step_over_function > does not care about step_frame_id at all any more, and thus there > is no need to fiddle with step_frame_id here ... >>>> > Finally, the patch below reintroduces a pc_in_sigtramp >>>> > gdbarch callback to s390-tdep.c; I had thought this would >>>> > be no longer necessary when using the new frame code, but >>>> > apparently there's still other users ... >> >>> >>> Yes, it shouldn't be needed. get_frame_type == SIGTRAMP_FRAME is >>> sufficient. work-in-progress. > > > Actually, when deleting the lines in step_over_function, it turns > out that I don't need pc_in_sigtramp any more ... > > Summing up: after completely reverting my patch, and simply > deleting those lines, I get a gdb that passes signals.exp > (and has no test suite regressions), and also handles stepping > out of a signal handler correctly. ya! thanks, Andrew --------------090407020308050409060100 Content-Type: text/plain; name="diffs" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="diffs" Content-length: 5500 Index: ChangeLog 2004-03-15 Andrew Cagney * infrun.c (handle_step_into_function, step_over_function): Only update and use STEP_FRAME_ID when the system is using legacy frames. Update comments. Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.139 diff -u -r1.139 infrun.c --- infrun.c 9 Mar 2004 16:40:08 -0000 1.139 +++ infrun.c 15 Mar 2004 16:58:35 -0000 @@ -1264,17 +1264,22 @@ { /* We're doing a "next". */ - if (pc_in_sigtramp (stop_pc) + if (legacy_frame_p (current_gdbarch) + && pc_in_sigtramp (stop_pc) && frame_id_inner (step_frame_id, frame_id_build (read_sp (), 0))) - /* We stepped out of a signal handler, and into its - calling trampoline. This is misdetected as a - subroutine call, but stepping over the signal - trampoline isn't such a bad idea. In order to do that, - we have to ignore the value in step_frame_id, since - that doesn't represent the frame that'll reach when we - return from the signal trampoline. Otherwise we'll - probably continue to the end of the program. */ + /* NOTE: cagney/2004-03-15: This is only needed for legacy + systems. On non-legacy systems step_over_function doesn't + use STEP_FRAME_ID and hence the below update "hack" isn't + needed. */ + /* We stepped out of a signal handler, and into its calling + trampoline. This is misdetected as a subroutine call, but + stepping over the signal trampoline isn't such a bad idea. + In order to do that, we have to ignore the value in + step_frame_id, since that doesn't represent the frame + that'll reach when we return from the signal trampoline. + Otherwise we'll probably continue to the end of the + program. */ step_frame_id = null_frame_id; step_over_function (ecs); @@ -2868,11 +2873,10 @@ However, if the callee is recursing, we want to be careful not to catch returns of those recursive calls, but only of THIS instance - of the call. + of the caller. To do this, we set the step_resume bp's frame to our current - caller's frame (step_frame_id, which is set by the "next" or - "until" command, before execution begins). */ + caller's frame (obtained by doing a frame ID unwind). */ static void step_over_function (struct execution_control_state *ecs) @@ -2923,24 +2927,40 @@ check_for_old_step_resume_breakpoint (); - if (frame_id_p (step_frame_id) - && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc)) - /* NOTE: cagney/2004-02-27: Use the global state's idea of the - stepping frame ID. I suspect this is done as it is lighter - weight than a call to get_prev_frame. */ - sr_id = step_frame_id; - else if (legacy_frame_p (current_gdbarch)) - /* NOTE: cagney/2004-02-27: This is the way it was 'cos this is - the way it always was. It should be using the unwound (or - caller's) ID, and not this (or the callee's) ID. It appeared - to work because: legacy architectures used the wrong end of the - frame for the ID.stack (inner-most rather than outer-most) so - that the callee's id.stack (un adjusted) matched the caller's - id.stack giving the "correct" id; more often than not - !IN_SOLIB_DYNSYM_RESOLVE_CODE and hence the code above (it was - originally later in the function) fixed the ID by using global - state. */ - sr_id = get_frame_id (get_current_frame ()); + /* NOTE: cagney/2004-03-15: Code using the current value of + "step_frame_id", instead of unwinding that frame ID, removed (at + least for non-legacy platforms). On s390 GNU/Linux, after taking + a signal, the program is directly resumed at the signal handler + and, consequently, the PC would point at at the first instruction + of that signal handler but STEP_FRAME_ID would [incorrectly] at + the interrupted code when it should point at the signal + trampoline. By always and locally doing a frame ID unwind, it's + possible to assert that the code is always using the correct + ID. */ + if (legacy_frame_p (current_gdbarch)) + { + if (frame_id_p (step_frame_id) + && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc)) + /* NOTE: cagney/2004-02-27: Use the global state's idea of the + stepping frame ID. I suspect this is done as it is lighter + weight than a call to get_prev_frame. */ + /* NOTE: cagney/2004-03-15: See comment above about how this + is also broken. */ + sr_id = step_frame_id; + else + /* NOTE: cagney/2004-03-15: This is the way it was 'cos this + is the way it always was. It should be using the unwound + (or caller's) ID, and not this (or the callee's) ID. It + appeared to work because: legacy architectures used the + wrong end of the frame for the ID.stack (inner-most rather + than outer-most) so that the callee's id.stack (un + adjusted) matched the caller's id.stack giving the + "correct" id; more often than not + !IN_SOLIB_DYNSYM_RESOLVE_CODE and hence the code above (it + was originally later in the function) fixed the ID by using + global state. */ + sr_id = get_frame_id (get_current_frame ()); + } else sr_id = get_frame_id (get_prev_frame (get_current_frame ())); --------------090407020308050409060100--