PATCH: Do not call xmalloc_failed in expandargv

PATCH: Do not call xmalloc_failed in expandargv

[Mark Mitchell]

This patch fixes the link error when compiling GDB.  The cause of the
failure is that GDB's utils.c defines xmalloc, but not xmalloc_failed;
therefore, expandargv pulls in xmalloc.c to get xmalloc_failed, which
causes a duplicate symbol error for xmalloc.  Arguably, GDB should
define xmalloc_failed, so as to completely replace the interface.  (In
fact, GDB provides "nomem", which has almost the same signature.)
However, this patch to libiberty will be more robust if there are
other similar programs out there that, like GDB, replace only some of
the interface.  We could also split xmalloc_failed into its own file,
so as to decouple it from xmalloc.

In any case, is this patch OK, to get GDB back to linking?

--
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com

2005-09-27  Mark Mitchell  <mark@codesourcery.com>

	* argv.c (expandargv): Do not use xmalloc_failed.

Index: argv.c
===================================================================
RCS file: /cvs/src/src/libiberty/argv.c,v
retrieving revision 1.14
diff -c -5 -p -r1.14 argv.c
*** argv.c	26 Sep 2005 21:02:59 -0000	1.14
--- argv.c	27 Sep 2005 14:45:31 -0000
*************** expandargv (argcp, argvp)
*** 363,375 ****
        /* If *ARGVP is not already dynamically allocated, copy it.  */
        if (!argv_dynamic)
  	{
  	  *argvp = dupargv (*argvp);
  	  if (!*argvp)
! 	    /* We do not know exactly many bytes dupargv tried to
! 	       allocate, so make a guess.  */
! 	    xmalloc_failed (*argcp * 32);
  	}
        /* Count the number of arguments.  */
        file_argc = 0;
        while (file_argv[file_argc] && *file_argv[file_argc])
  	++file_argc;
--- 363,376 ----
        /* If *ARGVP is not already dynamically allocated, copy it.  */
        if (!argv_dynamic)
  	{
  	  *argvp = dupargv (*argvp);
  	  if (!*argvp)
! 	    {
! 	      fprintf (stderr, "\n%sout of memory\n");
! 	      xexit (1);
! 	    }
  	}
        /* Count the number of arguments.  */
        file_argc = 0;
        while (file_argv[file_argc] && *file_argv[file_argc])
  	++file_argc;

Re: PATCH: Do not call xmalloc_failed in expandargv

[Ian Lance Taylor]

Mark Mitchell <mark@codesourcery.com> writes:

> ! 	    {
> ! 	      fprintf (stderr, "\n%sout of memory\n");
> ! 	      xexit (1);
> ! 	    }

You need an argument to fprintf there, although I don't know just what
it would be.  Or just call fputs.

Either change is approved.

Ian

Re: PATCH: Do not call xmalloc_failed in expandargv

[Mark Mitchell]

Ian Lance Taylor wrote:
> Mark Mitchell <mark@codesourcery.com> writes:
> 
> 
>>! 	    {
>>! 	      fprintf (stderr, "\n%sout of memory\n");
>>! 	      xexit (1);
>>! 	    }
> 
> 
> You need an argument to fprintf there, although I don't know just what
> it would be.  Or just call fputs.

Doh.  I changed that to use fputs, and checked it in.

Thanks!

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304

Re: PATCH: Do not call xmalloc_failed in expandargv

[DJ Delorie]

>   	  if (!*argvp)
> ! 	    {
> ! 	      fprintf (stderr, "\n%sout of memory\n");
> ! 	      xexit (1);
> ! 	    }

I seem to recall a general policy (before my time) that libiberty
functions shouldn't ever exit on error; the proper response is to
return some error condition to the application.  Since expandargv()
doesn't have an error response, IMHO the right thing to do is treat
@foo as if it weren't a file and just return the original argv[n].
The application will hopefully discover the out of memory condition
itself and use its own handlers to deal with it.

