From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8472 invoked by alias); 3 Oct 2008 07:04:33 -0000 Received: (qmail 8381 invoked by uid 22791); 3 Oct 2008 07:04:32 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 03 Oct 2008 07:03:57 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 26A242A96FD; Fri, 3 Oct 2008 03:03:55 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 070zqbJatCOx; Fri, 3 Oct 2008 03:03:55 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A821B2A95E4; Fri, 3 Oct 2008 03:03:54 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 6420BE7ACD; Fri, 3 Oct 2008 00:03:52 -0700 (PDT) Date: Fri, 03 Oct 2008 07:04:00 -0000 From: Joel Brobecker To: Paul Pluzhnikov Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [patch] Re: GDB aborts on missing command args. Which way to fix? Message-ID: <20081003070352.GR3665@adacore.com> References: <8ac60eac0809161049t6bd917bbk8127317a7d8b42cb@mail.gmail.com> <20080919220128.GA27150@caradoc.them.org> <8ac60eac0809261621o73e8cadfyf225a20212bddbb4@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8ac60eac0809261621o73e8cadfyf225a20212bddbb4@mail.gmail.com> User-Agent: Mutt/1.4.2.2i Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-10/txt/msg00084.txt.bz2 > * 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