Re: [PATCH 02/23] Don't rely on inferior_ptid in record_full_wait

Simon Marchi
2020-08-01T22:56:16Z

On 2020-08-01 4:46 p.m., Tom Tromey wrote:
> Simon> I don't know if this makes sense: you tell the core of GDB that some
> Simon> process was signalled (terminated by a signal), but you don't tell
> Simon> what process it is. I am not sure what the core of GDB can do with
> Simon> this information.
>
> Simon> The contract that the `wait` implementations must respect is not
> Simon> very well documented I believe, so it's not clear which value is
> Simon> valid with which event.
>
> Yeah. I think it would be good to fix this.
>
> The reason I had it return minus_one_ptid is this text in
> target/target.h:
>
> Return pid of child, or -1 in case of error;
> store status through argument pointer STATUS.

That (the -1 part) seems erroneous, because...

>
> remote_target::wait does this as well, though perhaps not with this
> target waitkind, e.g.:
>
> status->kind = TARGET_WAITKIND_NO_RESUMED;
> return minus_one_ptid;
>
> or
>
> if (status->kind == TARGET_WAITKIND_NO_RESUMED)
> return minus_one_ptid;

... TARGET_WAITKIND_NO_RESUMED is an example of waitkind that doesn't have any
ptid attached to it. It means "there is no resumed thread, so I won't have any
event to report", so it's not tied to a ptid.

AFAIK, the wait implementations don't really have a way to report "errors". That's
probably fine, because I am not sure what the core could really do about it.

> ... though not all returns set the kind.

Do you have an example? I think the kind is mandatory, because that's what the core
checks right when wait returns (see the do_wait lambda in do_target_wait). If the
target doesn't set the kind, then the core will read whatever was in there before.
Probably 0, since we memset `ecs` in fetch_inferior_event (don't know about other
code paths), which maps to TARGET_WAITKIND_EXITED.

Anyway, not setting the kind sounds like a bug to me, and this is something we should
be able to assert.

> Simon> Note that this waitpid call is blocking (and there's no async stuff in
> Simon> inf-ptrace.c), so I presume that this is only used in sync targets.
>
> Yeah. I found this because I was wondering what it would take to
> convert the remaining sync targets, and have only target-async left.
>
> I suspect the inf-ptrace targets can be converted by just lifting the
> Linux code into in inf_ptrace_target.

Assuming there isn't too much Linux-ism in there.

>This comment in nbsd-nat.c is
> worrying though:
>
> /* The common code passes WNOHANG that leads to crashes, overwrite it. */

Indeed, and that's a recent addition...

> Other targets require a little more research. For Windows, Darwin, and
> remote-sim, I thought perhaps we could put some of the current
> resume/wait code into a separate thread. (Though in the case of the
> sim, I think it would be better to move the sims to using the remote
> protocol.)

For Windows, that's what Pedro suggested here:

https://sourceware.org/legacy-ml/gdb/2016-09/msg00055.html

If we can avoid separate threads, I'd say it's better to, because it's more difficult
to debug. For Windows, that would mean finding if there's an equivalent of SIGCHLD +
the self pipe trick but for Windows. In other words, a way to wake up the event loop
when we know there's an event available.

> Simon> Perhaps we could return TARGET_WAITKIND_NO_RESUMED in both cases?
>
> remote.c does this in at least one case, though apparently not in all.

Simon