Re: [PATCH 5/6] Share fork_inferior et al with gdbserver

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Hey Luis,

Thanks for the review.

As usual, comments below.

On Tuesday, January 03 2017, Luis Machado wrote:

> Not a lot of comments at this point, but a few.
>
>> [...]
>> diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h
>> new file mode 100644
>> index 0000000..fb78109
>> --- /dev/null
>> +++ b/gdb/common/common-inferior.h
>> @@ -0,0 +1,113 @@
>> +/* Variables that describe the inferior process running under GDB:
>> +   Where it is, why it stopped, and how to step it.
>
> Common code that describes the inferior process under GDB/GDBserver ...?
>
> I'd check the other files that were created in common/ to make sure
> their descriptions in the copyright block aren't unchanged from their
> original files.

Yeah, I was going to do that already after your last comments on other
patches, but thanks for the heads up.

>> [...]
>> diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
>> index 249a063..56c734c 100644
>> --- a/gdb/gdbserver/target.c
>> +++ b/gdb/gdbserver/target.c
>> @@ -387,3 +387,27 @@ default_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
>>    (*the_target->sw_breakpoint_from_kind) (0, &size);
>>    return size;
>>  }
>> +
>> +/* 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.  */
>> +}
>
> Same suggestion as the other patch regarding either putting something
> in place now or adding more documentation about what these need to do
> in the future.

Yep, they're on my list, thanks.

>> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
>> index d098a92..3e74b86 100644
>> --- a/gdb/gdbserver/target.h
>> +++ b/gdb/gdbserver/target.h
>> @@ -28,6 +28,7 @@
>>  #include "target/waitstatus.h"
>>  #include "mem-break.h"
>>  #include "btrace-common.h"
>> +#include <vector>
>>
>>  struct emit_ops;
>>  struct buffer;
>> @@ -73,7 +74,7 @@ struct target_ops
>>       Returns the new PID on success, -1 on failure.  Registers the new
>>       process with the process list.  */
>>
>> -  int (*create_inferior) (char *program, char **args);
>> +  int (*create_inferior) (std::vector<char *> &program_argv);
>>
>>    /* Do additional setup after a new process is created, including
>>       exec-wrapper completion.  */
>> @@ -480,8 +481,8 @@ extern struct target_ops *the_target;
>>
>>  void set_target_ops (struct target_ops *);
>>
>> -#define create_inferior(program, args) \
>> -  (*the_target->create_inferior) (program, args)
>> +#define create_inferior(program) \
>> +  (*the_target->create_inferior) (program)
>>
>>  #define target_post_create_inferior()			 \
>>    do							 \
>> diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
>> index 37b9c89..8fa3ce5 100644
>> --- a/gdb/gdbserver/utils.c
>> +++ b/gdb/gdbserver/utils.c
>> @@ -137,3 +137,40 @@ pfildes (gdb_fildes_t fd)
>>    return plongest (fd);
>>  #endif
>>  }
>> +
>> +/* See common/common-utils.h.  */
>> +
>> +void
>> +gdb_flush_out_err (void)
>> +{
>> +  fflush (stdout);
>> +  fflush (stderr);
>> +}
>> +
>> +/* See common/common-utils.h.  */
>> +
>> +void
>> +free_vector_argv (std::vector<char *> &v)
>> +{
>> +  for (std::vector<char *>::iterator i = v.begin ();
>> +       i != v.end ();
>> +       ++i)
>> +    xfree (*i);
>> +
>> +  v.clear ();
>> +}
>
> Nothing wrong with the above, but with the decision to use C++ 11,
> maybe this could be further simplified with range for if you think
> it's worth it?
>
> {
>   for (char* arg : v)
>     xfree (arg);
>
>   v.clear ();
> }
>
> Or maybe it is a bit too fancy for our own good, though we will
> eventually need to get used to it.

Oh, I think I've lost some context then, because I thought we were still
waiting a bit before using C++11.

> Another thought is to make the elements std::string themselves, in
> which case std::vector::clear () will call each elements'
> destructor. Again, maybe too fancy.

