From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by sourceware.org (Postfix) with ESMTP id 883EB385E006 for ; Fri, 27 Mar 2020 13:06:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 883EB385E006 Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-126-ngD8g77EP_q8-TJsJNm4Aw-1; Fri, 27 Mar 2020 09:06:12 -0400 X-MC-Unique: ngD8g77EP_q8-TJsJNm4Aw-1 Received: by mail-ed1-f70.google.com with SMTP id i10so8251209edk.13 for ; Fri, 27 Mar 2020 06:06:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=wX894rpUpCXgC60g/GMn6ntQEXYUqOXicwpW8trEKo0=; b=bj+rJ0PobZnQxANtScezj+jV/B7SND6Sj8hsvGhRBblPkA2NLu8kBn5zy+SvgYMPz+ SZkIp2sTfMiFbBC8twlqLy7CvV49llA0NaOUjVY6J0Hme+LC+PxqcbW0Ihdmim+GkBkj clRvLPescN0EgwmPxX4uWMqAEPLS/ZTnbXxxwF96z8Z9AdcbT208iVYuANNBdU9DcIQp YyFJJ7BpRH6KqvOtNRMHrviiPzI3rPjk0MLvZ8U1wAWifYIn1ae5q7sowRVAg8UPuaWW RvQ6t9nr3vHELR769p+NTZcV7Zk/yk3bp7ccAktoJL53t3tJzBEHsZ50X+pIpK131jvN 1voQ== X-Gm-Message-State: ANhLgQ2zEvOpqbM28isMGz8RDPKzKcH5KSu0MJpmVFlbv/BnLcBnkST3 3LeuUzlH2SsEXGnD7j0hNSXsxoXYVKPFoUBKGcJxAm2hosuMQ9ai0i19725+tRnTDtAFaeJs8/S zuj7G+UfqqgceTt1LPDBKJA== X-Received: by 2002:a17:906:6bca:: with SMTP id t10mr8658179ejs.176.1585314371016; Fri, 27 Mar 2020 06:06:11 -0700 (PDT) X-Google-Smtp-Source: ADFU+vsPX5iqUbYuMgBzs8glTeHWpKl69REigHG6Giuwq8/UTL4F6zTBSymxGkGtsdZzWTyXzxMsbQ== X-Received: by 2002:a17:906:6bca:: with SMTP id t10mr8658128ejs.176.1585314370442; Fri, 27 Mar 2020 06:06:10 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id l62sm851165edl.89.2020.03.27.06.06.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 27 Mar 2020 06:06:09 -0700 (PDT) Subject: Re: [PATCH v2 3/5] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded To: Sergio Durigan Junior , GDB Patches References: <20200226200542.746617-1-sergiodj@redhat.com> <20200317154719.2078283-1-sergiodj@redhat.com> <20200317154719.2078283-4-sergiodj@redhat.com> Cc: Tom Tromey From: Pedro Alves Message-ID: <3f1d9201-8a45-7b6e-1e96-7dc6641da66e@redhat.com> Date: Fri, 27 Mar 2020 13:06:08 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200317154719.2078283-4-sergiodj@redhat.com> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-26.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Mar 2020 13:06:17 -0000 On 3/17/20 3:47 PM, Sergio Durigan Junior via Gdb-patches wrote: > This patch is one important piece of the series. It expands > 'fork_inferior' in order to deal with new steps in the process of > initializing the inferior. We now have to: > > - Create a pipe that will be used to communicate with our > fork (pre-exec), and which the fork will use to pass back to us the > errno value of the 'traceme_fun' call. > > - Close this pipe after it is used. > > - Check the errno value passed back from the fork, and report any > problems in the initialization to the user. > > I thought about and implemented a few designs for all of this, but > ended up sticking with the function overload one. 'fork_inferior' is > now two functions: one that will take a traceme function like > '(*traceme_fun) ()' --- i.e., the original 'fork_inferior' behaviour, > and other that will take a function like '(*traceme_fun) (int > trace_pipe_write)'. Depending on which function it takes, we know > whether the user does not want us to check whether the 'traceme_fun' > call was successful (former) or if she does (latter). Pedantically: user -> caller. she -> it. "user" normally refers to a person who uses gdb. > All in all, the patch is not complicated to understand and keeps the > interface clean enough so that we don't have to update every caller of > 'fork_inferior' (which was a problem with previous designs I tried). > > The subsequent patch will build on top of this one and implement the > errno-passing-via-pipe on the GNU/Linux target. > > gdb/ChangeLog: > yyyy-mm-dd Sergio Durigan Junior > > * nat/fork-inferior.c: Include "gdbsupport/scoped_pipe.h". > (default_trace_me_fail_reason): New function. > (trace_me_fail_reason): New variable. > (write_trace_errno_to_pipe): New function. > (read_trace_errno_from_pipe): Likewise. > (check_child_trace_me_errno): Likewise. > (traceme_info): New struct. > (fork_inferior_1): Renamed from 'fork_inferior'. > (fork_inferior): New overloads. > (trace_start_error_with_name): Add "append" parameter. > * nat/fork-inferior.h (fork_inferior): Expand comment. > Add overload declaration. > (trace_start_error_with_name): Add "append" parameter. > (trace_me_fail_reason): New variable. > (check_child_trace_me_errno): New function. > (write_trace_errno_to_pipe): Likewise. > --- > gdb/nat/fork-inferior.c | 231 ++++++++++++++++++++++++++++++++++++---- > gdb/nat/fork-inferior.h | 87 ++++++++++++--- > 2 files changed, 288 insertions(+), 30 deletions(-) > > diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c > index 1185ef8998..223ff44195 100644 > --- a/gdb/nat/fork-inferior.c > +++ b/gdb/nat/fork-inferior.c > @@ -27,6 +27,7 @@ > #include "gdbsupport/pathstuff.h" > #include "gdbsupport/signals-state-save-restore.h" > #include "gdbsupport/gdb_tilde_expand.h" > +#include "gdbsupport/scoped_pipe.h" > #include > > extern char **environ; > @@ -262,16 +263,157 @@ execv_argv::init_for_shell (const char *exec_file, > m_argv.push_back (NULL); > } > > -/* See nat/fork-inferior.h. */ > +/* Default implementation of 'trace_me_fail_reason'. Always return > + an empty string. */ > > -pid_t > -fork_inferior (const char *exec_file_arg, const std::string &allargs, > - char **env, void (*traceme_fun) (), > - gdb::function_view init_trace_fun, > - void (*pre_trace_fun) (), > - const char *shell_file_arg, > - void (*exec_fun)(const char *file, char * const *argv, > - char * const *env)) > +static std::string > +default_trace_me_fail_reason (int err) > +{ > + return {}; > +} > + > +/* See fork-inferior.h. */ > + > +std::string (*trace_me_fail_reason) (int err) > + = default_trace_me_fail_reason; > + > +/* See fork-inferior.h. */ > + > +void > +write_trace_errno_to_pipe (int writepipe, int trace_errno) > +{ > + ssize_t writeret; > + > + do > + { > + writeret = write (writepipe, &trace_errno, sizeof (trace_errno)); > + } > + while (writeret < 0 && (errno == EAGAIN || errno == EINTR)); > + > + if (writeret < 0) > + error (_("Could not write trace errno: %s"), safe_strerror (errno)); I'll have a comment about this error in a following patch. > +} > + > +/* Helper function to read TRACE_ERRNO from READPIPE, which handles > + EINTR/EAGAIN and throws and exception if there was an error. */ > + > +static int > +read_trace_errno_from_pipe (int readpipe) > +{ > + ssize_t readret; > + int trace_errno; > + > + do > + { > + readret = read (readpipe, &trace_errno, sizeof (trace_errno)); > + } > + while (readret < 0 && (errno == EAGAIN || errno == EINTR)); > + > + if (readret < 0) > + error (_("Could not read trace errno: %s"), safe_strerror (errno)); > + > + return trace_errno; > +} > + > +/* See fork-inferior.h. */ > + > +void > +check_child_trace_me_errno (int readpipe) > +{ > + fd_set rset; > + struct timeval timeout; > + int ret; > + > + /* Make sure we have a valid 'trace_me_fail_reason' function > + defined. */ > + gdb_assert (trace_me_fail_reason != nullptr); > + > + FD_ZERO (&rset); > + FD_SET (readpipe, &rset); > + > + /* Five seconds should be plenty of time to wait for the child's > + reply. */ > + timeout.tv_sec = 5; > + timeout.tv_usec = 0; > + > + do > + { > + ret = select (readpipe + 1, &rset, NULL, NULL, &timeout); > + } > + while (ret < 0 && (errno == EAGAIN || errno == EINTR)); > + > + if (ret < 0) > + perror_with_name ("select"); > + else if (ret == 0) > + error (_("Timeout while waiting for child's trace errno")); > + else > + { > + int child_errno; > + > + child_errno = read_trace_errno_from_pipe (readpipe); > + > + 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); > + > + if (reason.empty ()) > + reason = "Could not determine reason for trace failure."; Missing i18n. > + > + /* The child is supposed to display a warning containing the > + safe_strerror message before us, so we just display the > + possible reason for the failure. */ > + error ("%s", reason.c_str ()); > + } > + } > +} > + > +/* Helper struct for fork_inferior_1, containing information on > + whether we should check if TRACEME_FUN was successfully called or > + not. */ "was successfully called" seems ambiguous -- I first read it as, "whether we managed to call TRACEME_FUN", which is not what you meant, since we always call it. It'd suggest: "should check whether TRACEME_FUN succeeded tracing the child" > + > +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; > + > + union > + { > + /* The non-check version of TRACEME_FUN. It will be set if > + CHECK_TRACEME_FAIL_REASON is false. > + > + This function will usually just perform the call to whatever > + trace function needed to start tracing the inferior (e.g., > + ptrace). */ > + void (*traceme_fun_nocheck) (); > + > + /* The check version of TRACEME_FUN. It will be set if > + CHECK_TRACEME_FAIL_REASON is true. > + > + This function will usually perform the call to whatever trace > + function needed to start tracing the inferior, but will also > + write its errno value to TRACE_ERRNO_PIPE, so that > + fork_inferior_1 can check whether it suceeded. */ Typo: "suceeded" -> "succeeded". I'd drop the "usually" both places. It's not usually, it's always. > + void (*traceme_fun_check) (int trace_errno_pipe); > + } u; > +}; > + > +/* Helper function. > + > + Depending on the value of TRACEME_INFO.CHECK_TRACEME_FAIL_REASON, > + this function will check whether the call to TRACEME_FUN succeeded > + or not. */ > + > +static pid_t > +fork_inferior_1 (const char *exec_file_arg, const std::string &allargs, > + char **env, const struct traceme_info traceme_info, > + gdb::function_view init_trace_fun, > + void (*pre_trace_fun) (), > + const char *shell_file_arg, > + void (*exec_fun)(const char *file, char * const *argv, > + char * const *env)) > { > pid_t pid; > /* Set debug_fork then attach to the child while it sleeps, to debug. */ > @@ -283,6 +425,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > int save_errno; > const char *inferior_cwd; > std::string expanded_inferior_cwd; > + scoped_pipe trace_pipe; You could wrap this in a gdb::optional to avoid creating the pipe unless necessary. Like: gdb::optional trace_pipe; if (traceme_info.check_trace_me_fail_reason) trace_pipe.emplace (); > > /* If no exec file handed to us, get it from the exec-file command > -- with a good, common error message if none is specified. */ > @@ -365,12 +508,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > > if (pid == 0) > { > - /* Close all file descriptors except those that gdb inherited > - (usually 0/1/2), so they don't leak to the inferior. Note > - that this closes the file descriptors of all secondary > - UIs. */ > - close_most_fds (); > - > /* Change to the requested working directory if the user > requested it. */ > if (inferior_cwd != NULL) > @@ -392,7 +529,10 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > for the inferior. */ > > /* "Trace me, Dr. Memory!" */ > - (*traceme_fun) (); > + if (traceme_info.check_trace_me_fail_reason) > + (*traceme_info.u.traceme_fun_check) (trace_pipe.get_write_end ()); > + else > + (*traceme_info.u.traceme_fun_nocheck) (); > > /* The call above set this process (the "child") as debuggable > by the original gdb process (the "parent"). Since processes > @@ -403,6 +543,12 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > saying "not parent". Sorry; you'll have to use print > statements! */ > > + /* Close all file descriptors except those that gdb inherited > + (usually 0/1/2), so they don't leak to the inferior. Note > + that this closes the file descriptors of all secondary > + UIs, and the trace errno pipe (if it's open). */ > + close_most_fds (); > + > restore_original_signals_state (); > > /* There is no execlpe call, so we have to set the environment > @@ -431,6 +577,13 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > _exit (0177); > } > > + if (traceme_info.check_trace_me_fail_reason) > + { > + /* Check the trace errno, and inform the user about the reason > + of the failure, if there was any. */ > + check_child_trace_me_errno (trace_pipe.get_read_end ()); > + } > + check_child_trace_me_errno can throw an error. So it should be done after we've restored all global state. See the environ restore just below ... > /* Restore our environment in case a vforked child clob'd it. */ > environ = save_our_env; ... here. You'd miss restoring this. > > @@ -448,6 +601,48 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > return pid; > } > > +/* See fork-inferior.h. */ > + > +pid_t > +fork_inferior (const char *exec_file_arg, const std::string &allargs, > + char **env, void (*traceme_fun) (), > + gdb::function_view init_trace_fun, > + void (*pre_trace_fun) (), > + const char *shell_file_arg, > + void (*exec_fun)(const char *file, char * const *argv, Something odd with indentation here. Likely tabs vs spaces. > + char * const *env)) > +{ > + struct traceme_info traceme_info; > + > + traceme_info.check_trace_me_fail_reason = false; > + traceme_info.u.traceme_fun_nocheck = traceme_fun; > + > + return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info, > + init_trace_fun, pre_trace_fun, shell_file_arg, > + exec_fun); > +} > + > +/* See fork-inferior.h. */ > + > +pid_t > +fork_inferior (const char *exec_file_arg, const std::string &allargs, > + char **env, void (*traceme_fun) (int trace_errno_pipe), > + gdb::function_view init_trace_fun, > + void (*pre_trace_fun) (), > + const char *shell_file_arg, > + void (*exec_fun)(const char *file, char * const *argv, Ditto re. indentation. > + char * const *env)) > +{ > + struct traceme_info traceme_info; > + > + traceme_info.check_trace_me_fail_reason = true; > + traceme_info.u.traceme_fun_check = traceme_fun; > + > + return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info, > + init_trace_fun, pre_trace_fun, shell_file_arg, > + exec_fun); > +} > + > /* See nat/fork-inferior.h. */ > > ptid_t > @@ -592,7 +787,7 @@ trace_start_error (const char *fmt, ...) > /* See nat/fork-inferior.h. */ > > void > -trace_start_error_with_name (const char *string) > +trace_start_error_with_name (const char *string, const char *append) > { > - trace_start_error ("%s: %s", string, safe_strerror (errno)); > + trace_start_error ("%s: %s%s", string, safe_strerror (errno), append); > } > diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h > index cf6f137edd..b67215353f 100644 > --- a/gdb/nat/fork-inferior.h > +++ b/gdb/nat/fork-inferior.h > @@ -32,17 +32,41 @@ struct process_stratum_target; > #define START_INFERIOR_TRAPS_EXPECTED 1 > > /* Start an inferior Unix child process and sets inferior_ptid to its > - pid. EXEC_FILE is the file to run. ALLARGS is a string containing > - the arguments to the program. ENV is the environment vector to > - pass. SHELL_FILE is the shell file, or NULL if we should pick > - one. EXEC_FUN is the exec(2) function to use, or NULL for the default > - one. */ > - > -/* This function is NOT reentrant. Some of the variables have been > - made static to ensure that they survive the vfork call. */ > + pid. > + > + EXEC_FILE is the file to run. > + > + ALLARGS is a string containing the arguments to the program. > + > + ENV is the environment vector to pass. > + > + SHELL_FILE is the shell file, or NULL if we should pick one. > + > + EXEC_FUN is the exec(2) function to use, or NULL for the default > + one. > + > + This function is NOT reentrant. Some of the variables have been > + made static to ensure that they survive the vfork call. > + > + This function does not check whether the call to TRACEME_FUN > + succeeded or not. */ > extern pid_t fork_inferior (const char *exec_file_arg, > const std::string &allargs, > - char **env, void (*traceme_fun) (), > + char **env, > + void (*traceme_fun) (), > + gdb::function_view init_trace_fun, > + void (*pre_trace_fun) (), > + const char *shell_file_arg, > + void (*exec_fun) (const char *file, > + char * const *argv, > + char * const *env)); > + > +/* Like fork_inferior above, but check whether the call to TRACEME_FUN > + succeeded or not. */ > +extern pid_t fork_inferior (const char *exec_file_arg, > + const std::string &allargs, > + char **env, > + void (*traceme_fun) (int trace_errno_pipe), > gdb::function_view init_trace_fun, > void (*pre_trace_fun) (), > const char *shell_file_arg, > @@ -82,9 +106,48 @@ extern void trace_start_error (const char *fmt, ...) > ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2); > > /* Like "trace_start_error", but the error message is constructed by > - combining STRING with the system error message for errno. This > - function does not return. */ > -extern void trace_start_error_with_name (const char *string) > + combining STRING with the system error message for errno, and > + (optionally) with APPEND. This function does not return. */ > +extern void trace_start_error_with_name (const char *string, > + const char *append = "") > ATTRIBUTE_NORETURN; > > +/* 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); > + > +/* Check the "trace me" errno (generated when executing e.g. 'ptrace > + (PTRACE_ME, ...)') of the child process that was created by > + GDB/GDBserver when creating an inferior. The errno value will be > + passed via a pipe (see 'fork_inferior'), and READPIPE is the read > + end of the pipe. > + > + If possible (i.e., if 'trace_me_fail_reason' is defined by the > + target), then we also try to determine the possible reason for a > + failure. > + > + The idea is to wait a few seconds (via 'select') until something is > + written on READPIPE. When that happens, we check if the child's > + trace errno is different than 0. If it is, we call the function > + 'trace_me_fail_reason' and try to obtain the reason for the > + failure, and then throw an exception (with the reason as the > + exception's message). > + > + If nothing is written on the pipe, or if 'select' fails, we also > + throw exceptions. */ > +extern void check_child_trace_me_errno (int readpipe); > + > +/* Helper function to write TRACE_ERRNO to WRITEPIPE, which handles > + EINTR/EAGAIN and throws an exception if there was an error. */ > +extern void write_trace_errno_to_pipe (int writepipe, int trace_errno); > + > #endif /* NAT_FORK_INFERIOR_H */ > Thanks, Pedro Alves