From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10199 invoked by alias); 24 Nov 2010 12:43:18 -0000 Received: (qmail 10128 invoked by uid 22791); 24 Nov 2010 12:43:13 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_FC,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, 24 Nov 2010 12:43:03 +0000 Received: (qmail 32023 invoked from network); 24 Nov 2010 12:42:57 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 24 Nov 2010 12:42:57 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch 3/3] Fix stale tp->step_resume_breakpoint Date: Wed, 24 Nov 2010 12:43:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.33-29-realtime; KDE/4.4.5; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <20101124005034.GC7263@host0.dyn.jankratochvil.net> In-Reply-To: <20101124005034.GC7263@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201011241242.54099.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: 2010-11/txt/msg00390.txt.bz2 On Wednesday 24 November 2010 00:50:34, Jan Kratochvil wrote: > Hi, > > this is a follow-up to: > Re: [patch] Fix stale tp->step_resume_breakpoint > http://sourceware.org/ml/gdb-patches/2010-11/msg00011.html Thanks. > On Tue, 02 Nov 2010 02:05:18 +0100, Pedro Alves wrote: > > I think this raises an obvious question, and hints at > > a larger issue: if you find you you need to tuck away step_resume_breakpoint, > > then, how come you don't need to do the same for all the other execution > > command state? (step_range_start, step_range_end, step_frame_id, > > continuations, etc.). > > Some of them were already saved, just not into `struct inferior_thread_state' > but into `struct inferior_status'. So extended now further the latter. A-ha. > I find these two redundant, they could be merged? I did not try to. > This could be a different patch. Yeah, there's a bunch of duplication going around here, but these two do serve a slightly different purpose, as mentioned in the comment above each. But I guess they could be merged if the logic of what-to-restore-and-when is preserved. No opinion. There's a lot of duplication between inferior_status and thread_info as well. I've though before about a new structure that stores execution command state (what gdb is doing with the thread, infcmd + handle_inferior_event state machine status), e.g. (chosen names are only for illustration): struct exec_command_status { ... CORE_ADDR step_range_start; CORE_ADDR step_range_end; struct frame_id step_frame_id; struct frame_id step_stack_frame_id; ... }; and then making struct thread_info embed a field of those, instead of all the the field duplication. struct thread_info { ptid_t ptid; ... execution_command_status cmdstate; }; So when doing an infcall, you'd save/restore this object, which should make it a little harder to forget some field. > +++ b/gdb/testsuite/gdb.base/step-resume-infcall.c > @@ -0,0 +1,45 @@ (...) > +#include > + > +int > +cond (void) > +{ > + puts ("cond-hit"); Can you make the test not rely on stdio, please? Say, instead increment a global whenever this function is called, and check its value later? This is because not all remote targets support remote file io, and gdbserver does not. That is, this puts appears on gdbserver's terminal, not gdb's. This is the reason several tests are skipped if "target_info exists gdb,noinferiorio" is set. It's always better if we can avoid io in the first place, as it gives broader coverage. Otherwise looks good. Thanks again. -- Pedro Alves