This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch 3/3] Fix stale tp->step_resume_breakpoint


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 <stdio.h>
> +
> +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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]