From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9749 invoked by alias); 4 Dec 2003 23:24:10 -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 9741 invoked from network); 4 Dec 2003 23:24:08 -0000 Received: from unknown (HELO takamaka.act-europe.fr) (142.179.108.108) by sources.redhat.com with SMTP; 4 Dec 2003 23:24:08 -0000 Received: by takamaka.act-europe.fr (Postfix, from userid 507) id 5A2DA47D61; Thu, 4 Dec 2003 15:24:10 -0800 (PST) Date: Thu, 04 Dec 2003 23:24:00 -0000 From: Joel Brobecker To: Andrew Cagney Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] OSF/1 - "next" over prologueless function call Message-ID: <20031204232410.GI1652@gnat.com> References: <20031202042646.GW1186@gnat.com> <3FCD6468.9020708@gnu.org> <20031204005521.GD716@gnat.com> <3FCE92A1.6010007@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3FCE92A1.6010007@gnu.org> User-Agent: Mutt/1.4i X-SW-Source: 2003-12/txt/msg00156.txt.bz2 > (only took 15 years to realise how "obvious" it was :-) Right. It's only "obvious" because the we now have a frame ID which particularity is to be unique... Given how long it must have taken to "invent the wheel", maybe we didn't do too bad :-). > >However, I tried it on sparc-solaris with the gdb-6.0 sources because > >I knew this target hasn't transitioned to the new frame framework. > >It doesn't look like GDB is liking this change there (I've got a lot of > >timeouts in call-ar-st). I am currently retrying on the head right now, > >hoping the testsuite completes in a reasonable amount of time. I redid the testing this time using the head, and found no new regression introduced by this change [1]. > >--- infrun.c 25 Nov 2003 16:01:36 -0000 1.122 > >+++ infrun.c 3 Dec 2003 19:24:07 -0000 > >@@ -2473,6 +2473,8 @@ process_event_stop_test: > > } > > > > if (((stop_pc == ecs->stop_func_start /* Quick test */ > >+ || frame_id_eq (get_frame_id (get_prev_frame (get_current_frame > >())), > >+ step_frame_id) > > || in_prologue (stop_pc, ecs->stop_func_start)) > > && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name)) > > || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name) > You can use legacy_frame_p for differentiating old and new code. Yes. Despite the relative success on sparc-solaris (no new regression), I wasn't sure whether this wasn't potentially introducing new problems on the architectures where the legacy frames are still used. So I added it to the condition. That gives us a pretty horrible condition, something like that: if (((stop_pc == ecs->stop_func_start /* Quick test */ || (!legacy_frame_pd (current_gdbarch) && frame_id_eq (get_frame_id (get_prev_frame (get_current_frame ())), step_frame_id)) || in_prologue (stop_pc, ecs->stop_func_start)) && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name)) || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name) || ecs->stop_func_name == 0) How about we move this into a function? My current version only replaces the condition I added by: || frame_in_procedure_called_during_step () So the condition stay relatively readable (not too many levels in terms of nesting). But maybe it's time to turn the entire condition into a function and add more comments. Something like this? static int inside_function_called_from_step (CORE_ADDR stop_pc, CORE_ADDR stop_func_start) { /* If bla bla bla */ if (stop_pc == stop_func_start && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, stop_func_name)) return 1; /* If the frame ID of the previous frame is equal to step_frame_id, then this function was indeed called during a step. Only rely on this test if we don't use the legacy frames. */ if (!legacy_frame_p (current_gdbarch) && frame_id_eq (...) && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, stop_func_name)) return 1; /* cagney/2003-12-04: Do we really need this, now that we check the previous frame id? */ if (in_prologue (stop_pc, ecs->stop_func_start)) && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, stop_func_name)) return 1; /* Don't stop stepping inside a solib call trampoline. */ if (IN_SOLIB_CALL_TRAMPOLINE (stop_pc, stop_func_name)) return 1; /* Hmmm, continue stepping if we don't know anything about where we landed. */ if (stop_func_name == 0) return 1; return 0; } The trouble is that this change is not completly mechanical. And we also end up repeating the "!IN_SOLIB_CALL_TRAMPOLINE" test. We can also coalesce the first 3 tests into one if, but then we lose a bit the advantage of using a function. Anyway, it's more a matter of style, so I will defer to the preference of the maintainers, and send a patch along these lines. If we decide to move the entire condition into a new function, I can send 2 patches: One that moves the current condition, and then another that fixes the osf1 problem. > Hmm, is "stop_pc == ecs->stop_func_start" a valid test, what happens if > the program is at 1: and there's a next? > > foo: > ... > 1: goto foo Unless the function is completely prologueless, all should be well because the goto will branch at a location past the first instruction. If the function is prologueless, however... I think this would be a rare occurrence. > Hmm, is in_prologue() adding value when frame_id always works? Unless > it's being used to handle stepping through a prologue? Not sure about the real intent of in_prologue(). I know it saved us most of the time in the osf/1 case, but was it the primary intent when this condition was added? I think that once we have answered this question, we will be able to decide whether or not this condition can be removed. I also realize that I may not have been clear about the usage of legacy_frame_p. I am a bit nervous at doing without, but you may be more confident than me, since I don't have much knowledge of the new frame architecture yet. So the choice of using it or not is still open. Thanks, -- Joel [1]: There was one glitch that occurred during the testing, though. Even before I applied any change, the structs.exp test keeps timeouting. I tried letting it complete, but had to stop it after an entire night of timeouts. So I did my testing after having deactivated this test.