Re: [patch] Re: GDB aborts on missing command args. Which way to fix?

[Joel Brobecker <brobecker at adacore dot 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