[PATCH 0/3] Small cleanups in gdb/record*.c code

[PATCH 0/3] Small cleanups in gdb/record*.c code

[Andrew Burgess]

Series of trivial cleanups in the gdb/record*.c code.  The first two
should have no user visible effects.

The third does add a new error case, so I guess a user might hit that,
this is all explained in the commit message.

---

Andrew Burgess (3):
  gdb/record: remove unnecessary use of filename_completer
  gdb/record: add an assert in cmd_record_start
  gdb/record: minor clean, remove some unneeded arguments

 gdb/NEWS                                     |  4 ++++
 gdb/record-full.c                            | 15 +++++++-----
 gdb/record.c                                 |  9 +++++---
 gdb/testsuite/gdb.base/record-full-error.exp | 24 ++++++++++++++++++++
 4 files changed, 43 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/record-full-error.exp


base-commit: 032e5e0c0c08977e8109e8482cd944bac8572d92
-- 
2.25.4

[PATCH 1/3] gdb/record: remove unnecessary use of filename_completer

[Andrew Burgess]

Spotted that the 'record' command has its completer set to
filename_completer.  The problem is that 'record' is a prefix command,
as such, its completer is hard-coded to complete on sub-commands.  The
attempt to use filename_completer is irrelevant.

The 'record' command is itself a command though, that is, a user can
do this:

  (gdb) record

which is really just an alias for:

  (gdb) target record-full

Nowhere does cmd_record_start (which is called when 'record' is used)
expect a filename, and 'target record-full' doesn't expect a filename
either.

So lets just drop the line which sets filename_completer as the
completer for 'record'.
---
 gdb/record.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gdb/record.c b/gdb/record.c
index 1843969c2c9..25a4c1e71b6 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -791,8 +791,6 @@ A size of \"unlimited\" means unlimited lines.  The default is 10."),
     = add_prefix_cmd ("record", class_obscure, cmd_record_start,
 		      _("Start recording."),
 		      &record_cmdlist, 0, &cmdlist);
-  set_cmd_completer (record_cmd, filename_completer);
-
   add_com_alias ("rec", record_cmd, class_obscure, 1);
 
   set_show_commands setshow_record_cmds
-- 
2.25.4

[PATCH 2/3] gdb/record: add an assert in cmd_record_start

[Andrew Burgess]

The 'record' command is both a prefix command AND an alias for 'target
record-full'.  As it is a prefix command, if a user types:

  (gdb) record blah

Then GDB will look for, and try to invoke the 'blah' sub-command.
This will either succeed (if blah is found) or throw an error (if blah
is not found).

As such, the only way to invoke the 'record' command is like:

  (gdb) record

It is impossible to pass arguments to the 'record' command.  As we
know this is true then we can assert this in cmd_record_start.

I added this assert because initially I was going to try forwarding
ARGS from cmd_record_start to the 'target record-full' command, but
then I realised passing arguments to 'record' was impossible.

There should be no user visible changes after this commit.
---
 gdb/record.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/record.c b/gdb/record.c
index 25a4c1e71b6..5b1093dd12e 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -260,11 +260,16 @@ show_record_debug (struct ui_file *file, int from_tty,
 	      value);
 }
 
-/* Alias for "target record".  */
+/* Alias for "target record-full".  */
 
 static void
 cmd_record_start (const char *args, int from_tty)
 {
+  /* As 'record' is a prefix command then if the user types 'record blah'
+     GDB will search for the 'blah' sub-command and either run that instead
+     of calling this function, or throw an error if 'blah' doesn't exist.
+     As a result, we only get here if no args are given.  */
+  gdb_assert (args == nullptr);
   execute_command ("target record-full", from_tty);
 }
 
-- 
2.25.4

[PATCH 3/3] gdb/record: minor clean, remove some unneeded arguments

[Andrew Burgess]

I spotted that the two functions:

  record_full_open_1
  record_full_core_open_1

both took two arguments, neither of which are used.

I stumbled onto this while reviewing how filename_completer is used.
The 'record full restore' command uses filename_completer and invokes
the cmd_record_full_restore function.

The cmd_record_full_restore function calls core_file_command and then
record_full_open, which then calls one of the above functions.

As 'record full restore' takes a filename, this is passed to
cmd_record_full_restore, which forwards the filename to both
core_file_command and record_full_open.  However, record_full_open
never actually uses the filename that is passed in.

The record_full_open function is also used for 'target record-full'.

