From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16023 invoked by alias); 11 Mar 2004 16:08:36 -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 16014 invoked from network); 11 Mar 2004 16:08:31 -0000 Received: from unknown (HELO localhost.redhat.com) (216.129.200.20) by sources.redhat.com with SMTP; 11 Mar 2004 16:08:31 -0000 Received: from gnu.org (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 57F232B99; Thu, 11 Mar 2004 11:07:54 -0500 (EST) Message-ID: <40508EDA.1080502@gnu.org> Date: Thu, 11 Mar 2004 16:08: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: Re: [PATCH] Fix signals.exp test case on S/390 References: <200403110007.BAA05694@faui1d.informatik.uni-erlangen.de> In-Reply-To: <200403110007.BAA05694@faui1d.informatik.uni-erlangen.de> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2004-03.o/txt/msg00268.txt Message-ID: <20040311160800.YFKbgUiqzKwwijzSWpoflIVFZMCKfg7IBRQDw7pL7yU@z> > Andrew Cagney wrote: > > >>>I'm lost here. What happens with: >>> >>>- get_frame_id (get_prev_frame (signal handler)) >>>- get_frame_id (sigreturn trampoline) >>> >>>They should match > > > Well, they do match, that's why things work with my patch. Ok, good. > The problem is that without my patch, step_over_function > doesn't actually *use* get_frame_id (get_prev_frame ...), > see the 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; > 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 ()); > else > sr_id = get_frame_id (get_prev_frame (get_current_frame ())); > > 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? 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? That leaves the other problem, which is much harder :-( > *************** handle_step_into_function (struct execut > *** 1265,1272 **** > /* We're doing a "next". */ > > 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 > --- 1265,1271 ---- > /* We're doing a "next". */ > > if (pc_in_sigtramp (stop_pc) > ! && INNER_THAN (step_sp, read_sp ())) > /* We stepped out of a signal handler, and into its > calling trampoline. This is misdetected as a > subroutine call, but stepping over the signal Both INNER_THAN and frame_id_build(read_sp (),0) / frame_id_inner are wrong here. They test variations on: /* Returns non-zero when L is strictly inner-than R (they have different frame .bases). Neither L, nor R can be `null'. See note above about frameless functions. */ ... /* Methods for constructing and comparing Frame IDs. NOTE: Given stackless functions A and B, where A calls B (and hence B is inner-to A). The relationships: !eq(A,B); !eq(B,A); !inner(A,B); !inner(B,A); all hold. This is because, while B is inner-to A, B is not strictly inner-to A. Being stackless, they have an identical .stack_addr value, and differ only by their unordered .code_addr and/or .special_addr values. Because frame_id_inner is only used as a safety net (e.g., detect a corrupt stack) the lack of strictness is not a problem. Code needing to determine an exact relationship between two frames must instead use frame_id_eq and frame_id_unwind. For instance, in the above, to determine that A stepped-into B, the equation "A.id != B.id && A.id == id_unwind (B)" can be used. */ and that isn't sufficient. It's easy to construct a situtation where the handler and sigtramp have the same ID/SP. A better strategy here might be to, if the user isn't instruction stepping or cntrl-ced, single-step the inferior until it finds its way out of the sigtramp and then make a decision. Andrew PS: > 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.