Re: [PATCH v6 3/4] Share fork_inferior et al with gdbserver

[Sergio Durigan Junior <sergiodj@redhat.com>]

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 <http://www.gnu.org/licenses/>.  */
>> +
>> +#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<char *> &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<char *> 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 <vector>
>>  
>
> 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 <vector>
>> +
>
> 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<char *> &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 <vector>
>
> 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 <string>.

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/