Yeah, my initial plan was to make them std::string's, but there was some
reason (which I can't remember now, for the life of me) why I decided to
make them char *.  Anyway, I'll revisit this.

>> +
>> +/* See common/common-utils.h.  */
>> +
>> +std::string
>> +stringify_argv (std::vector<char *> &argv)
>> +{
>> +  std::string ret ("");
>> +
>> +  for (std::vector<char *>::iterator i = argv.begin () + 1;
>> +       i != argv.end ();
>> +       ++i)
>> +    ret += *i + std::string (" ");
>> +
>> +  return ret;
>> +}
>
> Same suggestion as above for the range for.
>
> {
>   for (char* arg : argv)
>     ret += arg + std::string (" ");
>
>   return ret;
> }
>
>> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
>> index 5e0cead..20c8dcd 100644
>> --- a/gdb/gdbserver/utils.h
>> +++ b/gdb/gdbserver/utils.h
>> @@ -19,7 +19,19 @@
>>  #ifndef UTILS_H
>>  #define UTILS_H
>>
>> +#include <vector>
>> +
>>  char *paddress (CORE_ADDR addr);
>>  char *pfildes (gdb_fildes_t fd);
>>
>> +/* Works like FREEARGV, but with std::vector.  */
>> +
>> +extern void free_vector_argv (std::vector<char *> &v);
>> +
>> +/* Given a vector of arguments ARGV, return a string equivalent to
>> +   joining all the arguments (starting from ARGV + 1) with a
>> +   whitespace separating them.  */
>> +
>> +extern std::string stringify_argv (std::vector<char *> &argv);
>> +
>>  #endif /* UTILS_H */
>> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
>> index 70abfcd..1f7f538 100644
>> --- a/gdb/gdbserver/win32-low.c
>> +++ b/gdb/gdbserver/win32-low.c
>> @@ -608,13 +608,11 @@ create_process (const char *program, char *args,
>>  }
>>
>>  /* Start a new process.
>> -   PROGRAM is a path to the program to execute.
>> -   ARGS is a standard NULL-terminated array of arguments,
>> -   to be passed to the inferior as ``argv''.
>> +   PROGRAM_ARGV is the vector containing the inferior's argv.
>>     Returns the new PID on success, -1 on failure.  Registers the new
>>     process with the process list.  */
>>  static int
>> -win32_create_inferior (char *program, char **program_args)
>> +win32_create_inferior (std::vector<char *> &program_argv)
>>  {
>>  #ifndef USE_WIN32API
>>    char real_path[PATH_MAX];
>> @@ -627,6 +625,9 @@ win32_create_inferior (char *program, char **program_args)
>>    int argc;
>>    PROCESS_INFORMATION pi;
>>    DWORD err;
>> +  char *program = program_argv[0];
>> +  std::string program_args = stringify_argv (program_argv);
>> +  char *args = (char *) program_args.c_str ();
>>
>>    /* win32_wait needs to know we're not attaching.  */
>>    attaching = 0;
>> @@ -636,6 +637,8 @@ win32_create_inferior (char *program, char **program_args)
>>
>>    flags = DEBUG_PROCESS | DEBUG_ONLY_THIS_PROCESS;
>>
>> +  pre_fork_inferior (program, argv);
>> +
>>  #ifndef USE_WIN32API
>>    orig_path = NULL;
>>    path_ptr = getenv ("PATH");
>> @@ -652,18 +655,6 @@ win32_create_inferior (char *program, char **program_args)
>>    program = real_path;
>>  #endif
>>
>> -  argslen = 1;
>> -  for (argc = 1; program_args[argc]; argc++)
>> -    argslen += strlen (program_args[argc]) + 1;
>> -  args = (char *) alloca (argslen);
>> -  args[0] = '\0';
>> -  for (argc = 1; program_args[argc]; argc++)
>> -    {
>> -      /* FIXME: Can we do better about quoting?  How does Cygwin
>> -	 handle this?  */
>> -      strcat (args, " ");
>> -      strcat (args, program_args[argc]);
>> -    }
>>    OUTMSG2 (("Command line is \"%s\"\n", args));
>>
>>  #ifdef CREATE_NEW_PROCESS_GROUP
>> @@ -704,6 +695,8 @@ win32_create_inferior (char *program, char **program_args)
>>
>>    do_initial_child_stuff (pi.hProcess, pi.dwProcessId, 0);
>>
>> +  post_fork_inferior (current_process_id, program_argv);
>> +
>>    return current_process_id;
>>  }
>>
>> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
>> index 85a53b8..3e2e226 100644
>> --- a/gdb/gnu-nat.c
>> +++ b/gdb/gnu-nat.c
>> @@ -2161,7 +2161,7 @@ gnu_create_inferior (struct target_ops *ops,
>>    thread_change_ptid (inferior_ptid,
>>  		      ptid_build (inf->pid, inf_pick_first_thread (), 0));
>>
>> -  startup_inferior (START_INFERIOR_TRAPS_EXPECTED);
>> +  startup_inferior (START_INFERIOR_TRAPS_EXPECTED, NULL, NULL);
>>    inf->pending_execs = 0;
>>    /* Get rid of the old shell threads.  */
>>    prune_threads ();
>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>> index 64aaabe..ceb1422 100644
>> --- a/gdb/inf-ptrace.c
>> +++ b/gdb/inf-ptrace.c
>> @@ -111,7 +111,7 @@ inf_ptrace_create_inferior (struct target_ops *ops,
>>
>>    discard_cleanups (back_to);
>>
>> -  startup_inferior (START_INFERIOR_TRAPS_EXPECTED);
>> +  startup_inferior (START_INFERIOR_TRAPS_EXPECTED, NULL, NULL);
>>
>>    /* On some targets, there must be some explicit actions taken after
>>       the inferior has been started up.  */
>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>> index 403c096..6dae6b2 100644
>> --- a/gdb/inferior.h
>> +++ b/gdb/inferior.h
>> @@ -42,6 +42,7 @@ struct target_desc_info;
>>
>>  #include "progspace.h"
>>  #include "registry.h"
>> +#include "common-inferior.h"
>>
>>  #include "symfile-add-flags.h"
>>
>> @@ -130,17 +131,6 @@ extern void child_terminal_init (struct target_ops *self);
>>
>>  extern void child_terminal_init_with_pgrp (int pgrp);
>>
>> -/* From fork-child.c */
>> -
>> -extern int fork_inferior (char *, char *, char **,
>> -			  void (*)(void),
>> -			  void (*)(int), void (*)(void), char *,
>> -                          void (*)(const char *,
>> -                                   char * const *, char * const *));
>> -
>> -
>> -extern void startup_inferior (int);
>> -
>>  extern char *construct_inferior_arguments (int, char **);
>>
>>  /* From infcmd.c */
>> diff --git a/gdb/procfs.c b/gdb/procfs.c
>> index ff814ba..5b64618 100644
>> --- a/gdb/procfs.c
>> +++ b/gdb/procfs.c
>> @@ -4380,7 +4380,7 @@ procfs_init_inferior (struct target_ops *ops, int pid)
>>    thread_change_ptid (pid_to_ptid (pid),
>>  		      ptid_build (pid, lwpid, 0));
>>
>> -  startup_inferior (START_INFERIOR_TRAPS_EXPECTED);
>> +  startup_inferior (START_INFERIOR_TRAPS_EXPECTED, NULL, NULL);
>>
>>  #ifdef SYS_syssgi
>>    /* On mips-irix, we need to stop the inferior early enough during
>> diff --git a/gdb/target.h b/gdb/target.h
>> index a54b3db..adec5f2 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -1529,17 +1529,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
>> @@ -1548,12 +1537,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);
>> -
>>  /* Return true if the target stack has a non-default
>>    "to_terminal_ours" method.  */
>>
>> diff --git a/gdb/target/target.h b/gdb/target/target.h
>> index 2f4c716..2f0d8ba 100644
>> --- a/gdb/target/target.h
>> +++ b/gdb/target/target.h
>> @@ -95,4 +95,21 @@ extern void target_mourn_inferior (ptid_t ptid);
>>
>>  extern int target_supports_multi_process (void);
>>
>> +/* Initialize the terminal settings we record for the inferior,
>> +   before we actually run the inferior.  */
>> +
>
> Spurious newline due to the following?
>
>
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Empty_line_between_subprogram_description_and_the_subprogram_implementation
>
>   "Note that this only applies to the case where the comment is placed
>   besides the subprogram implementation (typically in a .c file). In the
>   case of the documentation being placed next to the subprogram
>   declaration, then the comment should be placed immediately before the
>   declaration."

Yeah.

>> +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 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);
>> +
>>  #endif /* TARGET_COMMON_H */
>> diff --git a/gdb/testsuite/gdb.server/non-existing-program.exp b/gdb/testsuite/gdb.server/non-existing-program.exp
>> index f68029d..efdfb58 100644
>> --- a/gdb/testsuite/gdb.server/non-existing-program.exp
>> +++ b/gdb/testsuite/gdb.server/non-existing-program.exp
>> @@ -39,8 +39,14 @@ set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"]
>>  set msg "gdbserver exits cleanly"
>>  set saw_exiting 0
>>  expect {
>> -    # This is what we get on ptrace-based targets.
>> -    -re "stdin/stdout redirected.*No program to debug\r\nExiting\r\n$" {
>> +    # This is what we get on ptrace-based targets with
>> +    # startup-with-shell disabled.
>> +    -re "stdin/stdout redirected.*gdbserver: Cannot exec non-existing-program\r\ngdbserver: Error: No such file or directory\r\n\r\nDuring startup program exited with code 127\.\r\nExiting\r\n$" {
>> +	set saw_exiting 1
>> +	exp_continue
>> +    }
>> +    # Likewise, but with startup-with-shell enabled.
>> +    -re "stdin/stdout redirected.*exec: non-existing-program: not found\r\nDuring startup program exited with code 127\.\r\nExiting\r\n$" {
>>  	set saw_exiting 1
>>  	exp_continue
>>      }
>> diff --git a/gdb/top.h b/gdb/top.h
>> index 482ed3e..0c057d7 100644
>> --- a/gdb/top.h
>> +++ b/gdb/top.h
>> @@ -22,6 +22,7 @@
>>
>>  #include "buffer.h"
>>  #include "event-loop.h"
>> +#include "common-top.h"
>>
>>  struct tl_interp_info;
>>
>> @@ -144,14 +145,6 @@ struct ui
>>    struct ui_out *m_current_uiout;
>>  };
>>
>> -/* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
>> -   It always exists and is created automatically when GDB starts
>> -   up.  */
>> -extern struct ui *main_ui;
>> -
>> -/* The current UI.  */
>> -extern struct ui *current_ui;
>> -
>>  /* The list of all UIs.  */
>>  extern struct ui *ui_list;
>>
>> diff --git a/gdb/utils.c b/gdb/utils.c
>> index 6bf3716..026e9d7 100644
>> --- a/gdb/utils.c
>> +++ b/gdb/utils.c
>> @@ -3435,6 +3435,15 @@ strip_leading_path_elements (const char *path, int n)
>>    return p;
>>  }
>>
>> +/* See common/common-utils.h.  */
>> +
>> +void
>> +gdb_flush_out_err (void)
>> +{
>> +  gdb_flush (main_ui->m_gdb_stdout);
>> +  gdb_flush (main_ui->m_gdb_stderr);
>> +}
>> +
>>  /* Provide a prototype to silence -Wmissing-prototypes.  */
>>  extern initialize_file_ftype _initialize_utils;
>>
>>
>
> The patch looks reasonable to me, though it would be nice to do a
> second pass on a version without the git-copy in place. That way we
> can see the real changes for the code that was moved.

Yeah, agreed.  I'll take care to include the full diff next time.

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/