[v2,31/31] Support an "unlimited" number of user-defined arguments
Commit Message
I recently wrote a user-defined command that could benefit from
supporting an unlimited number of arguments:
http://palves.net/list-active-signal-handlers-with-gdb/
E.g., 'info signal-dispositions 1 2 3 4 5 6 7 8 9 10 11'
However, we currently only support up to 10 arguments passed to
user-defined commands ($arg0..$arg9).
I can't find a good reason for that, other than "old code with hard
coded limits". This patch removes that limit and modernizes the code
along the way:
- Makes the user_args struct a real C++ class that uses std::vector
for storage.
- Removes the "next" pointer from within user_args and uses a
std::vector to maintain a stack instead.
- Adds a new RAII-based scoped_user_args_level class to help
push/pop user args in the stack instead of using a cleanup.
I also needed a way to convert a number to a std::string, so I added a
new utility for that, gdb::to_string. Yet another thing that can go
away with C++11.
Should probably write a test for this. Will do if people agree this
is useful.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* NEWS: Mention that user commands now accept an unlimited number
of arguments.
* cli/cli-script.c: Include <vector>.
(struct string_view): New type.
(MAXUSERARGS): Delete.
(struct user_args): Now a C++ class.
(user_args_stack): New.
(struct scoped_user_args_level): New type.
(execute_user_command): Use scoped_user_args_level. Adjust to
call insert_user_args instead of insert_args.
(arg_cleanup): Delete.
(setup_user_args): Deleted, and refactored as ...
(user_args::user_args): ... this new constructor. Limit of number
of arguments removed.
(insert_args): Delete.
(insert_user_args) New.
(user_args::insert_args): New, bits based on old insert_args with
limit of number of arguments eliminated.
* common/common-utils.h: Include <sstream>.
(gdb::to_string): New.
gdb/doc/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.texinfo (User-defined Commands): Limit on number of
arguments passed to user-defined commands removed; update.
---
gdb/doc/gdb.texinfo | 6 +-
gdb/NEWS | 3 +
gdb/cli/cli-script.c | 200 +++++++++++++++++++++++++---------------------
gdb/common/common-utils.h | 18 +++++
4 files changed, 135 insertions(+), 92 deletions(-)
Comments
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 19 Oct 2016 02:12:19 +0100
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * NEWS: Mention that user commands now accept an unlimited number
> of arguments.
> * cli/cli-script.c: Include <vector>.
> (struct string_view): New type.
> (MAXUSERARGS): Delete.
> (struct user_args): Now a C++ class.
> (user_args_stack): New.
> (struct scoped_user_args_level): New type.
> (execute_user_command): Use scoped_user_args_level. Adjust to
> call insert_user_args instead of insert_args.
> (arg_cleanup): Delete.
> (setup_user_args): Deleted, and refactored as ...
> (user_args::user_args): ... this new constructor. Limit of number
> of arguments removed.
> (insert_args): Delete.
> (insert_user_args) New.
> (user_args::insert_args): New, bits based on old insert_args with
> limit of number of arguments eliminated.
> * common/common-utils.h: Include <sstream>.
> (gdb::to_string): New.
>
> gdb/doc/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * gdb.texinfo (User-defined Commands): Limit on number of
> arguments passed to user-defined commands removed; update.
OK for the documentation parts.
Thanks.
Hi
On Wed, 19 Oct 2016 02:12:19 +0100
Pedro Alves <palves@redhat.com> wrote:
> I recently wrote a user-defined command that could benefit from
> supporting an unlimited number of arguments:
>
> http://palves.net/list-active-signal-handlers-with-gdb/
>
> E.g., 'info signal-dispositions 1 2 3 4 5 6 7 8 9 10 11'
>
> However, we currently only support up to 10 arguments passed to
> user-defined commands ($arg0..$arg9).
>
> I can't find a good reason for that, other than "old code with hard
> coded limits". This patch removes that limit and modernizes the code
> along the way:
>
> - Makes the user_args struct a real C++ class that uses std::vector
> for storage.
>
> - Removes the "next" pointer from within user_args and uses a
> std::vector to maintain a stack instead.
>
> - Adds a new RAII-based scoped_user_args_level class to help
> push/pop user args in the stack instead of using a cleanup.
>
> I also needed a way to convert a number to a std::string, so I added a
> new utility for that, gdb::to_string. Yet another thing that can go
> away with C++11.
[... snip ...]
> +/* Returns a string representation of VAL. Replacement for
> + std::to_string, which is only available in C++11 or later. */
> +
> +namespace gdb {
> +
> +template <class T>
> +inline std::string
> +to_string (const T &val)
> +{
> + std::stringstream ss;
> +
> + ss << val;
> + return ss.str ();
> +}
> +
> +}
> +
> /* Make a copy of the string at PTR with LEN characters
> (and add a null character at the end in the copy).
> Uses malloc to get the space. Returns the address of the copy. */
Is this really necessary?
As far as I understood the discussion, we jump directly to C++11. Thus there is no need for an homemade to_string.
Philipp
On 10/19/2016 12:33 PM, Philipp Rudo wrote:
> Pedro Alves <palves@redhat.com> wrote:
>> I also needed a way to convert a number to a std::string, so I added a
>> new utility for that, gdb::to_string. Yet another thing that can go
>> away with C++11.
> [... snip ...]
>> +/* Returns a string representation of VAL. Replacement for
>> + std::to_string, which is only available in C++11 or later. */
>> +
>> +namespace gdb {
>> +
>> +template <class T>
>> +inline std::string
>> +to_string (const T &val)
>> +{
>> + std::stringstream ss;
>> +
>> + ss << val;
>> + return ss.str ();
>> +}
>> +
>> +}
>> +
>> /* Make a copy of the string at PTR with LEN characters
>> (and add a null character at the end in the copy).
>> Uses malloc to get the space. Returns the address of the copy. */
>
> Is this really necessary?
> As far as I understood the discussion, we jump directly to C++11. Thus there is no need for an homemade to_string.
We haven't actually officially committed to requiring
C++11, so I didn't want to be blocked by that. It's
trivial to remove that bit latter if this lands first.
On C++11, I was just giving time for the follow ups to
last week's discussion to settle:
https://sourceware.org/ml/gdb-patches/2016-10/msg00497.html
https://sourceware.org/ml/gdb-patches/2016-10/msg00556.html
and give everyone a chance to comment (or re-comment after
consideration).
No one commented on the patches linked to from here either:
https://sourceware.org/ml/gdb-patches/2016-10/msg00497.html
Up until last week I wasn't even thinking that going C++11
would be possible. So I don't want to look like I'm rushing
it. But now it feels like I'm the one delaying .... :-P
Maybe I should just press ahead and be done with it.
Thanks,
Pedro Alves
On Wed, 19 Oct 2016 13:47:22 +0100
Pedro Alves <palves@redhat.com> wrote:
> On 10/19/2016 12:33 PM, Philipp Rudo wrote:
> > Pedro Alves <palves@redhat.com> wrote:
>
> >> I also needed a way to convert a number to a std::string, so I added a
> >> new utility for that, gdb::to_string. Yet another thing that can go
> >> away with C++11.
> > [... snip ...]
> >> +/* Returns a string representation of VAL. Replacement for
> >> + std::to_string, which is only available in C++11 or later. */
> >> +
> >> +namespace gdb {
> >> +
> >> +template <class T>
> >> +inline std::string
> >> +to_string (const T &val)
> >> +{
> >> + std::stringstream ss;
> >> +
> >> + ss << val;
> >> + return ss.str ();
> >> +}
> >> +
> >> +}
> >> +
> >> /* Make a copy of the string at PTR with LEN characters
> >> (and add a null character at the end in the copy).
> >> Uses malloc to get the space. Returns the address of the copy. */
> >
> > Is this really necessary?
> > As far as I understood the discussion, we jump directly to C++11. Thus there is no need for an homemade to_string.
>
> We haven't actually officially committed to requiring
> C++11, so I didn't want to be blocked by that. It's
> trivial to remove that bit latter if this lands first.
True that C++11 is not official yet. But the "trivial to remove" is
exactly the reason I wrote, as those pieces of code tend to stay in the longest.
And in 10+ years nobody knows why there is such a messy mix of different
functions doing the same thing and why it hasn't been done right in the
first place ;)
So my mail should not push you to use std::to_string but be seen as a reminder,
that if we require C++11 your home made function should be deleted better sooner
than later.
Phil
On 10/19/2016 06:39 PM, Philipp Rudo wrote:
> Pedro Alves <palves@redhat.com> wrote:
>>>> I also needed a way to convert a number to a std::string, so I added a
>>>> new utility for that, gdb::to_string. Yet another thing that can go
>>>> away with C++11.
>>> [... snip ...]
>>>> +/* Returns a string representation of VAL. Replacement for
>>>> + std::to_string, which is only available in C++11 or later. */
>>> Is this really necessary?
>>> As far as I understood the discussion, we jump directly to C++11. Thus there is no need for an homemade to_string.
>>
>> We haven't actually officially committed to requiring
>> C++11, so I didn't want to be blocked by that. It's
>> trivial to remove that bit latter if this lands first.
>
> True that C++11 is not official yet. But the "trivial to remove" is
> exactly the reason I wrote, as those pieces of code tend to stay in the longest.
> And in 10+ years nobody knows why there is such a messy mix of different
> functions doing the same thing and why it hasn't been done right in the
> first place ;)
Well, in this case the comment leaves no doubt. :-)
>
> So my mail should not push you to use std::to_string but be seen as a reminder,
> that if we require C++11 your home made function should be deleted better sooner
> than later.
Agreed. If this lands first, I'll make sure to remove this piece
after C++11.
Thanks,
Pedro Alves
On 10/19/2016 02:12 AM, Pedro Alves wrote:
> Should probably write a test for this. Will do if people agree this
> is useful.
I've pushed the whole series in, except the this patch.
I wrote a test already, but it depends on a fix for:
"eval" command inside user commands misses $arg0...$arg9 / $argc substitution
https://sourceware.org/bugzilla/show_bug.cgi?id=20559
So I'll resend it separately soon.
Thanks,
Pedro Alves
@@ -24057,9 +24057,9 @@ files.
@cindex arguments, to user-defined commands
A @dfn{user-defined command} is a sequence of @value{GDBN} commands to
which you assign a new name as a command. This is done with the
-@code{define} command. User commands may accept up to 10 arguments
+@code{define} command. User commands may accept an unlimited number of arguments
separated by whitespace. Arguments are accessed within the user command
-via @code{$arg0@dots{}$arg9}. A trivial example:
+via @code{$arg0@dots{}$argN}. A trivial example:
@smallexample
define adder
@@ -24083,7 +24083,7 @@ functions calls.
@cindex argument count in user-defined commands
@cindex how many arguments (user-defined commands)
In addition, @code{$argc} may be used to find out how many arguments have
-been passed. This expands to a number in the range 0@dots{}10.
+been passed.
@smallexample
define adder
@@ -17,6 +17,9 @@
* Support for Java programs compiled with gcj has been removed.
+* User commands now accept an unlimited number of arguments.
+ Previously, only up to 10 was accepted.
+
* New targets
Synopsys ARC arc*-*-elf32
@@ -33,6 +33,8 @@
#include "interps.h"
#include "compile/compile.h"
+#include <vector>
+
/* Prototypes for local functions. */
static enum command_control_type
@@ -41,9 +43,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
void (*validator)(char *, void *),
void *closure);
-static std::string insert_args (const char *line);
-
-static struct cleanup * setup_user_args (char *p);
+static std::string insert_user_args (const char *line);
static char *read_next_line (void);
@@ -56,24 +56,69 @@ static int command_nest_depth = 1;
/* This is to prevent certain commands being printed twice. */
static int suppress_next_print_command_trace = 0;
+/* A non-owning slice of a string. */
+
+struct string_view
+{
+ string_view (const char *str_, size_t len_)
+ : str (str_), len (len_)
+ {}
+
+ const char *str;
+ size_t len;
+};
+
/* Structure for arguments to user defined functions. */
-#define MAXUSERARGS 10
-struct user_args
+
+class user_args
+{
+public:
+ /* Save the command line and store the locations of arguments passed
+ to the user defined function. */
+ explicit user_args (const char *line);
+
+ /* Insert the stored user defined arguments into the $arg arguments
+ found in LINE. */
+ std::string insert_args (const char *line);
+
+private:
+ /* Disable copy/assignment. (Since the elements of A point inside
+ COMMAND, copying would need to reconstruct the A vector in the
+ new copy.) */
+ user_args (const user_args &);
+ user_args &operator= (const user_args &);
+
+ /* It is necessary to store a copy of the command line to ensure
+ that the arguments are not overwritten before they are used. */
+ std::string m_command_line;
+
+ /* The arguments. Each element points inside M_COMMAND_LINE. */
+ std::vector<string_view> m_args;
+};
+
+/* The stack of arguments passed to user defined functions. We need a
+ stack because user-defined functions can call other user-defined
+ functions. The vector owns its elements. */
+static std::vector<user_args *> user_args_stack;
+
+/* An RAII-base class used to push/pop args on the user args
+ stack. */
+struct scoped_user_args_level
+{
+ /* Parse the command line and push the arguments in the user args
+ stack. */
+ explicit scoped_user_args_level (const char *line)
{
- struct user_args *next;
- /* It is necessary to store a malloced copy of the command line to
- ensure that the arguments are not overwritten before they are
- used. */
- char *command;
- struct
- {
- char *arg;
- int len;
- }
- a[MAXUSERARGS];
- int count;
+ user_args_stack.push_back (new user_args (line));
+ }
+
+ /* Pop the current user arguments from the stack. */
+ ~scoped_user_args_level ()
+ {
+ delete user_args_stack.back ();
+ user_args_stack.pop_back ();
}
- *user_args;
+};
/* Return non-zero if TYPE is a multi-line command (i.e., is terminated
@@ -348,7 +393,6 @@ do_restore_user_call_depth (void * call_depth)
in_user_command = 0;
}
-
void
execute_user_command (struct cmd_list_element *c, char *args)
{
@@ -364,12 +408,12 @@ execute_user_command (struct cmd_list_element *c, char *args)
/* Null command */
return;
- old_chain = setup_user_args (args);
+ scoped_user_args_level push_user_args (args);
if (++user_call_depth > max_user_call_depth)
error (_("Max user call depth exceeded -- command aborted."));
- make_cleanup (do_restore_user_call_depth, &user_call_depth);
+ old_chain = make_cleanup (do_restore_user_call_depth, &user_call_depth);
/* Set the instream to 0, indicating execution of a
user-defined function. */
@@ -456,7 +500,7 @@ execute_control_command (struct command_line *cmd)
case simple_control:
{
/* A simple command, execute it and return. */
- std::string new_line = insert_args (cmd->line);
+ std::string new_line = insert_user_args (cmd->line);
execute_command (&new_line[0], 0);
ret = cmd->control_type;
break;
@@ -487,7 +531,7 @@ execute_control_command (struct command_line *cmd)
print_command_trace (buffer);
/* Parse the loop control expression for the while statement. */
- std::string new_line = insert_args (cmd->line);
+ std::string new_line = insert_user_args (cmd->line);
expression_up expr = parse_expression (new_line.c_str ());
ret = simple_control;
@@ -552,7 +596,7 @@ execute_control_command (struct command_line *cmd)
print_command_trace (buffer);
/* Parse the conditional for the if statement. */
- std::string new_line = insert_args (cmd->line);
+ std::string new_line = insert_user_args (cmd->line);
expression_up expr = parse_expression (new_line.c_str ());
current = NULL;
@@ -592,7 +636,7 @@ execute_control_command (struct command_line *cmd)
{
/* Breakpoint commands list, record the commands in the
breakpoint's command list and return. */
- std::string new_line = insert_args (cmd->line);
+ std::string new_line = insert_user_args (cmd->line);
ret = commands_from_control_command (new_line.c_str (), cmd);
break;
}
@@ -678,62 +722,32 @@ if_command (char *arg, int from_tty)
do_cleanups (old_chain);
}
-/* Cleanup */
-static void
-arg_cleanup (void *ignore)
-{
- struct user_args *oargs = user_args;
-
- if (!user_args)
- internal_error (__FILE__, __LINE__,
- _("arg_cleanup called with no user args.\n"));
-
- user_args = user_args->next;
- xfree (oargs->command);
- xfree (oargs);
-}
-
-/* Bind the incomming arguments for a user defined command to
- $arg0, $arg1 ... $argMAXUSERARGS. */
+/* Bind the incoming arguments for a user defined command to $arg0,
+ $arg1 ... $argN. */
-static struct cleanup *
-setup_user_args (char *p)
+user_args::user_args (const char *command_line)
{
- struct user_args *args;
- struct cleanup *old_chain;
- unsigned int arg_count = 0;
-
- args = XNEW (struct user_args);
- memset (args, 0, sizeof (struct user_args));
-
- args->next = user_args;
- user_args = args;
-
- old_chain = make_cleanup (arg_cleanup, 0/*ignored*/);
+ const char *p;
- if (p == NULL)
- return old_chain;
+ if (command_line == NULL)
+ return;
- user_args->command = p = xstrdup (p);
+ m_command_line = command_line;
+ p = m_command_line.c_str ();
while (*p)
{
- char *start_arg;
+ const char *start_arg;
int squote = 0;
int dquote = 0;
int bsquote = 0;
- if (arg_count >= MAXUSERARGS)
- error (_("user defined function may only have %d arguments."),
- MAXUSERARGS);
-
/* Strip whitespace. */
while (*p == ' ' || *p == '\t')
p++;
/* P now points to an argument. */
start_arg = p;
- user_args->a[arg_count].arg = p;
/* Get to the end of this argument. */
while (*p)
@@ -767,11 +781,8 @@ setup_user_args (char *p)
}
}
- user_args->a[arg_count].len = p - start_arg;
- arg_count++;
- user_args->count++;
+ m_args.push_back (string_view (start_arg, p - start_arg));
}
- return old_chain;
}
/* Given character string P, return a point to the first argument
@@ -790,48 +801,59 @@ locate_arg (const char *p)
return NULL;
}
-/* Insert the user defined arguments stored in user_arg into the $arg
- arguments found in line. */
+/* Insert the user defined arguments stored at top of the user args
+ stack into the $arg arguments found in line. */
static std::string
-insert_args (const char *line)
+insert_user_args (const char *line)
{
/* If we are not in a user-defined function, treat $argc, $arg0, et
cetera as normal convenience variables. */
- if (user_args == NULL)
+ if (user_args_stack.empty ())
return line;
+ user_args *args = user_args_stack.back ();
+ return args->insert_args (line);
+}
+
+/* Insert the user defined arguments stored in user_args into the $arg
+ arguments found in line. */
+
+std::string
+user_args::insert_args (const char *line)
+{
std::string new_line;
const char *p;
while ((p = locate_arg (line)))
{
- int i, len;
-
new_line.append (line, p - line);
if (p[4] == 'c')
{
- gdb_assert (user_args->count >= 0 && user_args->count <= 10);
- if (user_args->count == 10)
- {
- new_line += '1';
- new_line += '0';
- }
- else
- new_line += user_args->count + '0';
+ new_line += gdb::to_string (m_args.size ());
+ line = p + 5;
}
else
{
- i = p[4] - '0';
- if (i >= user_args->count)
- error (_("Missing argument %d in user function."), i);
+ char *tmp;
+ unsigned long i;
- len = user_args->a[i].len;
- if (len > 0)
- new_line.append (user_args->a[i].arg, len);
+ errno = 0;
+ i = strtoul (p + 4, &tmp, 10);
+ if ((i == 0 && tmp == p + 4) || errno != 0)
+ {
+ line = p + 4;
+ }
+ else if (i >= m_args.size ())
+ error (_("Missing argument %ld in user function."), i);
+ else
+ {
+ new_line.append (m_args[i].str, m_args[i].len);
+
+ line = tmp;
+ }
}
- line = p + 5;
}
/* Don't forget the tail. */
new_line.append (line);
@@ -21,6 +21,7 @@
#define COMMON_UTILS_H
#include <string>
+#include <sstream>
/* If possible, define FUNCTION_NAME, a macro containing the name of
the function being defined. Since this macro may not always be
@@ -62,6 +63,23 @@ int xsnprintf (char *str, size_t size, const char *format, ...)
std::string string_printf (const char* fmt, ...)
ATTRIBUTE_PRINTF (1, 2);
+/* Returns a string representation of VAL. Replacement for
+ std::to_string, which is only available in C++11 or later. */
+
+namespace gdb {
+
+template <class T>
+inline std::string
+to_string (const T &val)
+{
+ std::stringstream ss;
+
+ ss << val;
+ return ss.str ();
+}
+
+}
+
/* Make a copy of the string at PTR with LEN characters
(and add a null character at the end in the copy).
Uses malloc to get the space. Returns the address of the copy. */