Re: PATCH: Do not call xmalloc_failed in expandargv

[Mark Mitchell]

DJ Delorie wrote:

> I seem to recall a general policy (before my time) that libiberty
> functions shouldn't ever exit on error; the proper response is to
> return some error condition to the application.  Since expandargv()
> doesn't have an error response, IMHO the right thing to do is treat
> @foo as if it weren't a file and just return the original argv[n].
> The application will hopefully discover the out of memory condition
> itself and use its own handlers to deal with it.

xmalloc is in libiberty, and it calls xmalloc_failed when it fails,
which itself calls xexit.  That's the source of the idiom; I was just
trying to be consistent.

I can make the change that you suggest, if you think it's important, but
it seems more friendly to the user to just tell them early that there
was a problem; the application won't know if "@foo" remains in the
argument list because there was no such file "foo" or because it
happened to be too big.  In the latter case, the program should exit; in
the former, we've agreed that "@foo" should be treated as a literal
command-line argument.

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304

Re: PATCH: Do not call xmalloc_failed in expandargv

[DJ Delorie]

> xmalloc is in libiberty, and it calls xmalloc_failed when it fails,
> which itself calls xexit.  That's the source of the idiom; I was
> just trying to be consistent.

I don't feel strongly about it.  I bring it up only to cover all
possibilities.  I guess if you run out of memory in the first
allocation after main starts, you're screwed anyway.  Er... don't
expect fputs() to work if you're out of memory at that point; it won't
be able to allocate stdio buffers.  write(2,...) would be safer, if
less portable.

Hmmm... dupargv calls malloc(), not xmalloc().  expandargv calls
xmalloc(), but that's the only call to xmalloc in that file.  I wonder
if we're looking at the wrong idiom for this file?

I also know that bfd itself has a rule that errors can't call exit;
they must return error codes to the caller.  This doesn't affect
expandargv because bfd itself has no applications, but it's food for
thought.

Re: PATCH: Do not call xmalloc_failed in expandargv

[Mark Mitchell]

DJ Delorie wrote:
>>xmalloc is in libiberty, and it calls xmalloc_failed when it fails,
>>which itself calls xexit.  That's the source of the idiom; I was
>>just trying to be consistent.
> 
> 
> I don't feel strongly about it.  I bring it up only to cover all
> possibilities.  I guess if you run out of memory in the first
> allocation after main starts, you're screwed anyway.  Er... don't
> expect fputs() to work if you're out of memory at that point; it won't
> be able to allocate stdio buffers.  write(2,...) would be safer, if
> less portable.

xmalloc_failed uses fprintf, which is surely at least as likely as
fprintf to allocate memory.  And, write(2, ...) is also considered
unsafe; we had this argument in the context of libstdc++ a while back.
(Some applications close file descriptor 2, and then open some other
file, which happens to be assigned descriptor 2.)  You can avoid this if
you mandate that all applications using libiberty make sure that file
descriptor 2 is stderr.

Anyhow, it seems weird to change expandargv if we don't change
xmalloc_failed.

> Hmmm... dupargv calls malloc(), not xmalloc().  expandargv calls
> xmalloc(), but that's the only call to xmalloc in that file.  I wonder
> if we're looking at the wrong idiom for this file?

In my original patch, expandargv returned an error code, or, actually,
the name of the file it couldn't read.  IIRC, you asked me to remove
that, so that callers would require as few changes as possible.  In
practice, we're typically going to call expandargv() exactly once, at
the top of main.  Bombing out fatally doesn't seem bad to me in this
case; applications aren't going to be very likely to recover.

It seems like we're worrying about a very low-probability situation.  If
we find an application that really wants to recover from "@foo" being
too big, we can always change expandargv to ignore the "@foo" option as
you suggested, or to return -1, or some such.

Of course, if you do want me to make this change, I will.

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304

Re: PATCH: Do not call xmalloc_failed in expandargv

[DJ Delorie]

Nah, leave it as is.  Not worth worrying about.