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

[Kevin Buettner <kevinb@redhat.com>]

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