* [PATCH 01/16] libiberty/buildargv: POSIX behaviour for backslash handling
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 02/16] gdb/testsuite: add some xfail in gdb.base/startup-with-shell.exp Andrew Burgess
` (17 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This is a libiberty patch. I've posted this to the gcc-patches list
here:
https://inbox.sourceware.org/gcc-patches/24a8d878590403540bc9b579ba58805985a4d2f7.1701881419.git.aburgess@redhat.com/
However, GCC is currently in stage 4 of its release cycle. Based on
the timing of previous releases, I'm not expecting this patch to be
merged before April.
One option is clearly to just wait until GCC hits stage 1, and then
try to get this patch merged.
Another option would be to create a GDB only fork of buildargv which
includes this patch. In April if/when I manage to get this patch
merged I would remove out GDB local copy. Of course, there's a risk
that this patch isn't accepted into GCC, in which case we might be
stuck with a GDB only fork.
Either way, I figure the first step is to address any issues that are
raised with the rest of this series, this could well take until April
anyway, in which case GCC might be back in stage 1.
Thanks,
Andrew
---
GDB makes use of the libiberty function buildargv for splitting the
inferior (program being debugged) argument string in some situations.
I have recently been working to fix some edge cases issues in this
area of GDB, and have tracked done some of the unexpected behaviour to
the libiberty function buildargv, and how it handles backslash
escapes.
For reference, I've been mostly reading:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
The issues that I would like to fix are:
1. Backslashes within single quotes should not be treated as an
escape, thus: '\a' should split to \a, retaining the backslash.
2. Backslashes within double quotes should only act as an escape if
they are immediately before one of the characters $ (dollar),
` (backtick), " (double quote), ` (backslash), or \n (newline). In
all other cases a backslash should not be treated as an escape
character. Thus: "\a" should split to \a, but "\$" should split to
$.
3. A backslash-newline sequence should be treated as a line
continuation, both the backslash and the newline should be removed.
I've updated libiberty and also added some tests. All the existing
libiberty tests continue to pass, but I'm not sure if there is
additional testing that should be done.
---
libiberty/argv.c | 8 +++++--
libiberty/testsuite/test-expandargv.c | 34 +++++++++++++++++++++++++++
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/libiberty/argv.c b/libiberty/argv.c
index c2823d3e4ba..6bae4ca2ee9 100644
--- a/libiberty/argv.c
+++ b/libiberty/argv.c
@@ -224,9 +224,13 @@ char **buildargv (const char *input)
if (bsquote)
{
bsquote = 0;
- *arg++ = *input;
+ if (*input != '\n')
+ *arg++ = *input;
}
- else if (*input == '\\')
+ else if (*input == '\\'
+ && !squote
+ && (!dquote
+ || strchr ("$`\"\\\n", *(input + 1)) != NULL))
{
bsquote = 1;
}
diff --git a/libiberty/testsuite/test-expandargv.c b/libiberty/testsuite/test-expandargv.c
index 30f2337ef77..b8dcc6a269a 100644
--- a/libiberty/testsuite/test-expandargv.c
+++ b/libiberty/testsuite/test-expandargv.c
@@ -142,6 +142,40 @@ const char *test_data[] = {
"b",
0,
+ /* Test 7 - No backslash removal within single quotes. */
+ "'a\\$VAR' '\\\"'", /* Test 7 data */
+ ARGV0,
+ "@test-expandargv-7.lst",
+ 0,
+ ARGV0,
+ "a\\$VAR",
+ "\\\"",
+ 0,
+
+ /* Test 8 - Remove backslash / newline pairs. */
+ "\"ab\\\ncd\" ef\\\ngh", /* Test 8 data */
+ ARGV0,
+ "@test-expandargv-8.lst",
+ 0,
+ ARGV0,
+ "abcd",
+ "efgh",
+ 0,
+
+ /* Test 9 - Backslash within double quotes. */
+ "\"\\$VAR\" \"\\`\" \"\\\"\" \"\\\\\" \"\\n\" \"\\t\"", /* Test 9 data */
+ ARGV0,
+ "@test-expandargv-9.lst",
+ 0,
+ ARGV0,
+ "$VAR",
+ "`",
+ "\"",
+ "\\",
+ "\\n",
+ "\\t",
+ 0,
+
0 /* Test done marker, don't remove. */
};
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 02/16] gdb/testsuite: add some xfail in gdb.base/startup-with-shell.exp
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
2024-01-09 14:26 ` [PATCH 01/16] libiberty/buildargv: POSIX behaviour for backslash handling Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 03/16] gdb: Support some escaping of args with startup-with-shell being off Andrew Burgess
` (16 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
There are two tests that fail in gdb.base/startup-with-shell.exp when
using the native-extended-remote board. I plan to fix these issues in
this series, but not straight away.
It is easier to test each patch if I can just check for no unexpected
failures, so first lets mark the currently failing tests as xfail.
This change will be removed by the end of this series, at which point
the tests will be passing.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/testsuite/gdb.base/startup-with-shell.exp | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp
index 51b1e4fa922..f3ad4ec4cbb 100644
--- a/gdb/testsuite/gdb.base/startup-with-shell.exp
+++ b/gdb/testsuite/gdb.base/startup-with-shell.exp
@@ -55,10 +55,19 @@ proc initial_setup_simple { startup_with_shell run_args } {
}
}
+# Are we using 'remote' or 'extended-remote' protocol?
+set is_remote_p [expr [string equal [target_info gdb_protocol] \
+ "remote"] \
+ || [string equal [target_info gdb_protocol] \
+ "extended-remote"]]
+
## Run the actual tests
with_test_prefix "startup_with_shell = on; run_args = *.unique-extension" {
initial_setup_simple "on" "$unique_file_dir/*.unique-extension"
+ if { $is_remote_p } {
+ setup_xfail "*-*-*" gdb/28392
+ }
gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"$unique_file\"" \
"first argument expanded"
}
@@ -72,6 +81,9 @@ with_test_prefix "startup_with_shell = off; run_args = *.unique-extension" {
with_test_prefix "startup_with_shell = on; run_args = \$TEST" {
set env(TEST) "1234"
initial_setup_simple "on" "\$TEST"
+ if { $is_remote_p } {
+ setup_xfail "*-*-*" gdb/28392
+ }
gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"1234\"" \
"testing first argument"
unset env(TEST)
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 03/16] gdb: Support some escaping of args with startup-with-shell being off
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
2024-01-09 14:26 ` [PATCH 01/16] libiberty/buildargv: POSIX behaviour for backslash handling Andrew Burgess
2024-01-09 14:26 ` [PATCH 02/16] gdb/testsuite: add some xfail in gdb.base/startup-with-shell.exp Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 04/16] gdb: remove the !startup_with_shell path from construct_inferior_arguments Andrew Burgess
` (15 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Michael Weghorn, Andrew Burgess
From: Michael Weghorn <m.weghorn@posteo.de>
I (Andrew Burgess) have taken this patch from this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
I started off reviewing that series, but wanted to explore some
alternative strategies for solving the problems this series addresses.
However, this patch I think is super useful, so I've taken it mostly
as it was in the original series.
I have made a few minor cleanups, and I've also added some more tests.
Any bugs should be considered mine (Andrew's), but I've left the
original author (Michael Weghorn) in place as the GDB side changes are
mostly their work.
The function execv_argv::init_for_no_shell (gdb/nat/fork-inferior.c),
is passed a single string ALLARGS containing all of the inferior
arguments, and contains some custom code for splitting this argument
string into a vector of separate arguments. This function is used
when startup-with-shell is off (which is not the default).
The algorithm in this function was just splitting on whitespace
characters, and ignoring any quoting, so for example:
(gdb) set startup-with-shell off
(gdb) set args "first arg" second_arg
would result in three arguments ("first), (arg"), and (second_arg)
being passed to the inferior (the parenthesis are not part of the
parsed arguments).
This commit replaces this custom argument splitting with a use of the
existing gdb_argv class (which uses the libiberty buildargv function).
This does a better job of supporting quoting and escaping, so for the
example given above we now pass two arguments (first arg)
and (second_arg), which is certainly what I would have expected as a
GDB user.
This commit changes the 'execv_argv' class accordingly and drops the
optimization to have all the 'char *' in 'm_argv' point to a single
string rather than allocating a separate string for each arg. This is
needed because we are now going to be stripping some escaping from the
arguments, for example:
(gdb) set startup-with-shell off
(gdb) set args "literal \$"
In this case we will pass the single argument (literal $) to the
inferior, the escaping backslash will be removed. This might seem
strange as usually the backslash would be stripped by the shell, and
now we have no shell. However, I think the consistent behaviour is a
good thing; whether we start with a shell or not the escaping will be
removed.
Using gdb_argv will mean that quote characters are also stripped. If
we consider the first example again:
(gdb) set startup-with-shell off
(gdb) set args "first arg" second_arg
This is now going to pass (first arg) and (second_arg), the quotes
have been removed. If the user did want the original behaviour then
they are going to have to now do this:
(gdb) set startup-with-shell off
(gdb) set args \"first arg\" second_arg
or they could do this:
(gdb) set startup-with-shell off
(gdb) set args '"first' 'arg"' second_arg
This commit also extends the three tests that cover inferior argument
passing to cover the case where 'startup-with-shell' is off. All of
these new tests pass for native targets, but there are still problems
when using remote targets. For now I have (rather crudely) disabled
these tests for remote targets. Don't worry too much, the iffy
testsuite code will be removed by the end of this series, by which
point remote targets will be as functional as native targets (for
argument passing at least).
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/nat/fork-inferior.c | 84 +++-------
gdb/testsuite/gdb.base/args.exp | 65 +++++---
gdb/testsuite/gdb.base/inferior-args.exp | 106 +++++++++++--
gdb/testsuite/gdb.base/startup-with-shell.exp | 146 ++++++++++++++----
4 files changed, 273 insertions(+), 128 deletions(-)
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 968983b2021..30310bdd3ee 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -27,6 +27,7 @@
#include "gdbsupport/pathstuff.h"
#include "gdbsupport/signals-state-save-restore.h"
#include "gdbsupport/gdb_tilde_expand.h"
+#include "gdbsupport/buildargv.h"
#include <vector>
extern char **environ;
@@ -42,6 +43,11 @@ class execv_argv
execv_argv (const char *exec_file, const std::string &allargs,
const char *shell_file);
+ ~execv_argv ()
+ {
+ free_vector_argv (m_argv);
+ }
+
/* Return a pointer to the built argv, in the type expected by
execv. The result is (only) valid for as long as this execv_argv
object is live. We return a "char **" because that's the type
@@ -50,7 +56,7 @@ class execv_argv
strings to which the array point. */
char **argv ()
{
- return const_cast<char **> (&m_argv[0]);
+ return m_argv.data ();
}
private:
@@ -69,66 +75,28 @@ class execv_argv
const std::string &allargs,
const char *shell_file);
- /* The argument vector built. Holds non-owning pointers. Elements
- either point to the strings passed to the execv_argv ctor, or
- inside M_STORAGE. */
- std::vector<const char *> m_argv;
-
- /* Storage. In the no-shell case, this contains a copy of the
- arguments passed to the ctor, split by '\0'. In the shell case,
- this contains the quoted shell command. I.e., SHELL_COMMAND in
- {"$SHELL" "-c", SHELL_COMMAND, NULL}. */
- std::string m_storage;
+ /* The argument vector. This owns the strings within it. */
+ std::vector<char *> m_argv;
};
-/* Create argument vector for straight call to execvp. Breaks up
- ALLARGS into an argument vector suitable for passing to execvp and
- stores it in M_ARGV. E.g., on "run a b c d" this routine would get
- as input the string "a b c d", and as output it would fill in
- M_ARGV with the four arguments "a", "b", "c", "d". Each argument
- in M_ARGV points to a substring of a copy of ALLARGS stored in
- M_STORAGE. */
+/* Create argument vector for straight call to execvp. Breaks up ALLARGS
+ into an argument vector suitable for passing to execvp and stores it in
+ M_ARGV. EXEC_FILE is the executable to be run.
+
+ E.g., if EXEC_FILE is "foo", and the user does "run a b c d" then
+ ALLARGS would be "a b c d", and this function would fill M_ARGV with
+ give arguments "foo", "a", "b", "c", and "d". */
void
execv_argv::init_for_no_shell (const char *exec_file,
const std::string &allargs)
{
+ m_argv.push_back (xstrdup (exec_file));
- /* Save/work with a copy stored in our storage. The pointers pushed
- to M_ARGV point directly into M_STORAGE, which is modified in
- place with the necessary NULL terminators. This avoids N heap
- allocations and string dups when 1 is sufficient. */
- std::string &args_copy = m_storage = allargs;
+ gdb_argv argv (allargs.c_str ());
- m_argv.push_back (exec_file);
-
- for (size_t cur_pos = 0; cur_pos < args_copy.size ();)
- {
- /* Skip whitespace-like chars. */
- std::size_t pos = args_copy.find_first_not_of (" \t\n", cur_pos);
-
- if (pos != std::string::npos)
- cur_pos = pos;
-
- /* Find the position of the next separator. */
- std::size_t next_sep = args_copy.find_first_of (" \t\n", cur_pos);
-
- if (next_sep == std::string::npos)
- {
- /* No separator found, which means this is the last
- argument. */
- next_sep = args_copy.size ();
- }
- else
- {
- /* Replace the separator with a terminator. */
- args_copy[next_sep++] = '\0';
- }
-
- m_argv.push_back (&args_copy[cur_pos]);
-
- cur_pos = next_sep;
- }
+ for (const auto &a : argv)
+ m_argv.push_back (xstrdup (a));
/* NULL-terminate the vector. */
m_argv.push_back (NULL);
@@ -182,11 +150,7 @@ execv_argv::init_for_shell (const char *exec_file,
/* We're going to call a shell. */
bool escape_bang = escape_bang_in_quoted_argument (shell_file);
- /* We need to build a new shell command string, and make argv point
- to it. So build it in the storage. */
- std::string &shell_command = m_storage;
-
- shell_command = "exec ";
+ std::string shell_command = "exec ";
/* Add any exec wrapper. That may be a program name with arguments,
so the user must handle quoting. */
@@ -256,9 +220,9 @@ execv_argv::init_for_shell (const char *exec_file,
"-c" says to interpret the next arg as a shell command to
execute, and this command is "exec <target-program> <args>". */
m_argv.reserve (4);
- m_argv.push_back (shell_file);
- m_argv.push_back ("-c");
- m_argv.push_back (shell_command.c_str ());
+ m_argv.push_back (xstrdup (shell_file));
+ m_argv.push_back (xstrdup ("-c"));
+ m_argv.push_back (xstrdup (shell_command.c_str ()));
m_argv.push_back (NULL);
}
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index cb50a4872b5..f97f1089d69 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -29,30 +29,55 @@ if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
return -1
}
+set startup_with_shell_modes { "on" }
+if {!([target_info gdb_protocol] == "remote"
+ || [target_info gdb_protocol] == "extended-remote")} {
+ lappend startup_with_shell_modes "off"
+} else {
+ # Some of these tests will not work when using the remote protocol
+ # due to bug PR gdb/28392.
+ unsupported "gdbserver 'startup-with-shell off' broken PR gdb/28392"
+}
+
# NAME is the name to use for the tests and ARGLIST is the list of
# arguments that are passed to GDB when it is started.
+#
+# The optional RE_LIST is the list of patterns to check the arguments
+# against, these patterns should match ARGLIST. If the arguments are
+# expected to show up unmodified in the test output then RE_LIST can
+# be dropped, and this proc will reuse ARGLIST.
+
+proc args_test { name arglist {re_list {}} } {
+
+ # If RE_LIST is not supplied then we can reuse ARGLIST, this
+ # implies that the arguments will appear unmodified in the test
+ # output.
+ if {[llength $re_list] == 0} {
+ set re_list $arglist
+ }
-proc args_test { name arglist } {
- save_vars { ::GDBFLAGS } {
- set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
+ foreach_with_prefix startup_with_shell $::startup_with_shell_modes {
+ save_vars { ::GDBFLAGS } {
+ set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
- clean_restart $::binfile
+ clean_restart $::binfile
- runto_main
- gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
- gdb_continue_to_breakpoint "breakpoint for $name"
+ gdb_test_no_output "set startup-with-shell ${startup_with_shell}" \
+ "set startup-with-shell for $name"
- set expected_len [expr 1 + [llength $arglist]]
- gdb_test "print argc" "\\\$$::decimal = $expected_len" "argc for $name"
+ runto_main
+ gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
+ gdb_continue_to_breakpoint "breakpoint for $name"
- set i 1
- foreach arg $arglist {
- if { $arg eq "\n" } {
- set arg {\\n}
+ set expected_len [expr 1 + [llength $re_list]]
+ gdb_test "print argc" "\\\$$::decimal = $expected_len" "argc for $name"
+
+ set i 1
+ foreach arg $re_list {
+ gdb_test "print argv\[$i\]" "\\\$$::decimal = $::hex \"$arg\"" \
+ "argv\[$i\] for $name"
+ set i [expr $i + 1]
}
- gdb_test "print argv\[$i\]" "\\\$$::decimal = $::hex \"$arg\"" \
- "argv\[$i\] for $name"
- set i [expr $i + 1]
}
}
}
@@ -78,6 +103,10 @@ args_test "two empty with single quotes" {{1} {''} {''} {3}}
# Try with arguments containing literal newlines.
-args_test "one newline" {{1} "\n" {3}}
+args_test "one newline" {{1} "\n" {3}} {1 \\\\n 3}
+
+args_test "two newlines" {{1} "\n" "\n" {3}} {1 \\\\n \\\\n 3}
+
+args_test "lone single quote" {{1} \' {3}}
-args_test "two newlines" {{1} "\n" "\n" {3}}
+args_test "lone double quote" {{1} \" {3}} {1 \\\\\" 3}
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 2c920ab14ec..bffbcf1862d 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -25,16 +25,27 @@ if {[build_executable "failed to prepare" $testfile $srcfile \
return
}
-proc do_test { method } {
+# STARTUP_WITH_SHELL is either 'on' or 'off' and determines if the
+# inferior is started under a shell or not. INFERIOR_ARGS is the list
+# of inferior arguments. EXPECTED_RESULTS is the list of expected
+# results, one for each argument.
+#
+# When STUB_SUITABLE is true this test is suitable for use with
+# gdbserver, i.e. INFERIOR_ARGS can be passed through to
+# gdbserver_start via gdb_run_cmd. Some of the weird quoting used in
+# some of the tests doesn't seem to play well with gdbserver_start.
+# This is a TCL issue, not a gdbserver issue. Manually testing with
+# gdbserver shows no problems. It's just that when we try to invoke
+# gdbserver from TCL the argument quoting gets messed up. For tests
+# that are problematic, STUB_SUITABLE is false.
+proc do_test { method startup_with_shell inferior_args expected_results \
+ stub_suitable } {
global binfile hex
- # The second arg is an empty string on purpose. The last argument
- # must be the empty argument -- we once had a bug where that
- # wouldn't work!
- set inferior_args { "first arg" "" "third-arg" "'" "\"" " " "" }
-
clean_restart $binfile
+ gdb_test_no_output "set startup-with-shell $startup_with_shell"
+
if { $method == "start" } {
# The start command does not make sense for a stub.
if { [use_gdb_stub] } {
@@ -80,6 +91,10 @@ proc do_test { method } {
return -1
}
+ if { [use_gdb_stub] && !$stub_suitable } {
+ return
+ }
+
# The run command does not make sense for a stub, but GDB_RUN_CMD
# does the right thing when the target is a stub (start the stub,
# connect to it, and "continue").
@@ -110,18 +125,75 @@ proc do_test { method } {
error "invalid method $method"
}
+ set argc [expr [llength $expected_results] + 1]
+
# Now that we are stopped at main, inspect argc/argv.
- gdb_test "print argc" " = 8"
- gdb_test "print argv\[0\]" " = $hex \".*\""
- gdb_test "print argv\[1\]" " = $hex \"first arg\""
- gdb_test "print argv\[2\]" " = $hex \"\""
- gdb_test "print argv\[3\]" " = $hex \"third-arg\""
- gdb_test "print argv\[4\]" " = $hex \"'\""
- gdb_test "print argv\[5\]" " = $hex \"\\\\\"\""
- gdb_test "print argv\[6\]" " = $hex \" \""
- gdb_test "print argv\[7\]" " = $hex \"\""
+ gdb_test "print argc" " = $argc"
+ gdb_test "print argv\[0\]" " = $hex \"\[^\r\n\]+\""
+ for { set i 1 } { $i < $argc } { incr i } {
+ set idx [expr $i - 1]
+ gdb_test "print argv\[$i\]" " = [lindex $expected_results $idx]"
+ }
+}
+
+set test_desc_list []
+
+# test one
+# --------
+#
+# The second arg is an empty string on purpose. The last argument
+# must be the empty argument -- we once had a bug where that wouldn't
+# work!
+lappend test_desc_list [list "test one" \
+ true \
+ { "first arg" "" "third-arg" "'" "\"" " " "" } \
+ [list "$hex \"first arg\"" \
+ "$hex \"\"" \
+ "$hex \"third-arg\"" \
+ "$hex \"'\"" \
+ "$hex \"\\\\\"\"" \
+ "$hex \" \"" \
+ "$hex \"\"" ]]
+
+# test two
+# --------
+#
+# The argument being passed here is '"', that is a single double quote
+# contained within single quotes.
+#
+# I build the test descriptor using this mess of code to avoid having
+# unbalanced quotes, which messes up indentation and syntax
+# highlighting within (at least) emacs. The 'format' of ascii code 34
+# gives us the double quote character. Then I have to jump through
+# the rest of this mess in order to avoid TCL escaping the quote for
+# me. It's super important that what we send to GDB is '"' not '\"'.
+set item [list "test two" false]
+set cmd [format "lappend item \{ '%c' '\\%c' \}" 34 34]
+eval $cmd
+set bs "\\\\"
+lappend item [list "$hex \"$bs\"\"" "$hex \"$bs$bs$bs\"\""]
+lappend test_desc_list $item
+
+set startup_with_shell_modes { "on" }
+if {!([target_info gdb_protocol] == "remote"
+ || [target_info gdb_protocol] == "extended-remote")} {
+ lappend startup_with_shell_modes "off"
+} else {
+ # Due to PR gdb/28392 gdbserver doesn't currently support having
+ # startup-with-shell off, and then attempting to pass arguments
+ # containing whitespace.
+ unsupported "bug gdb/28392: gdbserver doesn't support this"
}
-foreach_with_prefix method { "start" "starti" "run" "set args" } {
- do_test $method
+
+foreach desc $test_desc_list {
+ lassign $desc name stub_suitable args re_list
+ with_test_prefix $name {
+ foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
+ foreach_with_prefix startup_with_shell $startup_with_shell_modes {
+ do_test $set_method $startup_with_shell $args $re_list \
+ $stub_suitable
+ }
+ }
+ }
}
diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp
index f3ad4ec4cbb..62bb5c9c882 100644
--- a/gdb/testsuite/gdb.base/startup-with-shell.exp
+++ b/gdb/testsuite/gdb.base/startup-with-shell.exp
@@ -47,14 +47,57 @@ proc initial_setup_simple { startup_with_shell run_args } {
gdb_test_no_output "set args $run_args" \
"set args \$run_args"
- set test "inferior started"
- if { [runto_main] } {
- pass $test
- } else {
- fail $test
+ return [runto_main]
+}
+
+# Start GDB, set the inferior arguments to ARGS, and then run to main.
+# Once at main, read the first argument from the inferior and compare
+# it to ON_RE if startup-with-shell is on, otherwise compare to
+# OFF_RE.
+#
+# If PROBLEMATIC_ON is true then when startup-with-shell is on we
+# expect the comparison to fail, so setup an xfail.
+#
+# If PROBLEMATIC_OFF is true then when startup-with-shell is off we
+# expect the comparison to fail, so setup an xfail.
+#
+# TESTNAME is a string used in the test names.
+proc run_test { args on_re off_re testname { problematic_on false } \
+ { problematic_off false } } {
+ foreach startup_with_shell { "on" "off" } {
+ with_test_prefix "$testname, startup_with_shell: ${startup_with_shell}" {
+ if {![initial_setup_simple $startup_with_shell $args]} {
+ return -1
+ }
+
+ if { $startup_with_shell } {
+ set re $on_re
+ set problematic $problematic_on
+ } else {
+ set re $off_re
+ set problematic $problematic_off
+ }
+
+ if { $problematic } {
+ setup_xfail "*-*-*" gdb/28392
+ }
+
+ gdb_test "print argv\[1\]" "\\\$$::decimal = $::hex $re" $testname
+ }
}
}
+# This is like the run_test proc except that RE is used as the
+# expected argument regexp when startup-with-shell is both on and off.
+# For the other arguments, see run_test.
+proc run_test_same { args re testname { problematic_on false } \
+ { problematic_off false } } {
+ run_test $args $re $re $testname $problematic_on $problematic_off
+}
+
+# The regexp to match a single '\' character.
+set bs "\\\\"
+
# Are we using 'remote' or 'extended-remote' protocol?
set is_remote_p [expr [string equal [target_info gdb_protocol] \
"remote"] \
@@ -63,36 +106,73 @@ set is_remote_p [expr [string equal [target_info gdb_protocol] \
## Run the actual tests
-with_test_prefix "startup_with_shell = on; run_args = *.unique-extension" {
- initial_setup_simple "on" "$unique_file_dir/*.unique-extension"
- if { $is_remote_p } {
- setup_xfail "*-*-*" gdb/28392
- }
- gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"$unique_file\"" \
- "first argument expanded"
-}
+run_test "$unique_file_dir/*.unique-extension" \
+ "\"$unique_file\"" \
+ "\"$unique_file_dir/\\\*\.unique-extension\"" \
+ "arg is glob" \
+ $is_remote_p
-with_test_prefix "startup_with_shell = off; run_args = *.unique-extension" {
- initial_setup_simple "off" "$unique_file_dir/*.unique-extension"
- gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"$unique_file_dir/\\\*\.unique-extension\"" \
- "first argument not expanded"
-}
+run_test_same "$unique_file_dir/\\*.unique-extension" \
+ "\"$unique_file_dir/\\\*\.unique-extension\"" \
+ "arg is escaped glob"
-with_test_prefix "startup_with_shell = on; run_args = \$TEST" {
+save_vars { env(TEST) } {
set env(TEST) "1234"
- initial_setup_simple "on" "\$TEST"
- if { $is_remote_p } {
- setup_xfail "*-*-*" gdb/28392
- }
- gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"1234\"" \
- "testing first argument"
- unset env(TEST)
+ run_test "\$TEST" \
+ "\"1234\"" \
+ "\"\\\$TEST\"" \
+ "arg is shell variable" \
+ $is_remote_p
+
+ run_test_same "\\\$TEST" \
+ "\"\\\$TEST\"" \
+ "arg is escaped shell variable"
}
-with_test_prefix "startup_with_shell = off; run_args = \$TEST" {
- set env(TEST) "1234"
- initial_setup_simple "off" "\$TEST"
- gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"\\\$TEST\"" \
- "testing first argument"
- unset env(TEST)
-}
+run_test_same "\"\\a\"" \
+ "\"${bs}${bs}a\"" \
+ "retain backslash in double quote arg" \
+ false $is_remote_p
+
+run_test_same "'\\a'" \
+ "\"${bs}${bs}a\"" \
+ "retain backslash in single quote arg" \
+ false $is_remote_p
+
+run_test_same "\"\\\$\"" \
+ "\"\\\$\"" \
+ "'\$' can be escaped in double quote arg"
+
+run_test_same "'\\\$'" \
+ "\"${bs}${bs}\\\$\"" \
+ "'\$' is not escaped in single quote arg" \
+ false $is_remote_p
+
+run_test_same "\"\\`\"" \
+ "\"\\`\"" \
+ "'`' can be escaped in double quote arg"
+
+run_test_same "'\\`'" \
+ "\"${bs}${bs}`\"" \
+ "'`' is not escaped in single quote arg" \
+ false $is_remote_p
+
+run_test_same "\"\\\"\"" \
+ "\"${bs}\"\"" \
+ "'\"' can be escaped in double quote arg" \
+ false $is_remote_p
+
+run_test_same "'\\\"'" \
+ "\"${bs}${bs}${bs}\"\"" \
+ "'\"' is not escaped in single quote arg" \
+ false $is_remote_p
+
+run_test_same "\"\\\\\"" \
+ "\"${bs}${bs}\"" \
+ "'\\' can be escaped in double quote arg" \
+ false $is_remote_p
+
+run_test_same "'\\\\'" \
+ "\"${bs}${bs}${bs}${bs}\"" \
+ "'\\' is not escaped in single quote arg" \
+ false $is_remote_p
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 04/16] gdb: remove the !startup_with_shell path from construct_inferior_arguments
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (2 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 03/16] gdb: Support some escaping of args with startup-with-shell being off Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-21 3:56 ` Keith Seitz
2024-01-09 14:26 ` [PATCH 05/16] gdbserver: convert program_args to a single string Andrew Burgess
` (14 subsequent siblings)
18 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
In the previous commit nat/fork-inferior.c was updated such that when
we are starting an inferior without a shell we now remove escape
characters. The benefits of this are explained in the previous
commit, but having made that change we can now make an additional
change.
Currently, in construct_inferior_arguments, when startup_with_shell is
false we construct the inferior argument string differently than when
startup_with_shell is true; when true we apply some escaping to
special shell character, while when false we don't.
This commit removes this special handling, and instead we now apply
escaping in all cases. This is fine because, thanks to the previous
commit the escaping will be correctly removed when we call into
nat/fork-inferior.c (thanks to the previous commit).
For GDB's native targets construct_inferior_arguments is reached via
two code paths; first when GDB starts and we combine arguments from
the command line, and second when the Python API is used to set the
arguments from a sequence.
Now, it might seem like removing this code path is a bad thing for
native targets. This path allowed a "neat" trick to work around a
limitation of GDB's command line argument processing. Consider this:
$ gdb --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "\$FOO".
Notice that the argument has become \$FOO, the '$' is now quoted.
This is because, by quoting the argument in the shell command that
started GDB, GDB was passed a literal $FOO with no quotes. In order
to ensure that the inferior sees this same value, GDB then added the
extra escape character. When GDB starts with a shell we pass \$FOO,
which results in the inferior seeing a literal $FOO.
But what if the user _actually_ wanted to have the shell GDB uses to
start the inferior expand $FOO? Well, it appears this can't be done
from the command line, but from the GDB prompt we can just do:
(gdb) set args $FOO
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
And now the inferior will see the shell expanded version of $FOO.
But there's no obvious way to achieve this from the GDB command line,
except with this trick:
$ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
(gdb) show startup-with-shell
Use of shell to start subprocesses is off.
And now the $FOO is not escaped, but GDB is no longer using a shell to
start the inferior, however, we can extend our command line like this:
$ gdb -eiex 'set startup-with-shell off' \
-ex 'set startup-with-shell on' \
--args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
(gdb) show startup-with-shell
Use of shell to start subprocesses is on.
We use an early-initialisation command line option to disable
startup-with-shell, this is done before command line argument
processing, then a normal initialisation option turns
startup-with-shell back on after GDB has processed the command line
arguments!
Is this useful? Yes, absolutely. Is this a good user experience?
Absolutely not. And a later patch in this series is going to add a
new command line option to GDB (and gdbserver) that will allow users
to achieve the same result (this trick doesn't work in gdbserver as
there's no early-initialisation there). So, the fact that I plan to
remove the ability to do this from GDB is not going to be a problem
once this complete series is merged.
Now, for remote targets the impact of this change is greater, and is
only a good thing. When arguments arrive in gdbserver from GDB we use
construct_inferior_arguments to build the argument string. After the
previous commit we know that calling nat/fork-inferior.c will always
remove one "level" of escapes; either the shell gdbserver spawns will
remove the escapes, or nat/fork-inferior.c will manually remove one
level of escapes.
What this means is that, if we don't add a level of escapes when
building the arguments in construct_inferior_arguments, we will end up
removing an additional level of escapes in nat/fork-inferior.c.
After this commit a whole set of tests that were added as xfail in the
previous commit are now passing.
A change similar to this one can be found in this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
which I reviewed before writing this patch. I don't think there's any
one patch in that series that exactly corresponds with this patch
though, so I've listed the author of the original series as co-author
on this patch.
Co-Authored-By: Michael Weghorn <m.weghorn@posteo.de>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/testsuite/gdb.base/args.exp | 12 +-
gdb/testsuite/gdb.base/inferior-args.exp | 49 ++++++--
gdb/testsuite/gdb.base/startup-with-shell.exp | 37 ++----
gdbsupport/common-inferior.cc | 108 +++++++-----------
4 files changed, 91 insertions(+), 115 deletions(-)
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index f97f1089d69..8b0047999bf 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -29,16 +29,6 @@ if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
return -1
}
-set startup_with_shell_modes { "on" }
-if {!([target_info gdb_protocol] == "remote"
- || [target_info gdb_protocol] == "extended-remote")} {
- lappend startup_with_shell_modes "off"
-} else {
- # Some of these tests will not work when using the remote protocol
- # due to bug PR gdb/28392.
- unsupported "gdbserver 'startup-with-shell off' broken PR gdb/28392"
-}
-
# NAME is the name to use for the tests and ARGLIST is the list of
# arguments that are passed to GDB when it is started.
#
@@ -56,7 +46,7 @@ proc args_test { name arglist {re_list {}} } {
set re_list $arglist
}
- foreach_with_prefix startup_with_shell $::startup_with_shell_modes {
+ foreach_with_prefix startup_with_shell { on off } {
save_vars { ::GDBFLAGS } {
set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index bffbcf1862d..4b51b657326 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -174,23 +174,48 @@ set bs "\\\\"
lappend item [list "$hex \"$bs\"\"" "$hex \"$bs$bs$bs\"\""]
lappend test_desc_list $item
-set startup_with_shell_modes { "on" }
-if {!([target_info gdb_protocol] == "remote"
- || [target_info gdb_protocol] == "extended-remote")} {
- lappend startup_with_shell_modes "off"
-} else {
- # Due to PR gdb/28392 gdbserver doesn't currently support having
- # startup-with-shell off, and then attempting to pass arguments
- # containing whitespace.
- unsupported "bug gdb/28392: gdbserver doesn't support this"
-}
-
+# test three
+# ----------
+#
+# This test focuses on sending special shell characters within a
+# double quote argument, and each special character is prefixed with a
+# backslash.
+#
+# In a POSIX shell, within a double quoted argument, only $ (dollar),
+# ` (backtick), " (double quote), \ (backslash), and newline can be
+# escaped. All other backslash characters are literal backslashes.
+#
+# As with the previous test, the double quotes are lost when the
+# arguments are sent through gdbserver_start, as such, this test isn't
+# going to work when using the native-gdbserver board, hence we set
+# the second arguemnt to 'false'.
+lappend test_desc_list [list "test three" \
+ false \
+ { "\&" "\<" "\#" "\^" "\>" "\$" "\`" } \
+ [list "$hex \"\\\\\\\\&\"" \
+ "$hex \"\\\\\\\\<\"" \
+ "$hex \"\\\\\\\\#\"" \
+ "$hex \"\\\\\\\\\\^\"" \
+ "$hex \"\\\\\\\\>\"" \
+ "$hex \"\\\$\"" \
+ "$hex \"`\""]]
+
+# test four
+# ---------
+#
+# This test passes two arguments, a single and double quote, each
+# escaped with a backslash.
+lappend test_desc_list [list "test four" \
+ true \
+ { \' \" } \
+ [list "$hex \"'\"" \
+ "$hex \"\\\\\"\""]]
foreach desc $test_desc_list {
lassign $desc name stub_suitable args re_list
with_test_prefix $name {
foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
- foreach_with_prefix startup_with_shell $startup_with_shell_modes {
+ foreach_with_prefix startup_with_shell { on off } {
do_test $set_method $startup_with_shell $args $re_list \
$stub_suitable
}
diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp
index 62bb5c9c882..0424b20de3a 100644
--- a/gdb/testsuite/gdb.base/startup-with-shell.exp
+++ b/gdb/testsuite/gdb.base/startup-with-shell.exp
@@ -58,12 +58,8 @@ proc initial_setup_simple { startup_with_shell run_args } {
# If PROBLEMATIC_ON is true then when startup-with-shell is on we
# expect the comparison to fail, so setup an xfail.
#
-# If PROBLEMATIC_OFF is true then when startup-with-shell is off we
-# expect the comparison to fail, so setup an xfail.
-#
# TESTNAME is a string used in the test names.
-proc run_test { args on_re off_re testname { problematic_on false } \
- { problematic_off false } } {
+proc run_test { args on_re off_re testname { problematic_on false } } {
foreach startup_with_shell { "on" "off" } {
with_test_prefix "$testname, startup_with_shell: ${startup_with_shell}" {
if {![initial_setup_simple $startup_with_shell $args]} {
@@ -75,7 +71,7 @@ proc run_test { args on_re off_re testname { problematic_on false } \
set problematic $problematic_on
} else {
set re $off_re
- set problematic $problematic_off
+ set problematic false
}
if { $problematic } {
@@ -90,9 +86,8 @@ proc run_test { args on_re off_re testname { problematic_on false } \
# This is like the run_test proc except that RE is used as the
# expected argument regexp when startup-with-shell is both on and off.
# For the other arguments, see run_test.
-proc run_test_same { args re testname { problematic_on false } \
- { problematic_off false } } {
- run_test $args $re $re $testname $problematic_on $problematic_off
+proc run_test_same { args re testname } {
+ run_test $args $re $re $testname
}
# The regexp to match a single '\' character.
@@ -131,13 +126,11 @@ save_vars { env(TEST) } {
run_test_same "\"\\a\"" \
"\"${bs}${bs}a\"" \
- "retain backslash in double quote arg" \
- false $is_remote_p
+ "retain backslash in double quote arg"
run_test_same "'\\a'" \
"\"${bs}${bs}a\"" \
- "retain backslash in single quote arg" \
- false $is_remote_p
+ "retain backslash in single quote arg"
run_test_same "\"\\\$\"" \
"\"\\\$\"" \
@@ -145,8 +138,7 @@ run_test_same "\"\\\$\"" \
run_test_same "'\\\$'" \
"\"${bs}${bs}\\\$\"" \
- "'\$' is not escaped in single quote arg" \
- false $is_remote_p
+ "'\$' is not escaped in single quote arg"
run_test_same "\"\\`\"" \
"\"\\`\"" \
@@ -154,25 +146,20 @@ run_test_same "\"\\`\"" \
run_test_same "'\\`'" \
"\"${bs}${bs}`\"" \
- "'`' is not escaped in single quote arg" \
- false $is_remote_p
+ "'`' is not escaped in single quote arg"
run_test_same "\"\\\"\"" \
"\"${bs}\"\"" \
- "'\"' can be escaped in double quote arg" \
- false $is_remote_p
+ "'\"' can be escaped in double quote arg"
run_test_same "'\\\"'" \
"\"${bs}${bs}${bs}\"\"" \
- "'\"' is not escaped in single quote arg" \
- false $is_remote_p
+ "'\"' is not escaped in single quote arg"
run_test_same "\"\\\\\"" \
"\"${bs}${bs}\"" \
- "'\\' can be escaped in double quote arg" \
- false $is_remote_p
+ "'\\' can be escaped in double quote arg"
run_test_same "'\\\\'" \
"\"${bs}${bs}${bs}${bs}\"" \
- "'\\' is not escaped in single quote arg" \
- false $is_remote_p
+ "'\\' is not escaped in single quote arg"
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index 55149ec1f13..076ddc73d51 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -32,92 +32,66 @@ construct_inferior_arguments (gdb::array_view<char * const> argv)
{
std::string result;
- 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 = '"';
+ /* 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 = '\'';
+ /* 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
- for (int i = 0; i < argv.size (); ++i)
+ for (int i = 0; i < argv.size (); ++i)
+ {
+ if (i > 0)
+ result += ' ';
+
+ /* Need to handle empty arguments specially. */
+ if (argv[i][0] == '\0')
{
- if (i > 0)
- result += ' ';
+ result += quote;
+ result += quote;
+ }
+ else
+ {
+#ifdef __MINGW32__
+ bool quoted = false;
- /* Need to handle empty arguments specially. */
- if (argv[i][0] == '\0')
+ if (strpbrk (argv[i], special))
{
- result += quote;
+ quoted = true;
result += quote;
}
- else
+#endif
+ for (char *cp = argv[i]; *cp; ++cp)
{
-#ifdef __MINGW32__
- bool quoted = false;
-
- if (strpbrk (argv[i], special))
+ if (*cp == '\n')
{
- quoted = true;
+ /* A newline cannot be quoted with a backslash (it
+ just disappears), only by putting it inside
+ quotes. */
+ result += quote;
+ result += '\n';
result += quote;
}
-#endif
- for (char *cp = argv[i]; *cp; ++cp)
+ else
{
- if (*cp == '\n')
- {
- /* A newline cannot be quoted with a backslash (it
- just disappears), only by putting it inside
- quotes. */
- result += quote;
- result += '\n';
- result += quote;
- }
- else
- {
#ifdef __MINGW32__
- if (*cp == quote)
+ if (*cp == quote)
#else
- if (strchr (special, *cp) != NULL)
+ if (strchr (special, *cp) != NULL)
#endif
- result += '\\';
- result += *cp;
- }
+ result += '\\';
+ result += *cp;
}
+ }
#ifdef __MINGW32__
- if (quoted)
- result += quote;
+ if (quoted)
+ result += quote;
#endif
- }
- }
- }
- else
- {
- /* In this case we can't handle arguments that contain spaces,
- tabs, or newlines -- see breakup_args(). */
- for (char *arg : argv)
- {
- char *cp = strchr (arg, ' ');
- if (cp == NULL)
- cp = strchr (arg, '\t');
- if (cp == NULL)
- cp = strchr (arg, '\n');
- if (cp != NULL)
- error (_("can't handle command-line "
- "argument containing whitespace"));
- }
-
- for (int i = 0; i < argv.size (); ++i)
- {
- if (i > 0)
- result += " ";
- result += argv[i];
}
}
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 04/16] gdb: remove the !startup_with_shell path from construct_inferior_arguments
2024-01-09 14:26 ` [PATCH 04/16] gdb: remove the !startup_with_shell path from construct_inferior_arguments Andrew Burgess
@ 2024-01-21 3:56 ` Keith Seitz
0 siblings, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:56 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 1/9/24 06:26, Andrew Burgess wrote:
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index 55149ec1f13..076ddc73d51 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -32,92 +32,66 @@ construct_inferior_arguments (gdb::array_view<char * const> argv)
> {
> std::string result;
>
> - 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 = '"';
> + /* 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 = '\'';
> + /* 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
> - for (int i = 0; i < argv.size (); ++i)
> + for (int i = 0; i < argv.size (); ++i)
> + {
> + if (i > 0)
> + result += ' ';
> +
> + /* Need to handle empty arguments specially. */
> + if (argv[i][0] == '\0')
> {
> - if (i > 0)
> - result += ' ';
> + result += quote;
> + result += quote;
> + }
> + else
> + {
> +#ifdef __MINGW32__
> + bool quoted = false;
>
> - /* Need to handle empty arguments specially. */
> - if (argv[i][0] == '\0')
> + if (strpbrk (argv[i], special))
> {
> - result += quote;
> + quoted = true;
> result += quote;
> }
> - else
> +#endif
> + for (char *cp = argv[i]; *cp; ++cp)
Re: "*cp": Didn't we move to explicit checks some time ago?
Keith
> {
> -#ifdef __MINGW32__
> - bool quoted = false;
> -
> - if (strpbrk (argv[i], special))
> + if (*cp == '\n')
> {
> - quoted = true;
> + /* A newline cannot be quoted with a backslash (it
> + just disappears), only by putting it inside
> + quotes. */
> + result += quote;
> + result += '\n';
> result += quote;
> }
> -#endif
> - for (char *cp = argv[i]; *cp; ++cp)
> + else
> {
> - if (*cp == '\n')
> - {
> - /* A newline cannot be quoted with a backslash (it
> - just disappears), only by putting it inside
> - quotes. */
> - result += quote;
> - result += '\n';
> - result += quote;
> - }
> - else
> - {
> #ifdef __MINGW32__
> - if (*cp == quote)
> + if (*cp == quote)
> #else
> - if (strchr (special, *cp) != NULL)
> + if (strchr (special, *cp) != NULL)
> #endif
> - result += '\\';
> - result += *cp;
> - }
> + result += '\\';
> + result += *cp;
> }
> + }
> #ifdef __MINGW32__
> - if (quoted)
> - result += quote;
> + if (quoted)
> + result += quote;
> #endif
> - }
> - }
> - }
> - else
> - {
> - /* In this case we can't handle arguments that contain spaces,
> - tabs, or newlines -- see breakup_args(). */
> - for (char *arg : argv)
> - {
> - char *cp = strchr (arg, ' ');
> - if (cp == NULL)
> - cp = strchr (arg, '\t');
> - if (cp == NULL)
> - cp = strchr (arg, '\n');
> - if (cp != NULL)
> - error (_("can't handle command-line "
> - "argument containing whitespace"));
> - }
> -
> - for (int i = 0; i < argv.size (); ++i)
> - {
> - if (i > 0)
> - result += " ";
> - result += argv[i];
> }
> }
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 05/16] gdbserver: convert program_args to a single string
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (3 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 04/16] gdb: remove the !startup_with_shell path from construct_inferior_arguments Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 06/16] gdbsupport: have construct_inferior_arguments take an escape function Andrew Burgess
` (13 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This commit changes how gdbserver stores the inferior arguments from
being a vector of separate arguments into a single string with all of
the arguments combined together.
Making this change might feel a little strange; intuitively it feels
like we would be better off storing the arguments as a vector, but in
the context of the upcoming patches, this change makes things easier.
First, GDB already stores the inferior arguments as a single string,
so doing this moves gdbserver into line with GDB. The common code
into which gdbserver calls requires the arguments to be a single
string, so currently each target's create_inferior implementation
merged the arguments anyway, so all this commit really does is move
the merging up the call stack, and store the merged result rather than
storing the separate parts.
However, the biggest reason for why this commit is needed, is an issue
with passing arguments from GDB to gdbserver when starting a new
inferior.
Consider:
(gdb) set args $VAR
(gdb) run
...
When using a native target the inferior will see the value of $VAR
expanded by the shell GDB uses to start the inferior. However, if
using an extended-remote target the inferior will see literally $VAR,
the unexpanded name of the variable, the reason for this is that,
although GDB sends '$VAR' to gdbserver, when gdbserver receives this,
it converts this to '\$VAR', which prevents the variable from being
expanded by the shell.
The reason for this is that construct_inferior_arguments escapes all
special shell characters within its arguments, and it is
construct_inferior_arguments that is used to combine the separate
arguments into a single string.
In a later commit I will change construct_inferior_arguments so that
it can apply different escaping strategies. When this happens we will
want to escape arguments coming from the gdbserver command line
differently than arguments coming from GDB (via a vRun packet), which
means we need to call construct_inferior_arguments earlier, at the
point where we know if the arguments came from the gdbserver command
line, or from the vRun packet.
This argument escaping issue is discussed in PR gdb/28392.
This commit doesn't fix any issues, nor does it change
construct_inferior_arguments to actually do different escaping, that
will all come in a later commit. This is purely a restructuring.
There should be no user visible changes after this commit.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdbserver/linux-low.cc | 5 ++---
gdbserver/linux-low.h | 2 +-
gdbserver/netbsd-low.cc | 6 ++----
gdbserver/netbsd-low.h | 2 +-
gdbserver/server.cc | 24 +++++++++++++++++++-----
gdbserver/target.h | 6 +++---
gdbserver/win32-low.cc | 7 +++----
gdbserver/win32-low.h | 2 +-
8 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 4aa011c14ec..640c9f330c7 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -986,7 +986,7 @@ linux_ptrace_fun ()
int
linux_process_target::create_inferior (const char *program,
- const std::vector<char *> &program_args)
+ const std::string &program_args)
{
client_state &cs = get_client_state ();
struct lwp_info *new_lwp;
@@ -996,10 +996,9 @@ linux_process_target::create_inferior (const char *program,
{
maybe_disable_address_space_randomization restore_personality
(cs.disable_randomization);
- std::string str_program_args = construct_inferior_arguments (program_args);
pid = fork_inferior (program,
- str_program_args.c_str (),
+ program_args.c_str (),
get_environ ()->envp (), linux_ptrace_fun,
NULL, NULL, NULL, NULL);
}
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 51d1899893a..a8d7a93aaeb 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -141,7 +141,7 @@ class linux_process_target : public process_stratum_target
public:
int create_inferior (const char *program,
- const std::vector<char *> &program_args) override;
+ const std::string &program_args) override;
void post_create_inferior () override;
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index 10d8d280b98..0fce43299ac 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -79,11 +79,9 @@ netbsd_ptrace_fun ()
int
netbsd_process_target::create_inferior (const char *program,
- const std::vector<char *> &program_args)
+ const std::string &program_args)
{
- std::string str_program_args = construct_inferior_arguments (program_args);
-
- pid_t pid = fork_inferior (program, str_program_args.c_str (),
+ pid_t pid = fork_inferior (program, program_args.c_str (),
get_environ ()->envp (), netbsd_ptrace_fun,
nullptr, nullptr, nullptr, nullptr);
diff --git a/gdbserver/netbsd-low.h b/gdbserver/netbsd-low.h
index 050b43fc54f..d62fbcf4c7a 100644
--- a/gdbserver/netbsd-low.h
+++ b/gdbserver/netbsd-low.h
@@ -42,7 +42,7 @@ class netbsd_process_target : public process_stratum_target
public:
int create_inferior (const char *program,
- const std::vector<char *> &program_args) override;
+ const std::string &program_args) override;
void post_create_inferior () override;
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 2ec97d7ade4..508e42ee097 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -122,7 +122,20 @@ private:
/* The program name, adjusted if needed. */
std::string m_path;
} program_path;
-static std::vector<char *> program_args;
+
+/* All program arguments are merged into a single string. This is similar
+ to how GDB manages the inferior arguments, and actually makes our lives
+ easier; the rules for how arguments are merged into a single string
+ differ depending on where the arguments come from. Arguments arriving
+ form the gdbserver command line are quoted, while arguments arriving
+ from GDB (via a vRun packet) are already quoted.
+
+ NOTE: The comment above is ahead of its time. The differences between
+ how the PROGRAM_ARGS string is built up have not yet been implemented.
+ A later patch in this series will make this change, and remove this
+ note. */
+static std::string program_args;
+
static std::string wrapper_argv;
/* The PID of the originally created or attached inferior. Used to
@@ -3424,9 +3437,8 @@ handle_v_run (char *own_buf)
else
program_path.set (new_program_name.get ());
- /* Free the old argv and install the new one. */
- free_vector_argv (program_args);
- program_args = new_argv;
+ program_args = construct_inferior_arguments (new_argv);
+ free_vector_argv (new_argv);
target_create_inferior (program_path.get (), program_args);
@@ -4304,8 +4316,10 @@ captured_main (int argc, char *argv[])
n = argc - (next_arg - argv);
program_path.set (next_arg[0]);
+ std::vector<char *> temp_arg_vector;
for (i = 1; i < n; i++)
- program_args.push_back (xstrdup (next_arg[i]));
+ temp_arg_vector.push_back (next_arg[i]);
+ program_args = construct_inferior_arguments (temp_arg_vector);
/* Wait till we are at first instruction in program. */
target_create_inferior (program_path.get (), program_args);
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 28d134e7915..5757aa6b84e 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -77,13 +77,13 @@ class process_stratum_target
/* Start a new process.
PROGRAM is a path to the program to execute.
- PROGRAM_ARGS is a standard NULL-terminated array of arguments,
- to be passed to the inferior as ``argv'' (along with PROGRAM).
+ PROGRAM_ARGS is a string containing all of the arguments that will be
+ used to start the inferior.
Returns the new PID on success, -1 on failure. Registers the new
process with the process list. */
virtual int create_inferior (const char *program,
- const std::vector<char *> &program_args) = 0;
+ const std::string &program_args) = 0;
/* Do additional setup after a new process is created, including
exec-wrapper completion. */
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 90d9510c7b9..e991152a29e 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -512,12 +512,12 @@ create_process (const char *program, char *args,
/* Start a new process.
PROGRAM is the program name.
- PROGRAM_ARGS is the vector containing the inferior's args.
+ PROGRAM_ARGS is a string containing all the inferior's arguments.
Returns the new PID on success, -1 on failure. Registers the new
process with the process list. */
int
win32_process_target::create_inferior (const char *program,
- const std::vector<char *> &program_args)
+ const std::string &program_args)
{
client_state &cs = get_client_state ();
#ifndef USE_WIN32API
@@ -528,8 +528,7 @@ win32_process_target::create_inferior (const char *program,
DWORD flags;
PROCESS_INFORMATION pi;
DWORD err;
- std::string str_program_args = construct_inferior_arguments (program_args);
- char *args = (char *) str_program_args.c_str ();
+ char *args = (char *) program_args.c_str ();
/* win32_wait needs to know we're not attaching. */
windows_process.attaching = 0;
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index 6a0b8cf9c84..1526567bfad 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -94,7 +94,7 @@ class win32_process_target : public process_stratum_target
public:
int create_inferior (const char *program,
- const std::vector<char *> &program_args) override;
+ const std::string &program_args) override;
int attach (unsigned long pid) override;
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 06/16] gdbsupport: have construct_inferior_arguments take an escape function
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (4 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 05/16] gdbserver: convert program_args to a single string Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 07/16] gdbsupport: split escape_shell_characters in two Andrew Burgess
` (12 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This commit is a refactor in order to make later commits in this
series easier, and continues the work started in the previous commit.
The construct_inferior_arguments function takes a vector of strings
and does two things:
1. Applies escaping to each string, and
2. Combines all the individual strings into a single string, placing
a space between each.
The construct_inferior_arguments function is used in a couple of
places within GDB and gdbserver, and in each place, the same escaping
algorithm is used; it has to be, the algorithm is hard-coded into the
function construct_inferior_arguments.
However, in later commits I will propose that in different situations
we should actually apply different escaping algorithms, depending on
whether the incoming arguments are already escaped or not.
As a simple concrete example, if we invoke current GDB like this:
$ gdb -q --args /tmp/application '$SHELL'
...
(gdb) show args
Argument list to give program being debugged when it is started is "\$SHELL".
Here you can see an example of the default escaping used by
construct_inferior_arguments, special shell characters are
escaped (the '$' becomes '\$').
The downside of this is that the application will see a literal
'$SHELL' string, not the shell expanded $SHELL variable
value (e.g. /bin/bash). There's currently no way to get the second
behaviour from the GDB command line.
This is discussed in PR gdb/28392.
If we wanted to add a new GDB command line flag, maybe like this:
$ gdb -q --no-escape-args /tmp/application '$SHELL'
...
(gdb) show args
Argument list to give program being debugged when it is started is "$SHELL".
Now, the argument string contains '$SHELL', with no escaping. When
the inferior is run (with startup-with-shell on), the application will
see the shell expanded value of $SHELL.
To achieve this we need construct_inferior_arguments to apply a
different escaping algorithm.
Another example of where the current construct_inferior_arguments
behaviour is not correct is in gdbserver. Like GDB, arguments from
the command line are escaped, however, we currently also escape
arguments arriving in the vRun packet.
Thus, if in GDB we do this:
(gdb) set args $SHELL
And then run an extended-remote target, the $SHELL will be escaped
when it is passed to gdbserver, gdbserver will then hold \$SHELL,
which means the inferior will see a literal '$SHELL' string, rather
than the shell expanded value of the variable. This is a real bug
that exists today in GDB/gdbserver.
Given the upcoming patches, I think there is more than a boolean
choice between two escaping algorithms, so, at least for now, I
propose that construct_inferior_arguments should take a function
pointer for a function that will perform the argument escaping.
If it turns out that this is excessive then it is easy enough to fold
the algorithm selecting back into construct_inferior_arguments and
instead pass a bool or enum to choose the correct algorithm, but I
think this approach is simple enough.
After this commit construct_inferior_arguments merges all the
arguments into a single string, using the worker function to escape
each argument in turn.
However, this commit doesn't actually fix any of the above issues.
This is a restructuring commit. All this commit does is change
construct_inferior_arguments to take the escaping function, I've split
the existing escaping function out from construct_inferior_arguments,
and I've updated every call to construct_inferior_arguments to pass in
the one and only escaping function. As a result there should be no
change in behaviour after this commit.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/inferior.c | 5 ++-
gdb/inferior.h | 3 +-
gdb/main.c | 4 +-
gdb/python/py-inferior.c | 2 +-
gdbserver/server.cc | 6 ++-
gdbsupport/common-inferior.cc | 81 ++++++++++++++++++++---------------
gdbsupport/common-inferior.h | 17 ++++++--
7 files changed, 73 insertions(+), 45 deletions(-)
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 076801db51b..ed138888024 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -168,9 +168,10 @@ inferior::tty ()
/* See inferior.h. */
void
-inferior::set_args (gdb::array_view<char * const> args)
+inferior::set_args (gdb::array_view<char * const> args,
+ escape_args_func escape_func)
{
- set_args (construct_inferior_arguments (args));
+ set_args (construct_inferior_arguments (args, escape_func));
}
void
diff --git a/gdb/inferior.h b/gdb/inferior.h
index e5173e0adac..d3824236ef6 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -526,7 +526,8 @@ class inferior : public refcounted_object,
};
/* Set the argument string from some strings. */
- void set_args (gdb::array_view<char * const> args);
+ void set_args (gdb::array_view<char * const> args,
+ escape_args_func escape_func);
/* Get the argument string to use when running this inferior.
diff --git a/gdb/main.c b/gdb/main.c
index 688db7655a9..015ed396f58 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -1084,8 +1084,8 @@ captured_main_1 (struct captured_main_args *context)
symarg = argv[optind];
execarg = argv[optind];
++optind;
- current_inferior ()->set_args
- (gdb::array_view<char * const> (&argv[optind], argc - optind));
+ gdb::array_view<char * const> arg_view (&argv[optind], argc - optind);
+ current_inferior ()->set_args (arg_view, escape_shell_characters);
}
else
{
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index ed153d668ac..8641d8a068b 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -919,7 +919,7 @@ infpy_set_args (PyObject *self, PyObject *value, void *closure)
for (const auto &arg : args)
argvec.push_back (arg.get ());
gdb::array_view<char * const> view (argvec.data (), argvec.size ());
- inf->inferior->set_args (view);
+ inf->inferior->set_args (view, escape_shell_characters);
}
else
{
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 508e42ee097..52ce9240ca3 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3437,7 +3437,8 @@ handle_v_run (char *own_buf)
else
program_path.set (new_program_name.get ());
- program_args = construct_inferior_arguments (new_argv);
+ program_args = construct_inferior_arguments (new_argv,
+ escape_shell_characters);
free_vector_argv (new_argv);
target_create_inferior (program_path.get (), program_args);
@@ -4319,7 +4320,8 @@ captured_main (int argc, char *argv[])
std::vector<char *> temp_arg_vector;
for (i = 1; i < n; i++)
temp_arg_vector.push_back (next_arg[i]);
- program_args = construct_inferior_arguments (temp_arg_vector);
+ program_args = construct_inferior_arguments (temp_arg_vector,
+ escape_shell_characters);
/* Wait till we are at first instruction in program. */
target_create_inferior (program_path.get (), program_args);
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index 076ddc73d51..f5620ec89aa 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -28,7 +28,26 @@ bool startup_with_shell = true;
/* See common-inferior.h. */
std::string
-construct_inferior_arguments (gdb::array_view<char * const> argv)
+construct_inferior_arguments (gdb::array_view<char * const> argv,
+ escape_args_func escape_func)
+{
+ std::string result;
+
+ for (const char *a : argv)
+ {
+ if (!result.empty ())
+ result += " ";
+
+ result += escape_func (a);
+ }
+
+ return result;
+}
+
+/* See common-inferior.h. */
+
+std::string
+escape_shell_characters (const char *arg)
{
std::string result;
@@ -44,55 +63,49 @@ construct_inferior_arguments (gdb::array_view<char * const> argv)
static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
static const char quote = '\'';
#endif
- for (int i = 0; i < argv.size (); ++i)
+
+ /* Need to handle empty arguments specially. */
+ if (arg[0] == '\0')
+ {
+ result += quote;
+ result += quote;
+ }
+ else
{
- if (i > 0)
- result += ' ';
+#ifdef __MINGW32__
+ bool quoted = false;
- /* Need to handle empty arguments specially. */
- if (argv[i][0] == '\0')
+ if (strpbrk (arg, special))
{
- result += quote;
+ quoted = true;
result += quote;
}
- else
+#endif
+ for (const char *cp = arg; *cp; ++cp)
{
-#ifdef __MINGW32__
- bool quoted = false;
-
- if (strpbrk (argv[i], special))
+ if (*cp == '\n')
{
- quoted = true;
+ /* A newline cannot be quoted with a backslash (it just
+ disappears), only by putting it inside quotes. */
+ result += quote;
+ result += '\n';
result += quote;
}
-#endif
- for (char *cp = argv[i]; *cp; ++cp)
+ else
{
- if (*cp == '\n')
- {
- /* A newline cannot be quoted with a backslash (it
- just disappears), only by putting it inside
- quotes. */
- result += quote;
- result += '\n';
- result += quote;
- }
- else
- {
#ifdef __MINGW32__
- if (*cp == quote)
+ if (*cp == quote)
#else
- if (strchr (special, *cp) != NULL)
+ if (strchr (special, *cp) != NULL)
#endif
- result += '\\';
- result += *cp;
- }
+ result += '\\';
+ result += *cp;
}
+ }
#ifdef __MINGW32__
- if (quoted)
- result += quote;
+ if (quoted)
+ result += quote;
#endif
- }
}
return result;
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index c468c7bf6a8..92d1954c3fe 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -57,9 +57,20 @@ extern const std::string &get_inferior_cwd ();
the target is started up with a shell. */
extern bool startup_with_shell;
-/* Compute command-line string given argument vector. This does the
- same shell processing as fork_inferior. */
+/* The type for a function used by construct_inferior_arguments to add any
+ quoting needed to an individual argument before combining all the
+ arguments into a single string. */
+using escape_args_func = std::string (*) (const char *arg);
+
+/* Return a version of ARG that has special shell characters escaped. */
+extern std::string escape_shell_characters (const char *arg);
+
+/* Pass each element of ARGS through ESCAPE_FUNC and combine the results
+ into a single string, separating each element with a single space
+ character. */
+
extern std::string
-construct_inferior_arguments (gdb::array_view<char * const>);
+construct_inferior_arguments (gdb::array_view<char * const> args,
+ escape_args_func escape_func);
#endif /* COMMON_COMMON_INFERIOR_H */
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 07/16] gdbsupport: split escape_shell_characters in two
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (5 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 06/16] gdbsupport: have construct_inferior_arguments take an escape function Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 08/16] gdb: move remote arg splitting and joining into gdbsupport/ Andrew Burgess
` (11 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
Building on the previous commit, this commit performs further
refactoring.
The escape_shell_characters function is split into two. We have a
worker core (the escape_characters function) which takes a set of
"special" characters that need escaping, and applies that escaping.
Then we have escape_shell_characters, which just calls
escape_characters with the correct set of special characters.
There should be no user visible changes after this commit.
This commit is similar to some of the changes made in this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
Though I don't think there's one commit that is exactly the same as
this one. But I've listed the author of the original series as a
Co-Author, because I feel the work is similar enough.
Co-Authored-By: Michael Weghorn <m.weghorn@posteo.de>
---
gdbsupport/common-inferior.cc | 49 +++++++++++++++++++++++++----------
1 file changed, 36 insertions(+), 13 deletions(-)
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index f5620ec89aa..6717f7d5c08 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -44,23 +44,23 @@ construct_inferior_arguments (gdb::array_view<char * const> argv,
return result;
}
-/* See common-inferior.h. */
-
-std::string
-escape_shell_characters (const char *arg)
+/* Escape characters in ARG and return an updated string. The string
+ SPECIAL contains the set of characters that must be escaped. SPECIAL
+ must not be nullptr, and it is assumed that SPECIAL contains the newline
+ '\n' character. It is assumed that ARG is not nullptr, but ARG can
+ be the empty string. */
+
+static std::string
+escape_characters (const char *arg, const char *special)
{
+ gdb_assert (special != nullptr);
+ gdb_assert (arg != nullptr);
+
std::string result;
#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
@@ -70,12 +70,16 @@ escape_shell_characters (const char *arg)
result += quote;
result += quote;
}
+ /* The special character handling code here assumes that if SPECIAL is
+ not nullptr, then SPECIAL will contain '\n'. This is true for all our
+ current usages, but if this ever changes in the future the following
+ might need reworking. */
else
{
#ifdef __MINGW32__
bool quoted = false;
- if (strpbrk (arg, special))
+ if (strpbrk (argv[i], special))
{
quoted = true;
result += quote;
@@ -96,7 +100,7 @@ escape_shell_characters (const char *arg)
#ifdef __MINGW32__
if (*cp == quote)
#else
- if (strchr (special, *cp) != NULL)
+ if (strchr (special, *cp) != nullptr)
#endif
result += '\\';
result += *cp;
@@ -110,3 +114,22 @@ escape_shell_characters (const char *arg)
return result;
}
+
+/* See common-inferior.h. */
+
+std::string
+escape_shell_characters (const char *arg)
+{
+#ifdef __MINGW32__
+ /* This holds all the characters considered special to the
+ Windows shells. */
+ static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+#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";
+#endif
+
+ return escape_characters (arg, special);
+}
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 08/16] gdb: move remote arg splitting and joining into gdbsupport/
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (6 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 07/16] gdbsupport: split escape_shell_characters in two Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-21 3:57 ` Keith Seitz
2024-01-09 14:26 ` [PATCH 09/16] gdb/python: change escaping rules when setting arguments Andrew Burgess
` (10 subsequent siblings)
18 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This is a refactoring commit. When passing inferior arguments to
gdbserver we have two actions that need to be performed, splitting and
joining.
On the GDB side, we take the inferior arguments, a single string, and
split the string into a list of individual arguments. These are then
sent to gdbserver over the remote protocol.
On the gdbserver side we receive the list of individual arguments and
join these back together into a single inferior argument string.
In a later commit I would like to add some unit testing for this
remote argument passing process. Ideally, for unit testing, we want
to use the exact same code for splitting and joining the arguments as
is actually used -- we could duplicate the code within the unit test,
and this would validate the algorithm as it is today, but there is
always the risk that a future change would not be mirrored within the
tests, which makes the tests useless.
So in this commit I propose to move the splitting and joining logic
out into a separate file, we can then use this within GDB and
gdbserver when passing arguments between GDB and gdbserver, but we can
also use the exact same code for some unit tests.
In this commit I'm not adding the unit tests, they will be added later
in this series, so for now there should be no user visible changes
after this commit.
---
gdb/remote.c | 12 +++++------
gdbserver/server.cc | 4 ++--
gdbsupport/Makefile.am | 1 +
gdbsupport/Makefile.in | 13 +++++++-----
gdbsupport/remote-args.cc | 43 ++++++++++++++++++++++++++++++++++++++
gdbsupport/remote-args.h | 44 +++++++++++++++++++++++++++++++++++++++
6 files changed, 104 insertions(+), 13 deletions(-)
create mode 100644 gdbsupport/remote-args.cc
create mode 100644 gdbsupport/remote-args.h
diff --git a/gdb/remote.c b/gdb/remote.c
index dcc1a0d0639..ebef409ffed 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -79,6 +79,7 @@
#include <unordered_map>
#include "async-event.h"
#include "gdbsupport/selftest.h"
+#include "gdbsupport/remote-args.h"
/* The remote target. */
@@ -10625,16 +10626,15 @@ remote_target::extended_remote_run (const std::string &args)
if (!args.empty ())
{
- int i;
+ std::vector<std::string> split_args = gdb::remote_args::split (args);
- gdb_argv argv (args.c_str ());
- for (i = 0; argv[i] != NULL; i++)
+ for (const auto &a : split_args)
{
- if (strlen (argv[i]) * 2 + 1 + len >= get_remote_packet_size ())
+ if (a.size () * 2 + 1 + len >= get_remote_packet_size ())
error (_("Argument list too long for run packet"));
rs->buf[len++] = ';';
- len += 2 * bin2hex ((gdb_byte *) argv[i], rs->buf.data () + len,
- strlen (argv[i]));
+ len += 2 * bin2hex ((gdb_byte *) a.c_str (), rs->buf.data () + len,
+ a.size ());
}
}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 52ce9240ca3..13abb0b7636 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -51,6 +51,7 @@
#include "gdbsupport/gdb_select.h"
#include "gdbsupport/scoped_restore.h"
#include "gdbsupport/search.h"
+#include "gdbsupport/remote-args.h"
/* PBUFSIZ must also be at least as big as IPA_CMD_BUF_SIZE, because
the client state data is passed directly to some agent
@@ -3437,8 +3438,7 @@ handle_v_run (char *own_buf)
else
program_path.set (new_program_name.get ());
- program_args = construct_inferior_arguments (new_argv,
- escape_shell_characters);
+ program_args = gdb::remote_args::join (new_argv);
free_vector_argv (new_argv);
target_create_inferior (program_path.get (), program_args);
diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index f1a641308fe..6f5a5ef36c6 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -72,6 +72,7 @@ libgdbsupport_a_SOURCES = \
pathstuff.cc \
print-utils.cc \
ptid.cc \
+ remote-args.cc \
rsp-low.cc \
run-time-clock.cc \
safe-strerror.cc \
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index 9fdc23c39a9..5d860302baa 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -163,11 +163,12 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
gdb_tilde_expand.$(OBJEXT) gdb_wait.$(OBJEXT) \
gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) netstuff.$(OBJEXT) \
new-op.$(OBJEXT) pathstuff.$(OBJEXT) print-utils.$(OBJEXT) \
- ptid.$(OBJEXT) rsp-low.$(OBJEXT) run-time-clock.$(OBJEXT) \
- safe-strerror.$(OBJEXT) scoped_mmap.$(OBJEXT) search.$(OBJEXT) \
- signals.$(OBJEXT) signals-state-save-restore.$(OBJEXT) \
- tdesc.$(OBJEXT) thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) \
- $(am__objects_1) $(am__objects_2)
+ ptid.$(OBJEXT) remote-args.$(OBJEXT) rsp-low.$(OBJEXT) \
+ run-time-clock.$(OBJEXT) safe-strerror.$(OBJEXT) \
+ scoped_mmap.$(OBJEXT) search.$(OBJEXT) signals.$(OBJEXT) \
+ signals-state-save-restore.$(OBJEXT) tdesc.$(OBJEXT) \
+ thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) $(am__objects_1) \
+ $(am__objects_2)
libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
AM_V_P = $(am__v_P_@AM_V@)
am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -429,6 +430,7 @@ libgdbsupport_a_SOURCES = \
pathstuff.cc \
print-utils.cc \
ptid.cc \
+ remote-args.cc \
rsp-low.cc \
run-time-clock.cc \
safe-strerror.cc \
@@ -538,6 +540,7 @@ distclean-compile:
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pathstuff.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/print-utils.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ptid.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/remote-args.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/rsp-low.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/run-time-clock.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/safe-strerror.Po@am__quote@
diff --git a/gdbsupport/remote-args.cc b/gdbsupport/remote-args.cc
new file mode 100644
index 00000000000..96c12ffac67
--- /dev/null
+++ b/gdbsupport/remote-args.cc
@@ -0,0 +1,43 @@
+/* Copyright (C) 2023 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/>. */
+
+#include "gdbsupport/common-defs.h"
+#include "gdbsupport/remote-args.h"
+#include "gdbsupport/common-inferior.h"
+#include "gdbsupport/buildargv.h"
+
+/* See remote-args.h. */
+
+std::vector<std::string>
+gdb::remote_args::split (std::string args)
+{
+ std::vector<std::string> results;
+
+ gdb_argv argv (args.c_str ());
+ for (int i = 0; argv[i] != nullptr; i++)
+ results.emplace_back (argv[i]);
+
+ return results;
+}
+
+/* See remote-args.h. */
+
+std::string
+gdb::remote_args::join (std::vector<char *> &args)
+{
+ return construct_inferior_arguments (args, escape_shell_characters);
+}
diff --git a/gdbsupport/remote-args.h b/gdbsupport/remote-args.h
new file mode 100644
index 00000000000..c0acce9b7c4
--- /dev/null
+++ b/gdbsupport/remote-args.h
@@ -0,0 +1,44 @@
+/* Functions to help when passing arguments between GDB and gdbserver.
+
+ Copyright (C) 2023 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/>. */
+
+namespace gdb
+{
+
+namespace remote_args
+{
+
+/* ARGS is an inferior argument string. This function splits ARGS into
+ individual arguments and returns a vector containing each argument. */
+
+extern std::vector<std::string> split (std::string args);
+
+/* Join together the separate arguments in ARGS and build a single
+ inferior argument string. The string returned by this function will be
+ equivalent, but not necessarily identical to the string passed to
+ ::split, for example passing the string '"a b"' (without the single
+ quotes, but including the double quotes) to ::split, will return an
+ argument of 'a b' (without the single quotes). When this argument is
+ passed through ::join we will get back the string 'a\ b' (without the
+ single quotes), that is, we choose to escape the white space, rather
+ than wrap the argument in quotes. */
+extern std::string join (std::vector<char *> &args);
+
+} /* namespace remote_args */
+
+} /* namespac gdb */
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 08/16] gdb: move remote arg splitting and joining into gdbsupport/
2024-01-09 14:26 ` [PATCH 08/16] gdb: move remote arg splitting and joining into gdbsupport/ Andrew Burgess
@ 2024-01-21 3:57 ` Keith Seitz
0 siblings, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:57 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 1/9/24 06:26, Andrew Burgess wrote:
> diff --git a/gdbsupport/remote-args.h b/gdbsupport/remote-args.h
> new file mode 100644
> index 00000000000..c0acce9b7c4
> --- /dev/null
> +++ b/gdbsupport/remote-args.h
> @@ -0,0 +1,44 @@
> +/* Functions to help when passing arguments between GDB and gdbserver.
> +
> + Copyright (C) 2023 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/>. */
> +
> +namespace gdb
> +{
> +
> +namespace remote_args
> +{
> +
I feel like a (generic?) statement about escaping characters would be
nice to have here. Maybe even just a reference to point folks
at gdb_argv?
Keith
> +/* ARGS is an inferior argument string. This function splits ARGS into
> + individual arguments and returns a vector containing each argument. */
> +
> +extern std::vector<std::string> split (std::string args);
> +
> +/* Join together the separate arguments in ARGS and build a single
> + inferior argument string. The string returned by this function will be
> + equivalent, but not necessarily identical to the string passed to
> + ::split, for example passing the string '"a b"' (without the single
> + quotes, but including the double quotes) to ::split, will return an
> + argument of 'a b' (without the single quotes). When this argument is
> + passed through ::join we will get back the string 'a\ b' (without the
> + single quotes), that is, we choose to escape the white space, rather
> + than wrap the argument in quotes. */
> +extern std::string join (std::vector<char *> &args);
> +
> +} /* namespace remote_args */
> +
> +} /* namespac gdb */
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 09/16] gdb/python: change escaping rules when setting arguments
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (7 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 08/16] gdb: move remote arg splitting and joining into gdbsupport/ Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 16:30 ` Eli Zaretskii
2024-01-09 14:26 ` [PATCH 10/16] gdb: add remote argument passing self tests Andrew Burgess
` (9 subsequent siblings)
18 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
It is possible to set an inferior's arguments through the Python API
by assigning to the gdb.Inferior.arguments attribute.
This attribute can be assigned a string, in which case the string is
taken verbatim as the inferior's argument string. Or this attribute
can be assigned a sequence, in which case the members of the sequence
are combined (with some escaping applied) to create the inferior's
argument string.
Currently, the when the arguments from a Python list are escaped, we
use escape_shell_characters. I suspect the reasons for this are
mostly accidental.
When the gdb.Inferior.arguments attribute was introduced in commit:
commit 3153113252f3b949a159439a17e88af8ff0dce30
Date: Mon May 1 13:53:59 2023 -0600
Add attributes and methods to gdb.Inferior
GDB's inferior::set_args method called construct_inferior_arguments,
and construct_inferior_arguments didn't take an escaping function as a
parameter, the only option was escape_shell_characters as that was the
escaping hard-coded into construct_inferior_arguments. The commit
message makes no comments for or against escaping of special shell
characters, and no tests were added that checked this behaviour. All
of this leads me to think that the handling of special shell
characters wasn't really considered (an understandable oversight).
But I'd like to consider it, and I think the current behaviour is not
ideal.
Consider this case:
(gdb) python gdb.selected_inferior().arguments = ['$VAR']
(gdb) show args
Argument list to give program being debugged when it is started is "\$VAR".
This means that when the inferior is run it will see literal '$VAR' as
its argument. If instead, the user wants to pass the shell expanded
value of $VAR to the inferior, there's no way to achieve this result
using the list assignment method.
In this commit I propose that we change this behaviour so that we
instead see this:
(gdb) python gdb.selected_inferior().arguments = ['$VAR']
(gdb) show args
Argument list to give program being debugged when it is started is "$VAR".
Now the '$' character is not escaped. If the inferior is started
under a shell then the user will see the shell expanded value of
'$VAR'.
Of course, if the user wants to pass a literal '$VAR' (with no
expansion) then they can do:
(gdb) python gdb.selected_inferior().arguments = ['\$VAR']
This actually feels more natural to me, the user writes the argument
as they would present it to a shell.
So, after this commit, GDB only escapes quote characters and white
space characters. This keeps some level of backward compatibility
with the existing behaviour (for things other than shell special
characters), but also seems natural, from the user's point of view,
the arguments they are providing are already quoted (by Python's
string quotes) so there's no need to quote white space. It's only
when GDB converts the Python sequence into a single string that the
white space actually needs quoting.
There are tests for the updated functionality, and I've updated the
docs and added a NEWS entry.
---
gdb/NEWS | 4 +++
gdb/doc/python.texi | 7 +++--
gdb/python/py-inferior.c | 2 +-
gdb/testsuite/gdb.python/py-inferior.exp | 36 ++++++++++++++++++++----
gdbsupport/common-inferior.cc | 14 +++++++++
gdbsupport/common-inferior.h | 5 ++++
6 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index 11cd6c0663e..b72ba3d87e8 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -87,6 +87,10 @@ show remote thread-options-packet
** New function gdb.interrupt(), that interrupts GDB as if the user
typed control-c.
+ ** When assigning a sequence to gdb.Inferior.arguments, only quote
+ and whitespace characters will be escaped. Everything else will
+ be left unmodified.
+
* Debugger Adapter Protocol changes
** GDB now emits the "process" event.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index d74defeec0c..e04e79cafa5 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3580,8 +3580,11 @@
Either a string or a sequence of strings can be assigned to this
attribute. When a string is assigned, it is assumed to have any
-necessary quoting for the shell; when a sequence is assigned, the
-quoting is applied by @value{GDBN}.
+necessary quoting for the shell; when a sequence is assigned, quoting
+is applied by @value{GDBN} so that the individual strings can be
+concatenated into a single string, with a single space between each
+argument. This means that shell quote characters and whitespace
+characters will be escaped.
@end defvar
A @code{gdb.Inferior} object has the following methods:
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 8641d8a068b..5b7c7fb9365 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -919,7 +919,7 @@ infpy_set_args (PyObject *self, PyObject *value, void *closure)
for (const auto &arg : args)
argvec.push_back (arg.get ());
gdb::array_view<char * const> view (argvec.data (), argvec.size ());
- inf->inferior->set_args (view, escape_shell_characters);
+ inf->inferior->set_args (view, escape_quotes_and_white_space);
}
else
{
diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 6fbcdd6822f..108c85c0165 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -410,11 +410,37 @@ gdb_test "show args" \
[string_to_regexp "Argument list to give program being debugged when it is started is \"a b c\"."] \
"show args from string"
-gdb_test_no_output "python gdb.selected_inferior().arguments = \['a', 'b c'\]" \
- "set arguments from list"
-gdb_test "show args" \
- [string_to_regexp "Argument list to give program being debugged when it is started is \"a b\\ c\"."] \
- "show args from list"
+# Test setting inferior arguments from a Python list. INPUT is a
+# single string that contains the Python list, this is inserted into
+# the argument setting command, and should include the surrouning
+# square brackets.
+#
+# The OUTPUT is the string that describes the arguments as GDB will
+# have stored them within the inferior, as seen in the 'show args'
+# command output. OUTPUT should include the surrounding quotes.
+# OUTPUT will be passed through string_to_regexp, so should be a plain
+# string, not a regexp.
+proc test_setting_arguments_from_list { input output } {
+ with_test_prefix "input: ${input}" {
+ gdb_test_no_output "python gdb.selected_inferior().arguments = ${input}" \
+ "set arguments from list"
+ gdb_test "show args" \
+ [string_to_regexp \
+ "Argument list to give program being debugged when it is started is ${output}."] \
+ "show args from list"
+ }
+}
+
+# Test setting inferior arguments from a list. Try to hit all the
+# potentially problematic cases. Notice that shell characters are not
+# automatically quoted, if a user wants a shell character quoted then
+# they must do that themselves.
+test_setting_arguments_from_list "\['a', 'b c'\]" "\"a b\\ c\""
+test_setting_arguments_from_list "\[' ', '\\t', '\\n']" "\"\\ \\\t '\r\n'\""
+test_setting_arguments_from_list "\['', '']" "\"'' ''\""
+test_setting_arguments_from_list "\['\"']" "\"\\\"\""
+test_setting_arguments_from_list "\[\"'\"]" "\"\\\'\""
+test_setting_arguments_from_list "\[\"\$VAR\", \";\"]" "\"\$VAR ;\""
gdb_test_no_output "python gdb.selected_inferior().clear_env()" \
"clear environment"
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index 6717f7d5c08..cf2cd9a090a 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -133,3 +133,17 @@ escape_shell_characters (const char *arg)
return escape_characters (arg, special);
}
+
+/* See common-inferior.h. */
+
+std::string
+escape_quotes_and_white_space (const char * arg)
+{
+#ifdef __MINGW32__
+ static const char special[] = "\" \t\n";
+#else
+ static const char special[] = "\"' \t\n";
+#endif
+
+ return escape_characters (arg, special);
+}
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 92d1954c3fe..7cc01fb2f28 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -65,6 +65,11 @@ using escape_args_func = std::string (*) (const char *arg);
/* Return a version of ARG that has special shell characters escaped. */
extern std::string escape_shell_characters (const char *arg);
+/* Return a version of ARG that has quote characters and white space
+ characters escaped. No other special shell characters will have been
+ escaped though. */
+extern std::string escape_quotes_and_white_space (const char *arg);
+
/* Pass each element of ARGS through ESCAPE_FUNC and combine the results
into a single string, separating each element with a single space
character. */
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 09/16] gdb/python: change escaping rules when setting arguments
2024-01-09 14:26 ` [PATCH 09/16] gdb/python: change escaping rules when setting arguments Andrew Burgess
@ 2024-01-09 16:30 ` Eli Zaretskii
0 siblings, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:30 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:32 +0000
>
> It is possible to set an inferior's arguments through the Python API
> by assigning to the gdb.Inferior.arguments attribute.
>
> This attribute can be assigned a string, in which case the string is
> taken verbatim as the inferior's argument string. Or this attribute
> can be assigned a sequence, in which case the members of the sequence
> are combined (with some escaping applied) to create the inferior's
> argument string.
>
> Currently, the when the arguments from a Python list are escaped, we
> use escape_shell_characters. I suspect the reasons for this are
> mostly accidental.
>
> When the gdb.Inferior.arguments attribute was introduced in commit:
>
> commit 3153113252f3b949a159439a17e88af8ff0dce30
> Date: Mon May 1 13:53:59 2023 -0600
>
> Add attributes and methods to gdb.Inferior
>
> GDB's inferior::set_args method called construct_inferior_arguments,
> and construct_inferior_arguments didn't take an escaping function as a
> parameter, the only option was escape_shell_characters as that was the
> escaping hard-coded into construct_inferior_arguments. The commit
> message makes no comments for or against escaping of special shell
> characters, and no tests were added that checked this behaviour. All
> of this leads me to think that the handling of special shell
> characters wasn't really considered (an understandable oversight).
>
> But I'd like to consider it, and I think the current behaviour is not
> ideal.
>
> Consider this case:
>
> (gdb) python gdb.selected_inferior().arguments = ['$VAR']
> (gdb) show args
> Argument list to give program being debugged when it is started is "\$VAR".
>
> This means that when the inferior is run it will see literal '$VAR' as
> its argument. If instead, the user wants to pass the shell expanded
> value of $VAR to the inferior, there's no way to achieve this result
> using the list assignment method.
>
> In this commit I propose that we change this behaviour so that we
> instead see this:
>
> (gdb) python gdb.selected_inferior().arguments = ['$VAR']
> (gdb) show args
> Argument list to give program being debugged when it is started is "$VAR".
>
> Now the '$' character is not escaped. If the inferior is started
> under a shell then the user will see the shell expanded value of
> '$VAR'.
>
> Of course, if the user wants to pass a literal '$VAR' (with no
> expansion) then they can do:
>
> (gdb) python gdb.selected_inferior().arguments = ['\$VAR']
>
> This actually feels more natural to me, the user writes the argument
> as they would present it to a shell.
>
> So, after this commit, GDB only escapes quote characters and white
> space characters. This keeps some level of backward compatibility
> with the existing behaviour (for things other than shell special
> characters), but also seems natural, from the user's point of view,
> the arguments they are providing are already quoted (by Python's
> string quotes) so there's no need to quote white space. It's only
> when GDB converts the Python sequence into a single string that the
> white space actually needs quoting.
>
> There are tests for the updated functionality, and I've updated the
> docs and added a NEWS entry.
> ---
> gdb/NEWS | 4 +++
> gdb/doc/python.texi | 7 +++--
> gdb/python/py-inferior.c | 2 +-
> gdb/testsuite/gdb.python/py-inferior.exp | 36 ++++++++++++++++++++----
> gdbsupport/common-inferior.cc | 14 +++++++++
> gdbsupport/common-inferior.h | 5 ++++
> 6 files changed, 60 insertions(+), 8 deletions(-)
The documentation parts are okay, thanks.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 10/16] gdb: add remote argument passing self tests
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (8 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 09/16] gdb/python: change escaping rules when setting arguments Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-21 3:57 ` Keith Seitz
2024-01-09 14:26 ` [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string Andrew Burgess
` (8 subsequent siblings)
18 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This commit adds some remote argument passing self-tests. There are
not many tests right now -- there are known bugs in the remote
argument passing mechanism (see PR gdb/28392) -- but some simple cases
are covered here, and I plan to add additional tests once I've fixed
some of the problems with the existing code.
The tests take an inferior argument string, this is the string that
GDB would carry around as inferior::m_args. This string is then split
using gdb::remote_args::split, this gives a vector of strings, these
are the strings that are passed over the remote protocol. These split
strings are validated as part of the test.
The split strings are then combined using gdb::remote_args::join which
gives the inferior argument string that gdbserver will use, this is
held in server.cc, program_args, this joined string is then checked as
part of the test.
There are no changes to GDB's behaviour as part of this commit, other
than adding the new tests.
---
gdb/Makefile.in | 1 +
gdb/unittests/remote-arg-selftests.c | 189 +++++++++++++++++++++++++++
2 files changed, 190 insertions(+)
create mode 100644 gdb/unittests/remote-arg-selftests.c
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 195f3a2e2d1..cc2e62b6f3c 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -476,6 +476,7 @@ SELFTESTS_SRCS = \
unittests/ptid-selftests.c \
unittests/main-thread-selftests.c \
unittests/mkdir-recursive-selftests.c \
+ unittests/remote-arg-selftests.c \
unittests/rsp-low-selftests.c \
unittests/scoped_fd-selftests.c \
unittests/scoped_ignore_signal-selftests.c \
diff --git a/gdb/unittests/remote-arg-selftests.c b/gdb/unittests/remote-arg-selftests.c
new file mode 100644
index 00000000000..3240ab9aeea
--- /dev/null
+++ b/gdb/unittests/remote-arg-selftests.c
@@ -0,0 +1,189 @@
+/* Self tests for GDB's argument splitting and merging.
+
+ Copyright (C) 2023 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/>. */
+
+#include "defs.h"
+#include "gdbsupport/selftest.h"
+#include "gdbsupport/buildargv.h"
+#include "gdbsupport/common-inferior.h"
+#include "gdbsupport/remote-args.h"
+
+namespace selftests {
+namespace remote_args_tests {
+
+/* The data needed to perform a single remote argument test. */
+struct arg_test_desc
+{
+ /* The original inferior argument string. */
+ std::string input;
+
+ /* The individual arguments once they have been split. */
+ std::vector<std::string> split;
+
+ /* The new inferior argument string, created by joining SPLIT. */
+ std::string joined;
+};
+
+/* The list of tests. */
+arg_test_desc desc[] = {
+ { "abc", { "abc" }, "abc" },
+ { "a b c", { "a", "b", "c" }, "a b c" },
+ { "\"a b\" 'c d'", { "a b", "c d" }, "a\\ b c\\ d" },
+ { "\\' \\\"", { "'", "\"" }, "\\' \\\"" },
+ { "'\\'", { "\\" }, "\\\\" },
+ { "\"\\\\\" \"\\\\\\\"\"", { "\\", "\\\"" }, "\\\\ \\\\\\\"" },
+ { "\\ \" \" ' '", { " ", " ", " "}, "\\ \\ \\ " },
+ { "\"'\"", { "'" }, "\\'" },
+ { "'\"' '\\\"'", { "\"", "\\\"" } , "\\\" \\\\\\\""},
+ { "\"first arg\" \"\" \"third-arg\" \"'\" \"\\\"\" \"\\\\\\\"\" \" \" \"\"",
+ { "first arg", "", "third-arg", "'", "\"", "\\\""," ", "" },
+ "first\\ arg '' third-arg \\' \\\" \\\\\\\" \\ ''"},
+ { "\"\\a\" \"\\&\" \"\\#\" \"\\<\" \"\\^\"",
+ { "\\a", "\\&", "\\#" , "\\<" , "\\^"},
+ "\\\\a \\\\\\& \\\\\\# \\\\\\< \\\\\\^" },
+ { "1 '\n' 3", { "1", "\n", "3" }, "1 '\n' 3" },
+};
+
+/* Convert a std::vector<std::string> into std::vector<char *>. This
+ requires copying all of the string content. This class takes care of
+ freeing the memory once we are done with it. */
+
+struct args_as_c_strings
+{
+ args_as_c_strings (std::vector<std::string> args)
+ {
+ for (const auto & a : args)
+ m_data.push_back (xstrdup (a.c_str ()));
+ }
+
+ ~args_as_c_strings ()
+ {
+ free_vector_argv (m_data);
+ }
+
+ std::vector<char *> &get ()
+ {
+ return m_data;
+ }
+
+private:
+ std::vector<char *> m_data;
+};
+
+/* Run the remote argument passing self tests. */
+
+static void
+self_test ()
+{
+ int failure_count = 0;
+ for (const auto &d : desc)
+ {
+ if (run_verbose ())
+ {
+ if (&d != &desc[0])
+ debug_printf ("------------------------------\n");
+ debug_printf ("Input (%s)\n", d.input.c_str ());
+ }
+
+ /* Split argument string into individual arguments. */
+ std::vector<std::string> split_args = gdb::remote_args::split (d.input);
+
+ if (run_verbose ())
+ {
+ debug_printf ("Split:\n");
+
+ size_t len = std::max (split_args.size (), d.split.size ());
+ for (size_t i = 0; i < len; ++i)
+ {
+ const char *got = "N/A";
+ const char *expected = got;
+
+ if (i < split_args.size ())
+ got = split_args[i].c_str ();
+
+ if (i < d.split.size ())
+ expected = d.split[i].c_str ();
+
+ debug_printf (" got (%s), expected (%s)\n", got, expected);
+ }
+ }
+
+ if (split_args != d.split)
+ {
+ ++failure_count;
+ if (run_verbose ())
+ debug_printf ("FAIL\n");
+ continue;
+ }
+
+ /* Now join the arguments. */
+ args_as_c_strings split_args_c_str (split_args);
+ std::string joined_args
+ = gdb::remote_args::join (split_args_c_str.get ());
+
+ if (run_verbose ())
+ debug_printf ("Joined (%s), expected (%s)\n",
+ joined_args.c_str (), d.joined.c_str ());
+
+ if (joined_args != d.joined)
+ {
+ ++failure_count;
+ if (run_verbose ())
+ debug_printf ("FAIL\n");
+ continue;
+ }
+
+ /* The contents of JOINED_ARGS will not be identical to D.INPUT.
+ There are multiple ways that an argument can be escaped, and out
+ join function just picks one. However, if we split JOINED_ARGS
+ again then each individual argument should be the same as those in
+ SPLIT_ARGS. So lets test that next. */
+ std::vector<std::string> split_args_v2
+ = gdb::remote_args::split (joined_args);
+
+ if (split_args_v2 != split_args)
+ {
+ ++failure_count;
+ if (run_verbose ())
+ {
+ debug_printf ("Re-split:\n");
+ for (const auto &a : split_args_v2)
+ debug_printf (" got (%s)\n", a.c_str ());
+ debug_printf ("FAIL\n");
+ }
+ continue;
+ }
+
+ if (run_verbose ())
+ debug_printf ("PASS\n");
+ }
+
+ SELF_CHECK (failure_count == 0);
+}
+
+} /* namespace remote_args_tests */
+} /* namespace selftests */
+
+void _initialize_remote_arg_selftests ();
+
+void
+_initialize_remote_arg_selftests ()
+{
+ selftests::register_test ("remote-args",
+ selftests::remote_args_tests::self_test);
+}
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 10/16] gdb: add remote argument passing self tests
2024-01-09 14:26 ` [PATCH 10/16] gdb: add remote argument passing self tests Andrew Burgess
@ 2024-01-21 3:57 ` Keith Seitz
0 siblings, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:57 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Just some typos, I think.
On 1/9/24 06:26, Andrew Burgess wrote:
> diff --git a/gdb/unittests/remote-arg-selftests.c b/gdb/unittests/remote-arg-selftests.c
> new file mode 100644
> index 00000000000..3240ab9aeea
> --- /dev/null
> +++ b/gdb/unittests/remote-arg-selftests.c
> @@ -0,0 +1,189 @@
> +/* Self tests for GDB's argument splitting and merging.
> +
> + Copyright (C) 2023 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/>. */
> +
> +#include "defs.h"
> +#include "gdbsupport/selftest.h"
> +#include "gdbsupport/buildargv.h"
> +#include "gdbsupport/common-inferior.h"
> +#include "gdbsupport/remote-args.h"
> +
> +namespace selftests {
> +namespace remote_args_tests {
> +
> +/* The data needed to perform a single remote argument test. */
> +struct arg_test_desc
> +{
> + /* The original inferior argument string. */
> + std::string input;
> +
> + /* The individual arguments once they have been split. */
> + std::vector<std::string> split;
> +
> + /* The new inferior argument string, created by joining SPLIT. */
> + std::string joined;
> +};
> +
> +/* The list of tests. */
> +arg_test_desc desc[] = {
> + { "abc", { "abc" }, "abc" },
> + { "a b c", { "a", "b", "c" }, "a b c" },
> + { "\"a b\" 'c d'", { "a b", "c d" }, "a\\ b c\\ d" },
> + { "\\' \\\"", { "'", "\"" }, "\\' \\\"" },
> + { "'\\'", { "\\" }, "\\\\" },
> + { "\"\\\\\" \"\\\\\\\"\"", { "\\", "\\\"" }, "\\\\ \\\\\\\"" },
> + { "\\ \" \" ' '", { " ", " ", " "}, "\\ \\ \\ " },
> + { "\"'\"", { "'" }, "\\'" },
> + { "'\"' '\\\"'", { "\"", "\\\"" } , "\\\" \\\\\\\""},
> + { "\"first arg\" \"\" \"third-arg\" \"'\" \"\\\"\" \"\\\\\\\"\" \" \" \"\"",
> + { "first arg", "", "third-arg", "'", "\"", "\\\""," ", "" },
> + "first\\ arg '' third-arg \\' \\\" \\\\\\\" \\ ''"},
> + { "\"\\a\" \"\\&\" \"\\#\" \"\\<\" \"\\^\"",
> + { "\\a", "\\&", "\\#" , "\\<" , "\\^"},
> + "\\\\a \\\\\\& \\\\\\# \\\\\\< \\\\\\^" },
> + { "1 '\n' 3", { "1", "\n", "3" }, "1 '\n' 3" },
> +};
> +
> +/* Convert a std::vector<std::string> into std::vector<char *>. This
> + requires copying all of the string content. This class takes care of
> + freeing the memory once we are done with it. */
> +
> +struct args_as_c_strings
> +{
> + args_as_c_strings (std::vector<std::string> args)
> + {
> + for (const auto & a : args)
> + m_data.push_back (xstrdup (a.c_str ()));
> + }
> +
> + ~args_as_c_strings ()
> + {
> + free_vector_argv (m_data);
> + }
> +
> + std::vector<char *> &get ()
> + {
> + return m_data;
> + }
> +
> +private:
> + std::vector<char *> m_data;
> +};
> +
> +/* Run the remote argument passing self tests. */
> +
> +static void
> +self_test ()
> +{
> + int failure_count = 0;
> + for (const auto &d : desc)
> + {
> + if (run_verbose ())
> + {
> + if (&d != &desc[0])
> + debug_printf ("------------------------------\n");
> + debug_printf ("Input (%s)\n", d.input.c_str ());
> + }
> +
> + /* Split argument string into individual arguments. */
> + std::vector<std::string> split_args = gdb::remote_args::split (d.input);
> +
> + if (run_verbose ())
> + {
> + debug_printf ("Split:\n");
> +
> + size_t len = std::max (split_args.size (), d.split.size ());
> + for (size_t i = 0; i < len; ++i)
> + {
> + const char *got = "N/A";
> + const char *expected = got;
> +
> + if (i < split_args.size ())
> + got = split_args[i].c_str ();
> +
> + if (i < d.split.size ())
> + expected = d.split[i].c_str ();
> +
> + debug_printf (" got (%s), expected (%s)\n", got, expected);
> + }
> + }
> +
> + if (split_args != d.split)
> + {
> + ++failure_count;
> + if (run_verbose ())
> + debug_printf ("FAIL\n");
> + continue;
> + }
> +
> + /* Now join the arguments. */
> + args_as_c_strings split_args_c_str (split_args);
> + std::string joined_args
> + = gdb::remote_args::join (split_args_c_str.get ());
> +
> + if (run_verbose ())
> + debug_printf ("Joined (%s), expected (%s)\n",
> + joined_args.c_str (), d.joined.c_str ());
> +
> + if (joined_args != d.joined)
> + {
> + ++failure_count;
> + if (run_verbose ())
> + debug_printf ("FAIL\n");
> + continue;
> + }
> +
> + /* The contents of JOINED_ARGS will not be identical to D.INPUT.
> + There are multiple ways that an argument can be escaped, and out
"and our"?
> + join function just picks one. However, if we split JOINED_ARGS
> + again then each individual argument should be the same as those in
> + SPLIT_ARGS. So lets test that next. */
"let's"
> + std::vector<std::string> split_args_v2
> + = gdb::remote_args::split (joined_args);
> +
> + if (split_args_v2 != split_args)
> + {
> + ++failure_count;
> + if (run_verbose ())
> + {
> + debug_printf ("Re-split:\n");
> + for (const auto &a : split_args_v2)
> + debug_printf (" got (%s)\n", a.c_str ());
> + debug_printf ("FAIL\n");
> + }
> + continue;
> + }
> +
> + if (run_verbose ())
> + debug_printf ("PASS\n");
> + }
> +
> + SELF_CHECK (failure_count == 0);
> +}
> +
> +} /* namespace remote_args_tests */
> +} /* namespace selftests */
> +
> +void _initialize_remote_arg_selftests ();
> +
> +void
> +_initialize_remote_arg_selftests ()
> +{
> + selftests::register_test ("remote-args",
> + selftests::remote_args_tests::self_test);
> +}
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (9 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 10/16] gdb: add remote argument passing self tests Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 16:34 ` Eli Zaretskii
2024-01-09 16:35 ` Eli Zaretskii
2024-01-09 14:26 ` [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option Andrew Burgess
` (7 subsequent siblings)
18 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
GDB holds the inferior arguments as a single string. Currently when
GDB needs to pass the inferior arguments to a remote target as part of
a vRun packet, this is done by splitting the single argument string
into its component arguments by calling gdb::remote_args::split, which
uses the gdb_argv class to split the arguments for us.
The same gdb_argv class is used when the user has asked GDB/gdbserver
to start the inferior without first invoking a shell; the gdb_argv
class is used to split the argument string into it component
arguments, and each is passed as a separate argument to the execve
call which spawns the inferior.
There is however, a problem with using gdb_argv to split the arguments
before passing them to a remote target. To understand this problem we
must first understand how gdb_argv is used when invoking an inferior
without a shell.
And to understand how gdb_argv is used to start an inferior without a
shell, I feel we need to first look at an example of starting an
inferior with a shell.
Consider these two cases:
(a) (gdb) set args \$VAR
(b) (gdb) set args $VAR
When starting with a shell, in case (a) the user expects the inferior
to receive a literal '$VAR' string as an argument, while in case (b)
the user expects to see the shell expanded value of the variable $VAR.
If the user does 'set startup-with-shell off', then in (a) GDB will
strip the '\' while splitting the arguments, and the inferior will be
passed a literal '$VAR'. In (b) there is no '\' to strip, so also in
this case the inferior will receive a literal '$VAR', remember
startup-with-shell is off, so there is no shell than can ever expand
$VAR.
Notice, that when startup-with-shell is off, we end up with a many to
one mapping, both (a) and (b) result in the literal string $VAR being
passed to the inferior. I think this is the correct behaviour in this
case.
However, as we use gdb_argv to split the remote arguments we have the
same many to one mapping within the vRun packet. But the vRun packet
will be used when startup-with-shell is both on and off. What this
means is that when gdbserver receives a vRun packet containing '$VAR'
it doesn't know if GDB actually had '$VAR', or if GDB had '\$VAR'.
And this is a huge problem.
We can try to address this by making the argument splitting for remote
targets smarter. And later in this series I will do that. However, I
think that splitting and joining the arguments as we do was a
mistake. The later patch in this series handles unquoted, single
quoted, and double quoted strings. But doesn't really address
parameter substitution, command substitution, or arithmetic
expansion. And even if we did try to address these cases, what rules
exactly would we implement? Probably POSIX shell rules, but what if
the remote target doesn't have a POSIX shell? Why do we need to pick
an particular rule set?
Clearly, for backward compatibility we need to maintain some degree of
argument splitting and joining as we currently have; and that's why I
have a later patch in this series that tries to improve that splitting
and joining a little. But I think, what we should really do, is add a
new feature flag (as used by the qSupported packet) and, if GDB and
the remote target agree, we should pass the inferior arguments as a
single string.
This solves all our problems. In the startup with shell case, we no
longer need to worry about splitting at all. The arguments are passed
unmodified to the remote target, who can then pass the arguments to
the shell directly.
In the 'startup-with-shell off' case it is now up to the remote target
to split the arguments, though in gdbserver we already did this as we
always joined the arguments, so for gdbserver it's no significant
hardship.
And if the remote target doesn't have a POSIX shell, well GDB just
doesn't need to worry about it!
Something similar to this was originally suggested in this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
though this series didn't try to maintain backward compatibility,
which I think is an issue that my patch solves. Additionally, this
series only passed the arguments as a single string in some cases,
I've simplified this so that when GDB and the remote agree, the
arguments are always passed as a single string. I think this makes my
version a little cleaner.
I've also added documentation and some tests with this commit,
including ensuring that we test both the new single string approach,
and the fallback split/join approach.
I've credited the author of the referenced series as co-author as they
did come to a similar conclusion, though I think my implementation is
different enough that I'm happy to list myself as primary author.
Co-Authored-By: Michael Weghorn <m.weghorn@posteo.de>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/NEWS | 7 +
gdb/doc/gdb.texinfo | 23 +++
gdb/remote.c | 25 ++-
gdb/testsuite/gdb.base/args.exp | 47 +++--
gdb/testsuite/gdb.base/inferior-args.exp | 31 +++-
gdb/testsuite/gdb.base/startup-with-shell.exp | 161 ++++++++++--------
gdbserver/server.cc | 21 ++-
gdbserver/server.h | 5 +
8 files changed, 226 insertions(+), 94 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index b72ba3d87e8..65a082808e4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -120,6 +120,13 @@ QThreadOptions in qSupported
QThreadOptions packet, and the qSupported response can contain the
set of thread options the remote stub supports.
+single-inf-arg in qSupported
+ The new single-inf-arg feature within the qSupported packet allows
+ GDB to inform the stub that it would like to send the inferior
+ arguments as a single string within the vRun packet. The stub can
+ reply with the single-inf-arg feature to indicate that it is able to
+ accept arguments as a single string.
+
*** Changes in GDB 14
* GDB now supports the AArch64 Scalable Matrix Extension 2 (SME2), which
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 4ada257d256..9dbe53384e1 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -43041,6 +43041,12 @@
(e.g.@: the last program run). The program is created in the stopped
state.
+If @value{GDBN} sent the @samp{single-inf-arg+} feature in the
+@samp{qSupported} packet, and the stub replied with
+@samp{single-inf-arg+}, then there will only be a single
+@var{argument} string, which includes all inferior arguments,
+separated with whitespace.
+
@c FIXME: What about non-stop mode?
This packet is only available in extended mode (@pxref{extended mode}).
@@ -44471,6 +44477,13 @@
@item vContSupported
This feature indicates whether @value{GDBN} wants to know the
supported actions in the reply to @samp{vCont?} packet.
+
+@item single-inf-arg
+This feature indicates that @value{GDBN} would like to send the
+inferior arguments as a single string within the @samp{vRun} packet.
+@value{GDBN} will not send the arguments as a single string unless the
+stub also reports that is supports this behaviour by including
+@samp{single-inf-arg+} in its @samp{qSupported} reply.
@end table
Stubs should ignore any unknown values for
@@ -44754,6 +44767,11 @@
@tab @samp{-}
@tab No
+@item @samp{single-inf-arg}
+@tab No
+@tab @samp{-}
+@tab No
+
@end multitable
These are the currently defined stub features, in more detail:
@@ -44985,6 +45003,11 @@
@file{/proc/@var{pid}/smaps} file so memory mapping page flags can be inspected.
This is done via the @samp{vFile} requests.
+@item single-inf-arg
+The remote stub would like to receive the inferior arguments as a
+single string within the @samp{vRun} packet. The stub should only
+send this feature if @value{GDBN} sent @samp{single-inf-arg+} in the
+@samp{qSupported} packet.
@end table
@item qSymbol::
diff --git a/gdb/remote.c b/gdb/remote.c
index ebef409ffed..75d275d38df 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -306,6 +306,10 @@ enum {
packets and the tag violation stop replies. */
PACKET_memory_tagging_feature,
+ /* Not really a packet; this indicates support for sending the vRun
+ inferior arguments as a single string. */
+ PACKET_vRun_single_argument,
+
PACKET_MAX
};
@@ -727,6 +731,11 @@ struct remote_features
bool remote_memory_tagging_p () const
{ return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
+ /* Returns true if there is support for sending vRun inferior arguments
+ as a single string. */
+ bool remote_vrun_single_arg_p () const
+ { return packet_support (PACKET_vRun_single_argument) == PACKET_ENABLE; }
+
/* Reset all packets back to "unknown support". Called when opening a
new connection to a remote target. */
void reset_all_packet_configs_support ();
@@ -5724,6 +5733,8 @@ static const struct protocol_feature remote_protocol_features[] = {
{ "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
{ "memory-tagging", PACKET_DISABLE, remote_supported_packet,
PACKET_memory_tagging_feature },
+ { "single-inf-arg", PACKET_DISABLE, remote_supported_packet,
+ PACKET_vRun_single_argument },
};
static char *remote_support_xml;
@@ -5835,6 +5846,10 @@ remote_target::remote_query_supported ()
!= AUTO_BOOLEAN_FALSE)
remote_query_supported_append (&q, "memory-tagging+");
+ if (m_features.packet_set_cmd_state (PACKET_vRun_single_argument)
+ != AUTO_BOOLEAN_FALSE)
+ remote_query_supported_append (&q, "single-inf-arg+");
+
/* Keep this one last to work around a gdbserver <= 7.10 bug in
the qSupported:xmlRegisters=i386 handling. */
if (remote_support_xml != NULL
@@ -10626,7 +10641,11 @@ remote_target::extended_remote_run (const std::string &args)
if (!args.empty ())
{
- std::vector<std::string> split_args = gdb::remote_args::split (args);
+ std::vector<std::string> split_args;
+ if (!m_features.remote_vrun_single_arg_p ())
+ split_args = gdb::remote_args::split (args);
+ else
+ split_args.push_back (args);
for (const auto &a : split_args)
{
@@ -15990,6 +16009,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
add_packet_config_cmd (PACKET_memory_tagging_feature,
"memory-tagging-feature", "memory-tagging-feature", 0);
+ add_packet_config_cmd (PACKET_vRun_single_argument,
+ "single-inferior-argument-feature",
+ "single-inferior-argument-feature", 0);
+
/* Assert that we've registered "set remote foo-packet" commands
for all packet configs. */
{
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 8b0047999bf..7c123e36404 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -72,31 +72,48 @@ proc args_test { name arglist {re_list {}} } {
}
}
-# Test that the --args are processed correctly.
+# Run all the tests.
+proc run_all_tests {} {
+ # Test that the --args are processed correctly.
-args_test basic {{1} {3}}
+ args_test basic {{1} {3}}
-# Test that the --args are processed correctly even if one of them is
-# empty.
+ # Test that the --args are processed correctly even if one of them is
+ # empty.
-args_test "one empty" {{1} {} {3}}
+ args_test "one empty" {{1} {} {3}}
-# Try with 2 empty args.
+ # Try with 2 empty args.
-args_test "two empty" {{1} {} {} 3}
+ args_test "two empty" {{1} {} {} 3}
-# Try with arguments containing literal single quotes.
+ # Try with arguments containing literal single quotes.
-args_test "one empty with single quotes" {{1} {''} {3}}
+ args_test "one empty with single quotes" {{1} {''} {3}}
-args_test "two empty with single quotes" {{1} {''} {''} {3}}
+ args_test "two empty with single quotes" {{1} {''} {''} {3}}
-# Try with arguments containing literal newlines.
+ # Try with arguments containing literal newlines.
-args_test "one newline" {{1} "\n" {3}} {1 \\\\n 3}
+ args_test "one newline" {{1} "\n" {3}} {1 \\\\n 3}
-args_test "two newlines" {{1} "\n" "\n" {3}} {1 \\\\n \\\\n 3}
+ args_test "two newlines" {{1} "\n" "\n" {3}} {1 \\\\n \\\\n 3}
-args_test "lone single quote" {{1} \' {3}}
+ args_test "lone single quote" {{1} \' {3}}
-args_test "lone double quote" {{1} \" {3}} {1 \\\\\" 3}
+ args_test "lone double quote" {{1} \" {3}} {1 \\\\\" 3}
+}
+
+run_all_tests
+
+# For extended-remote targets, disable the packet which passes
+# inferior arguments as a single string. This changes how the vRun
+# (extended-remote only) packet works.
+if {[target_info gdb_protocol] == "extended-remote"} {
+ with_test_prefix "single-inferior-arg disabled" {
+ save_vars { GDBFLAGS } {
+ append GDBFLAGS " -ex \"set remote single-inferior-argument-feature-packet off\""
+ run_all_tests
+ }
+ }
+}
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 4b51b657326..6c22ecb3c54 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -211,14 +211,31 @@ lappend test_desc_list [list "test four" \
[list "$hex \"'\"" \
"$hex \"\\\\\"\""]]
-foreach desc $test_desc_list {
- lassign $desc name stub_suitable args re_list
- with_test_prefix $name {
- foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
- foreach_with_prefix startup_with_shell { on off } {
- do_test $set_method $startup_with_shell $args $re_list \
- $stub_suitable
+# Run all tests in the global TEST_DESC_LIST.
+proc run_all_tests {} {
+ foreach desc $::test_desc_list {
+ lassign $desc name stub_suitable args re_list
+ with_test_prefix $name {
+ foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
+ foreach_with_prefix startup_with_shell { on off } {
+ do_test $set_method $startup_with_shell $args $re_list \
+ $stub_suitable
+ }
}
}
}
}
+
+run_all_tests
+
+# For extended-remote targets, disable the packet which passes
+# inferior arguments as a single string. This changes how the vRun
+# (extended-remote only) packet works.
+if {[target_info gdb_protocol] == "extended-remote"} {
+ with_test_prefix "single-inferior-arg disabled" {
+ save_vars { GDBFLAGS } {
+ append GDBFLAGS " -ex \"set remote single-inferior-argument-feature-packet off\""
+ run_all_tests
+ }
+ }
+}
diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp
index 0424b20de3a..f467774dc5f 100644
--- a/gdb/testsuite/gdb.base/startup-with-shell.exp
+++ b/gdb/testsuite/gdb.base/startup-with-shell.exp
@@ -90,76 +90,97 @@ proc run_test_same { args re testname } {
run_test $args $re $re $testname
}
-# The regexp to match a single '\' character.
-set bs "\\\\"
-
-# Are we using 'remote' or 'extended-remote' protocol?
-set is_remote_p [expr [string equal [target_info gdb_protocol] \
- "remote"] \
- || [string equal [target_info gdb_protocol] \
- "extended-remote"]]
-
-## Run the actual tests
-
-run_test "$unique_file_dir/*.unique-extension" \
- "\"$unique_file\"" \
- "\"$unique_file_dir/\\\*\.unique-extension\"" \
- "arg is glob" \
- $is_remote_p
-
-run_test_same "$unique_file_dir/\\*.unique-extension" \
- "\"$unique_file_dir/\\\*\.unique-extension\"" \
- "arg is escaped glob"
-
-save_vars { env(TEST) } {
- set env(TEST) "1234"
- run_test "\$TEST" \
- "\"1234\"" \
- "\"\\\$TEST\"" \
- "arg is shell variable" \
- $is_remote_p
-
- run_test_same "\\\$TEST" \
- "\"\\\$TEST\"" \
- "arg is escaped shell variable"
-}
-
-run_test_same "\"\\a\"" \
- "\"${bs}${bs}a\"" \
- "retain backslash in double quote arg"
-
-run_test_same "'\\a'" \
- "\"${bs}${bs}a\"" \
- "retain backslash in single quote arg"
-
-run_test_same "\"\\\$\"" \
- "\"\\\$\"" \
- "'\$' can be escaped in double quote arg"
-
-run_test_same "'\\\$'" \
- "\"${bs}${bs}\\\$\"" \
- "'\$' is not escaped in single quote arg"
-
-run_test_same "\"\\`\"" \
- "\"\\`\"" \
- "'`' can be escaped in double quote arg"
-
-run_test_same "'\\`'" \
- "\"${bs}${bs}`\"" \
- "'`' is not escaped in single quote arg"
-
-run_test_same "\"\\\"\"" \
- "\"${bs}\"\"" \
- "'\"' can be escaped in double quote arg"
+# Run the actual tests
+proc run_all_tests { { is_remote_with_split_args false } } {
+ # The regexp to match a single '\' character.
+ set bs "\\\\"
+
+ run_test "$::unique_file_dir/*.unique-extension" \
+ "\"$::unique_file\"" \
+ "\"$::unique_file_dir/\\\*\.unique-extension\"" \
+ "arg is glob" \
+ $is_remote_with_split_args
+
+ run_test_same "$::unique_file_dir/\\*.unique-extension" \
+ "\"$::unique_file_dir/\\\*\.unique-extension\"" \
+ "arg is escaped glob"
+
+ save_vars { ::env(TEST) } {
+ set ::env(TEST) "1234"
+ run_test "\$TEST" \
+ "\"1234\"" \
+ "\"\\\$TEST\"" \
+ "arg is shell variable" \
+ $is_remote_with_split_args
+
+ run_test_same "\\\$TEST" \
+ "\"\\\$TEST\"" \
+ "arg is escaped shell variable"
+ }
-run_test_same "'\\\"'" \
- "\"${bs}${bs}${bs}\"\"" \
- "'\"' is not escaped in single quote arg"
+ run_test "\$(echo foo)" \
+ "\"foo\"" \
+ "\"\\\$\\(echo\"" \
+ "arg is parameter expansion, command execution" \
+ $is_remote_with_split_args
+
+ run_test "\$((2 + 3))" \
+ "\"5\"" \
+ "\"\\\$\\(\\(2\"" \
+ "arg is parameter expansion, expression evaluation" \
+ $is_remote_with_split_args
+
+ run_test_same "\"\\a\"" \
+ "\"${bs}${bs}a\"" \
+ "retain backslash in double quote arg"
+
+ run_test_same "'\\a'" \
+ "\"${bs}${bs}a\"" \
+ "retain backslash in single quote arg"
+
+ run_test_same "\"\\\$\"" \
+ "\"\\\$\"" \
+ "'\$' can be escaped in double quote arg"
+
+ run_test_same "'\\\$'" \
+ "\"${bs}${bs}\\\$\"" \
+ "'\$' is not escaped in single quote arg"
+
+ run_test_same "\"\\`\"" \
+ "\"\\`\"" \
+ "'`' can be escaped in double quote arg"
+
+ run_test_same "'\\`'" \
+ "\"${bs}${bs}`\"" \
+ "'`' is not escaped in single quote arg"
+
+ run_test_same "\"\\\"\"" \
+ "\"${bs}\"\"" \
+ "'\"' can be escaped in double quote arg"
+
+ run_test_same "'\\\"'" \
+ "\"${bs}${bs}${bs}\"\"" \
+ "'\"' is not escaped in single quote arg"
+
+ run_test_same "\"\\\\\"" \
+ "\"${bs}${bs}\"" \
+ "'\\' can be escaped in double quote arg"
+
+ run_test_same "'\\\\'" \
+ "\"${bs}${bs}${bs}${bs}\"" \
+ "'\\' is not escaped in single quote arg"
+}
-run_test_same "\"\\\\\"" \
- "\"${bs}${bs}\"" \
- "'\\' can be escaped in double quote arg"
+run_all_tests
-run_test_same "'\\\\'" \
- "\"${bs}${bs}${bs}${bs}\"" \
- "'\\' is not escaped in single quote arg"
+# For extended-remote targets, disable the packet which passes
+# inferior arguments as a single string. This changes how the vRun
+# (extended-remote only) packet works.
+if {[target_info gdb_protocol] == "extended-remote"} {
+ with_test_prefix "single-inferior-arg disabled" {
+ save_vars { GDBFLAGS } {
+ append GDBFLAGS " -ex \"set remote single-inferior-argument-feature-packet off\""
+ run_all_tests true
+ }
+ }
+}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 13abb0b7636..65df03ef309 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2725,6 +2725,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (target_supports_memory_tagging ())
cs.memory_tagging_feature = true;
}
+ else if (feature == "single-inf-arg+")
+ cs.single_inferior_argument = true;
else
{
/* Move the unknown features all together. */
@@ -2854,6 +2856,9 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (target_supports_memory_tagging ())
strcat (own_buf, ";memory-tagging+");
+ if (cs.single_inferior_argument)
+ strcat (own_buf, ";single-inf-arg+");
+
/* Reinitialize components as needed for the new connection. */
hostio_handle_new_gdb_connection ();
target_handle_new_gdb_connection ();
@@ -3438,7 +3443,21 @@ handle_v_run (char *own_buf)
else
program_path.set (new_program_name.get ());
- program_args = gdb::remote_args::join (new_argv);
+ if (cs.single_inferior_argument)
+ {
+ if (new_argv.size () > 1)
+ {
+ write_enn (own_buf);
+ return;
+ }
+ else if (new_argv.size () == 1)
+ program_args = std::string (new_argv[0]);
+ else
+ program_args.clear ();
+ }
+ else
+ program_args = gdb::remote_args::join (new_argv);
+
free_vector_argv (new_argv);
target_create_inferior (program_path.get (), program_args);
diff --git a/gdbserver/server.h b/gdbserver/server.h
index 2bca4718941..93914098728 100644
--- a/gdbserver/server.h
+++ b/gdbserver/server.h
@@ -192,6 +192,11 @@ struct client_state
/* If true, memory tagging features are supported. */
bool memory_tagging_feature = false;
+ /* If true then we've agreed that the debugger will send all inferior
+ arguments as a single string. When false the debugger will attempt
+ to split the inferior arguments before sending them. */
+ bool single_inferior_argument = false;
+
};
client_state &get_client_state ();
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string
2024-01-09 14:26 ` [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string Andrew Burgess
@ 2024-01-09 16:34 ` Eli Zaretskii
2024-01-09 16:35 ` Eli Zaretskii
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:34 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:34 +0000
>
> gdb/NEWS | 7 +
> gdb/doc/gdb.texinfo | 23 +++
> gdb/remote.c | 25 ++-
> gdb/testsuite/gdb.base/args.exp | 47 +++--
> gdb/testsuite/gdb.base/inferior-args.exp | 31 +++-
> gdb/testsuite/gdb.base/startup-with-shell.exp | 161 ++++++++++--------
> gdbserver/server.cc | 21 ++-
> gdbserver/server.h | 5 +
> 8 files changed, 226 insertions(+), 94 deletions(-)
The documentation parts are okay, thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string
2024-01-09 14:26 ` [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string Andrew Burgess
2024-01-09 16:34 ` Eli Zaretskii
@ 2024-01-09 16:35 ` Eli Zaretskii
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:35 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:34 +0000
>
> gdb/NEWS | 7 +
> gdb/doc/gdb.texinfo | 23 +++
> gdb/remote.c | 25 ++-
> gdb/testsuite/gdb.base/args.exp | 47 +++--
> gdb/testsuite/gdb.base/inferior-args.exp | 31 +++-
> gdb/testsuite/gdb.base/startup-with-shell.exp | 161 ++++++++++--------
> gdbserver/server.cc | 21 ++-
> gdbserver/server.h | 5 +
> 8 files changed, 226 insertions(+), 94 deletions(-)
The documentation parts are okay, thanks.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (10 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 11/16] gdb/gdbserver: pass inferior arguments as a single string Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 16:43 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
2024-01-09 14:26 ` [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines Andrew Burgess
` (6 subsequent siblings)
18 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Michael Weghorn, Andrew Burgess
From: Michael Weghorn <m.weghorn@posteo.de>
This introduces a new '--no-escape-args' option for gdb and gdbserver.
I (Andrew Burgess) have based this patch from work done in this
series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
I have changed things slightly from the original series, however, I
think this work is close enough that I've left the original
author (Michael) in place and added myself as co-author. Any bugs
introduced by my modifications to the original patch should be
considered mine. I've also added documentation and tests which were
missing from the originally proposed patch.
When the startup-with-shell option is enabled, arguments passed
directly as 'gdb --args <args>' or 'gdbserver <args>', are by default
escaped so that they are passed to the inferior as passed on the
command line, no globbing or variable substitution happens within the
shell GDB uses to start the inferior.
For gdbserver, this is the case since commit:
commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
Date: Mon May 25 11:39:43 2020 -0400
Use construct_inferior_arguments which handles special chars
Only arguments set via 'set args <args>', 'run <args>', or through the
Python API are not escaped in standard upstream GDB right now.
For the 'gdb --args' case, directly settings unescaped args on gdb
invocation is possible e.g. by using the "--eval-command='set args
<args>'", while this possibility does not exist for gdbserver.
This commit adds a new '--no-escape-args' command line option for GDB
and gdbserver. This option is used with GDB as a replacement for the
current '--args' option, and for gdbserver this new option is a flag
which changes how gdbserver handles inferior arguments on the command
line. When '--no-escape-args' is used inferior arguments passed on
the command line will not have escaping added by GDB or gdbserver.
For gdbserver, using this new option allows having the behaviour from
before commit bea571ebd78ee29cb94adf648fbcda1e109e1be6, while keeping
the default behaviour unified between GDB and GDBserver.
For GDB the --no-escape-args option can be used as a replacement for
--args, like this:
shell> gdb --no-escape-args my-program arg1 arg2 arg3
While for gdbserver, the --no-escape-args option is a flag, which can
be used like:
shell> gdbserver --no-escape-args --once localhost:54321 \
my-program arg1 arg2 arg3
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/NEWS | 8 ++
gdb/doc/gdb.texinfo | 114 ++++++++++++++-
gdb/main.c | 28 +++-
gdb/testsuite/gdb.base/args.exp | 101 ++++++++-----
gdb/testsuite/gdb.server/inferior-args.c | 27 ++++
gdb/testsuite/gdb.server/inferior-args.exp | 157 +++++++++++++++++++++
gdbserver/server.cc | 21 ++-
7 files changed, 407 insertions(+), 49 deletions(-)
create mode 100644 gdb/testsuite/gdb.server/inferior-args.c
create mode 100644 gdb/testsuite/gdb.server/inferior-args.exp
diff --git a/gdb/NEWS b/gdb/NEWS
index 65a082808e4..80c766eeeda 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -9,6 +9,14 @@
* GDB index now contains information about the main function. This speeds up
startup when it is being used for some large binaries.
+* GDB now accepts --no-escape-args as an alternative to --args on the
+ command line. GDB will not escape special shell characters within
+ arguments after --no-escape-args.
+
+* gdbserver now accepts --no-escape-args as a command line flag. When
+ this flag is used gdbserver will not escape special shell characters
+ within the inferior arguments.
+
* Changed commands
disassemble
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9dbe53384e1..abb07d74baf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -898,9 +898,9 @@
``process'', and there is often no way to get a core dump. @value{GDBN}
will warn you if it is unable to attach or to read core dumps.
-You can optionally have @code{@value{GDBP}} pass any arguments after the
-executable file to the inferior using @code{--args}. This option stops
-option processing.
+You can optionally have @code{@value{GDBP}} pass any arguments after
+the executable file to the inferior using @code{--args} or
+@code{--no-escape-args}. These options stops option processing.
@smallexample
@value{GDBP} --args gcc -O2 -c foo.c
@end smallexample
@@ -1246,6 +1246,56 @@
executable file are passed as command line arguments to the inferior.
This option stops option processing.
+Argument supplied using @code{--args} will have backslashes applied to
+escape any special shell characters. This ensures that when the
+inferior starts it is passed arguments exactly as @value{GDBN}
+receives them.
+
+For example, consider the following command run under a shell:
+@smallexample
+@value{GDBP} --args ls *.c
+@end smallexample
+@noindent
+In this case the shell will expand @kbd{*.c} at the time @value{GDBN}
+is invoked, not at the time that the inferior is invoked. As a
+result, if an additional @kbd{.c} file is created after @value{GDBN}
+is started, but before the inferior is started, then the inferior will
+not show the file in its output; the list of matching files was
+resolved at the time @value{GDBN} was started.
+
+If you quote the @kbd{*} character used in the @value{GDBN} command
+line argument then this will prevent the shell that starts
+@value{GDBN} from expanding the @kbd{*.c} pattern, however, this
+quoting will also be passed to the shell that @value{GDBN} invokes in
+order to start the inferior (@pxref{set startup-with-shell}), and this
+will prevent the @kbd{*.c} pattern being expanded at this point either:
+@smallexample
+@value{GDBP} --args ls '*.c'
+(@value{GDBP}) show args
+Argument list to give program being debugged when it is started is "\*.log".
+@end smallexample
+@noindent
+If this quoting behaviour does not meet your needs, then you could use
+@code{--no-escape-args} instead, which is described below.
+
+@item --no-escape-args
+@cindex @code{--no-escape-args}
+Change interpretation of command line so that arguments following the
+executable file are passed as command line arguments to the inferior.
+This option stops option processing.
+
+Unlike @code{--args}, arguments after the executable name will not
+have any escaping applied to them. As a result, any special shell
+characters that are not expanded by the shell that invokes
+@value{GDBN} will be expanded by the shell that @value{GDBN} uses to
+start the inferior.
+
+@smallexample
+@value{GDBP} --no-escape-args ls '*.c'
+(@value{GDBP}) show args
+Argument list to give program being debugged when it is started is "*.log".
+@end smallexample
+
@item -baud @var{bps}
@itemx -b @var{bps}
@cindex @code{--baud}
@@ -50498,9 +50548,10 @@
directly to @code{stdout}, will also be made silent.
@item --args @var{prog} [@var{arglist}]
-Change interpretation of command line so that arguments following this
-option are passed as arguments to the inferior. As an example, take
-the following command:
+@itemx --no-escape-args @var{prog} [@var{arglist}]
+Change interpretation of command line so that arguments following
+either of these options are passed as arguments to the inferior. As
+an example, take the following command:
@smallexample
gdb ./a.out -q
@@ -50515,7 +50566,44 @@
@end smallexample
@noindent
-starts @value{GDBN} with the introductory message, and passes the option to the inferior.
+starts @value{GDBN} with the introductory message, and passes the
+option @code{-q} to the inferior.
+
+The difference between @option{--args} and @option{--no-escape-args}
+is whether @value{GDBN} applies escapes to the argument it sees:
+
+@smallexample
+gdb --args ./a.out *.c
+@end smallexample
+
+@noindent
+in this case the @code{*.c} is expanded by the shell that invokes
+@value{GDBN}, the list of matching files will be fixed in the inferior
+argument list. If instead this is used:
+
+@smallexample
+gdb --args ./a.out '*.c'
+@end smallexample
+
+@noindent
+then the shell that invokes @value{GDBN} will not expand @code{*.c},
+but instead @value{GDBN} will escape the @code{*} character so when
+a.out is invoked it will be passed a literal @code{*.c}. If instead
+this is used:
+
+@smallexample
+gdb --no-escape-args ./a.out '*.c'
+@end smallexample
+
+@noindent
+now @value{GDBN} will not escape the @code{*} character. When the
+inferior is invoked the @code{*.c} will be expanded, and the inferior
+will be passed the list of files as present at the time the inferior
+is invoked.
+
+This change of behaviour can be important if the list of matching
+files could change between the time that @value{GDBN} is started, and
+the time the inferior is started.
@item --pid=@var{pid}
Attach @value{GDBN} to an already running program, with the PID @var{pid}.
@@ -50857,6 +50945,18 @@
with the @option{--once} option, it will stop listening for any further
connection attempts after connecting to the first @value{GDBN} session.
+@item --no-escape-args
+By default, inferior arguments passed on the @command{gdbserver}
+command line will have any special shell characters escaped by
+@command{gdbserver}. This ensures that when @command{gdbserver}
+invokes the inferior, the arguments passed to the inferior are
+identical to the arguments passed to @command{gdbserver}.
+
+To disable this escaping, use @option{--no-escape-args}. With this
+option special shell characters will not be escaped. When
+@command{gdbserver} starts a new shell in order to invoke the
+inferior, this new shell will expand any special shell characters.
+
@c --disable-packet is not documented for users.
@c --disable-randomization and --no-disable-randomization are superseded by
diff --git a/gdb/main.c b/gdb/main.c
index 015ed396f58..e9cd4172e4a 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -622,9 +622,10 @@ captured_main_1 (struct captured_main_args *context)
char **argv = context->argv;
static int quiet = 0;
- static int set_args = 0;
static int inhibit_home_gdbinit = 0;
+ enum { NO_ARGS, SET_ESC_ARGS, SET_NO_ESC_ARGS } set_args = NO_ARGS;
+
/* Pointers to various arguments from command line. */
char *symarg = NULL;
char *execarg = NULL;
@@ -773,7 +774,9 @@ captured_main_1 (struct captured_main_args *context)
OPT_EIX,
OPT_EIEX,
OPT_READNOW,
- OPT_READNEVER
+ OPT_READNEVER,
+ OPT_SET_ESC_ARGS,
+ OPT_SET_NO_ESC_ARGS,
};
/* This struct requires int* in the struct, but write_files is a bool.
So use this temporary int that we write back after argument parsing. */
@@ -846,7 +849,8 @@ captured_main_1 (struct captured_main_args *context)
{"windows", no_argument, NULL, OPT_WINDOWS},
{"statistics", no_argument, 0, OPT_STATISTICS},
{"write", no_argument, &write_files_1, 1},
- {"args", no_argument, &set_args, 1},
+ {"args", no_argument, nullptr, OPT_SET_ESC_ARGS},
+ {"no-escape-args", no_argument, nullptr, OPT_SET_NO_ESC_ARGS},
{"l", required_argument, 0, 'l'},
{"return-child-result", no_argument, &return_child_result, 1},
{0, no_argument, 0, 0}
@@ -858,7 +862,7 @@ captured_main_1 (struct captured_main_args *context)
c = getopt_long_only (argc, argv, "",
long_options, &option_index);
- if (c == EOF || set_args)
+ if (c == EOF || set_args != NO_ARGS)
break;
/* Long option that takes an argument. */
@@ -939,6 +943,12 @@ captured_main_1 (struct captured_main_args *context)
case OPT_EIEX:
cmdarg_vec.emplace_back (CMDARG_EARLYINIT_COMMAND, optarg);
break;
+ case OPT_SET_ESC_ARGS:
+ set_args = SET_ESC_ARGS;
+ break;
+ case OPT_SET_NO_ESC_ARGS:
+ set_args = SET_NO_ESC_ARGS;
+ break;
case 'B':
batch_flag = batch_silent = 1;
gdb_stdout = new null_file ();
@@ -1072,7 +1082,7 @@ captured_main_1 (struct captured_main_args *context)
/* Now that gdb_init has created the initial inferior, we're in
position to set args for that inferior. */
- if (set_args)
+ if (set_args != NO_ARGS)
{
/* The remaining options are the command-line options for the
inferior. The first one is the sym/exec file, and the rest
@@ -1084,8 +1094,11 @@ captured_main_1 (struct captured_main_args *context)
symarg = argv[optind];
execarg = argv[optind];
++optind;
+ escape_args_func escape_func
+ = ((set_args == SET_ESC_ARGS) ? escape_shell_characters
+ : escape_quotes_and_white_space);
gdb::array_view<char * const> arg_view (&argv[optind], argc - optind);
- current_inferior ()->set_args (arg_view, escape_shell_characters);
+ current_inferior ()->set_args (arg_view, escape_func);
}
else
{
@@ -1398,7 +1411,8 @@ This is the GNU debugger. Usage:\n\n\
gdb_puts (_("\
Selection of debuggee and its files:\n\n\
--args Arguments after executable-file are passed to inferior.\n\
- --core=COREFILE Analyze the core dump COREFILE.\n\
+ --no-escape-args Like --args, but arguments are not escaped.\n \
+ --core=COREFILE Analyze the core dump COREFILE.\n \
--exec=EXECFILE Use EXECFILE as the executable.\n\
--pid=PID Attach to running process PID.\n\
--directory=DIR Search for source files in DIR.\n\
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 7c123e36404..9ff9e7ee6d1 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -32,41 +32,73 @@ if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
# NAME is the name to use for the tests and ARGLIST is the list of
# arguments that are passed to GDB when it is started.
#
-# The optional RE_LIST is the list of patterns to check the arguments
-# against, these patterns should match ARGLIST. If the arguments are
-# expected to show up unmodified in the test output then RE_LIST can
-# be dropped, and this proc will reuse ARGLIST.
-
-proc args_test { name arglist {re_list {}} } {
-
- # If RE_LIST is not supplied then we can reuse ARGLIST, this
- # implies that the arguments will appear unmodified in the test
- # output.
- if {[llength $re_list] == 0} {
- set re_list $arglist
+# The optional RE_ESC_LIST is the list of patterns to check the
+# inferior arguments against when GDB is started using --args. If
+# RE_ESC_LIST is not given then ARGLIST is reused, this implies that
+# the inferior arguments appear unchanged in the test output.
+#
+# The optional RE_NO_ESC_LIST is the list of patterns to check the
+# inferior arguments against when GDB is started using
+# --no-escape-args. If RE_NO_ESC_LIST is not given then RE_ESC_LIST
+# is reused, this implies that there's no difference between the test
+# output when the arguments are escaped or not.
+
+proc args_test { name arglist {re_esc_list {}} {re_no_esc_list {}} } {
+
+ # If either of the two regexp lists are not specificed then we can
+ # use an earlier argument value instead.
+ #
+ # For the first regexp list, if this is missing then we use the
+ # argument list, this assumes that the arguments will appear
+ # unmodified in the output.
+ if {[llength $re_esc_list] == 0} {
+ set re_esc_list $arglist
}
- foreach_with_prefix startup_with_shell { on off } {
- save_vars { ::GDBFLAGS } {
- set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
-
- clean_restart $::binfile
-
- gdb_test_no_output "set startup-with-shell ${startup_with_shell}" \
- "set startup-with-shell for $name"
-
- runto_main
- gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
- gdb_continue_to_breakpoint "breakpoint for $name"
-
- set expected_len [expr 1 + [llength $re_list]]
- gdb_test "print argc" "\\\$$::decimal = $expected_len" "argc for $name"
+ # If the second regexp list is missing then we reuse the first
+ # regexp list. This assumes there's no difference between escaped
+ # and unescaped arguments in the output.
+ if {[llength $re_no_esc_list] == 0} {
+ set re_no_esc_list $re_esc_list
+ }
- set i 1
- foreach arg $re_list {
- gdb_test "print argv\[$i\]" "\\\$$::decimal = $::hex \"$arg\"" \
- "argv\[$i\] for $name"
- set i [expr $i + 1]
+ foreach_with_prefix startup_with_shell { on off } {
+ foreach_with_prefix arg_flag { args no-escape-args } {
+ save_vars { ::GDBFLAGS } {
+ set ::GDBFLAGS "$::GDBFLAGS --${arg_flag} $::binfile $arglist"
+
+ clean_restart $::binfile
+
+ gdb_test_no_output \
+ "set startup-with-shell ${startup_with_shell}" \
+ "set startup-with-shell for $name"
+
+ runto_main
+ gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
+ gdb_continue_to_breakpoint "breakpoint for $name"
+
+ if { $arg_flag eq "args" || $startup_with_shell eq "off" } {
+ set re_list $re_esc_list
+ } else {
+ set re_list $re_no_esc_list
+ }
+
+ set expected_len [expr 1 + [llength $re_list]]
+ gdb_test "print argc" \
+ "\\\$$::decimal = $expected_len" "argc for $name"
+
+ set i 1
+ foreach arg $re_list {
+ if { $arg eq "\n" } {
+ set arg {\\n}
+ } elseif { $arg eq "\"" } {
+ set arg {\\\"}
+ }
+ gdb_test "print argv\[$i\]" \
+ "\\\$$::decimal = $::hex \"$arg\"" \
+ "argv\[$i\] for $name"
+ set i [expr $i + 1]
+ }
}
}
}
@@ -102,6 +134,11 @@ proc run_all_tests {} {
args_test "lone single quote" {{1} \' {3}}
args_test "lone double quote" {{1} \" {3}} {1 \\\\\" 3}
+
+ save_vars { ::env(TEST) } {
+ set ::env(TEST) "ABCD"
+ args_test "shell variable" {{$TEST}} {\\$TEST} {{ABCD}}
+ }
}
run_all_tests
diff --git a/gdb/testsuite/gdb.server/inferior-args.c b/gdb/testsuite/gdb.server/inferior-args.c
new file mode 100644
index 00000000000..5fd215f50a8
--- /dev/null
+++ b/gdb/testsuite/gdb.server/inferior-args.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2023 Free Software Foundation, Inc.
+
+ 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/>. */
+
+#include <stdio.h>
+
+int
+main (int argc, char **argv)
+{
+ for (int i = 0; i < argc; i++)
+ printf ("[%d] %s\n", i, argv[i]);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.server/inferior-args.exp b/gdb/testsuite/gdb.server/inferior-args.exp
new file mode 100644
index 00000000000..9b2aeb249e0
--- /dev/null
+++ b/gdb/testsuite/gdb.server/inferior-args.exp
@@ -0,0 +1,157 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# 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/>.
+
+# Test passing inferior arguments on the gdbserver command line. Tests the
+# flags --no-startup-with-shell and --no-escape-args that change how GDB
+# interprets the arguments being passed.
+
+# This test relies on starting gdbserver using the pipe syntax. Not sure
+# how well this will run if part of this test is being run elsewhere.
+require {!is_remote target} {!is_remote host}
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+require allow_gdbserver_tests
+
+set gdbserver [find_gdbserver]
+if { $gdbserver == "" } {
+ unsupported "could not find gdbserver"
+ return
+}
+
+standard_testfile
+if {[build_executable "failed to prepare" $testfile $srcfile]} {
+ return -1
+}
+
+# EXTENDED_P is a boolean, when true gdbserver is started with --multi, and
+# GDB connects using extended-remote protocol. Otherwise, no --multi flag
+# is passed, and GDB connects with the remote protocol.
+#
+# WITH_SHELL_P is a boolean, when true gdbserver starts the inferior using a
+# shell, when false gdbserver is passed the --no-startup-with-shell command
+# line option, and should not start the inferior through a shell.
+#
+# ESCAPE_P is a boolean, when true gdbserver applies escapes to the inferior
+# arguments, when false gdbserver is passed the --no-escape-args command
+# line option, and should not apply escaping to the inferior arguments.
+#
+# ARGLIST is a list of inferior arguments to add to the gdbserver command
+# line.
+#
+# RE_LIST is a list of patterns to match, one for each of ARGLIST. Once the
+# inferior is started we check that each argument matches its corresponding
+# entry in RE_LIST.
+proc do_test_inner { extended_p with_shell_p escape_p arglist re_list } {
+
+ clean_restart ${::binfile}
+
+ gdb_test_no_output "set sysroot"
+
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
+
+ if { $extended_p } {
+ set protocol "extended-remote"
+ } else {
+ set protocol "remote"
+ }
+
+ if { $escape_p } {
+ set esc_opt ""
+ } else {
+ set esc_opt "--no-escape-args"
+ }
+
+ if { $with_shell_p } {
+ set shell_opt ""
+ } else {
+ set shell_opt "--no-startup-with-shell"
+ }
+
+ gdb_test "target ${protocol} | ${::gdbserver} --once ${esc_opt} ${shell_opt} - ${::binfile} ${arglist}" \
+ ".*" \
+ "start gdbserver over stdin"
+
+ gdb_breakpoint main
+ gdb_continue_to_breakpoint main
+
+ set expected_len [expr 1 + [llength $re_list]]
+ gdb_test "print argc" \
+ "\\\$$::decimal = $expected_len" "check argc"
+
+ set i 1
+ foreach arg $re_list {
+ verbose -log "APB ($arg)"
+ gdb_test "print argv\[$i\]" \
+ "\\\$$::decimal = $::hex \"$arg\"" \
+ "check argv\[$i\]"
+ set i [expr $i + 1]
+ }
+}
+
+# Wrapper around do_test_inner. NAME is the name of this test, used to make
+# the test names unique. ARGLIST is the list of inferior arguments to add
+# to the gdbserver command line.
+#
+# The optional RE_ESC_LIST is a list of patterns to match against the
+# inferior arguments once the inferior is started, one pattern for each
+# argument. If RE_ESC_LIST is not given then ARGLIST is reused, which
+# implies the arguments appear unmodified in the test output.
+#
+# The optional RE_NO_ESC_LIST is a list of patterns ot match against the
+# inferior arguments when gdbserver is started with --no-escape-args or
+# --no-startup-with-shell. There should be one pattern for each argument.
+# If RE_NO_ESC_LIST is not given then RE_ESC_LIST is resused, which implies
+# there's no difference in how the arguments are printed.
+proc args_test { name arglist {re_esc_list {}} {re_no_esc_list {}} } {
+ if {[llength $re_esc_list] == 0} {
+ set re_esc_list $arglist
+ }
+
+ if {[llength $re_no_esc_list] == 0} {
+ set re_no_esc_list $re_esc_list
+ }
+
+ foreach_with_prefix extended_p { yes no } {
+ foreach_with_prefix startup_with_shell { on off } {
+ foreach_with_prefix escape_p { yes no } {
+ if { $escape_p || !$startup_with_shell } {
+ set re_list $re_esc_list
+ } else {
+ set re_list $re_no_esc_list
+ }
+
+ with_test_prefix "$name" {
+ do_test_inner $extended_p $startup_with_shell \
+ $escape_p $arglist $re_list
+ }
+ }
+ }
+ }
+}
+
+args_test "basic" {a b c}
+args_test "one empty" {1 "" 3}
+args_test "two empty" {1 "" "" 3}
+args_test "one with single quotes" {1 "''" 3}
+args_test "lone double quote" {"1" \" 3} {1 \\\\\" 3}
+save_vars { env(TEST) } {
+ set env(TEST) "ABCD"
+ args_test "shell variable" {\$TEST} {\\$TEST} {ABCD}
+}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 65df03ef309..0445fa0237f 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3837,10 +3837,20 @@ gdbserver_usage (FILE *stream)
" --startup-with-shell\n"
" Start PROG using a shell. I.e., execs a shell that\n"
" then execs PROG. (default)\n"
+ " To make use of globbing and variable subsitution for\n"
+ " arguments passed directly on gdbserver invocation,\n"
+ " see the --no-escape-args command line option in\n"
+ " addition\n"
" --no-startup-with-shell\n"
" Exec PROG directly instead of using a shell.\n"
- " Disables argument globbing and variable substitution\n"
- " on UNIX-like systems.\n"
+ " --no-escape-args\n"
+ " If PROG is started using a shell (see the\n"
+ " --[no-]startup-with-shell option),\n"
+ " ARGS passed directly on gdbserver invocation are\n"
+ " escaped, so no globbing or variable substitution\n"
+ " happens for those. This option disables escaping, so\n"
+ " globbing and variable substituation in the shell\n"
+ " are done for ARGS on UNIX-like systems.\n"
"\n"
"Debug options:\n"
"\n"
@@ -4074,6 +4084,7 @@ captured_main (int argc, char *argv[])
volatile int attach = 0;
int was_running;
bool selftest = false;
+ bool escape_args = true;
#if GDB_SELF_TEST
std::vector<const char *> selftest_filters;
@@ -4230,6 +4241,8 @@ captured_main (int argc, char *argv[])
startup_with_shell = true;
else if (strcmp (*next_arg, "--no-startup-with-shell") == 0)
startup_with_shell = false;
+ else if (strcmp (*next_arg, "--no-escape-args") == 0)
+ escape_args = false;
else if (strcmp (*next_arg, "--once") == 0)
run_once = true;
else if (strcmp (*next_arg, "--selftest") == 0)
@@ -4339,8 +4352,10 @@ captured_main (int argc, char *argv[])
std::vector<char *> temp_arg_vector;
for (i = 1; i < n; i++)
temp_arg_vector.push_back (next_arg[i]);
+ escape_args_func escape_func = (escape_args ? escape_shell_characters
+ : escape_quotes_and_white_space);
program_args = construct_inferior_arguments (temp_arg_vector,
- escape_shell_characters);
+ escape_func);
/* Wait till we are at first instruction in program. */
target_create_inferior (program_path.get (), program_args);
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option
2024-01-09 14:26 ` [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option Andrew Burgess
@ 2024-01-09 16:43 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:43 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Michael Weghorn <m.weghorn@posteo.de>, Andrew Burgess <aburgess@redhat.com>
> Date: Tue, 9 Jan 2024 14:26:35 +0000
>
> gdb/NEWS | 8 ++
> gdb/doc/gdb.texinfo | 114 ++++++++++++++-
> gdb/main.c | 28 +++-
> gdb/testsuite/gdb.base/args.exp | 101 ++++++++-----
> gdb/testsuite/gdb.server/inferior-args.c | 27 ++++
> gdb/testsuite/gdb.server/inferior-args.exp | 157 +++++++++++++++++++++
> gdbserver/server.cc | 21 ++-
> 7 files changed, 407 insertions(+), 49 deletions(-)
> create mode 100644 gdb/testsuite/gdb.server/inferior-args.c
> create mode 100644 gdb/testsuite/gdb.server/inferior-args.exp
Thanks, the documentation parts are okay.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option
2024-01-09 14:26 ` [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option Andrew Burgess
2024-01-09 16:43 ` Eli Zaretskii
@ 2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:57 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
[I realize the documentation has already been approved.
Nonetheles, I noticed some typos.]
On 1/9/24 06:26, Andrew Burgess wrote:
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9dbe53384e1..abb07d74baf 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -898,9 +898,9 @@
> ``process'', and there is often no way to get a core dump. @value{GDBN}
> will warn you if it is unable to attach or to read core dumps.
>
> -You can optionally have @code{@value{GDBP}} pass any arguments after the
> -executable file to the inferior using @code{--args}. This option stops
> -option processing.
> +You can optionally have @code{@value{GDBP}} pass any arguments after
> +the executable file to the inferior using @code{--args} or
> +@code{--no-escape-args}. These options stops option processing.
> @smallexample
> @value{GDBP} --args gcc -O2 -c foo.c
> @end smallexample
> @@ -1246,6 +1246,56 @@
> executable file are passed as command line arguments to the inferior.
> This option stops option processing.
>
> +Argument supplied using @code{--args} will have backslashes applied to
I think this should be "Arguments".
> +escape any special shell characters. This ensures that when the
> +inferior starts it is passed arguments exactly as @value{GDBN}
and a comma is needed after the introductory phrase, "when the
inferior starts."
> +receives them.
> +
> +For example, consider the following command run under a shell:
> +@smallexample
> +@value{GDBP} --args ls *.c
> +@end smallexample
> +@noindent
> +In this case the shell will expand @kbd{*.c} at the time @value{GDBN}
> +is invoked, not at the time that the inferior is invoked. As a
> +result, if an additional @kbd{.c} file is created after @value{GDBN}
> +is started, but before the inferior is started, then the inferior will
> +not show the file in its output; the list of matching files was
> +resolved at the time @value{GDBN} was started.
> +
> +If you quote the @kbd{*} character used in the @value{GDBN} command
> +line argument then this will prevent the shell that starts
> +@value{GDBN} from expanding the @kbd{*.c} pattern, however, this
> +quoting will also be passed to the shell that @value{GDBN} invokes in
> +order to start the inferior (@pxref{set startup-with-shell}), and this
> +will prevent the @kbd{*.c} pattern being expanded at this point either:
> +@smallexample
> +@value{GDBP} --args ls '*.c'
> +(@value{GDBP}) show args
> +Argument list to give program being debugged when it is started is "\*.log".
Is this supposed to be "\*.c"?
> +@end smallexample
> +@noindent
> +If this quoting behaviour does not meet your needs, then you could use
> +@code{--no-escape-args} instead, which is described below.
> +
> +@item --no-escape-args
> +@cindex @code{--no-escape-args}
> +Change interpretation of command line so that arguments following the
> +executable file are passed as command line arguments to the inferior.
> +This option stops option processing.
> +
> +Unlike @code{--args}, arguments after the executable name will not
> +have any escaping applied to them. As a result, any special shell
> +characters that are not expanded by the shell that invokes
> +@value{GDBN} will be expanded by the shell that @value{GDBN} uses to
> +start the inferior.
> +
> +@smallexample
> +@value{GDBP} --no-escape-args ls '*.c'
> +(@value{GDBP}) show args
> +Argument list to give program being debugged when it is started is "*.log".
> +@end smallexample
> +
> @item -baud @var{bps}
> @itemx -b @var{bps}
> @cindex @code{--baud}
> @@ -50498,9 +50548,10 @@
> directly to @code{stdout}, will also be made silent.
>
> @item --args @var{prog} [@var{arglist}]
> -Change interpretation of command line so that arguments following this
> -option are passed as arguments to the inferior. As an example, take
> -the following command:
> +@itemx --no-escape-args @var{prog} [@var{arglist}]
> +Change interpretation of command line so that arguments following
"Change the"?
> +either of these options are passed as arguments to the inferior. As
> +an example, take the following command:
>
> @smallexample
> gdb ./a.out -q
> @@ -50515,7 +50566,44 @@
> @end smallexample
>
> @noindent
> -starts @value{GDBN} with the introductory message, and passes the option to the inferior.
> +starts @value{GDBN} with the introductory message, and passes the
> +option @code{-q} to the inferior.
> +
> +The difference between @option{--args} and @option{--no-escape-args}
> +is whether @value{GDBN} applies escapes to the argument it sees:
> +
> +@smallexample
> +gdb --args ./a.out *.c
> +@end smallexample
> +
> +@noindent
> +in this case the @code{*.c} is expanded by the shell that invokes
> +@value{GDBN}, the list of matching files will be fixed in the inferior
> +argument list. If instead this is used:
> +
> +@smallexample
> +gdb --args ./a.out '*.c'
> +@end smallexample
> +
> +@noindent
> +then the shell that invokes @value{GDBN} will not expand @code{*.c},
> +but instead @value{GDBN} will escape the @code{*} character so when
> +a.out is invoked it will be passed a literal @code{*.c}. If instead
> +this is used:
> +
> +@smallexample
> +gdb --no-escape-args ./a.out '*.c'
> +@end smallexample
> +
> +@noindent
> +now @value{GDBN} will not escape the @code{*} character. When the
> +inferior is invoked the @code{*.c} will be expanded, and the inferior
> +will be passed the list of files as present at the time the inferior
> +is invoked.
> +
> +This change of behaviour can be important if the list of matching
> +files could change between the time that @value{GDBN} is started, and
> +the time the inferior is started.
>
> @item --pid=@var{pid}
> Attach @value{GDBN} to an already running program, with the PID @var{pid}.
> @@ -50857,6 +50945,18 @@
> with the @option{--once} option, it will stop listening for any further
> connection attempts after connecting to the first @value{GDBN} session.
>
> +@item --no-escape-args
> +By default, inferior arguments passed on the @command{gdbserver}
> +command line will have any special shell characters escaped by
> +@command{gdbserver}. This ensures that when @command{gdbserver}
> +invokes the inferior, the arguments passed to the inferior are
> +identical to the arguments passed to @command{gdbserver}.
> +
> +To disable this escaping, use @option{--no-escape-args}. With this
> +option special shell characters will not be escaped. When
Need a comma between "option" and "special."
Keith
> +@command{gdbserver} starts a new shell in order to invoke the
> +inferior, this new shell will expand any special shell characters.
> +
> @c --disable-packet is not documented for users.
>
> @c --disable-randomization and --no-disable-randomization are superseded by
> diff --git a/gdb/main.c b/gdb/main.c
> index 015ed396f58..e9cd4172e4a 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -622,9 +622,10 @@ captured_main_1 (struct captured_main_args *context)
> char **argv = context->argv;
>
> static int quiet = 0;
> - static int set_args = 0;
> static int inhibit_home_gdbinit = 0;
>
> + enum { NO_ARGS, SET_ESC_ARGS, SET_NO_ESC_ARGS } set_args = NO_ARGS;
> +
> /* Pointers to various arguments from command line. */
> char *symarg = NULL;
> char *execarg = NULL;
> @@ -773,7 +774,9 @@ captured_main_1 (struct captured_main_args *context)
> OPT_EIX,
> OPT_EIEX,
> OPT_READNOW,
> - OPT_READNEVER
> + OPT_READNEVER,
> + OPT_SET_ESC_ARGS,
> + OPT_SET_NO_ESC_ARGS,
> };
> /* This struct requires int* in the struct, but write_files is a bool.
> So use this temporary int that we write back after argument parsing. */
> @@ -846,7 +849,8 @@ captured_main_1 (struct captured_main_args *context)
> {"windows", no_argument, NULL, OPT_WINDOWS},
> {"statistics", no_argument, 0, OPT_STATISTICS},
> {"write", no_argument, &write_files_1, 1},
> - {"args", no_argument, &set_args, 1},
> + {"args", no_argument, nullptr, OPT_SET_ESC_ARGS},
> + {"no-escape-args", no_argument, nullptr, OPT_SET_NO_ESC_ARGS},
> {"l", required_argument, 0, 'l'},
> {"return-child-result", no_argument, &return_child_result, 1},
> {0, no_argument, 0, 0}
> @@ -858,7 +862,7 @@ captured_main_1 (struct captured_main_args *context)
>
> c = getopt_long_only (argc, argv, "",
> long_options, &option_index);
> - if (c == EOF || set_args)
> + if (c == EOF || set_args != NO_ARGS)
> break;
>
> /* Long option that takes an argument. */
> @@ -939,6 +943,12 @@ captured_main_1 (struct captured_main_args *context)
> case OPT_EIEX:
> cmdarg_vec.emplace_back (CMDARG_EARLYINIT_COMMAND, optarg);
> break;
> + case OPT_SET_ESC_ARGS:
> + set_args = SET_ESC_ARGS;
> + break;
> + case OPT_SET_NO_ESC_ARGS:
> + set_args = SET_NO_ESC_ARGS;
> + break;
> case 'B':
> batch_flag = batch_silent = 1;
> gdb_stdout = new null_file ();
> @@ -1072,7 +1082,7 @@ captured_main_1 (struct captured_main_args *context)
>
> /* Now that gdb_init has created the initial inferior, we're in
> position to set args for that inferior. */
> - if (set_args)
> + if (set_args != NO_ARGS)
> {
> /* The remaining options are the command-line options for the
> inferior. The first one is the sym/exec file, and the rest
> @@ -1084,8 +1094,11 @@ captured_main_1 (struct captured_main_args *context)
> symarg = argv[optind];
> execarg = argv[optind];
> ++optind;
> + escape_args_func escape_func
> + = ((set_args == SET_ESC_ARGS) ? escape_shell_characters
> + : escape_quotes_and_white_space);
> gdb::array_view<char * const> arg_view (&argv[optind], argc - optind);
> - current_inferior ()->set_args (arg_view, escape_shell_characters);
> + current_inferior ()->set_args (arg_view, escape_func);
> }
> else
> {
> @@ -1398,7 +1411,8 @@ This is the GNU debugger. Usage:\n\n\
> gdb_puts (_("\
> Selection of debuggee and its files:\n\n\
> --args Arguments after executable-file are passed to inferior.\n\
> - --core=COREFILE Analyze the core dump COREFILE.\n\
> + --no-escape-args Like --args, but arguments are not escaped.\n \
> + --core=COREFILE Analyze the core dump COREFILE.\n \
> --exec=EXECFILE Use EXECFILE as the executable.\n\
> --pid=PID Attach to running process PID.\n\
> --directory=DIR Search for source files in DIR.\n\
> diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
> index 7c123e36404..9ff9e7ee6d1 100644
> --- a/gdb/testsuite/gdb.base/args.exp
> +++ b/gdb/testsuite/gdb.base/args.exp
> @@ -32,41 +32,73 @@ if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
> # NAME is the name to use for the tests and ARGLIST is the list of
> # arguments that are passed to GDB when it is started.
> #
> -# The optional RE_LIST is the list of patterns to check the arguments
> -# against, these patterns should match ARGLIST. If the arguments are
> -# expected to show up unmodified in the test output then RE_LIST can
> -# be dropped, and this proc will reuse ARGLIST.
> -
> -proc args_test { name arglist {re_list {}} } {
> -
> - # If RE_LIST is not supplied then we can reuse ARGLIST, this
> - # implies that the arguments will appear unmodified in the test
> - # output.
> - if {[llength $re_list] == 0} {
> - set re_list $arglist
> +# The optional RE_ESC_LIST is the list of patterns to check the
> +# inferior arguments against when GDB is started using --args. If
> +# RE_ESC_LIST is not given then ARGLIST is reused, this implies that
> +# the inferior arguments appear unchanged in the test output.
> +#
> +# The optional RE_NO_ESC_LIST is the list of patterns to check the
> +# inferior arguments against when GDB is started using
> +# --no-escape-args. If RE_NO_ESC_LIST is not given then RE_ESC_LIST
> +# is reused, this implies that there's no difference between the test
> +# output when the arguments are escaped or not.
> +
> +proc args_test { name arglist {re_esc_list {}} {re_no_esc_list {}} } {
> +
> + # If either of the two regexp lists are not specificed then we can
> + # use an earlier argument value instead.
> + #
> + # For the first regexp list, if this is missing then we use the
> + # argument list, this assumes that the arguments will appear
> + # unmodified in the output.
> + if {[llength $re_esc_list] == 0} {
> + set re_esc_list $arglist
> }
>
> - foreach_with_prefix startup_with_shell { on off } {
> - save_vars { ::GDBFLAGS } {
> - set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
> -
> - clean_restart $::binfile
> -
> - gdb_test_no_output "set startup-with-shell ${startup_with_shell}" \
> - "set startup-with-shell for $name"
> -
> - runto_main
> - gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
> - gdb_continue_to_breakpoint "breakpoint for $name"
> -
> - set expected_len [expr 1 + [llength $re_list]]
> - gdb_test "print argc" "\\\$$::decimal = $expected_len" "argc for $name"
> + # If the second regexp list is missing then we reuse the first
> + # regexp list. This assumes there's no difference between escaped
> + # and unescaped arguments in the output.
> + if {[llength $re_no_esc_list] == 0} {
> + set re_no_esc_list $re_esc_list
> + }
>
> - set i 1
> - foreach arg $re_list {
> - gdb_test "print argv\[$i\]" "\\\$$::decimal = $::hex \"$arg\"" \
> - "argv\[$i\] for $name"
> - set i [expr $i + 1]
> + foreach_with_prefix startup_with_shell { on off } {
> + foreach_with_prefix arg_flag { args no-escape-args } {
> + save_vars { ::GDBFLAGS } {
> + set ::GDBFLAGS "$::GDBFLAGS --${arg_flag} $::binfile $arglist"
> +
> + clean_restart $::binfile
> +
> + gdb_test_no_output \
> + "set startup-with-shell ${startup_with_shell}" \
> + "set startup-with-shell for $name"
> +
> + runto_main
> + gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
> + gdb_continue_to_breakpoint "breakpoint for $name"
> +
> + if { $arg_flag eq "args" || $startup_with_shell eq "off" } {
> + set re_list $re_esc_list
> + } else {
> + set re_list $re_no_esc_list
> + }
> +
> + set expected_len [expr 1 + [llength $re_list]]
> + gdb_test "print argc" \
> + "\\\$$::decimal = $expected_len" "argc for $name"
> +
> + set i 1
> + foreach arg $re_list {
> + if { $arg eq "\n" } {
> + set arg {\\n}
> + } elseif { $arg eq "\"" } {
> + set arg {\\\"}
> + }
> + gdb_test "print argv\[$i\]" \
> + "\\\$$::decimal = $::hex \"$arg\"" \
> + "argv\[$i\] for $name"
> + set i [expr $i + 1]
> + }
> }
> }
> }
> @@ -102,6 +134,11 @@ proc run_all_tests {} {
> args_test "lone single quote" {{1} \' {3}}
>
> args_test "lone double quote" {{1} \" {3}} {1 \\\\\" 3}
> +
> + save_vars { ::env(TEST) } {
> + set ::env(TEST) "ABCD"
> + args_test "shell variable" {{$TEST}} {\\$TEST} {{ABCD}}
> + }
> }
>
> run_all_tests
> diff --git a/gdb/testsuite/gdb.server/inferior-args.c b/gdb/testsuite/gdb.server/inferior-args.c
> new file mode 100644
> index 00000000000..5fd215f50a8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/inferior-args.c
> @@ -0,0 +1,27 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2023 Free Software Foundation, Inc.
> +
> + 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/>. */
> +
> +#include <stdio.h>
> +
> +int
> +main (int argc, char **argv)
> +{
> + for (int i = 0; i < argc; i++)
> + printf ("[%d] %s\n", i, argv[i]);
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.server/inferior-args.exp b/gdb/testsuite/gdb.server/inferior-args.exp
> new file mode 100644
> index 00000000000..9b2aeb249e0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/inferior-args.exp
> @@ -0,0 +1,157 @@
> +# Copyright 2023 Free Software Foundation, Inc.
> +
> +# 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/>.
> +
> +# Test passing inferior arguments on the gdbserver command line. Tests the
> +# flags --no-startup-with-shell and --no-escape-args that change how GDB
> +# interprets the arguments being passed.
> +
> +# This test relies on starting gdbserver using the pipe syntax. Not sure
> +# how well this will run if part of this test is being run elsewhere.
> +require {!is_remote target} {!is_remote host}
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +require allow_gdbserver_tests
> +
> +set gdbserver [find_gdbserver]
> +if { $gdbserver == "" } {
> + unsupported "could not find gdbserver"
> + return
> +}
> +
> +standard_testfile
> +if {[build_executable "failed to prepare" $testfile $srcfile]} {
> + return -1
> +}
> +
> +# EXTENDED_P is a boolean, when true gdbserver is started with --multi, and
> +# GDB connects using extended-remote protocol. Otherwise, no --multi flag
> +# is passed, and GDB connects with the remote protocol.
> +#
> +# WITH_SHELL_P is a boolean, when true gdbserver starts the inferior using a
> +# shell, when false gdbserver is passed the --no-startup-with-shell command
> +# line option, and should not start the inferior through a shell.
> +#
> +# ESCAPE_P is a boolean, when true gdbserver applies escapes to the inferior
> +# arguments, when false gdbserver is passed the --no-escape-args command
> +# line option, and should not apply escaping to the inferior arguments.
> +#
> +# ARGLIST is a list of inferior arguments to add to the gdbserver command
> +# line.
> +#
> +# RE_LIST is a list of patterns to match, one for each of ARGLIST. Once the
> +# inferior is started we check that each argument matches its corresponding
> +# entry in RE_LIST.
> +proc do_test_inner { extended_p with_shell_p escape_p arglist re_list } {
> +
> + clean_restart ${::binfile}
> +
> + gdb_test_no_output "set sysroot"
> +
> + # Make sure we're disconnected, in case we're testing with an
> + # extended-remote board, therefore already connected.
> + gdb_test "disconnect" ".*"
> +
> + if { $extended_p } {
> + set protocol "extended-remote"
> + } else {
> + set protocol "remote"
> + }
> +
> + if { $escape_p } {
> + set esc_opt ""
> + } else {
> + set esc_opt "--no-escape-args"
> + }
> +
> + if { $with_shell_p } {
> + set shell_opt ""
> + } else {
> + set shell_opt "--no-startup-with-shell"
> + }
> +
> + gdb_test "target ${protocol} | ${::gdbserver} --once ${esc_opt} ${shell_opt} - ${::binfile} ${arglist}" \
> + ".*" \
> + "start gdbserver over stdin"
> +
> + gdb_breakpoint main
> + gdb_continue_to_breakpoint main
> +
> + set expected_len [expr 1 + [llength $re_list]]
> + gdb_test "print argc" \
> + "\\\$$::decimal = $expected_len" "check argc"
> +
> + set i 1
> + foreach arg $re_list {
> + verbose -log "APB ($arg)"
> + gdb_test "print argv\[$i\]" \
> + "\\\$$::decimal = $::hex \"$arg\"" \
> + "check argv\[$i\]"
> + set i [expr $i + 1]
> + }
> +}
> +
> +# Wrapper around do_test_inner. NAME is the name of this test, used to make
> +# the test names unique. ARGLIST is the list of inferior arguments to add
> +# to the gdbserver command line.
> +#
> +# The optional RE_ESC_LIST is a list of patterns to match against the
> +# inferior arguments once the inferior is started, one pattern for each
> +# argument. If RE_ESC_LIST is not given then ARGLIST is reused, which
> +# implies the arguments appear unmodified in the test output.
> +#
> +# The optional RE_NO_ESC_LIST is a list of patterns ot match against the
> +# inferior arguments when gdbserver is started with --no-escape-args or
> +# --no-startup-with-shell. There should be one pattern for each argument.
> +# If RE_NO_ESC_LIST is not given then RE_ESC_LIST is resused, which implies
> +# there's no difference in how the arguments are printed.
> +proc args_test { name arglist {re_esc_list {}} {re_no_esc_list {}} } {
> + if {[llength $re_esc_list] == 0} {
> + set re_esc_list $arglist
> + }
> +
> + if {[llength $re_no_esc_list] == 0} {
> + set re_no_esc_list $re_esc_list
> + }
> +
> + foreach_with_prefix extended_p { yes no } {
> + foreach_with_prefix startup_with_shell { on off } {
> + foreach_with_prefix escape_p { yes no } {
> + if { $escape_p || !$startup_with_shell } {
> + set re_list $re_esc_list
> + } else {
> + set re_list $re_no_esc_list
> + }
> +
> + with_test_prefix "$name" {
> + do_test_inner $extended_p $startup_with_shell \
> + $escape_p $arglist $re_list
> + }
> + }
> + }
> + }
> +}
> +
> +args_test "basic" {a b c}
> +args_test "one empty" {1 "" 3}
> +args_test "two empty" {1 "" "" 3}
> +args_test "one with single quotes" {1 "''" 3}
> +args_test "lone double quote" {"1" \" 3} {1 \\\\\" 3}
> +save_vars { env(TEST) } {
> + set env(TEST) "ABCD"
> + args_test "shell variable" {\$TEST} {\\$TEST} {ABCD}
> +}
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 65df03ef309..0445fa0237f 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -3837,10 +3837,20 @@ gdbserver_usage (FILE *stream)
> " --startup-with-shell\n"
> " Start PROG using a shell. I.e., execs a shell that\n"
> " then execs PROG. (default)\n"
> + " To make use of globbing and variable subsitution for\n"
> + " arguments passed directly on gdbserver invocation,\n"
> + " see the --no-escape-args command line option in\n"
> + " addition\n"
> " --no-startup-with-shell\n"
> " Exec PROG directly instead of using a shell.\n"
> - " Disables argument globbing and variable substitution\n"
> - " on UNIX-like systems.\n"
> + " --no-escape-args\n"
> + " If PROG is started using a shell (see the\n"
> + " --[no-]startup-with-shell option),\n"
> + " ARGS passed directly on gdbserver invocation are\n"
> + " escaped, so no globbing or variable substitution\n"
> + " happens for those. This option disables escaping, so\n"
> + " globbing and variable substituation in the shell\n"
> + " are done for ARGS on UNIX-like systems.\n"
> "\n"
> "Debug options:\n"
> "\n"
> @@ -4074,6 +4084,7 @@ captured_main (int argc, char *argv[])
> volatile int attach = 0;
> int was_running;
> bool selftest = false;
> + bool escape_args = true;
> #if GDB_SELF_TEST
> std::vector<const char *> selftest_filters;
>
> @@ -4230,6 +4241,8 @@ captured_main (int argc, char *argv[])
> startup_with_shell = true;
> else if (strcmp (*next_arg, "--no-startup-with-shell") == 0)
> startup_with_shell = false;
> + else if (strcmp (*next_arg, "--no-escape-args") == 0)
> + escape_args = false;
> else if (strcmp (*next_arg, "--once") == 0)
> run_once = true;
> else if (strcmp (*next_arg, "--selftest") == 0)
> @@ -4339,8 +4352,10 @@ captured_main (int argc, char *argv[])
> std::vector<char *> temp_arg_vector;
> for (i = 1; i < n; i++)
> temp_arg_vector.push_back (next_arg[i]);
> + escape_args_func escape_func = (escape_args ? escape_shell_characters
> + : escape_quotes_and_white_space);
> program_args = construct_inferior_arguments (temp_arg_vector,
> - escape_shell_characters);
> + escape_func);
>
> /* Wait till we are at first instruction in program. */
> target_create_inferior (program_path.get (), program_args);
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (11 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 12/16] gdb/gdbserver: add a '--no-escape-args' command line option Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 16:44 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
2024-01-09 14:26 ` [PATCH 14/16] gdb/gdbserver: remove some uses of free_vector_argv Andrew Burgess
` (5 subsequent siblings)
18 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
When starting GDB it is possible to set an inferior argument that
contains a newline, for example:
shell> gdb --args my.app "abc
> def"
...
(gdb) show args
Argument list to give program being debugged when it is started is "abc'
'def".
However, once GDB is started, the only way to install an argument
containing a newline is to use the Python API.
This commit changes that.
After this commit 'set args' as well as 'run', 'start', and 'starti',
will now accept multi-line inferior arguments, e.g.:
(gdb) set args "abc
> def"
(gdb) show args
Argument list to give program being debugged when it is started is ""abc
def"".
And also:
(gdb) run "abc
> def"
... etc ...
Once GDB has presented the secondary prompt to gather the remaining
inferior arguments then it is possible for the user to quit argument
entry by sending SIGINT (usually, Ctrl-c). For the 'set args' case
this will abort the argument change, leaving the arguments as they
were previously. For the run style commands, this aborts the run
command completely, the inferior is not changed, and the partially
collected arguments are not installed.
---
gdb/NEWS | 10 ++
gdb/doc/gdb.texinfo | 20 ++++
gdb/infcmd.c | 128 +++++++++++++++++++++--
gdb/testsuite/gdb.base/inferior-args.exp | 71 ++++++++++++-
4 files changed, 219 insertions(+), 10 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index 80c766eeeda..2ba1899f78c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -51,6 +51,16 @@ set remote thread-options-packet
show remote thread-options-packet
Set/show the use of the thread options packet.
+* Changed commands
+
+set args
+run
+start
+starti
+ These commands now all allow for entering inferior arguments that
+ contain a newline character. The newline must be contained within a
+ single or double quoted argument.
+
* New features in the GDB remote stub, GDBserver
** The --remote-debug and --event-loop-debug command line options
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index abb07d74baf..2015293ee0d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -2893,6 +2893,26 @@
using @code{set args} before the next @code{run} is the only way to run
it again without arguments.
+It is possible to set arguments containing a newline character. This
+can be done by enclosing an argument within a single or double quote.
+For example, start by entering:
+
+@smallexample
+(@value{GDBP}) set args "ab
+@end smallexample
+
+@noindent
+and then enter the newline, @value{GDBN} gives a secondary prompt
+@code{>} and allows you to continue entering the arguments:
+
+@smallexample
+(@value{GDBP}) set args "ab
+>cd"
+(@value{GDBP}) show args
+Argument list to give program being debugged when it is started is ""ab
+cd"".
+@end smallexample
+
@kindex show args
@item show args
Show the arguments to give your program when it is started.
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a3c32792491..bad8736aad2 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -120,11 +120,112 @@ show_inferior_tty_command (struct ui_file *file, int from_tty,
"is \"%s\".\n"), inferior_tty.c_str ());
}
+/* Return true if the inferior argument string ARGS represents a "complete"
+ set of arguments. Arguments are considered complete so long as they
+ don't contain unbalanced single or double quoted strings. Unbalanced
+ means that a single or double quoted argument is started, but not
+ finished. */
+
+static bool
+args_complete_p (const std::string &args)
+{
+ const char *input = args.c_str ();
+ bool squote = false, dquote = false;
+
+ while (*input != '\0')
+ {
+ input = skip_spaces (input);
+
+ if (squote)
+ {
+ /* Inside a single quoted argument, look for the closing single
+ quote. */
+ if (*input == '\'')
+ squote = false;
+ }
+ else if (dquote)
+ {
+ /* If we see either '\"' or '\\' within a double quoted argument
+ then skip both characters (one is skipped here, and the other
+ at the end of the loop). We need to skip the '\"' so that we
+ don't consider the '"' as closing the double quoted argument,
+ and we don't skip the entire '\\' then we'll only skip the
+ first '\', in which case we might see the second '\' as a '\"'
+ sequence, which would be wrong. */
+ if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
+ ++input;
+ /* Otherwise, just look for the closing double quote. */
+ else if (*input == '"')
+ dquote = false;
+ }
+ else
+ {
+ /* Outside of either a single or double quoted argument, we need
+ to check for '\"', '\'', and '\\'. The escaped quotes we
+ obviously need to skip so we don't think that we have started
+ a quoted argument. The '\\' we need to skip so we don't just
+ skip the first '\' and then incorrectly consider the second
+ '\' are part of a '\"' or '\'' sequence. */
+ if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
+ ++input;
+ /* Otherwise, check for the start of a single or double quoted
+ argument. */
+ else if (*input == '\'')
+ squote = true;
+ else if (*input == '"')
+ dquote = true;
+ }
+
+ ++input;
+ }
+
+ return (!dquote && !squote);
+}
+
+/* ... */
+
+static std::string
+get_complete_args (std::string args)
+{
+ /* If the user wants an argument containing a newline then they need to
+ do so within quotes. Use args_complete_p to check if the ARGS string
+ contains balanced double and single quotes. If not then prompt the
+ user for additional arguments and append this to ARGS. */
+ const char *prompt = nullptr;
+ while (!args_complete_p (args))
+ {
+ if (prompt == nullptr)
+ {
+ prompt = getenv ("PS2");
+ if (prompt == nullptr)
+ prompt = "> ";
+ }
+
+ std::string buffer;
+ const char *content = command_line_input (buffer, prompt, "set_args");
+ if (content == nullptr)
+ return {};
+
+ args += "\n" + buffer;
+ }
+
+ return args;
+}
+
/* Store the new value passed to 'set args'. */
static void
-set_args_value (const std::string &args)
+set_args_value (const std::string &args_in)
{
+ std::string args;
+
+ if (!args_in.empty ())
+ {
+ args = get_complete_args (args_in);
+ if (args.empty ())
+ return;
+ }
+
current_inferior ()->set_args (args);
}
@@ -376,6 +477,21 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
dont_repeat ();
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
+ args = stripped.get ();
+
+ std::string inf_args;
+ /* If there were other args, beside '&', process them. */
+ if (args != nullptr)
+ {
+ /* If ARGS is only a partial argument string then this call will
+ interactively read more arguments from the user. If the user
+ quits then we shouldn't start the inferior. */
+ inf_args = get_complete_args (args);
+ if (inf_args.empty ())
+ return;
+ }
+
scoped_disable_commit_resumed disable_commit_resumed ("running");
kill_if_already_running (from_tty);
@@ -397,9 +513,6 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
reopen_exec_file ();
reread_symbols (from_tty);
- gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
- args = stripped.get ();
-
/* Do validation and preparation before possibly changing anything
in the inferior. */
@@ -412,6 +525,9 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
/* Done. Can now set breakpoints, change inferior args, etc. */
+ if (!inf_args.empty ())
+ current_inferior ()->set_args (inf_args);
+
/* Insert temporary breakpoint in main function if requested. */
if (run_how == RUN_STOP_AT_MAIN)
{
@@ -433,10 +549,6 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
the user has to manually nuke all symbols between runs if they
want them to go away (PR 2207). This is probably reasonable. */
- /* If there were other args, beside '&', process them. */
- if (args != nullptr)
- current_inferior ()->set_args (args);
-
if (from_tty)
{
uiout->field_string (nullptr, "Starting program");
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 6c22ecb3c54..b259e4a4217 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -211,12 +211,15 @@ lappend test_desc_list [list "test four" \
[list "$hex \"'\"" \
"$hex \"\\\\\"\""]]
-# Run all tests in the global TEST_DESC_LIST.
+# Run all tests in the global TEST_DESC_LIST, as well as some tests of
+# inferior arguments containing newlines.
proc run_all_tests {} {
+ set all_methods { "start" "starti" "run" "set args" }
+
foreach desc $::test_desc_list {
lassign $desc name stub_suitable args re_list
with_test_prefix $name {
- foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
+ foreach_with_prefix set_method $all_methods {
foreach_with_prefix startup_with_shell { on off } {
do_test $set_method $startup_with_shell $args $re_list \
$stub_suitable
@@ -224,6 +227,70 @@ proc run_all_tests {} {
}
}
}
+
+ # Check the multi-line argument entry. This isn't going to work when
+ # using the gdbstub, as the only way to set arguments in this case is
+ # via the gdbserver command line, which isn't what we're testing here.
+ if { ![use_gdb_stub] } {
+ foreach_with_prefix set_method $all_methods {
+ clean_restart $::binfile
+
+ # First check that we can abort entering multi-line arguments.
+ set saw_prompt false
+ gdb_test_multiple "$set_method \"ab" "abort argument entry" {
+ -re "^$set_method \"ab\r\n" {
+ exp_continue
+ }
+ -re "^> $" {
+ set saw_prompt true
+ send_gdb "\004"
+ exp_continue
+ }
+ -re "quit\r\n$::gdb_prompt $" {
+ gdb_assert {$saw_prompt} \
+ $gdb_test_name
+ }
+ }
+
+ # Now place a breakpoint on main.
+ if { ![gdb_breakpoint "main" message] } {
+ fail "could not set breakpoint on main"
+ continue
+ }
+
+ # And actually enter some multi-line arguments.
+ set saw_prompt false
+ gdb_test_multiple "$set_method \"xy" "complete argument entry" {
+ -re "^$set_method \"xy\r\n" {
+ exp_continue
+ }
+ -re "^> $" {
+ set saw_prompt true
+ send_gdb "12\"\n"
+ exp_continue
+ }
+
+ -re "$::gdb_prompt $" {
+ gdb_assert { $saw_prompt } \
+ $gdb_test_name
+ }
+ }
+
+ # For the two methods that don't automatically run to main,
+ # poke the inferior along to main.
+ if { $set_method == "set args" } {
+ if { ![runto_main] } {
+ continue
+ }
+ } elseif { $set_method == "starti" } {
+ gdb_continue_to_breakpoint "b/p in main"
+ }
+
+ # And check we correctly see the argument containing a newline.
+ gdb_test "print argc" " = 2"
+ gdb_test "print argv\[1\]" " = $::hex \"xy\\\\n12\""
+ }
+ }
}
run_all_tests
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines
2024-01-09 14:26 ` [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines Andrew Burgess
@ 2024-01-09 16:44 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:44 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:36 +0000
>
> gdb/NEWS | 10 ++
> gdb/doc/gdb.texinfo | 20 ++++
> gdb/infcmd.c | 128 +++++++++++++++++++++--
> gdb/testsuite/gdb.base/inferior-args.exp | 71 ++++++++++++-
> 4 files changed, 219 insertions(+), 10 deletions(-)
Thanks, the documentation parts are okay.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines
2024-01-09 14:26 ` [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines Andrew Burgess
2024-01-09 16:44 ` Eli Zaretskii
@ 2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:57 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches; +Cc: Michael Weghorn
On 1/9/24 06:26, Andrew Burgess wrote:
> After this commit 'set args' as well as 'run', 'start', and 'starti',
> will now accept multi-line inferior arguments, e.g.:
>
> (gdb) set args "abc
> > def"
> (gdb) show args
> Argument list to give program being debugged when it is started is ""abc
> def"".
>
While playing with this, I noticed some odd behavior. I don't know if
this was introduced by your patch or (more likely) I just noticed it
with your patch. [I don't think there's any reason to block this patch
pending a solution in either case.]
I'm noticing some extra newlines output with this new command if you
use ^d to interrupt entering input:
(gdb) set args "
> <!-- hit ^d -->quit
(gdb) show args
Argument list to give program being debugged when it is started is "".
(gdb)
Notice the extra newline output after the last command.
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index a3c32792491..bad8736aad2 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -120,11 +120,112 @@ show_inferior_tty_command (struct ui_file *file, int from_tty,
> "is \"%s\".\n"), inferior_tty.c_str ());
> }
>
> +/* Return true if the inferior argument string ARGS represents a "complete"
> + set of arguments. Arguments are considered complete so long as they
> + don't contain unbalanced single or double quoted strings. Unbalanced
> + means that a single or double quoted argument is started, but not
> + finished. */
> +
> +static bool
> +args_complete_p (const std::string &args)
> +{
> + const char *input = args.c_str ();
> + bool squote = false, dquote = false;
> +
> + while (*input != '\0')
> + {
> + input = skip_spaces (input);
> +
> + if (squote)
> + {
> + /* Inside a single quoted argument, look for the closing single
> + quote. */
> + if (*input == '\'')
> + squote = false;
> + }
> + else if (dquote)
> + {
> + /* If we see either '\"' or '\\' within a double quoted argument
> + then skip both characters (one is skipped here, and the other
> + at the end of the loop). We need to skip the '\"' so that we
> + don't consider the '"' as closing the double quoted argument,
> + and we don't skip the entire '\\' then we'll only skip the
> + first '\', in which case we might see the second '\' as a '\"'
> + sequence, which would be wrong. */
> + if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
> + ++input;
> + /* Otherwise, just look for the closing double quote. */
> + else if (*input == '"')
> + dquote = false;
> + }
> + else
> + {
> + /* Outside of either a single or double quoted argument, we need
> + to check for '\"', '\'', and '\\'. The escaped quotes we
> + obviously need to skip so we don't think that we have started
> + a quoted argument. The '\\' we need to skip so we don't just
> + skip the first '\' and then incorrectly consider the second
> + '\' are part of a '\"' or '\'' sequence. */
> + if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
> + ++input;
> + /* Otherwise, check for the start of a single or double quoted
> + argument. */
> + else if (*input == '\'')
> + squote = true;
> + else if (*input == '"')
> + dquote = true;
> + }
> +
> + ++input;
> + }
> +
> + return (!dquote && !squote);
> +}
> +
> +/* ... */
^^^ Did you forget to finish your comment here?
Keith
> + > +static std::string
> +get_complete_args (std::string args)
> +{
> + /* If the user wants an argument containing a newline then they need to
> + do so within quotes. Use args_complete_p to check if the ARGS string
> + contains balanced double and single quotes. If not then prompt the
> + user for additional arguments and append this to ARGS. */
> + const char *prompt = nullptr;
> + while (!args_complete_p (args))
> + {
> + if (prompt == nullptr)
> + {
> + prompt = getenv ("PS2");
> + if (prompt == nullptr)
> + prompt = "> ";
> + }
> +
> + std::string buffer;
> + const char *content = command_line_input (buffer, prompt, "set_args");
> + if (content == nullptr)
> + return {};
> +
> + args += "\n" + buffer;
> + }
> +
> + return args;
> +}
> +
> /* Store the new value passed to 'set args'. */
>
> static void
> -set_args_value (const std::string &args)
> +set_args_value (const std::string &args_in)
> {
> + std::string args;
> +
> + if (!args_in.empty ())
> + {
> + args = get_complete_args (args_in);
> + if (args.empty ())
> + return;
> + }
> +
> current_inferior ()->set_args (args);
> }
>
> @@ -376,6 +477,21 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
>
> dont_repeat ();
>
> + gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
> + args = stripped.get ();
> +
> + std::string inf_args;
> + /* If there were other args, beside '&', process them. */
> + if (args != nullptr)
> + {
> + /* If ARGS is only a partial argument string then this call will
> + interactively read more arguments from the user. If the user
> + quits then we shouldn't start the inferior. */
> + inf_args = get_complete_args (args);
> + if (inf_args.empty ())
> + return;
> + }
> +
> scoped_disable_commit_resumed disable_commit_resumed ("running");
>
> kill_if_already_running (from_tty);
> @@ -397,9 +513,6 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
> reopen_exec_file ();
> reread_symbols (from_tty);
>
> - gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
> - args = stripped.get ();
> -
> /* Do validation and preparation before possibly changing anything
> in the inferior. */
>
> @@ -412,6 +525,9 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
>
> /* Done. Can now set breakpoints, change inferior args, etc. */
>
> + if (!inf_args.empty ())
> + current_inferior ()->set_args (inf_args);
> +
> /* Insert temporary breakpoint in main function if requested. */
> if (run_how == RUN_STOP_AT_MAIN)
> {
> @@ -433,10 +549,6 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
> the user has to manually nuke all symbols between runs if they
> want them to go away (PR 2207). This is probably reasonable. */
>
> - /* If there were other args, beside '&', process them. */
> - if (args != nullptr)
> - current_inferior ()->set_args (args);
> -
> if (from_tty)
> {
> uiout->field_string (nullptr, "Starting program");
> diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
> index 6c22ecb3c54..b259e4a4217 100644
> --- a/gdb/testsuite/gdb.base/inferior-args.exp
> +++ b/gdb/testsuite/gdb.base/inferior-args.exp
> @@ -211,12 +211,15 @@ lappend test_desc_list [list "test four" \
> [list "$hex \"'\"" \
> "$hex \"\\\\\"\""]]
>
> -# Run all tests in the global TEST_DESC_LIST.
> +# Run all tests in the global TEST_DESC_LIST, as well as some tests of
> +# inferior arguments containing newlines.
> proc run_all_tests {} {
> + set all_methods { "start" "starti" "run" "set args" }
> +
> foreach desc $::test_desc_list {
> lassign $desc name stub_suitable args re_list
> with_test_prefix $name {
> - foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
> + foreach_with_prefix set_method $all_methods {
> foreach_with_prefix startup_with_shell { on off } {
> do_test $set_method $startup_with_shell $args $re_list \
> $stub_suitable
> @@ -224,6 +227,70 @@ proc run_all_tests {} {
> }
> }
> }
> +
> + # Check the multi-line argument entry. This isn't going to work when
> + # using the gdbstub, as the only way to set arguments in this case is
> + # via the gdbserver command line, which isn't what we're testing here.
> + if { ![use_gdb_stub] } {
> + foreach_with_prefix set_method $all_methods {
> + clean_restart $::binfile
> +
> + # First check that we can abort entering multi-line arguments.
> + set saw_prompt false
> + gdb_test_multiple "$set_method \"ab" "abort argument entry" {
> + -re "^$set_method \"ab\r\n" {
> + exp_continue
> + }
> + -re "^> $" {
> + set saw_prompt true
> + send_gdb "\004"
> + exp_continue
> + }
> + -re "quit\r\n$::gdb_prompt $" {
> + gdb_assert {$saw_prompt} \
> + $gdb_test_name
> + }
> + }
> +
> + # Now place a breakpoint on main.
> + if { ![gdb_breakpoint "main" message] } {
> + fail "could not set breakpoint on main"
> + continue
> + }
> +
> + # And actually enter some multi-line arguments.
> + set saw_prompt false
> + gdb_test_multiple "$set_method \"xy" "complete argument entry" {
> + -re "^$set_method \"xy\r\n" {
> + exp_continue
> + }
> + -re "^> $" {
> + set saw_prompt true
> + send_gdb "12\"\n"
> + exp_continue
> + }
> +
> + -re "$::gdb_prompt $" {
> + gdb_assert { $saw_prompt } \
> + $gdb_test_name
> + }
> + }
> +
> + # For the two methods that don't automatically run to main,
> + # poke the inferior along to main.
> + if { $set_method == "set args" } {
> + if { ![runto_main] } {
> + continue
> + }
> + } elseif { $set_method == "starti" } {
> + gdb_continue_to_breakpoint "b/p in main"
> + }
> +
> + # And check we correctly see the argument containing a newline.
> + gdb_test "print argc" " = 2"
> + gdb_test "print argv\[1\]" " = $::hex \"xy\\\\n12\""
> + }
> + }
> }
>
> run_all_tests
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 14/16] gdb/gdbserver: remove some uses of free_vector_argv
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (12 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 13/16] gdb: allow 'set args' and run commands to contain newlines Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 14:26 ` [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues Andrew Burgess
` (4 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This commit removes some of the uses of free_vector_argv and instead
makes use of gdb::unique_xmalloc_ptr<char> to manage memory.
Most of this patch is about changing various APIs to accept the
gdb::unique_xmalloc_ptr<char> vectors instead of raw 'char *'.
However, there are a couple of places, one in gdbserver, and one in
GDB, where retaining the old API will remove the need to malloc copies
of all the incoming arguments -- this is for the initial set of
arguments delivered from the OS. To support these I've added an
overload of construct_inferior_arguments that retains the old
API (taking a gdb::array_view<char *>), and we make use of this to
avoid unnecessary malloc/free calls.
There is a place in gdb/nat/fork-inferior.c where it seems unavoidable
that free_vector_argv is needed. In this case we need to build an
array of 'char *' to pass to an exec call, so using
gdb::unique_xmalloc_ptr<char> is out of the question I think.
There should be no user visible changes after this commit.
---
gdb/inferior.c | 9 -----
gdb/inferior.h | 8 +++--
gdb/python/py-inferior.c | 7 ++--
gdb/unittests/remote-arg-selftests.c | 33 +++----------------
gdbserver/server.cc | 22 ++++---------
gdbsupport/common-inferior.cc | 49 ++++++++++++++++++++++++----
gdbsupport/common-inferior.h | 7 ++++
gdbsupport/remote-args.cc | 2 +-
gdbsupport/remote-args.h | 2 +-
9 files changed, 71 insertions(+), 68 deletions(-)
diff --git a/gdb/inferior.c b/gdb/inferior.c
index ed138888024..d2cdf31551a 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -165,15 +165,6 @@ inferior::tty ()
return m_terminal;
}
-/* See inferior.h. */
-
-void
-inferior::set_args (gdb::array_view<char * const> args,
- escape_args_func escape_func)
-{
- set_args (construct_inferior_arguments (args, escape_func));
-}
-
void
inferior::set_arch (gdbarch *arch)
{
diff --git a/gdb/inferior.h b/gdb/inferior.h
index d3824236ef6..3dad403b244 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -526,8 +526,12 @@ class inferior : public refcounted_object,
};
/* Set the argument string from some strings. */
- void set_args (gdb::array_view<char * const> args,
- escape_args_func escape_func);
+ template<typename T>
+ void set_args (gdb::array_view<T const> args,
+ escape_args_func escape_func)
+ {
+ this->set_args (construct_inferior_arguments (args, escape_func));
+ }
/* Get the argument string to use when running this inferior.
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 5b7c7fb9365..c7a47e92ded 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -915,11 +915,8 @@ infpy_set_args (PyObject *self, PyObject *value, void *closure)
return -1;
args.push_back (std::move (str));
}
- std::vector<char *> argvec;
- for (const auto &arg : args)
- argvec.push_back (arg.get ());
- gdb::array_view<char * const> view (argvec.data (), argvec.size ());
- inf->inferior->set_args (view, escape_quotes_and_white_space);
+ gdb::array_view<gdb::unique_xmalloc_ptr<char> const> arg_view (args);
+ inf->inferior->set_args (arg_view, escape_quotes_and_white_space);
}
else
{
diff --git a/gdb/unittests/remote-arg-selftests.c b/gdb/unittests/remote-arg-selftests.c
index 3240ab9aeea..b732d94792d 100644
--- a/gdb/unittests/remote-arg-selftests.c
+++ b/gdb/unittests/remote-arg-selftests.c
@@ -59,32 +59,6 @@ arg_test_desc desc[] = {
{ "1 '\n' 3", { "1", "\n", "3" }, "1 '\n' 3" },
};
-/* Convert a std::vector<std::string> into std::vector<char *>. This
- requires copying all of the string content. This class takes care of
- freeing the memory once we are done with it. */
-
-struct args_as_c_strings
-{
- args_as_c_strings (std::vector<std::string> args)
- {
- for (const auto & a : args)
- m_data.push_back (xstrdup (a.c_str ()));
- }
-
- ~args_as_c_strings ()
- {
- free_vector_argv (m_data);
- }
-
- std::vector<char *> &get ()
- {
- return m_data;
- }
-
-private:
- std::vector<char *> m_data;
-};
-
/* Run the remote argument passing self tests. */
static void
@@ -132,9 +106,10 @@ self_test ()
}
/* Now join the arguments. */
- args_as_c_strings split_args_c_str (split_args);
- std::string joined_args
- = gdb::remote_args::join (split_args_c_str.get ());
+ std::vector<gdb::unique_xmalloc_ptr<char>> temp_args;
+ for (const auto & a : split_args)
+ temp_args.push_back (make_unique_xstrdup (a.c_str ()));
+ std::string joined_args = gdb::remote_args::join (temp_args);
if (run_verbose ())
debug_printf ("Joined (%s), expected (%s)\n",
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0445fa0237f..9c5d8ee4f54 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3385,7 +3385,7 @@ handle_v_run (char *own_buf)
{
client_state &cs = get_client_state ();
char *p, *next_p;
- std::vector<char *> new_argv;
+ std::vector<gdb::unique_xmalloc_ptr<char>> new_argv;
gdb::unique_xmalloc_ptr<char> new_program_name;
int i;
@@ -3405,7 +3405,7 @@ handle_v_run (char *own_buf)
else if (p == next_p)
{
/* Empty argument. */
- new_argv.push_back (xstrdup (""));
+ new_argv.push_back (make_unique_xstrdup (""));
}
else
{
@@ -3416,14 +3416,13 @@ handle_v_run (char *own_buf)
if (arg == nullptr)
{
write_enn (own_buf);
- free_vector_argv (new_argv);
return;
}
if (i == 0)
new_program_name = std::move (arg);
else
- new_argv.push_back (arg.release ());
+ new_argv.push_back (std::move (arg));
}
if (*next_p == '\0')
break;
@@ -3436,7 +3435,6 @@ handle_v_run (char *own_buf)
if (program_path.get () == nullptr)
{
write_enn (own_buf);
- free_vector_argv (new_argv);
return;
}
}
@@ -3451,15 +3449,13 @@ handle_v_run (char *own_buf)
return;
}
else if (new_argv.size () == 1)
- program_args = std::string (new_argv[0]);
+ program_args = std::string (new_argv[0].get ());
else
program_args.clear ();
}
else
program_args = gdb::remote_args::join (new_argv);
- free_vector_argv (new_argv);
-
target_create_inferior (program_path.get (), program_args);
if (cs.last_status.kind () == TARGET_WAITKIND_STOPPED)
@@ -4345,16 +4341,12 @@ captured_main (int argc, char *argv[])
if (pid == 0 && *next_arg != NULL)
{
- int i, n;
-
- n = argc - (next_arg - argv);
program_path.set (next_arg[0]);
- std::vector<char *> temp_arg_vector;
- for (i = 1; i < n; i++)
- temp_arg_vector.push_back (next_arg[i]);
escape_args_func escape_func = (escape_args ? escape_shell_characters
: escape_quotes_and_white_space);
- program_args = construct_inferior_arguments (temp_arg_vector,
+ int arg_count = argc - (next_arg - argv) - 1;
+ gdb::array_view<char *> arg_view (&next_arg[1], arg_count);
+ program_args = construct_inferior_arguments (arg_view,
escape_func);
/* Wait till we are at first instruction in program. */
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index cf2cd9a090a..458d78295ab 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -20,30 +20,67 @@
#include "gdbsupport/common-defs.h"
#include "gdbsupport/common-inferior.h"
+#include "gdbsupport/function-view.h"
/* See common-inferior.h. */
bool startup_with_shell = true;
-/* See common-inferior.h. */
+/* Helper function for the two construct_inferior_arguments overloads
+ below. Accept a gdb::array_view over objects of type T. Convert each T
+ to a std::string by calling CB, and join all the resulting strings
+ together with a single space between each. */
-std::string
-construct_inferior_arguments (gdb::array_view<char * const> argv,
- escape_args_func escape_func)
+template<typename T>
+static std::string
+construct_inferior_arguments_1 (gdb::array_view<T const> argv,
+ gdb::function_view<std::string (const T &)> cb)
{
std::string result;
- for (const char *a : argv)
+ for (const T &a : argv)
{
if (!result.empty ())
result += " ";
- result += escape_func (a);
+ result += cb (a);
}
return result;
}
+/* See common-inferior.h. */
+
+std::string
+construct_inferior_arguments
+ (gdb::array_view<gdb::unique_xmalloc_ptr<char> const> argv,
+ escape_args_func escape_func)
+{
+ /* Convert ARG to a std::string by applying ESCAPE_FUNC. */
+ auto escape_cb = [&] (const gdb::unique_xmalloc_ptr<char> &arg)
+ {
+ return escape_func (arg.get ());
+ };
+
+ return construct_inferior_arguments_1<gdb::unique_xmalloc_ptr<char>>
+ (argv, escape_cb);
+}
+
+/* See common-inferior.h. */
+
+std::string
+construct_inferior_arguments (gdb::array_view<char * const> argv,
+ escape_args_func escape_func)
+{
+ /* Convert ARG to a std::string by applying ESCAPE_FUNC. */
+ auto escape_cb = [&] (const char * const &arg)
+ {
+ return escape_func (arg);
+ };
+
+ return construct_inferior_arguments_1<char *> (argv, escape_cb);
+}
+
/* Escape characters in ARG and return an updated string. The string
SPECIAL contains the set of characters that must be escaped. SPECIAL
must not be nullptr, and it is assumed that SPECIAL contains the newline
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 7cc01fb2f28..c607ab6d98e 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -74,6 +74,13 @@ extern std::string escape_quotes_and_white_space (const char *arg);
into a single string, separating each element with a single space
character. */
+extern std::string
+construct_inferior_arguments
+ (gdb::array_view<gdb::unique_xmalloc_ptr<char> const> args,
+ escape_args_func escape_func);
+
+/* An overload of the above that takes an array of raw pointers. */
+
extern std::string
construct_inferior_arguments (gdb::array_view<char * const> args,
escape_args_func escape_func);
diff --git a/gdbsupport/remote-args.cc b/gdbsupport/remote-args.cc
index 96c12ffac67..b808a64efce 100644
--- a/gdbsupport/remote-args.cc
+++ b/gdbsupport/remote-args.cc
@@ -37,7 +37,7 @@ gdb::remote_args::split (std::string args)
/* See remote-args.h. */
std::string
-gdb::remote_args::join (std::vector<char *> &args)
+gdb::remote_args::join (std::vector<gdb::unique_xmalloc_ptr<char>> &args)
{
return construct_inferior_arguments (args, escape_shell_characters);
}
diff --git a/gdbsupport/remote-args.h b/gdbsupport/remote-args.h
index c0acce9b7c4..1397a4d16d9 100644
--- a/gdbsupport/remote-args.h
+++ b/gdbsupport/remote-args.h
@@ -37,7 +37,7 @@ extern std::vector<std::string> split (std::string args);
passed through ::join we will get back the string 'a\ b' (without the
single quotes), that is, we choose to escape the white space, rather
than wrap the argument in quotes. */
-extern std::string join (std::vector<char *> &args);
+extern std::string join (std::vector <gdb::unique_xmalloc_ptr<char>> &args);
} /* namespace remote_args */
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (13 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 14/16] gdb/gdbserver: remove some uses of free_vector_argv Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 16:32 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
2024-01-09 14:26 ` [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining Andrew Burgess
` (3 subsequent siblings)
18 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
Add a new maintenance command 'maint test-remote-args', this command
takes an argument string and splits it using gdb::remote_args::split
and then joins the result using gdb::remote_args::join and prints all
of the results. This is useful for diagnosing problems with remote
argument passing.
This new command is identical to what the remote argument self-tests
do, I found it easier to have a maintenance command available for
testing rather than having to add a new selftest and recompile GDB.
---
gdb/NEWS | 4 ++
gdb/doc/gdb.texinfo | 21 +++++++
gdb/remote.c | 59 +++++++++++++++++++
.../gdb.base/maint-test-remote-args.exp | 40 +++++++++++++
4 files changed, 124 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/maint-test-remote-args.exp
diff --git a/gdb/NEWS b/gdb/NEWS
index 2ba1899f78c..8f0bf34fccc 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -51,6 +51,10 @@ set remote thread-options-packet
show remote thread-options-packet
Set/show the use of the thread options packet.
+maintenance test-remote-args ARGS
+ Test splitting and joining of inferior arguments ARGS as they would
+ be split and joined when being passed to a remote target.
+
* Changed commands
set args
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2015293ee0d..6ff059d14ed 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42204,6 +42204,26 @@
@value{GDBN} supports. They are used by the testsuite for exercising
the settings infrastructure.
+@kindex maint test-remote-args
+@item maint test-remote-args @var{args}
+For targets that don't support passing inferior arguments as a single
+string (@pxref{single-inf-arg}), @value{GDBN} will attempt to split
+the inferior arguments before passing them to the remote target, and
+the remote target might choose to join the inferior arguments upon
+receipt. Historically gdbserver did join inferior arguments, but now
+it will request inferior arguments be passed as a single string if
+@value{GDBN} supports this feature.
+
+This maintenance command splits @var{args} as @value{GDBN} would
+normally split such an argument string before passing the arguments to
+a remote target, the split arguments are then printed.
+
+The split arguments are then joined together as gdbserver would join
+them, and the result is printed.
+
+This command is intended to help diagnose issues passing inferior
+arguments to remote targets.
+
@kindex maint set backtrace-on-fatal-signal
@kindex maint show backtrace-on-fatal-signal
@item maint set backtrace-on-fatal-signal [on|off]
@@ -44548,6 +44568,7 @@
This feature indicates whether @value{GDBN} wants to know the
supported actions in the reply to @samp{vCont?} packet.
+@anchor{single-inf-arg}
@item single-inf-arg
This feature indicates that @value{GDBN} would like to send the
inferior arguments as a single string within the @samp{vRun} packet.
diff --git a/gdb/remote.c b/gdb/remote.c
index 75d275d38df..9b4d31dc633 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12029,6 +12029,51 @@ cli_packet_command (const char *args, int from_tty)
send_remote_packet (view, &cb);
}
+/* Implement 'maint test-remote-args' command.
+
+ Treat ARGS as an argument string. Split the remote arguments using
+ gdb::remote_args::split, and then join using gdb::remote_args::join.
+ The split and joined arguments are printed out. Additionally, the
+ joined arguments are split and joined a second time, and compared to the
+ result of the first join, this provides some basic validation that GDB
+ sess the joined arguments as equivalent to the original argument
+ string. */
+
+static void
+test_remote_args_command (const char *args, int from_tty)
+{
+ std::vector<std::string> split_args = gdb::remote_args::split (args);
+
+ gdb_printf ("Input (%s)\n", args);
+ for (const auto & a : split_args)
+ gdb_printf (" (%s)\n", a.c_str ());
+
+ std::vector<gdb::unique_xmalloc_ptr<char>> tmp_split_args;
+ for (const auto & a : split_args)
+ tmp_split_args.emplace_back (xstrdup (a.c_str ()));
+
+ std::string joined_args = gdb::remote_args::join (tmp_split_args);
+
+ gdb_printf ("Output (%s)\n", joined_args.c_str ());
+
+ std::vector<std::string> resplit = gdb::remote_args::split (joined_args);
+
+ tmp_split_args.clear ();
+ for (const auto & a : resplit)
+ tmp_split_args.emplace_back (xstrdup (a.c_str ()));
+
+ std::string rejoined = gdb::remote_args::join (tmp_split_args);
+
+ if (joined_args != rejoined || split_args != resplit)
+ {
+ gdb_printf ("FAILURE ON REJOINING\n");
+ gdb_printf ("Resplit args:\n");
+ for (const auto & a : resplit)
+ gdb_printf (" (%s)\n", a.c_str ());
+ gdb_printf ("Rejoined (%s)\n", rejoined.c_str ());
+ }
+}
+
#if 0
/* --------- UNIT_TEST for THREAD oriented PACKETS ------------------- */
@@ -16145,6 +16190,20 @@ from the target."),
/* Eventually initialize fileio. See fileio.c */
initialize_remote_fileio (&remote_set_cmdlist, &remote_show_cmdlist);
+ add_cmd ("test-remote-args", class_maintenance,
+ test_remote_args_command, _("\
+Test remote argument splitting and joining.\n \
+ maintenance test-remote-args ARGS\n\
+For remote targets that don't support passing inferior arguments as a\n\
+single string, GDB needs to split the inferior arguments before passing\n\
+them, and gdbserver needs to join the arguments it receives.\n\
+This command splits ARGS just as GDB would before passing them to a\n\
+remote target, and prints the result. This command then joins the\n\
+arguments just as gdbserver would, and prints the results.\n\
+This command is useful in diagnosing problems when passing arguments\n\
+between GDB and a remote target."),
+ &maintenancelist);
+
#if GDB_SELF_TEST
selftests::register_test ("remote_memory_tagging",
selftests::test_memory_tagging_functions);
diff --git a/gdb/testsuite/gdb.base/maint-test-remote-args.exp b/gdb/testsuite/gdb.base/maint-test-remote-args.exp
new file mode 100644
index 00000000000..3daf0725932
--- /dev/null
+++ b/gdb/testsuite/gdb.base/maint-test-remote-args.exp
@@ -0,0 +1,40 @@
+# Copyright 2024 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Test the 'maint test-remote-args' command.
+#
+# We do minimal testing in here. If you are thinking of adding a new
+# test here then you are most likely adding the test in the wrong
+# place. Remote argument testing is checked in the following test
+# scripts: gdb.base/args.exp, gdb.base/inferior-args.exp,
+# gdb.base/startup-with-shell.exp, and gdb.python/py-inferior.exp.
+# The test gdb.gdb/unittest.exp also runs 'maint selftest
+# remote-args', which are the remote argument self tests.
+#
+# If you have a new test for an argument that was being passed
+# incorrectly, then add the test to one of those scripts.
+#
+# This file is ONLY for validating that the 'maint test-remote-args'
+# command itself is working.
+
+gdb_start
+
+gdb_test "maint test-remote-args a b c" \
+ [multi_line \
+ "Input \\(a b c\\)" \
+ " \\(a\\)" \
+ " \\(b\\)" \
+ " \\(c\\)" \
+ "Output \\(a b c\\)"]
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues
2024-01-09 14:26 ` [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues Andrew Burgess
@ 2024-01-09 16:32 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:32 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:38 +0000
>
> Add a new maintenance command 'maint test-remote-args', this command
> takes an argument string and splits it using gdb::remote_args::split
> and then joins the result using gdb::remote_args::join and prints all
> of the results. This is useful for diagnosing problems with remote
> argument passing.
>
> This new command is identical to what the remote argument self-tests
> do, I found it easier to have a maintenance command available for
> testing rather than having to add a new selftest and recompile GDB.
> ---
> gdb/NEWS | 4 ++
> gdb/doc/gdb.texinfo | 21 +++++++
> gdb/remote.c | 59 +++++++++++++++++++
> .../gdb.base/maint-test-remote-args.exp | 40 +++++++++++++
> 4 files changed, 124 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/maint-test-remote-args.exp
The documentation parts are okay, thanks.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues
2024-01-09 14:26 ` [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues Andrew Burgess
2024-01-09 16:32 ` Eli Zaretskii
@ 2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:57 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches; +Cc: Michael Weghorn
On 1/9/24 06:26, Andrew Burgess wrote:
> diff --git a/gdb/testsuite/gdb.base/maint-test-remote-args.exp b/gdb/testsuite/gdb.base/maint-test-remote-args.exp
> new file mode 100644
> index 00000000000..3daf0725932
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/maint-test-remote-args.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +#
> +# 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/>.
> +
> +# Test the 'maint test-remote-args' command.
> +#
> +# We do minimal testing in here. If you are thinking of adding a new
> +# test here then you are most likely adding the test in the wrong
> +# place. Remote argument testing is checked in the following test
> +# scripts: gdb.base/args.exp, gdb.base/inferior-args.exp,
> +# gdb.base/startup-with-shell.exp, and gdb.python/py-inferior.exp.
> +# The test gdb.gdb/unittest.exp also runs 'maint selftest
> +# remote-args', which are the remote argument self tests.
> +#
> +# If you have a new test for an argument that was being passed
> +# incorrectly, then add the test to one of those scripts.
> +#
> +# This file is ONLY for validating that the 'maint test-remote-args'
> +# command itself is working.
> +
> +gdb_start
> +
> +gdb_test "maint test-remote-args a b c" \
> + [multi_line \
> + "Input \\(a b c\\)" \
> + " \\(a\\)" \
> + " \\(b\\)" \
> + " \\(c\\)" \
> + "Output \\(a b c\\)"]
Is it possible to add a test to exercise the failure mode, too?
Keith
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (14 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 15/16] gdb: new maintenance command to help debug remote argument issues Andrew Burgess
@ 2024-01-09 14:26 ` Andrew Burgess
2024-01-09 16:37 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
2024-01-09 16:58 ` [PATCH 00/16] Inferior argument (inc for remote targets) changes Eli Zaretskii
` (2 subsequent siblings)
18 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-09 14:26 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Michael Weghorn
This commit attempts to improve how GDB splits and joins inferior
arguments when passing them to a remote target. The splitting and
joining is now the legacy approach for passing arguments to a remote
target; the preference is to pass arguments as a single string,
however, if GDB is connecting to a target that doesn't support passing
inferior arguments as a single string then the joining and splitting
approach is still used, and this commit tries to improve that approach
a little.
Currently when GDB passes arguments to a remote target this is done by
using the gdb_argv class. This is the same technique uses to split
the argument vector in order to start an inferior without a shell.
This approach has a problem. Consider a native target, and the user
sets the inferior arguments like this:
(gdb) set args \$VAR
In this case the user has escaped the '$' character, removing it's
special meaning. When started under a shell the '\$VAR' will not
result in variable expansion, and the inferior will see a literal
'$VAR'. When startup-with-shell is off then gdb_argv is used to split
the inferior arguments, and gdb_argv also removes the backslash
escape, so the inferior again sees a literal '$VAR' string. I think
this consistency makes sense.
Now consider if the user sets the inferior arguments like this:
(gdb) set args $VAR
In the startup-with-shell case the shell will expand '$VAR' and the
inferior will see whatever the value of '$VAR' was. But if
startup-with-shell is off then there is no shell, and the inferior
will just be passed a literal '$VAR' string.
Given the above, when we consider using gdb_argv for splitting
inferior arguments in order to pass them to a remote target, the
problem we have is this: there is a many to one between input strings
and output strings. Both '\$VAR' and '$VAR' both result in an output
string of '$VAR'. The remote target can't know what the actual input
was. If the remote adds the escaping then (when startup-with-shell is
on) the shell will not substitute variables when it should, and if the
remote doesn't add escaping then the shell will try to substitute
variables when it shouldn't.
So I think we have to admit that the current approach is not ideal,
but we cannot change things too much; in simple cases (i.e. without
too many special shell characters) the existing approach does work, so
if we do something too different then existing targets are going to
break even for simple cases.
The current approach then is for the remote target to escape any
special shell characters within the arguments received from GDB. This
is too aggressive.
An ideal solution might be to have GDB remove quoting, but to keep, or
apply all required escaping, and for the remote target to do nothing
but join the incoming arguments with a single whitespace character.
This would certainly be easiest all around, but would break a lot of
backward compatibility, for example given this:
(gdb) set args 'ab cd'
GDB could strip the single quotes, and escape the whitespace, sending
the argument to the remote target as (ab\ cd) -- without the
parenthesis -- then on the remote end the target could use this
argument directly.
However, if we adopt this approach existing targets, those that escape
special shell characters, are likely to do the wrong thing, it's
likely that both the backslash and space would be escaped.
So, what we are searching for is a solution that sits somewhere half
way between GDB performs all the escaping, and the current situation,
where the target is expected to apply the escaping.
I propose that the rules be something like:
+ GDB removes the quotes, and escapes all special shell characters
except for the following:
+ Whitespace is not escaped, the remote is expected to escape any
whitespace within an argument,
+ Quote characters are not escaped, the remote is expected to escape
any single or double quote characters within an argument.
+ Any other special shell characters will have been escaped, if
needed, by GDB, and the escaping included in the transmitted
argument.
The solution presented here takes into account unquoted arguments, as
well as single and double quoted arguments.
**WARNING**: This is a potentially breaking change in the remote
protocol. A user, using an existing remote target, that handles
inferior arguments in a particular way may find that with this change
now causes some cases to behave differently. My hope is that by
handling whitespace and quotes differently we can preserve enough
behaviour that the problem cases are not too common.
As part of this commit I've updated unittests/remote-arg-selftests.c,
and it's possible to see examples of when the arguments passed from
GDB to the remote will change, and when they remain the same.
And there are some tests in gdb.base/startup-with-shell.exp that now
pass even when using a remote target board and when the
single-inferior-arg feature is turned off.
All of the other argument passing tests, e.g. gdb.base/args.exp, and
gdb.base/inferior-args.exp continue to pass, and also already run with
the single-inferior-arg feature both enabled and disabled, so the
changes in this commit should be well tested already.
What isn't addressed at all in this commit is parameter expansion,
command substitution, or arithmetic expansion. In some way these are
similar to quotes, but instead of single or double quotes the argument
is surrounded with ${...}, $(...), or $((....)). However, unlike
single or double quotes I don't think that we can remove the quoting
in these cases, if we did, how would the remote target know what to
add back?
However, I'm not proposing to even try to address these cases just
yet. Ideally, if this whole series is accepted then by this point we
will have already merged the patch that passes inferior arguments as a
single string, a solution that handles all of the above cases. We can
always revisit this later on and look at supporting these cases if
needed.
This work was inspired by this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
I originally started reviewing this series, and figured that, surely
it must be possible to solve the problem that series tries to solve by
just being "smarter" about how we split arguments. It was only when I
actually tried to implement this, and started writing tests, that I
realised that above series was right, and what we really need to do is
pass all arguments as a single string. But I think that series is
worth a read, and should get credit for reaching the right answer
quicker than me!
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
---
gdb/NEWS | 8 +
gdb/doc/gdb.texinfo | 13 +
gdb/testsuite/gdb.base/startup-with-shell.exp | 27 +--
gdb/unittests/remote-arg-selftests.c | 18 +-
gdbsupport/remote-args.cc | 229 +++++++++++++++++-
5 files changed, 264 insertions(+), 31 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index 8f0bf34fccc..206f72070bb 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -17,6 +17,14 @@
this flag is used gdbserver will not escape special shell characters
within the inferior arguments.
+* There have been changes to how inferior arguments are passed to
+ remote targets. When using the latest GDB and gdbserver the new
+ single-inf-arg will be used for argument passing. When using remote
+ targets that don't support single-inf-arg, GDB will now escape
+ special shell characters (except for whitespace and quotes) within
+ arguments before passing them to the remote target. Remote targets
+ will need updating to take this into account.
+
* Changed commands
disassemble
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6ff059d14ed..cc9c5ef0ccc 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -43137,6 +43137,19 @@
@var{argument} string, which includes all inferior arguments,
separated with whitespace.
+If @value{GDBN} didn't offer @samp{single-inf-arg+}, or the remote
+didn't choose to use this feature, then @value{GDBN} will split the
+inferior arguments before passing them to the remote target. Single
+and double quotes surrounding any argument will have been removed by
+@value{GDBN}. Any special shell characters within each argument will
+have been escaped with a backslash character except for quote and
+whitespace characters, these will not be escaped within the arguments,
+the remote target is expected to escape these characters itself.
+
+These slightly complex rules exist for backward compatibility reasons.
+When implementing a new remote target it is suggested that the
+@samp{single-inf-arg} feature be supported.
+
@c FIXME: What about non-stop mode?
This packet is only available in extended mode (@pxref{extended mode}).
diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp
index f467774dc5f..3952bd774d4 100644
--- a/gdb/testsuite/gdb.base/startup-with-shell.exp
+++ b/gdb/testsuite/gdb.base/startup-with-shell.exp
@@ -55,11 +55,8 @@ proc initial_setup_simple { startup_with_shell run_args } {
# it to ON_RE if startup-with-shell is on, otherwise compare to
# OFF_RE.
#
-# If PROBLEMATIC_ON is true then when startup-with-shell is on we
-# expect the comparison to fail, so setup an xfail.
-#
# TESTNAME is a string used in the test names.
-proc run_test { args on_re off_re testname { problematic_on false } } {
+proc run_test { args on_re off_re testname } {
foreach startup_with_shell { "on" "off" } {
with_test_prefix "$testname, startup_with_shell: ${startup_with_shell}" {
if {![initial_setup_simple $startup_with_shell $args]} {
@@ -68,14 +65,8 @@ proc run_test { args on_re off_re testname { problematic_on false } } {
if { $startup_with_shell } {
set re $on_re
- set problematic $problematic_on
} else {
set re $off_re
- set problematic false
- }
-
- if { $problematic } {
- setup_xfail "*-*-*" gdb/28392
}
gdb_test "print argv\[1\]" "\\\$$::decimal = $::hex $re" $testname
@@ -91,15 +82,14 @@ proc run_test_same { args re testname } {
}
# Run the actual tests
-proc run_all_tests { { is_remote_with_split_args false } } {
+proc run_all_tests { } {
# The regexp to match a single '\' character.
set bs "\\\\"
run_test "$::unique_file_dir/*.unique-extension" \
"\"$::unique_file\"" \
"\"$::unique_file_dir/\\\*\.unique-extension\"" \
- "arg is glob" \
- $is_remote_with_split_args
+ "arg is glob"
run_test_same "$::unique_file_dir/\\*.unique-extension" \
"\"$::unique_file_dir/\\\*\.unique-extension\"" \
@@ -110,8 +100,7 @@ proc run_all_tests { { is_remote_with_split_args false } } {
run_test "\$TEST" \
"\"1234\"" \
"\"\\\$TEST\"" \
- "arg is shell variable" \
- $is_remote_with_split_args
+ "arg is shell variable"
run_test_same "\\\$TEST" \
"\"\\\$TEST\"" \
@@ -121,14 +110,12 @@ proc run_all_tests { { is_remote_with_split_args false } } {
run_test "\$(echo foo)" \
"\"foo\"" \
"\"\\\$\\(echo\"" \
- "arg is parameter expansion, command execution" \
- $is_remote_with_split_args
+ "arg is parameter expansion, command execution"
run_test "\$((2 + 3))" \
"\"5\"" \
"\"\\\$\\(\\(2\"" \
- "arg is parameter expansion, expression evaluation" \
- $is_remote_with_split_args
+ "arg is parameter expansion, expression evaluation"
run_test_same "\"\\a\"" \
"\"${bs}${bs}a\"" \
@@ -180,7 +167,7 @@ if {[target_info gdb_protocol] == "extended-remote"} {
with_test_prefix "single-inferior-arg disabled" {
save_vars { GDBFLAGS } {
append GDBFLAGS " -ex \"set remote single-inferior-argument-feature-packet off\""
- run_all_tests true
+ run_all_tests
}
}
}
diff --git a/gdb/unittests/remote-arg-selftests.c b/gdb/unittests/remote-arg-selftests.c
index b732d94792d..7abc9d46345 100644
--- a/gdb/unittests/remote-arg-selftests.c
+++ b/gdb/unittests/remote-arg-selftests.c
@@ -45,18 +45,26 @@ arg_test_desc desc[] = {
{ "a b c", { "a", "b", "c" }, "a b c" },
{ "\"a b\" 'c d'", { "a b", "c d" }, "a\\ b c\\ d" },
{ "\\' \\\"", { "'", "\"" }, "\\' \\\"" },
- { "'\\'", { "\\" }, "\\\\" },
- { "\"\\\\\" \"\\\\\\\"\"", { "\\", "\\\"" }, "\\\\ \\\\\\\"" },
+ { "'\\'", { "\\\\" }, "\\\\" },
+ { "\"\\\\\" \"\\\\\\\"\"", { "\\\\", "\\\\\"" }, "\\\\ \\\\\\\"" },
{ "\\ \" \" ' '", { " ", " ", " "}, "\\ \\ \\ " },
{ "\"'\"", { "'" }, "\\'" },
- { "'\"' '\\\"'", { "\"", "\\\"" } , "\\\" \\\\\\\""},
+ { "'\"' '\\\"'", { "\"", "\\\\\"" } , "\\\" \\\\\\\""},
{ "\"first arg\" \"\" \"third-arg\" \"'\" \"\\\"\" \"\\\\\\\"\" \" \" \"\"",
- { "first arg", "", "third-arg", "'", "\"", "\\\""," ", "" },
+ { "first arg", "", "third-arg", "'", "\"", "\\\\\""," ", "" },
"first\\ arg '' third-arg \\' \\\" \\\\\\\" \\ ''"},
{ "\"\\a\" \"\\&\" \"\\#\" \"\\<\" \"\\^\"",
- { "\\a", "\\&", "\\#" , "\\<" , "\\^"},
+ { "\\\\a", "\\\\\\&", "\\\\\\#" , "\\\\\\<" , "\\\\\\^"},
+ "\\\\a \\\\\\& \\\\\\# \\\\\\< \\\\\\^" },
+ { "\"\\\\a\" \"\\\\&\" \"\\\\#\" \"\\\\<\" \"\\\\^\"",
+ { "\\\\a", "\\\\\\&", "\\\\\\#" , "\\\\\\<" , "\\\\\\^"},
"\\\\a \\\\\\& \\\\\\# \\\\\\< \\\\\\^" },
{ "1 '\n' 3", { "1", "\n", "3" }, "1 '\n' 3" },
+ { "abc* abc\\* abc\\\\*", { "abc*", "abc\\*", "abc\\\\*" },
+ "abc* abc\\* abc\\\\*" },
+ { "\"ab\\ cd\" \"ef\\'gh\"", { "ab\\\\ cd", "ef\\\\'gh" },
+ "ab\\\\\\ cd ef\\\\\\'gh" },
+ { "\"$VAR\" \"ab`foo`cd\"", { "$VAR", "ab`foo`cd" }, "$VAR ab`foo`cd" },
};
/* Run the remote argument passing self tests. */
diff --git a/gdbsupport/remote-args.cc b/gdbsupport/remote-args.cc
index b808a64efce..77e4c1a4bc1 100644
--- a/gdbsupport/remote-args.cc
+++ b/gdbsupport/remote-args.cc
@@ -20,18 +20,235 @@
#include "gdbsupport/common-inferior.h"
#include "gdbsupport/buildargv.h"
+/* This file contains the function used for splitting an argument string
+ into separate arguments in preparation for sending the argument over the
+ remote protocol, as well as the function to merge the separate arguments
+ back together into a single argument string.
+
+ The logic within these functions is slightly more complex that it should
+ be. This is in order to maintain some level of backward compatibility.
+
+ In the following test example of command line arguments will be given.
+ To avoid confusion arguments, and argument strings will be delimited
+ with '(' and ')', the parenthesis are not part of the argument itself.
+ This is clearer than using quotes, as some of the examples will include
+ quotes within the arguments.
+
+ Historically, the algorithm used to split the argument string into
+ separate arguments removed a level of quoting from the arguments. For
+ example consider the following argument string: (abc* abc\*). The
+ historic algorithm would split this into (abc*) and (abc*). Notice that
+ the two arguments are identical. On the remote end we are now destined
+ for failure, either we apply an escape to both '*' characters, or we
+ apply an escape to neither. In either case, we get one of the arguments
+ wrong. The historic approach was just broken.
+
+ However, the historic approach has been in place for many years.
+ Clearly not all arguments were corrupted in the manor described above,
+ so lots of things did work. For example, the string: ("ab cd" "ef")
+ will be split into (ab cd) and (ef). And the string ('"') will become
+ just (").
+
+ What we can observer from all of these examples, is that the historic
+ approach at the remote end was to simple apply an escape to every
+ special shell character, quotes, white space, as well as every other
+ special character (e.g. (*)). The problem with this approach is that
+ sometimes special shell characters shouldn't be escaped.
+
+ If we could start from scratch, then the simple approach would be to
+ retain all escaping while splitting the argument string, and, where
+ quotes are used, convert this into backslash escaping as neeeded. Thus
+ the argument string ("ab cd" "ef") would become (ab\ cd) and (ef). And
+ the argument string (abc* abc\*) would become (abc*) (abc\*). On the
+ remote end, joining these arguments is as simple as concatenation with a
+ single space character between.
+
+ However, if we took this approach, then consider ("ab cd"). Previously
+ this was sent as (ab cd), but now it would be (ab\ cd). This breaks
+ backward compatibility.
+
+ And so, this is where the complexity comes in.
+
+ The strategy here is to split the arguments, removing all double and
+ single quotes. While removing quotes, special shell characters are
+ escaped as needed. But, white space characters, and quote characters
+ are not escaped. These characters must always be escaped, and so we can
+ safely drop the escape in these cases, this provides some degree of
+ backward compatibility. */
+
+/* Return true if C is a double or single quote character. */
+
+static bool
+isquote (const char c)
+{
+ return c == '"' || c == '\'';
+}
+
/* See remote-args.h. */
std::vector<std::string>
gdb::remote_args::split (std::string args)
{
- std::vector<std::string> results;
+ std::vector<std::string> remote_args;
+
+ const char *input = args.c_str ();
+ bool squote = false, dquote = false;
+
+#ifdef __MINGW32__
+ /* This holds all the characters considered special to the
+ Windows shells. */
+ static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+#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";
+#endif
+
+ /* Characters that are special within double quotes. */
+ static const char dquote_special[] = "$`\\";
+
+ input = skip_spaces (input);
+
+ do
+ {
+ std::string arg;
+
+ while (*input != '\0')
+ {
+ if (isspace (*input) && !squote && !dquote)
+ break;
+ else if (*input == '\\' && !squote)
+ {
+ if (input[1] == '\0')
+ arg += input[0];
+ else if (input[1] == '\n')
+ ++input;
+ else if (dquote && input[1] == '"')
+ {
+ arg += input[1];
+ ++input;
+ }
+ else if (dquote && strchr (dquote_special, input[1]) != nullptr)
+ {
+ /* Within double quotes, these characters have special
+ meaning. If they are escaped with a backslash then we
+ need to preserve the escape once we remove the
+ quotes. */
+ arg += input[0]; /* Backslash. */
+ arg += input[1]; /* Special character. */
+ ++input;
+ }
+ else if (dquote)
+ {
+ /* Within double quotes, none of the remaining characters
+ have any special meaning, the backslash before the
+ character is a literal backslash.
+
+ To retain the literal backslash with the quotes
+ removed, we need to escape the backslash. */
+ arg += input[0]; /* Backslash. */
+ arg += input[0]; /* Backslash. */
+
+ /* If the character after the backslash has special
+ meaning outside of the double quotes, then we need to
+ escape it now, otherwise it will gain special meaning
+ as we remove the surrounding quotes. However, as per
+ the comments at the head of this file; we don't
+ escape white space or quotes. */
+ if (!isspace (input[1])
+ && !isquote (input[1])
+ && strchr (special, input[1]) != nullptr)
+ arg += input[0]; /* Backslash. */
+
+ arg += input[1]; /* Character. */
+ ++input;
+ }
+ else if (isspace (input[1]) || isquote (input[1]))
+ {
+ /* We remove the escaping from white space and quote
+ characters. */
+ arg += input[1]; /* Character. */
+ ++input;
+ }
+ else
+ {
+ /* For everything else, retain the escaping. */
+ arg += input[0]; /* Backslash. */
+ arg += input[1]; /* Character. */
+ ++input;
+ }
+ }
+ else if (squote)
+ {
+ /* Inside a single quote argument there are no special
+ characters. A single quote finishes the argument. */
+ if (*input == '\'')
+ squote = false;
+ /* Don't add escaping to white space or quotes. We already
+ handled single quotes above, so the isquote call here will
+ only find double quotes. */
+ else if (isspace (*input) || isquote (*input))
+ arg += *input;
+ /* Any other special shell character needs a backslash adding
+ to avoid the character gaining special meaning outside of
+ the single quotes. */
+ else if (strchr (special, *input) != NULL)
+ {
+ arg += '\\';
+ arg += *input;
+ }
+ /* Any other character just gets added to the output. */
+ else
+ arg += *input;
+ }
+ else if (dquote)
+ {
+ /* Inside a double quoted argument. A double quote closes
+ the argument. An escaped double quote will have been
+ handled in the backslash handling block above. */
+ if (*input == '"')
+ dquote = false;
+ /* Don't add escaping for white space or quotes. We already
+ handled double quotes above, so the isquote call here will
+ only find single quotes. */
+ else if (isspace (*input) || isquote (*input))
+ arg += *input;
+ /* Any character that is not one of the few characters that
+ retains its special meaning without double quotes, but is
+ otherwise a special character needs an escape character
+ adding, to avoid the character gaining special meaning
+ outside of the quotes. */
+ else if (strchr (dquote_special, *input) == nullptr
+ && strchr (special, *input) != nullptr)
+ {
+ arg += '\\';
+ arg += *input;
+ }
+ /* Anything else just gets passed through to the output. */
+ else
+ arg += *input;
+ }
+ else
+ {
+ /* Found a character outside of a single or double quoted
+ argument, and not preceded by a backslash. */
+ if (*input == '\'')
+ squote = true;
+ else if (*input == '"')
+ dquote = true;
+ else
+ arg += *input;
+ }
+ ++input;
+ }
- gdb_argv argv (args.c_str ());
- for (int i = 0; argv[i] != nullptr; i++)
- results.emplace_back (argv[i]);
+ remote_args.push_back (std::move (arg));
+ input = skip_spaces (input);
+ }
+ while (*input != '\0');
- return results;
+ return remote_args;
}
/* See remote-args.h. */
@@ -39,5 +256,5 @@ gdb::remote_args::split (std::string args)
std::string
gdb::remote_args::join (std::vector<gdb::unique_xmalloc_ptr<char>> &args)
{
- return construct_inferior_arguments (args, escape_shell_characters);
+ return construct_inferior_arguments (args, escape_quotes_and_white_space);
}
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining
2024-01-09 14:26 ` [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining Andrew Burgess
@ 2024-01-09 16:37 ` Eli Zaretskii
2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:37 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:39 +0000
>
> gdb/NEWS | 8 +
> gdb/doc/gdb.texinfo | 13 +
> gdb/testsuite/gdb.base/startup-with-shell.exp | 27 +--
> gdb/unittests/remote-arg-selftests.c | 18 +-
> gdbsupport/remote-args.cc | 229 +++++++++++++++++-
> 5 files changed, 264 insertions(+), 31 deletions(-)
The documentation parts are okay, thanks.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining
2024-01-09 14:26 ` [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining Andrew Burgess
2024-01-09 16:37 ` Eli Zaretskii
@ 2024-01-21 3:57 ` Keith Seitz
1 sibling, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:57 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 1/9/24 06:26, Andrew Burgess wrote:
> I propose that the rules be something like:
>
> + GDB removes the quotes, and escapes all special shell characters
> except for the following:
>
> + Whitespace is not escaped, the remote is expected to escape any
> whitespace within an argument,
>
> + Quote characters are not escaped, the remote is expected to escape
> any single or double quote characters within an argument.
>
> + Any other special shell characters will have been escaped, if
> needed, by GDB, and the escaping included in the transmitted
> argument.
This is a difficult situation to resolve, isn't it? After reading
through your tests, I believe I have a tiny understanding of what
these rules are doing.
In the end, you've given this far more thought (and play) than I have
while attempting to review this.
Having said that, I only have typos to point out.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 6ff059d14ed..cc9c5ef0ccc 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -43137,6 +43137,19 @@
> @var{argument} string, which includes all inferior arguments,
> separated with whitespace.
>
> +If @value{GDBN} didn't offer @samp{single-inf-arg+}, or the remote
> +didn't choose to use this feature, then @value{GDBN} will split the
> +inferior arguments before passing them to the remote target. Single
> +and double quotes surrounding any argument will have been removed by
> +@value{GDBN}. Any special shell characters within each argument will
> +have been escaped with a backslash character except for quote and
> +whitespace characters, these will not be escaped within the arguments,
> +the remote target is expected to escape these characters itself.
> +
> +These slightly complex rules exist for backward compatibility reasons.
> +When implementing a new remote target it is suggested that the
> +@samp{single-inf-arg} feature be supported.
"When implementing a new remote target," (missing comma)
> +
> @c FIXME: What about non-stop mode?
>
> This packet is only available in extended mode (@pxref{extended mode}).
Keith
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/16] Inferior argument (inc for remote targets) changes
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (15 preceding siblings ...)
2024-01-09 14:26 ` [PATCH 16/16] gdb/gdbserver: rework argument splitting and joining Andrew Burgess
@ 2024-01-09 16:58 ` Eli Zaretskii
2024-01-20 22:46 ` Andrew Burgess
2024-01-10 8:28 ` Michael Weghorn
2024-01-21 3:56 ` Keith Seitz
18 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-09 16:58 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
> Date: Tue, 9 Jan 2024 14:26:23 +0000
>
> This series relates to bug PR gdb/28392. For background, check out
> this series:
>
> https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de
>
> which is a previous attempt to address this bug.
>
> It might also be worth reading:
>
> https://inbox.sourceware.org/gdb-patches/2b98ca58e47638b4760d86bd6e1fa9a9a79fa2ad.1695817255.git.aburgess@redhat.com
>
> which is a previous series of mine which covers some of the work from
> the original patch series.
>
> This series does include many ideas taken from the original patch
> series (thanks for Michael (original series author) for his work). I
> think this iteration does include some additional fixes beyond the
> original series, also there are more tests and documentation changes
> in this version.
>
> There is one small problem: patch #1 is within libiberty. I have
> posted this to the gcc list here:
>
> https://inbox.sourceware.org/gcc-patches/24a8d878590403540bc9b579ba58805985a4d2f7.1701881419.git.aburgess@redhat.com/
>
> However, GCC is currently in stage 4 of its release cycle, so I'm not
> expecting to see that patch merged before April, I've expanded on this
> more within the patch #1 email.
>
> Still, there's plenty here to comment on, and I figure between now and
> April I can address any feedback that's given.
Thanks. It is hard to follow all these changes especially as they are
broken into so many pieces, but I cannot avoid asking: what does this
mean for the quoting/escaping of arguments by GDB as a native debugger
on MS-Windows?
For starters, AFAIK on Windows there's no such thing as "startup with
shell", as the inferior is never invoked via the Windows shell. So
what does this feature mean and how (and whether) will it work in a
native GDB running on Windows debugging Windows programs? Likewise
with the --no-escape-args command-line option?
And this text:
> When starting GDB it is possible to set an inferior argument that
> contains a newline, for example:
>
> shell> gdb --args my.app "abc
> > def"
> ...
> (gdb) show args
> Argument list to give program being debugged when it is started is "abc'
> 'def".
>
> However, once GDB is started, the only way to install an argument
> containing a newline is to use the Python API.
>
> This commit changes that.
>
> After this commit 'set args' as well as 'run', 'start', and 'starti',
> will now accept multi-line inferior arguments, e.g.:
>
> (gdb) set args "abc
> > def"
> (gdb) show args
> Argument list to give program being debugged when it is started is ""abc
> def"".
>
> And also:
>
> (gdb) run "abc
> > def"
> ... etc ...
seems to say that quoting arguments can cause the inferior's
command-line arguments to include a newline, but AFAIK there's no way
to make that work on MS-Windows. If that is true, we should at least
document the lack of support for this on Windows.
Thanks, and apologies in advance if my questions make no sense.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 00/16] Inferior argument (inc for remote targets) changes
2024-01-09 16:58 ` [PATCH 00/16] Inferior argument (inc for remote targets) changes Eli Zaretskii
@ 2024-01-20 22:46 ` Andrew Burgess
2024-01-21 10:22 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2024-01-20 22:46 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches, m.weghorn
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: Andrew Burgess <aburgess@redhat.com>, Michael Weghorn <m.weghorn@posteo.de>
>> Date: Tue, 9 Jan 2024 14:26:23 +0000
>>
>> This series relates to bug PR gdb/28392. For background, check out
>> this series:
>>
>> https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de
>>
>> which is a previous attempt to address this bug.
>>
>> It might also be worth reading:
>>
>> https://inbox.sourceware.org/gdb-patches/2b98ca58e47638b4760d86bd6e1fa9a9a79fa2ad.1695817255.git.aburgess@redhat.com
>>
>> which is a previous series of mine which covers some of the work from
>> the original patch series.
>>
>> This series does include many ideas taken from the original patch
>> series (thanks for Michael (original series author) for his work). I
>> think this iteration does include some additional fixes beyond the
>> original series, also there are more tests and documentation changes
>> in this version.
>>
>> There is one small problem: patch #1 is within libiberty. I have
>> posted this to the gcc list here:
>>
>> https://inbox.sourceware.org/gcc-patches/24a8d878590403540bc9b579ba58805985a4d2f7.1701881419.git.aburgess@redhat.com/
>>
>> However, GCC is currently in stage 4 of its release cycle, so I'm not
>> expecting to see that patch merged before April, I've expanded on this
>> more within the patch #1 email.
>>
>> Still, there's plenty here to comment on, and I figure between now and
>> April I can address any feedback that's given.
>
> Thanks. It is hard to follow all these changes especially as they are
> broken into so many pieces, but I cannot avoid asking: what does this
> mean for the quoting/escaping of arguments by GDB as a native debugger
> on MS-Windows?
>
> For starters, AFAIK on Windows there's no such thing as "startup with
> shell", as the inferior is never invoked via the Windows shell. So
> what does this feature mean and how (and whether) will it work in a
> native GDB running on Windows debugging Windows programs? Likewise
> with the --no-escape-args command-line option?
>
> And this text:
>
>> When starting GDB it is possible to set an inferior argument that
>> contains a newline, for example:
>>
>> shell> gdb --args my.app "abc
>> > def"
>> ...
>> (gdb) show args
>> Argument list to give program being debugged when it is started is "abc'
>> 'def".
>>
>> However, once GDB is started, the only way to install an argument
>> containing a newline is to use the Python API.
>>
>> This commit changes that.
>>
>> After this commit 'set args' as well as 'run', 'start', and 'starti',
>> will now accept multi-line inferior arguments, e.g.:
>>
>> (gdb) set args "abc
>> > def"
>> (gdb) show args
>> Argument list to give program being debugged when it is started is ""abc
>> def"".
>>
>> And also:
>>
>> (gdb) run "abc
>> > def"
>> ... etc ...
>
> seems to say that quoting arguments can cause the inferior's
> command-line arguments to include a newline, but AFAIK there's no way
> to make that work on MS-Windows. If that is true, we should at least
> document the lack of support for this on Windows.
>
> Thanks, and apologies in advance if my questions make no sense.
The question makes perfect sense.
My hope is that the situation on Windows is no better or worse than it
ever was.
I'm reluctant to start writing documentation saying feature X does or
does not work on Windows and why though as I don't have access to a
Windows machine on which I can test any of this.
Specifically on the question of passing a newline within an argument, I
guess (having taken a quick peek at the window-nat.c file) that
embedding a newline within the argument string would cause problems?
I believe it's already possible today to inject newline characters via
the Python API. Maybe the window-nat.c code should be checking for
invalid characters and throw an error if the user tries to start an
inferior. I guess the other option would be to check the arguments at
the moment the user sets them ... but this isn't going to work if the
user sets the arguments before connecting to a remote target, so I
suspect it makes more sense to detect invalid argument characters just
prior to starting the inferior and giving an error at that point.
I agree that platform specific limitations should be documented. I'm
willing to help however I can to get that done. But I'm a little
uncomfortable trying to document something I can't test. I'll see if I
can source a Windows laptop, maybe I can build/test in that setup.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/16] Inferior argument (inc for remote targets) changes
2024-01-20 22:46 ` Andrew Burgess
@ 2024-01-21 10:22 ` Eli Zaretskii
2024-01-22 10:29 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2024-01-21 10:22 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, m.weghorn
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org, m.weghorn@posteo.de
> Date: Sat, 20 Jan 2024 22:46:02 +0000
>
> The question makes perfect sense.
>
> My hope is that the situation on Windows is no better or worse than it
> ever was.
>
> I'm reluctant to start writing documentation saying feature X does or
> does not work on Windows and why though as I don't have access to a
> Windows machine on which I can test any of this.
How about saying in the manual that for the embedded newlines to work,
the inferior should be started via a Posix shell? This should be
enough to hint Windows users this will not work for them.
> I believe it's already possible today to inject newline characters via
> the Python API. Maybe the window-nat.c code should be checking for
> invalid characters and throw an error if the user tries to start an
> inferior.
If doing that produces some error message from the inferior or the
APIs we use to start it, that should be enbough.
> I agree that platform specific limitations should be documented. I'm
> willing to help however I can to get that done. But I'm a little
> uncomfortable trying to document something I can't test. I'll see if I
> can source a Windows laptop, maybe I can build/test in that setup.
Maybe someone else can test that, and we could then amend the
documentation as needed? IOW, I see no reason to delay installing
your patch series due to this.
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/16] Inferior argument (inc for remote targets) changes
2024-01-21 10:22 ` Eli Zaretskii
@ 2024-01-22 10:29 ` Andrew Burgess
0 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2024-01-22 10:29 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches, m.weghorn
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org, m.weghorn@posteo.de
>> Date: Sat, 20 Jan 2024 22:46:02 +0000
>>
>> The question makes perfect sense.
>>
>> My hope is that the situation on Windows is no better or worse than it
>> ever was.
>>
>> I'm reluctant to start writing documentation saying feature X does or
>> does not work on Windows and why though as I don't have access to a
>> Windows machine on which I can test any of this.
>
> How about saying in the manual that for the embedded newlines to work,
> the inferior should be started via a Posix shell? This should be
> enough to hint Windows users this will not work for them.
That makes sense. I other feedback to address, so I'll add some
suitable words in this area and draw your attention to it on V2.
Thanks,
Andrew
>
>> I believe it's already possible today to inject newline characters via
>> the Python API. Maybe the window-nat.c code should be checking for
>> invalid characters and throw an error if the user tries to start an
>> inferior.
>
> If doing that produces some error message from the inferior or the
> APIs we use to start it, that should be enbough.
>
>> I agree that platform specific limitations should be documented. I'm
>> willing to help however I can to get that done. But I'm a little
>> uncomfortable trying to document something I can't test. I'll see if I
>> can source a Windows laptop, maybe I can build/test in that setup.
>
> Maybe someone else can test that, and we could then amend the
> documentation as needed? IOW, I see no reason to delay installing
> your patch series due to this.
>
> Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/16] Inferior argument (inc for remote targets) changes
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (16 preceding siblings ...)
2024-01-09 16:58 ` [PATCH 00/16] Inferior argument (inc for remote targets) changes Eli Zaretskii
@ 2024-01-10 8:28 ` Michael Weghorn
2024-01-21 3:56 ` Keith Seitz
18 siblings, 0 replies; 37+ messages in thread
From: Michael Weghorn @ 2024-01-10 8:28 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
[-- Attachment #1.1: Type: text/plain, Size: 1011 bytes --]
Hi Andrew,
On 2024-01-09 15:26, Andrew Burgess wrote:
> This series relates to bug PR gdb/28392. For background, check out
> this series:
>
> https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de
>
> which is a previous attempt to address this bug.
>
> It might also be worth reading:
>
> https://inbox.sourceware.org/gdb-patches/2b98ca58e47638b4760d86bd6e1fa9a9a79fa2ad.1695817255.git.aburgess@redhat.com
>
> which is a previous series of mine which covers some of the work from
> the original patch series.
>
> This series does include many ideas taken from the original patch
> series (thanks for Michael (original series author) for his work). I
> think this iteration does include some additional fixes beyond the
> original series, also there are more tests and documentation changes
> in this version.
Thanks a lot for looking into that series and coming up with an improved
solution for the underlying problem!
Best,
Michael
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 00/16] Inferior argument (inc for remote targets) changes
2024-01-09 14:26 [PATCH 00/16] Inferior argument (inc for remote targets) changes Andrew Burgess
` (17 preceding siblings ...)
2024-01-10 8:28 ` Michael Weghorn
@ 2024-01-21 3:56 ` Keith Seitz
18 siblings, 0 replies; 37+ messages in thread
From: Keith Seitz @ 2024-01-21 3:56 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 1/9/24 06:26, Andrew Burgess wrote:
> This series relates to bug PR gdb/28392. For background, check out
> this series:
>
> https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de
Having run through this code in years gone by, this series is long
overdue, so thank you for that.
> Still, there's plenty here to comment on, and I figure between now and
> April I can address any feedback that's given.
Re: libiberty, I am sure you'll make something work. :-)
FWIW, I've regression tested the series on our internal testing harness
and found no regressions on Fedora 39 (x86_64, aarch64, ppc64le, s390x).
I only have comments for a handful of the patches, and most of those
comments are pretty trivial. Feel free to add my Reviewed-by.
Keith
> Andrew Burgess (14):
> libiberty/buildargv: POSIX behaviour for backslash handling
> gdb/testsuite: add some xfail in gdb.base/startup-with-shell.exp
> gdb: remove the !startup_with_shell path from
> construct_inferior_arguments
> gdbserver: convert program_args to a single string
> gdbsupport: have construct_inferior_arguments take an escape function
> gdbsupport: split escape_shell_characters in two
> gdb: move remote arg splitting and joining into gdbsupport/
> gdb/python: change escaping rules when setting arguments
> gdb: add remote argument passing self tests
> gdb/gdbserver: pass inferior arguments as a single string
> gdb: allow 'set args' and run commands to contain newlines
> gdb/gdbserver: remove some uses of free_vector_argv
> gdb: new maintenance command to help debug remote argument issues
> gdb/gdbserver: rework argument splitting and joining
>
> Michael Weghorn (2):
> gdb: Support some escaping of args with startup-with-shell being off
> gdb/gdbserver: add a '--no-escape-args' command line option
>
> gdb/Makefile.in | 1 +
> gdb/NEWS | 41 +++
> gdb/doc/gdb.texinfo | 191 ++++++++++++-
> gdb/doc/python.texi | 7 +-
> gdb/infcmd.c | 128 ++++++++-
> gdb/inferior.c | 8 -
> gdb/inferior.h | 7 +-
> gdb/main.c | 30 +-
> gdb/nat/fork-inferior.c | 84 ++----
> gdb/python/py-inferior.c | 7 +-
> gdb/remote.c | 94 ++++++-
> gdb/testsuite/gdb.base/args.exp | 137 ++++++---
> gdb/testsuite/gdb.base/inferior-args.exp | 215 +++++++++++++--
> .../gdb.base/maint-test-remote-args.exp | 40 +++
> gdb/testsuite/gdb.base/startup-with-shell.exp | 143 ++++++++--
> gdb/testsuite/gdb.python/py-inferior.exp | 36 ++-
> gdb/testsuite/gdb.server/inferior-args.c | 27 ++
> gdb/testsuite/gdb.server/inferior-args.exp | 157 +++++++++++
> gdb/unittests/remote-arg-selftests.c | 172 ++++++++++++
> gdbserver/linux-low.cc | 5 +-
> gdbserver/linux-low.h | 2 +-
> gdbserver/netbsd-low.cc | 6 +-
> gdbserver/netbsd-low.h | 2 +-
> gdbserver/server.cc | 74 +++--
> gdbserver/server.h | 5 +
> gdbserver/target.h | 6 +-
> gdbserver/win32-low.cc | 7 +-
> gdbserver/win32-low.h | 2 +-
> gdbsupport/Makefile.am | 1 +
> gdbsupport/Makefile.in | 13 +-
> gdbsupport/common-inferior.cc | 207 +++++++++-----
> gdbsupport/common-inferior.h | 29 +-
> gdbsupport/remote-args.cc | 260 ++++++++++++++++++
> gdbsupport/remote-args.h | 44 +++
> libiberty/argv.c | 8 +-
> libiberty/testsuite/test-expandargv.c | 34 +++
> 36 files changed, 1927 insertions(+), 303 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/maint-test-remote-args.exp
> create mode 100644 gdb/testsuite/gdb.server/inferior-args.c
> create mode 100644 gdb/testsuite/gdb.server/inferior-args.exp
> create mode 100644 gdb/unittests/remote-arg-selftests.c
> create mode 100644 gdbsupport/remote-args.cc
> create mode 100644 gdbsupport/remote-args.h
>
>
> base-commit: b7a5722ebdd24a0d15d56e96d30a649ea1d7b0ee
^ permalink raw reply [flat|nested] 37+ messages in thread