From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75267 invoked by alias); 5 May 2017 19:05:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 75245 invoked by uid 89); 5 May 2017 19:05:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f50.google.com Received: from mail-wm0-f50.google.com (HELO mail-wm0-f50.google.com) (74.125.82.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 05 May 2017 19:05:00 +0000 Received: by mail-wm0-f50.google.com with SMTP id b84so1545647wmh.0 for ; Fri, 05 May 2017 12:05:02 -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:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=tVj6G5QQaVgfu/zcFuY99Ag5dY2to4mb7/TRtngjguE=; b=a1p3b8HXtuDjsWvCY/Y8Z9eM7zg/nXHwQomBTA85+dfwDK/YjPMLA0RrXJCJ27xwHj hbpjthJjX5i8BcTzHG8y++fl8NZZLbNu1HhIbPNS6BFJehMYqHgk44y9TAGSO+JcuZN9 HT4sCx6TkBwJqQmySjP5Gg9Pdh/UojJnvmzTYN9sDByl2BhA54TxDCltfvzBv9jsprCW iSrfeJzA+LGQYBDwi0jDVPM8P945XY6MaPtTsuB29S/5qjC2jWZqC822Q8N3h154ZIvS kISIei+dGErmXD96/uACQvPPWpAD7AeEE1Btf6th+Ef+BDTJn1dqR74miE5t0B67BqNu XMWg== X-Gm-Message-State: AN3rC/4Rbvlro/siIvt9Ah7dQCP/ijjfponLwWylC/HVtkLalxcq/Luv xTjaXhZlwDphn3PX94rmqQ== X-Received: by 10.28.132.145 with SMTP id g139mr6051294wmd.35.1494011100071; Fri, 05 May 2017 12:05:00 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id n193sm3061779wmg.30.2017.05.05.12.04.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 05 May 2017 12:04:59 -0700 (PDT) Subject: Re: [PATCH v6 3/4] Share fork_inferior et al with gdbserver To: Sergio Durigan Junior , GDB Patches References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170504052954.16936-1-sergiodj@redhat.com> <20170504052954.16936-4-sergiodj@redhat.com> From: Pedro Alves Message-ID: <0e95d746-9293-3301-15ed-84d6c4280116@redhat.com> Date: Fri, 05 May 2017 19:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170504052954.16936-4-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00150.txt.bz2 On 05/04/2017 06:29 AM, Sergio Durigan Junior wrote: > diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h > new file mode 100644 > index 0000000..206a36a > --- /dev/null > +++ b/gdb/common/common-inferior.h > @@ -0,0 +1,75 @@ > +/* Variables that describe the inferior process running under GDB and > + GDBserver: Where it is, why it stopped, and how to step it. Stale comment. > + > + Copyright (C) 1986-2017 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#ifndef COMMON_INFERIOR_H > +#define COMMON_INFERIOR_H > + > +/* Number of traps that happen between exec'ing the shell to run an > + inferior and when we finally get to the inferior code, not counting > + the exec for the shell. This is 1 on all supported > + implementations. */ > +#define START_INFERIOR_TRAPS_EXPECTED 1 The copy in gdb/inferior.h should be removed instead of being left duplicated. > + > +/* Whether to start up the debuggee under a shell. > + > + If startup-with-shell is set, GDB's "run" will attempt to start up > + the debuggee under a shell. This also happens when using GDBserver > + under extended remote mode. > + > + This is in order for argument-expansion to occur. E.g., > + > + (gdb) run * > + > + The "*" gets expanded by the shell into a list of files. > + > + While this is a nice feature, it may be handy to bypass the shell > + in some cases. To disable this feature, do "set startup-with-shell > + false". > + > + The catch-exec traps expected during start-up will be one more if > + the target is started up with a shell. */ > +extern int startup_with_shell; > + > +/* Perform any necessary tasks before a fork/vfork takes place. ARGS > + is a string containing all the arguments received by the inferior. > + This function is mainly used by fork_inferior. */ > +extern void prefork_hook (const char *args); > + > +/* Perform any necessary tasks after a fork/vfork takes place. This > + function is mainly used by fork_inferior. */ > +extern void postfork_hook (pid_t pid); > + > +/* Perform any necessary tasks *on the child* after a fork/vfork takes > + place. DEBUG_FORK is the number of seconds that we should sleep > + before exec'ing (see fork_inferior). > + > + This function is mainly used by fork_inferior. */ > +extern void postfork_child_hook (int debug_fork); Wouldn't it better to put these in nat/fork-inferior.h instead? Likewise START_INFERIOR_TRAPS_EXPECTED, maybe? > + > +/* Return the exec wrapper to be used when starting the inferior, or NULL > + otherwise. */ > +extern const char *get_exec_wrapper (void); Should really drop the "(void)" in all new code throughout the series. > + > +/* Return the name of the executable file as a string. > + ERR nonzero means get error if there is none specified; > + otherwise return 0 in that case. */ > +extern char *get_exec_file (int err); > + > +#endif /* ! COMMON_INFERIOR_H */ > +/* See common/common-utils.h. */ > + > +std::string > +stringify_argv (const std::vector &args) > +{ > + std::string ret (""); std::string ret; > + > + if (!args.empty ()) > + { > + for (auto s : args) > + if (s != NULL) > + ret += s + std::string (" "); Avoid unnecessary temporaries: { ret += s; ret += ' '; } > + > + /* Erase the last whitespace. */ > + ret.erase (ret.end () - 1); > + } > + > + return ret; > +} > > -/* Accept NTRAPS traps from the inferior. */ > +/* See common/common-inferior.h. */ > > void > -startup_inferior (int ntraps) > +postfork_child_hook (int debug_fork) > { > - int pending_execs = ntraps; > - int terminal_initted = 0; > - ptid_t resume_ptid; > - > - if (startup_with_shell) > - { > - /* One trap extra for exec'ing the shell. */ > - pending_execs++; > - } > + /* This is set to the result of setpgrp, which if vforked, will be > + visible to you in the parent process. It's only used by humans > + for debugging. */ > + static int debug_setpgrp = 657473; > > - if (target_supports_multi_process ()) > - resume_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid)); > - else > - resume_ptid = minus_one_ptid; > + /* Make sure we switch to main_ui here in order to be able to > + use the fprintf_unfiltered/warning/error functions. */ > + current_ui = main_ui; > > - /* The process was started by the fork that created it, but it will > - have stopped one instruction after execing the shell. Here we > - must get it up to actual execution of the real program. */ > + /* 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 (); Any reason this closing isn't done in the common code instead of having it done on both hook implementations? > > - if (exec_wrapper) > - pending_execs++; > + if (debug_fork) > + sleep (debug_fork); Similarly, why not leave this sleep to the common code? It's missing on the gdbserver side, AFAICS. > + case TARGET_WAITKIND_SPURIOUS: > + case TARGET_WAITKIND_LOADED: > + case TARGET_WAITKIND_FORKED: > + case TARGET_WAITKIND_VFORKED: > + case TARGET_WAITKIND_SYSCALL_ENTRY: > + case TARGET_WAITKIND_SYSCALL_RETURN: > + case TARGET_WAITKIND_VFORK_DONE: > + case TARGET_WAITKIND_IGNORE: > + case TARGET_WAITKIND_NO_HISTORY: > + case TARGET_WAITKIND_NO_RESUMED: > + case TARGET_WAITKIND_THREAD_CREATED: > + case TARGET_WAITKIND_THREAD_EXITED: You added several of these at the same time (but they don't really make sense here). Did you run into some of them in gdbserver? > index 4ea7913..8aa85db 100644 > --- a/gdb/gdbserver/configure.ac > +++ b/gdb/gdbserver/configure.ac > @@ -462,7 +462,9 @@ esac], > > if $want_ipa ; then > if $have_ipa ; then > - IPA_DEPFILES="$ipa_obj" > + # Needed because safe_strerror's definition is host-dependent Why do we end up needing safe_strerror in the IPA in the first place? > + strerror_obj="`echo $srv_host_obs | sed 's/\(.*-strerror\)\.o/\1-ipa.o/'`" > + IPA_DEPFILES="$ipa_obj $strerror_obj" > extra_libraries="$extra_libraries libinproctrace.so" > else > AC_MSG_ERROR([inprocess agent not supported for this target]) > -#if defined(__UCLIBC__) && defined(HAS_NOMMU) > - pid = vfork (); Does fork_inferior end up always using vfork on no-MMU ports somehow? > -#else > - pid = fork (); > -#endif > +const char * > +get_exec_wrapper (void) > { > - std::vector new_argv; > + static std::string ret; > + static bool initialized_p = false; > > - if (!wrapper_argv.empty ()) > - new_argv.insert (new_argv.begin (), > - wrapper_argv.begin (), > - wrapper_argv.end ()); > + if (wrapper_argv.empty ()) > + return NULL; > > - for (int i = 0; argv[i] != NULL; ++i) > - new_argv.push_back (argv[i]); > + if (!initialized_p) > + { > + for (auto s : wrapper_argv) > + ret += s + std::string (" "); Avoid unnecessary temporaries. > > - new_argv.push_back (NULL); > + /* Erase the last whitespace. */ > + ret.erase (ret.end () - 1); > + > + initialized_p = true; > + } > + > + return ret.c_str (); > +} I'm actually having a bit of trouble understanding why do we need the wrapper_argv global plus this initialized_p indirection, instead of just building the exec wrapper string in captured_main. > @@ -2848,6 +2893,7 @@ handle_v_run (char *own_buf) > { > + /* FIXME: Fail request if out of memory instead of dying. */ > size_t len = (next_p - p) / 2; Did you really mean to add that comment? > diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h > index d5fee38..5087614 100644 > --- a/gdb/gdbserver/server.h > +++ b/gdb/gdbserver/server.h > @@ -132,6 +132,7 @@ extern int in_queued_stop_replies (ptid_t ptid); > #include "utils.h" > #include "debug.h" > #include "gdb_vecs.h" > +#include > Why? > /* Maximum number of bytes to read/write at once. The value here > is chosen to fill up a packet (the headers account for the 32). */ > @@ -148,4 +149,13 @@ extern int in_queued_stop_replies (ptid_t ptid); > /* Definition for any syscall, used for unfiltered syscall reporting. */ > #define ANY_SYSCALL (-2) > > +/* After fork_inferior has been called, we need to adjust a few > + signals and call startup_inferior. This is done here. PID is the > + pid of the new inferior, and PROGRAM is its name. */ > +extern void post_fork_inferior (int pid, const char *program); > + > +/* Get the 'struct gdb_environ *' being used in the current > + session. */ > +extern struct gdb_environ *get_environ (void); > + > #endif /* SERVER_H */ > + > +/* See target/target.h. */ > + > +void > +target_terminal_init (void) > +{ > + /* To be implemented. */ > +} > + > +/* See target/target.h. */ > + > +void > +target_terminal_inferior (void) > +{ > + /* To be implemented. */ > +} > + > +/* See target/target.h. */ > + > +void > +target_terminal_ours (void) > +{ > + /* To be implemented. */ > +} s/To be implemented/Not necessary/ or some such. > diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h > index b4ded31..48804c5 100644 > --- a/gdb/gdbserver/utils.h > +++ b/gdb/gdbserver/utils.h > @@ -19,6 +19,8 @@ > #ifndef UTILS_H > #define UTILS_H > > +#include > + Why? > char *paddress (CORE_ADDR addr); > char *pfildes (gdb_fildes_t fd); > > static int > -win32_create_inferior (char *program, char **program_args) > +win32_create_inferior (const char *program, > + const std::vector &program_args) > { > #ifndef USE_WIN32API > char real_path[PATH_MAX]; > @@ -627,6 +627,8 @@ win32_create_inferior (char *program, char **program_args) > int argc; > PROCESS_INFORMATION pi; > DWORD err; > + std::string str_program_args = stringify_argv (program_argv); > + char *args = (char *) str_program_args.c_str (); Can it be const? > + > +/* When executing a command under the given shell, return non-zero if > + the '!' character should be escaped when embedded in a quoted > + command-line argument. */ > + > +static int > +escape_bang_in_quoted_argument (const char *shell_file) > +{ > + const int shell_file_len = strlen (shell_file); > + > + /* Bang should be escaped only in C Shells. For now, simply check > + that the shell name ends with 'csh', which covers at least csh > + and tcsh. This should be good enough for now. */ > + > + if (shell_file_len < 3) > + return 0; > + > + if (shell_file[shell_file_len - 3] == 'c' > + && shell_file[shell_file_len - 2] == 's' > + && shell_file[shell_file_len - 1] == 'h') > + return 1; > + > + return 0; > +} This function lost some edits done in the execv_argv patch. bool + size_t. Please run a diff of the old gdb/fork-child.c against the new nat/fork-inferior.c to make sure nothing else was lost. > diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h > new file mode 100644 > index 0000000..6a8528e > --- /dev/null > +++ b/gdb/nat/fork-inferior.h > @@ -0,0 +1,51 @@ ... > + > +#ifndef FORK_CHILD_H > +#define FORK_CHILD_H Incorrectly named multiple-inclusion guard. > + > +#include Why? Nothing uses std::vector here. > + > +/* Accept NTRAPS traps from the inferior. > + > + Return the ptid of the inferior being started. */ > +extern ptid_t startup_inferior (pid_t pid, int ntraps, > + struct target_waitstatus *mystatus, > + ptid_t *myptid); > + > +/* 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. */ > +extern pid_t fork_inferior (const char *exec_file_arg, > + const std::string &allargs, Should include . > + char **env, void (*traceme_fun) (void), > + void (*init_trace_fun) (int), > + void (*pre_trace_fun) (void), > + const char *shell_file_arg, > + void (*exec_fun) (const char *file, > + char * const *argv, > + char * const *env)); > + > +#endif /* ! FORK_CHILD_H */ > > diff --git a/gdb/target.h b/gdb/target.h > index a971adf..6c995a1 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -1538,17 +1538,6 @@ extern int target_terminal_is_inferior (void); > > extern int target_terminal_is_ours (void); > > -/* Initialize the terminal settings we record for the inferior, > - before we actually run the inferior. */ > - > -extern void target_terminal_init (void); > - > -/* Put the inferior's terminal settings into effect. This is > - preparation for starting or resuming the inferior. This is a no-op > - unless called with the main UI as current UI. */ > - > -extern void target_terminal_inferior (void); > - > /* Put some of our terminal settings into effect, enough to get proper > results from our output, but do not change into or out of RAW mode > so that no input is discarded. This is a no-op if terminal_ours > @@ -1557,12 +1546,6 @@ extern void target_terminal_inferior (void); > > extern void target_terminal_ours_for_output (void); > > -/* Put our terminal settings into effect. First record the inferior's > - terminal settings so they can be restored properly later. This is > - a no-op unless called with the main UI as current UI. */ > - > -extern void target_terminal_ours (void); > - Please leave breakcrumb comments behind. > /* Return true if the target stack has a non-default > "to_terminal_ours" method. */ > Thanks, Pedro Alves