From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17497 invoked by alias); 5 Jan 2017 20:11:58 -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 17478 invoked by uid 89); 5 Jan 2017 20:11:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Internals, life 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; Thu, 05 Jan 2017 20:11:44 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 741968123A; Thu, 5 Jan 2017 20:11:44 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v05KBhU0014545 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 5 Jan 2017 15:11:44 -0500 From: Sergio Durigan Junior To: Luis Machado Cc: GDB Patches , Subject: Re: [PATCH 5/6] Share fork_inferior et al with gdbserver References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <1482464361-4068-6-git-send-email-sergiodj@redhat.com> <527b35bd-f2c2-094d-b10a-588df7ad7a3a@codesourcery.com> X-URL: http://blog.sergiodj.net Date: Thu, 05 Jan 2017 20:11:00 -0000 In-Reply-To: <527b35bd-f2c2-094d-b10a-588df7ad7a3a@codesourcery.com> (Luis Machado's message of "Tue, 3 Jan 2017 17:32:22 -0600") Message-ID: <87vattnu69.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-01/txt/msg00080.txt.bz2 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 >> >> 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 &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 &v) >> +{ >> + for (std::vector::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 &argv) >> +{ >> + std::string ret (""); >> + >> + for (std::vector::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 >> + >> 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 &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 &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 &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/