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

Sergio Durigan Junior
2017-01-05T20:11:58Z

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