This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Re: GDB aborts on missing command args. Which way to fix?
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>
- Cc: Tom Tromey <tromey at redhat dot com>, gdb-patches at sourceware dot org
- Date: Fri, 3 Oct 2008 00:03:52 -0700
- Subject: Re: [patch] Re: GDB aborts on missing command args. Which way to fix?
- References: <8ac60eac0809161049t6bd917bbk8127317a7d8b42cb@mail.gmail.com> <m3wsh7956c.fsf@fleche.redhat.com> <20080919220128.GA27150@caradoc.them.org> <8ac60eac0809261621o73e8cadfyf225a20212bddbb4@mail.gmail.com>
> * remote-sim.c (gdbsim_kill, gdbsim_create_inferior,
> gdbsim_open): Likewise.
Minor detail. You need to close the parens on the first line, and
then reopen it on the next line. This is what the GNU Coding Standards
require. So:
* remote-sim.c (gdbsim_kill, gdbsim_create_inferior)
(gdbsim_open): Likewise.
You have other cases of the same issues in the changelog, so please
be sure to fix these as well.
> @@ -371,11 +371,11 @@ interpreter_exec_cmd (char *args, int fr
> unsigned int i;
> int old_quiet, use_quiet;
>
> - prules = buildargv (args);
> + prules = gdb_buildargv (args);
> if (prules == NULL)
> - {
> - error (_("unable to parse arguments"));
> - }
> + error (_("unable to parse arguments"));
> + else
> + make_cleanup_freeargv (prules);
I think it would be nice to be consistent in the way we are handling
the argument. In this case, it's a bit obscure at first sight what
it would mean for prules to be NULL. You have to know the semantics
of gdb_buildargv and determine from there that the only way it can
be null is if args is NULL as well. How about we change this to:
if (args == NULL)
error_no_arg (_("interpreter-exec command"));
prules = gdb_buildargv (args)
make_cleanup_freeargv (prules)
?
> - char **argv = buildargv (args);
> + char **argv = gdb_buildargv (args);
> char *prog;
>
> - if (argv == NULL)
> - nomem (0);
> + if (args == NULL)
> + error_no_arg (_("program to load"));
I guess I'm being a little anal about this, but it's strange to be using
args before you check whether it's null or not. I know gdb_buildargv
would return NULL is args is NULL, but can you move the call after
the check for args.
> @@ -5546,13 +5546,16 @@ extended_remote_run (char *args)
> error (_("Remote file name too long for run packet"));
> len += 2 * bin2hex ((gdb_byte *) remote_exec_file, rs->buf + len, 0);
>
> + if (args == NULL)
> + error_no_arg (_("remote arguments"));
This should never happen. There are a few indirections involved, but
essentially thiis function is called from "run_command" in infcmd,
and ARGS is pretty much guaranteed to be non NULL.
Two options: Add a "gdb_assert (args != NULL);" or make the function
handle the case where args is NULL the same as if args == "". For that,
changing the following if should work:
> if (*args)
if (args && *args).
> @@ -1929,11 +1926,7 @@ generic_load (char *args, int from_tty)
>
> make_cleanup (clear_memory_write_data, &cbdata.requests);
>
> - argv = buildargv (args);
> -
> - if (argv == NULL)
> - nomem(0);
> -
> + argv = gdb_buildargv (args);
> make_cleanup_freeargv (argv);
I think this one is missing the check for args first. The rest
of this function is accessing argv freely, so it's definitely
expecting argv to be non-null, which means args must be non-null.
> +
> +char **
> +gdb_buildargv (const char *s)
> +{
> + char **argv = buildargv (s);
> + if (s != NULL && argv == NULL)
> + nomem (0);
> + return argv;
> +}
Can you add a comment just before the function that explains what
the function does, exactly. I think it's fine to describe the function
compared to what libiberty's buildargv does. Something like this:
/* Call libiberty's buildargv, and return the result, except that
it calls nomem if buildargv ran out of memory. Therefore, the
returned value is guarantied to be non-NULL unless S is null. */
Your patch is pre-approved with the corrections mentioned above.
Nice cleanup!
--
Joel