Re: [PATCH 4/4] Introduce class target_stack

[Pedro Alves <palves@redhat.com>]

On 05/29/2018 04:13 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> The problem then is in finding a target's "beneath" target, if we
> Pedro> consider that for some target_ops types, we'll be sharing a
> Pedro> single target_ops instance between several inferiors.  For
> Pedro> example, so far, I found no need to have multiple instances of
> Pedro> the spu_multiarch_target / exec_target / dummy_target targets.
> 
> Is it the case that sometimes a particular target_ops instance will have
> to be shared among multiple target stacks?
> 
> If so, then this all makes sense to me.

Right, it is the case.  That is true for targets that support
multi-process.  E.g., native targets and remote targets.

The current model in the branch is:

- Each inferior gets its own target stack.  

- There's only ever one instance of the native target_ops.
  All native processes/inferiors use the same target_ops, because
  it's the same ptrace API "instance" that talks to all of them.
  For example, if you do "info os processes" to list all processes,
  it's the single native target_ops instance that implements that
  call.

- There's one remote_target (target_ops) instance for each
  remote connection.  All processes/inferiors debugged via that
  connection share the same remote_target instance.
  You can have multiple remote connections.

- Each process_stratum target_ops instance has its own PID number
  space.  IOW, a process is uniquely identified by the
  (process_stratum target_ops *, int PID) tuple.


Since each inferior has its own target stack, we should be able to
do things like activate "target record full" for inferior 1, and
"target record btrace" for inferior 2, though I haven't done that.


> 
> Pedro> +/* The target stack.  */
> Pedro> +static target_stack g_target_stack;
> 
> As an aside, I often wonder about blank lines between comments and
> definitions.
> 
> I prefer not to have them, though not for any actual reason.  But some
> spots have them and some do not, so in practice what I do is either
> follow the local style; or leave the blank line out if I think I can; or
> else feel guilty since I vaguely recall there was a rule to have one and
> so put it in even though I'd rather not.

I think the main rule is: blank line in definition, no blank line
in declarations:

 https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Documenting_Your_Code

That's what I think most newer code follows, but it's like you say,
in reality I guess most folks follow the local style.

I've fixed that case you pointed out.

The case in our codebase where I most tend to find blank lines odd,
is in the documentation of structure fields:

struct S
{
  /* This is foo.  */

  int foo;

  /* This is bar.  */

  int bar;
};

I think I dislike it because that way you lose the visual grouping
between comment and field that you otherwise have with:

struct S
{
  /* This is foo.  */
  int foo;

  /* This is bar.  */
  int bar;
};

Curiously, this case that is not explicitly specified in the wiki,
though the struct example there does not include spaces.

> 
> Pedro> +   Note that rather than allow an empty stack, we always have the
> Pedro> +   dummy target at the bottom stratum, so we can call the function
> Pedro> +   vectors without checking them.  */
> 
> Probably just "methods" rather than "function vectors".

Indeed, probably less confusing to newcomers.  That was just
copied over from elsewhere, but I'll adjust it.

Thanks,
Pedro Alves