From patchwork Wed Feb 26 20:05:39 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 38328 Received: (qmail 100940 invoked by alias); 26 Feb 2020 20:06:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 100919 invoked by uid 89); 26 Feb 2020 20:06:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=Five, sticking X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Feb 2020 20:06:24 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582747583; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XQWt1Jz5Z9Y9TOTX1rGKb8wk/YYIkjZ7HJ0O/R0h2dU=; b=Q2Jlei7hvPcdwR13UnDwLo+pXLBWHTJXZShhSBpDqRMwFKXAf2COso9c37y4m9RGxDxy7Y gWwF23/g1m7aTF4yPULZaPuBJ8HJ+jWx46XY29G8Dd2EDu9xjDD+Z/KZVtRT62xzMzPmWq gXaOFxN/OLUg3v9Mb8i/bSNq+imrqgw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-26-fMJDKTPbNhavsZ8RLAZryA-1; Wed, 26 Feb 2020 15:06:21 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 353A0107ACC5; Wed, 26 Feb 2020 20:06:20 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-54.yyz.redhat.com [10.15.17.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id 76B0F5DA76; Wed, 26 Feb 2020 20:06:19 +0000 (UTC) From: Sergio Durigan Junior To: GDB Patches Cc: Pedro Alves , Tom Tromey , Eli Zaretskii , Ruslan Kabatsayev , Sergio Durigan Junior Subject: [PATCH 3/6] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded Date: Wed, 26 Feb 2020 15:05:39 -0500 Message-Id: <20200226200542.746617-4-sergiodj@redhat.com> In-Reply-To: <20200226200542.746617-1-sergiodj@redhat.com> References: <20190926042155.31481-1-sergiodj@redhat.com> <20200226200542.746617-1-sergiodj@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-IsSubscribed: yes 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). 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)); +} + +/* 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."; + + /* 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. */ + +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. */ + 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; /* 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 ()); + } + /* Restore our environment in case a vforked child clob'd it. */ environ = save_our_env; @@ -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, + 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, + 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 */