From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127517 invoked by alias); 31 May 2017 03:43:49 -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 127498 invoked by uid 89); 31 May 2017 03:43:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Needed, dying, junior, humans X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 31 May 2017 03:43:37 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EFCA54E4F3 for ; Wed, 31 May 2017 03:43:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EFCA54E4F3 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com EFCA54E4F3 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AA00F18A45; Wed, 31 May 2017 03:43:39 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches Subject: Re: [PATCH v6 3/4] Share fork_inferior et al with gdbserver References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170504052954.16936-1-sergiodj@redhat.com> <20170504052954.16936-4-sergiodj@redhat.com> <0e95d746-9293-3301-15ed-84d6c4280116@redhat.com> Date: Wed, 31 May 2017 03:43:00 -0000 Message-ID: <87poep8zdg.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00628.txt.bz2 Hey Pedro, Sorry for the long delay in getting back to this. Comments below. On Friday, May 05 2017, Pedro Alves wrote: > 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. Replaced by: /* Functions to deal with the inferior being executed on GDB or GDBserver. Sorry, couldn't think of a better description. >> + >> + 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. Sorry, a mistake here. Removed the copy in gdb/inferior.h. >> + >> +/* 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? I remember struggling to decide where to put these functions. In the end, I decided to put them on commom/common-inferior.h for a very good reason that I now do not remember anymore :-/. Anyway, since I can't really argue against your point, I will do what you're suggesting. Only get_exec_wrapper and get_exec_file will remain in common-inferior.h >> + >> +/* 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. Oh, OK. I wasn't sure if we were following this practice for all functions or only for methods. >> + >> +/* 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; Done. >> + >> + if (!args.empty ()) >> + { >> + for (auto s : args) >> + if (s != NULL) >> + ret += s + std::string (" "); > > Avoid unnecessary temporaries: > > { > ret += s; > ret += ' '; > } Done. >> + >> + /* 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? You're right, close_most_fds can be shared between them. >> >> - 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. Right, I clearly got confused about debug_fork and its usefulness on gdbserver. Moved to common code, and removed the "debug_fork" parameter from postfork_child_hook. >> + 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? No. At the time I thought it would be better to cover all the TARGET_WAITKIND_* entries here. Some of them seemed to make sense to me, like the TARGET_WAITKIND_THREAD_* ones. Based on your comment, I removed the ones that I had added: 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: >> 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? This is needed because I moved the definition of trace_start_error_with_name from the old gdb/fork-child.c to common/common-utils.c. This function which uses safe_strerror, and common/common-utils.c is compiled by IPA. An option would be to keep these trace_start_error.* functions in nat/fork-inferior.c, but I think it is more logical to keep them on common-utils.c. Now, I can obviously expand the comment if that's what you meant. >> + 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? Sorry, I am not sure. How would I go about finding that? > >> -#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. Done. >> >> - 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. Alright, I'm now using a global std::string wrapper_argv which is constructed on captured_main, and get_exec_wrapper now just returns the c_str () version of it, or NULL if the wrapper_argv.size () is zero. > >> @@ -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? No. Removed. > >> 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? Leftover from a previous patch. Removed. >> /* 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. Changed to: /* Placeholder needed because of fork_inferior. Not necessary on GDBserver. */ >> 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? Sigh... Leftover. Removed. Thanks, and sorry. >> 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? It can, but then I'd have to cast it to "char *" when passing to create_process. CreateProcessA's second argument is of the type LPSTR, which translates to "char *". So I'd rather keep args as "char *". >> + >> +/* 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. Thanks, I'll make sure that nothing else is missed. > >> 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. Fixed. >> + >> +#include > > Why? Nothing uses std::vector here. Removed. >> + >> +/* 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 . Should it? It's compiling fine without it. I included just for the sake of clarity anyway. >> + 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. Will do. Now, something that came up while I was testing things with mingw here. gdb/gdbserver/server.c now calls startup_inferior (define in nat/fork-inferior.c) directly, instead of doing things on its own. This is one of the goals of this series, but for targets that don't compile fork-inferior.c (like Windows) this is an obvious problem. So here's what I'm doing for the new version of the series: I'll move startup_inferior to common/ (probably common/common-fork-inferior.c or some such), so that all targets can have access to it (it's target-agnostic anyway). If you have any comments about this, please let me know. Expect a new version of the series tomorrow. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/