| Message ID | m31sgdwowr.fsf@oc1027705133.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 2018-03-21 09:15 AM, Andreas Arnez wrote: > Currently "info proc cmdline" on GNU/Linux does not show the full command > line, but only argument 0. And even a warning is shown if there are more. > This was discussed in 2014 already: > > https://sourceware.org/ml/gdb-patches/2014-04/msg00212.html > > Follow the advice there and avoid target_fileio_read_stralloc. Instead, > use target_fileio_read_alloc to read the whole command line and then > replace NUL characters by spaces. Also add an appropriate test case. > Note that gdbserver already handles this correctly. Hi Andreas, This LGTM with two minor nits: > gdb/ChangeLog: > > * linux-tdep.c (linux_info_proc): For "info proc cmdline", print > command line args instead of emitting a warning. > > gdb/testsuite/ChangeLog: > > * gdb.base/info-proc.exp: Add test for "info proc cmdline". > --- > gdb/linux-tdep.c | 20 ++++++++++++++++---- > gdb/testsuite/gdb.base/info-proc.exp | 13 +++++++++++++ > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c > index b4b87dd..0ac78c2 100644 > --- a/gdb/linux-tdep.c > +++ b/gdb/linux-tdep.c > @@ -754,10 +754,22 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args, > if (cmdline_f) > { > xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid); > - gdb::unique_xmalloc_ptr<char> cmdline > - = target_fileio_read_stralloc (NULL, filename); > - if (cmdline) > - printf_filtered ("cmdline = '%s'\n", cmdline.get ()); > + gdb_byte *buffer; > + ssize_t len = target_fileio_read_alloc (NULL, filename, &buffer); > + > + if (len > 0) > + { > + gdb::unique_xmalloc_ptr<char> cmdline ((char *) buffer); > + ssize_t pos; > + > + for (pos = 0; pos < len - 1; pos++) > + { > + if (buffer[pos] == '\0') > + buffer[pos] = ' '; > + } > + buffer[len - 1] = '\0'; > + printf_filtered ("cmdline = '%s'\n", buffer); > + } > else > warning (_("unable to open /proc file '%s'"), filename); > } > diff --git a/gdb/testsuite/gdb.base/info-proc.exp b/gdb/testsuite/gdb.base/info-proc.exp > index 72355bf..eadcb15 100644 > --- a/gdb/testsuite/gdb.base/info-proc.exp > +++ b/gdb/testsuite/gdb.base/info-proc.exp > @@ -38,6 +38,16 @@ gdb_test_multiple "info proc" "info proc without a process" { > } > } > > +# Set command line arguments to be verified later with "info proc > +# cmdline". However, if we're using a stub, then "set args" would not > +# have any effect, so then just skip this. > + > +set cmdline "" > +if { ! [target_info exists use_gdb_stub] } { The use_gdb_stub proc from lib/gdb.exp should be used instead (its comment explains why). > + set cmdline "-i foo bar -o baz 1234" > + gdb_test_no_output "set args $cmdline" "set args" > +} > + > if { ! [ runto_main ] } then { > untested "could not run to main" > return -1 > @@ -50,6 +60,9 @@ gdb_test "info proc mapping" \ > "info proc mapping" > > if {[istarget "*-*-linux*"]} { > + if { $cmdline != "" } { > + gdb_test "info proc cmdline" "cmdline = \'.* $cmdline\'" The backslashes are unnecessary. Simon
On Wed, Mar 21 2018, Simon Marchi wrote: > On 2018-03-21 09:15 AM, Andreas Arnez wrote: >> Currently "info proc cmdline" on GNU/Linux does not show the full command >> line, but only argument 0. And even a warning is shown if there are more. >> This was discussed in 2014 already: >> >> https://sourceware.org/ml/gdb-patches/2014-04/msg00212.html >> >> Follow the advice there and avoid target_fileio_read_stralloc. Instead, >> use target_fileio_read_alloc to read the whole command line and then >> replace NUL characters by spaces. Also add an appropriate test case. >> Note that gdbserver already handles this correctly. > > Hi Andreas, > > This LGTM with two minor nits: > [...] >> >> +# Set command line arguments to be verified later with "info proc >> +# cmdline". However, if we're using a stub, then "set args" would not >> +# have any effect, so then just skip this. >> + >> +set cmdline "" >> +if { ! [target_info exists use_gdb_stub] } { > > The use_gdb_stub proc from lib/gdb.exp should be used instead (its comment > explains why). Ah, OK. There are still some occurrences of "target_info exists use_gdb_stub" in the test suite. Should these be replaced as well? > >> + set cmdline "-i foo bar -o baz 1234" >> + gdb_test_no_output "set args $cmdline" "set args" >> +} >> + >> if { ! [ runto_main ] } then { >> untested "could not run to main" >> return -1 >> @@ -50,6 +60,9 @@ gdb_test "info proc mapping" \ >> "info proc mapping" >> >> if {[istarget "*-*-linux*"]} { >> + if { $cmdline != "" } { >> + gdb_test "info proc cmdline" "cmdline = \'.* $cmdline\'" > > The backslashes are unnecessary. Right. Thanks for your review. Pushed with these fixes. -- Andreas
On 2018-03-22 05:04, Andreas Arnez wrote: >>> >>> +# Set command line arguments to be verified later with "info proc >>> +# cmdline". However, if we're using a stub, then "set args" would >>> not >>> +# have any effect, so then just skip this. >>> + >>> +set cmdline "" >>> +if { ! [target_info exists use_gdb_stub] } { >> >> The use_gdb_stub proc from lib/gdb.exp should be used instead (its >> comment >> explains why). > > Ah, OK. There are still some occurrences of "target_info exists > use_gdb_stub" in the test suite. Should these be replaced as well? Yes, they probably should. >> >>> + set cmdline "-i foo bar -o baz 1234" >>> + gdb_test_no_output "set args $cmdline" "set args" >>> +} >>> + >>> if { ! [ runto_main ] } then { >>> untested "could not run to main" >>> return -1 >>> @@ -50,6 +60,9 @@ gdb_test "info proc mapping" \ >>> "info proc mapping" >>> >>> if {[istarget "*-*-linux*"]} { >>> + if { $cmdline != "" } { >>> + gdb_test "info proc cmdline" "cmdline = \'.* $cmdline\'" >> >> The backslashes are unnecessary. > > Right. > > Thanks for your review. Pushed with these fixes. Thanks! Simon
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index b4b87dd..0ac78c2 100644 --- a/gdb/linux-tdep.c +++ b/gdb/linux-tdep.c @@ -754,10 +754,22 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args, if (cmdline_f) { xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid); - gdb::unique_xmalloc_ptr<char> cmdline - = target_fileio_read_stralloc (NULL, filename); - if (cmdline) - printf_filtered ("cmdline = '%s'\n", cmdline.get ()); + gdb_byte *buffer; + ssize_t len = target_fileio_read_alloc (NULL, filename, &buffer); + + if (len > 0) + { + gdb::unique_xmalloc_ptr<char> cmdline ((char *) buffer); + ssize_t pos; + + for (pos = 0; pos < len - 1; pos++) + { + if (buffer[pos] == '\0') + buffer[pos] = ' '; + } + buffer[len - 1] = '\0'; + printf_filtered ("cmdline = '%s'\n", buffer); + } else warning (_("unable to open /proc file '%s'"), filename); } diff --git a/gdb/testsuite/gdb.base/info-proc.exp b/gdb/testsuite/gdb.base/info-proc.exp index 72355bf..eadcb15 100644 --- a/gdb/testsuite/gdb.base/info-proc.exp +++ b/gdb/testsuite/gdb.base/info-proc.exp @@ -38,6 +38,16 @@ gdb_test_multiple "info proc" "info proc without a process" { } } +# Set command line arguments to be verified later with "info proc +# cmdline". However, if we're using a stub, then "set args" would not +# have any effect, so then just skip this. + +set cmdline "" +if { ! [target_info exists use_gdb_stub] } { + set cmdline "-i foo bar -o baz 1234" + gdb_test_no_output "set args $cmdline" "set args" +} + if { ! [ runto_main ] } then { untested "could not run to main" return -1 @@ -50,6 +60,9 @@ gdb_test "info proc mapping" \ "info proc mapping" if {[istarget "*-*-linux*"]} { + if { $cmdline != "" } { + gdb_test "info proc cmdline" "cmdline = \'.* $cmdline\'" + } set gcorefile [standard_output_file $testfile.gcore] if {[gdb_gcore_cmd $gcorefile "save a core file"]} { clean_restart $binfile