Re: [PATCH v2 3/5] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded

Kevin Buettner
2020-03-27T04:14:57Z

Hi Sergio,

See my comments below.

On Tue, 17 Mar 2020 11:47:17 -0400
Sergio Durigan Junior <sergiodj@redhat.com> wrote:

> + if (child_errno != 0)
> + {
> + /* The child can't use TRACE_TRACEME. We have to check whether
> + we know the reason for the failure, and then error out. */
> + std::string reason = trace_me_fail_reason (child_errno);

I think you mean PTRACE_TRACEME in the comment, right?

[...]
> +/* Helper struct for fork_inferior_1, containing information on
> + whether we should check if TRACEME_FUN was successfully called or
> + not. */
> +
> +struct traceme_info
> +{
> + /* True if we should check whether the call to 'traceme_fun
> + (TRACE_ME...)' succeeded or not. */
> + bool check_trace_me_fail_reason;

While reading the patch, I noticed that some things have "traceme" or
"TRACEME" in the their names while others instead use "trace_me" or
"TRACE_ME". While it's already the case that GDB is inconsistent in
this regard, I think it might be a good to not propogate that
inconsistency further.

It's likely that this came about due to the two differently named
constants used with ptrace(), i.e. PTRACE_TRACEME and PT_TRACE_ME.
Since these constants are used on different platforms, I don't think
there's any way we can reconcile these. But for, for names that we
pick, I think we should choose either "traceme" or "trace_me" and stick
with it.

I prefer "traceme" / "TRACEME", but won't object though if you choose
the other option.

> +
> + union
> + {
> + /* The non-check version of TRACEME_FUN. It will be set if
> + CHECK_TRACEME_FAIL_REASON is false.

"CHECK_TRACEME_FAIL_REASON" in this comment doesn't match the
declaration above in which check_trace_me_fail_reason (with the _
between "trace" and "me") is used instead. There are several
instances; I'll only point out this one.

[...]
> +/* Pointer to function which can be called by
> + 'check_child_trace_me_errno' when we need to determine the reason
> + of a e.g. 'ptrace (PTRACE_ME, ...)' failure. ERR is the ERRNO
> + value set by the failing ptrace call.
> +
> + By default, the function returns an empty string (see
> + fork-inferior.c).
> +
> + This pointer can be overriden by targets that want to personalize
> + the error message printed when trace fails (see linux-nat.c or
> + gdbserver/linux-low.c, for example). */
> +extern std::string (*trace_me_fail_reason) (int err);

On my first pass through this patch, I had concerns about the
introduction of this global variable. (That's why I asked for a
new set of patches.) I can't think of a better way to do it though.

Kevin