Re: [PATCH 0/4] gdb: Move construct_inferior_arguments to gdbsupport

Simon Marchi
2020-05-07T19:31:54Z

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> This moves the function construct_inferior_arguments from
> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>
> The intention is to use it from gdbserver in a follow-up commit.
>
> gdb/ChangeLog:
>
> 2020-04-29 Michael Weghorn <m.weghorn@posteo.de>
>
> * infcmd.c, inferior.h: (construct_inferior_arguments):
> Moved function from here to gdbsupport/common-inferior.{h,cc}
>
> gdbsupport/ChangeLog:
>
> 2020-04-29 Michael Weghorn <m.weghorn@posteo.de>
>
> * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
> Move function here from gdb/infcmd.c, gdb/inferior.h
> ---
> gdb/infcmd.c | 124 ----------------------------------
> gdb/inferior.h | 2 -
> gdbsupport/common-inferior.cc | 123 +++++++++++++++++++++++++++++++++
> gdbsupport/common-inferior.h | 2 +
> 4 files changed, 125 insertions(+), 126 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9bbb413d4e..8f7482347c 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -259,130 +259,6 @@ server's cwd if remote debugging.\n"));
> "when starting the inferior is \"%s\".\n"), cwd);
> }
>
> -\f
> -/* Compute command-line string given argument vector. This does the
> - same shell processing as fork_inferior. */
> -
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> -{
> - char *result;
> -
> - /* ARGC should always be at least 1, but we double check this
> - here. This is also needed to silence -Werror-stringop
> - warnings. */
> - gdb_assert (argc > 0);
> -
> - if (startup_with_shell)
> - {
> -#ifdef __MINGW32__
> - /* This holds all the characters considered special to the
> - Windows shells. */
> - static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
> - static const char quote = '"';
> -#else
> - /* This holds all the characters considered special to the
> - typical Unix shells. We include `^' because the SunOS
> - /bin/sh treats it as a synonym for `|'. */
> - static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
> - static const char quote = '\'';
> -#endif
> - int i;
> - int length = 0;
> - char *out, *cp;
> -
> - /* We over-compute the size. It shouldn't matter. */
> - for (i = 0; i < argc; ++i)
> - length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
> -
> - result = (char *) xmalloc (length);
> - out = result;
> -
> - for (i = 0; i < argc; ++i)
> - {
> - if (i > 0)
> - *out++ = ' ';
> -
> - /* Need to handle empty arguments specially. */
> - if (argv[i][0] == '\0')
> - {
> - *out++ = quote;
> - *out++ = quote;
> - }
> - else
> - {
> -#ifdef __MINGW32__
> - int quoted = 0;
> -
> - if (strpbrk (argv[i], special))
> - {
> - quoted = 1;
> - *out++ = quote;
> - }
> -#endif
> - for (cp = argv[i]; *cp; ++cp)
> - {
> - if (*cp == '\n')
> - {
> - /* A newline cannot be quoted with a backslash (it
> - just disappears), only by putting it inside
> - quotes. */
> - *out++ = quote;
> - *out++ = '\n';
> - *out++ = quote;
> - }
> - else
> - {
> -#ifdef __MINGW32__
> - if (*cp == quote)
> -#else
> - if (strchr (special, *cp) != NULL)
> -#endif
> - *out++ = '\\';
> - *out++ = *cp;
> - }
> - }
> -#ifdef __MINGW32__
> - if (quoted)
> - *out++ = quote;
> -#endif
> - }
> - }
> - *out = '\0';
> - }
> - else
> - {
> - /* In this case we can't handle arguments that contain spaces,
> - tabs, or newlines -- see breakup_args(). */
> - int i;
> - int length = 0;
> -
> - for (i = 0; i < argc; ++i)
> - {
> - char *cp = strchr (argv[i], ' ');
> - if (cp == NULL)
> - cp = strchr (argv[i], '\t');
> - if (cp == NULL)
> - cp = strchr (argv[i], '\n');
> - if (cp != NULL)
> - error (_("can't handle command-line "
> - "argument containing whitespace"));
> - length += strlen (argv[i]) + 1;
> - }
> -
> - result = (char *) xmalloc (length);
> - result[0] = '\0';
> - for (i = 0; i < argc; ++i)
> - {
> - if (i > 0)
> - strcat (result, " ");
> - strcat (result, argv[i]);
> - }
> - }
> -
> - return result;
> -}
> -\f
>
> /* This function strips the '&' character (indicating background
> execution) that is added as *the last* of the arguments ARGS of a
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 1ac51369df..95af474eed 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -184,8 +184,6 @@ extern void child_interrupt (struct target_ops *self);
> STARTUP_INFERIOR. */
> extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
>
> -extern char *construct_inferior_arguments (int, char **);
> -
> /* From infcmd.c */
>
> /* Initial inferior setup. Determines the exec file is not yet known,
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index ed16e89a52..71b9a11e02 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -24,3 +24,126 @@
> /* See common-inferior.h. */
>
> bool startup_with_shell = true;
> +
> +/* Compute command-line string given argument vector. This does the
> + same shell processing as fork_inferior. */

Let's just do a bit of spring cleaning and align this code with our current
standards while moving it. Move the function comment to the .h, and in the.cc
file write:

/* See common-inferior.h. */

Other than that, this patch LGTM.

Simon