I propose that record_full_open should no longer expect to see any
user supplied arguments passed in (it doesn't use any).  In fact, I've
added a check that if we do get any user supplied arguments we'll
throw an error.

Now that we know record_full_open isn't being passed any user
arguments we can stop passing the arguments to record_full_open_1 and
record_full_core_open_1, this will make no user visible difference as
these arguments were not used.

It is possible that a user was previously doing:

  (gdb) target record-full blah blah blah

And this previously would work fine, the 'blah blah blah' was
ignored.  Now this will give an error.  Other than this case there
should be no user visible changes after this commit.
---
 gdb/NEWS                                     |  4 ++++
 gdb/record-full.c                            | 15 +++++++-----
 gdb/testsuite/gdb.base/record-full-error.exp | 24 ++++++++++++++++++++
 3 files changed, 37 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/record-full-error.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index feb3a37393a..942453c7064 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -36,6 +36,10 @@ set unwindonsignal on|off
 show unwindonsignal
   These commands are now aliases for the new set/show unwind-on-signal.
 
+target record-full
+  This command now gives an error if any unexpected arguments are
+  found after the command.
+
 * New commands
 
 info missing-debug-handler
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 4c3667f48ba..4ef85b0cdb6 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -910,7 +910,7 @@ record_full_async_inferior_event_handler (gdb_client_data data)
 /* Open the process record target for 'core' files.  */
 
 static void
-record_full_core_open_1 (const char *name, int from_tty)
+record_full_core_open_1 ()
 {
   regcache *regcache = get_thread_regcache (inferior_thread ());
   int regnum = gdbarch_num_regs (regcache->arch ());
@@ -933,7 +933,7 @@ record_full_core_open_1 (const char *name, int from_tty)
 /* Open the process record target for 'live' processes.  */
 
 static void
-record_full_open_1 (const char *name, int from_tty)
+record_full_open_1 ()
 {
   if (record_debug)
     gdb_printf (gdb_stdlog, "Process record: record_full_open_1\n");
@@ -957,11 +957,14 @@ static void record_full_init_record_breakpoints (void);
 /* Open the process record target.  */
 
 static void
-record_full_open (const char *name, int from_tty)
+record_full_open (const char *args, int from_tty)
 {
   if (record_debug)
     gdb_printf (gdb_stdlog, "Process record: record_full_open\n");
 
+  if (args != nullptr)
+    error (_("trailing junk: '%s'"), args);
+
   record_preopen ();
 
   /* Reset */
@@ -971,9 +974,9 @@ record_full_open (const char *name, int from_tty)
   record_full_list->next = NULL;
 
   if (current_program_space->core_bfd ())
-    record_full_core_open_1 (name, from_tty);
+    record_full_core_open_1 ();
   else
-    record_full_open_1 (name, from_tty);
+    record_full_open_1 ();
 
   /* Register extra event sources in the event loop.  */
   record_full_async_inferior_event_token
@@ -2520,7 +2523,7 @@ static void
 cmd_record_full_restore (const char *args, int from_tty)
 {
   core_file_command (args, from_tty);
-  record_full_open (args, from_tty);
+  record_full_open (nullptr, from_tty);
 }
 
 /* Save the execution log to a file.  We use a modified elf corefile
diff --git a/gdb/testsuite/gdb.base/record-full-error.exp b/gdb/testsuite/gdb.base/record-full-error.exp
new file mode 100644
index 00000000000..63ef03dcc0e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/record-full-error.exp
@@ -0,0 +1,24 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# 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/>.
+
+# Check an error when passing unexpected arguments to 'target
+# record-full'.
+
+gdb_start
+
+gdb_test "target record-full blah" \
+    "trailing junk: 'blah'"
-- 
2.25.4

Re: [PATCH 3/3] gdb/record: minor clean, remove some unneeded arguments

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Mon, 15 Apr 2024 15:19:28 +0100
> 
> I spotted that the two functions:
> 
>   record_full_open_1
>   record_full_core_open_1
> 
> both took two arguments, neither of which are used.
> 
> I stumbled onto this while reviewing how filename_completer is used.
> The 'record full restore' command uses filename_completer and invokes
> the cmd_record_full_restore function.
> 
> The cmd_record_full_restore function calls core_file_command and then
> record_full_open, which then calls one of the above functions.
> 
> As 'record full restore' takes a filename, this is passed to
> cmd_record_full_restore, which forwards the filename to both
> core_file_command and record_full_open.  However, record_full_open
> never actually uses the filename that is passed in.
> 
> The record_full_open function is also used for 'target record-full'.
> 
> I propose that record_full_open should no longer expect to see any
> user supplied arguments passed in (it doesn't use any).  In fact, I've
> added a check that if we do get any user supplied arguments we'll
> throw an error.
> 
> Now that we know record_full_open isn't being passed any user
> arguments we can stop passing the arguments to record_full_open_1 and
> record_full_core_open_1, this will make no user visible difference as
> these arguments were not used.
> 
> It is possible that a user was previously doing:
> 
>   (gdb) target record-full blah blah blah
> 
> And this previously would work fine, the 'blah blah blah' was
> ignored.  Now this will give an error.  Other than this case there
> should be no user visible changes after this commit.
> ---
>  gdb/NEWS                                     |  4 ++++
>  gdb/record-full.c                            | 15 +++++++-----
>  gdb/testsuite/gdb.base/record-full-error.exp | 24 ++++++++++++++++++++
>  3 files changed, 37 insertions(+), 6 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/record-full-error.exp

OK for the NEWS part, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 3/3] gdb/record: minor clean, remove some unneeded arguments

[Keith Seitz]

On 4/15/24 07:19, Andrew Burgess wrote:
> I spotted that the two functions:
> 
>    record_full_open_1
>    record_full_core_open_1
> 
> both took two arguments, neither of which are used.

Nice catch. I've looked over this series, and I just have one really
trivial -- if not silly -- comment.

> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 4c3667f48ba..4ef85b0cdb6 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -957,11 +957,14 @@ static void record_full_init_record_breakpoints (void);
>   /* Open the process record target.  */
>   
>   static void
> -record_full_open (const char *name, int from_tty)
> +record_full_open (const char *args, int from_tty)
>   {
>     if (record_debug)
>       gdb_printf (gdb_stdlog, "Process record: record_full_open\n");
>   
> +  if (args != nullptr)
> +    error (_("trailing junk: '%s'"), args);
> +
>     record_preopen ();
>   
>     /* Reset */

While I am no fan of this type of error, I understand the motive
in terms of consistency. Grep'ping for this string in gdb gives
numerous wordings of this type of error:

ada-lang.c:    error (_("Junk at end of expression"));
ada-lang.c:    error (_("Junk at end of arguments."));
break-catch-exec.c:    error (_("Junk at end of arguments."));
break-catch-fork.c:    error (_("Junk at end of arguments."));
break-catch-throw.c:    error (_("Junk at end of arguments."));
breakpoint.c:	    error (_("Junk at end of expression"));
breakpoint.c:		    error (_("Junk at end of expression"));
breakpoint.c:	    error (_("Junk after thread keyword."));
breakpoint.c:	    error (_("Junk after inferior keyword."));
breakpoint.c:	    error (_("Junk after task keyword."));
breakpoint.c:	error (_("Junk at end of arguments."));
breakpoint.c:		error (_("Junk after thread keyword."));
breakpoint.c:		error (_("Junk after task keyword."));
breakpoint.c:    error (_("Junk at end of command."));
breakpoint.c:    error (_("Junk at end of arguments."));
breakpoint.c:	error (_("Junk at end of arguments."));
btrace.c:    error (_("Junk after argument: %s."), arg);
linespec.c:    error (_("Junk at end of line specification: %s"), string);
linespec.c:    error (_("Junk at end of line specification: %s"), string);
parse.c:    error (_("Junk after end of expression."));
psymtab.c:	error (_("Junk at end of command"));
record-btrace.c:    error (_("Trailing junk: '%s'."), args);
record-btrace.c:    error (_("Trailing junk: '%s'."), args);
record-btrace.c:	error (_("Trailing junk: '%s'."), args + l2);
record-btrace.c:	error (_("Trailing junk: '%s'."), args + l1);
record-btrace.c:    error (_("Trailing junk: '%s'."), args);
record.c:    error (_("Junk after argument: %s."), arg);
record.c:    error (_("Junk after argument: %s."), arg);
record.c:    error (_("Junk after argument: %s."), arg);
symfile.c:	error (_("Junk after %s"), argv[1]);
symfile.c:	error (_("Junk after %s"), argv[0]);
symmisc.c:	error (_("Junk at end of command"));
symmisc.c:	error (_("Junk at end of command"));

So i guess as far as the record feature goes, the error message should
at least be "Trailing junk: %s". [Yeah, that is one heck of a silly nit,
isn't it?]

Otherwise, LGTM.

Keith

Re: [PATCH 0/3] Small cleanups in gdb/record*.c code

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Series of trivial cleanups in the gdb/record*.c code.  The first two
Andrew> should have no user visible effects.

Andrew> The third does add a new error case, so I guess a user might hit that,
Andrew> this is all explained in the commit message.

I read through these and they seem fine to me.
It would be good to address Keith's comments about the error message,
but aside from that --
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 0/3] Small cleanups in gdb/record*.c code

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Series of trivial cleanups in the gdb/record*.c code.  The first two
> Andrew> should have no user visible effects.
>
> Andrew> The third does add a new error case, so I guess a user might hit that,
> Andrew> this is all explained in the commit message.
>
> I read through these and they seem fine to me.
> It would be good to address Keith's comments about the error message,
> but aside from that --
> Approved-By: Tom Tromey <tom@tromey.com>

I addresses Keith's comment, and pushed this series.

Thanks,
Andrew