Re: Libiberty's snprintf for v3?

Re: Libiberty's snprintf for v3?

[Kaveh R. Ghazi]

 > From: Paolo Carlini <pcarlini@unitus.it>
 > 
 > Hi Kaveh, hi everyone,
 > 
 > now that snprintf is available in libiberty we can consider actually 
 > using it in v3.
 > 
 > I briefly recall (for Kaveh and the accidental readers) that currently 
 > we use either snprintf or the unsafe sprintf depending on 
 > _GLIBCPP_USE_C99 (see config/locale/gnu/c_locale.h).
 > 
 > Having snprintf unconditionally available would simplify and made safer 
 > a few functions in include/bits/locale_facets.tcc
 > 
 > Do we really want that? In case, I volunteer to do the needed work 
 > (provided you guys help me a little bit ;).
 > Paolo.


Hi Paolo,

I'd like to see this happen, but there's a license snag.  I think I
mentioned this last time snprintf came up, but in case not:

While the new files I added for [v]snprintf use GPL plus special
exception, the implementation I wrote relies on libiberty's
vasprintf.c or a working system vasprintf.  I think vasprintf is more
rare than [v]snprintf since I believe vasprintf didn't make it into
c99.  So if we just suck [v]snprintf.c into libstdc++, it probably
won't be very useful since any system which needs it probably also
needs vasprintf.c.

The vasprintf.c file in libiberty uses LGPL which has different terms
than the special exception clause used in libstdc++ and [v]snprintf.c.

I see two remedies.

1.  Someone (not me, probably Paolo :-) ) needs to write to
    assign@gnu.org, explain the situation and ask them to change the
    license on vasprintf.c to GPL + special exception.  This isn't so
    bad, I've found them to be reasonable and responsive.  Then you
    can suck in that file too.

Or:

2.  Someone (probably me) needs to rewrite [v]snprintf.c to avoid
    using vasprintf.  E.g. we can use vfprintf to /dev/null to get the
    length of the resulting string.  I imagine something like this for
    vsnprintf.c:

    (Assume `s' is the buffer, `n' is the max length, and `msg' is the
    format and `ap' is the va_list.)

    len = vfprintf (dev_null_stream, msg, ap)
    if (len < n)
      vsprintf (s, msg, ap)
    else
      {
        buf = alloca (len+1)
	vsprintf (buf, msg, ap)
	memcpy (s, buf, n);
      }

    (It's probably more complicated in reality, but you get the idea.)
    

The downsides to this approach are:

A.  We assume /dev/null exists.

B.  We either open and close the stream every time (very slow) or
    cache the open /dev/null stream (close it with atexit) and waste
    one open file descriptor and one atexit slot for the entire run of
    any program which uses libiberty's [v]snprintf.c.

C.  I'm not sure if it's legal (portable) to traverse the va_list
    twice inside vsnprintf.c, once with vfprintf and once is vsprintf.

But, on the upside we won't need vasprintf.c, and the resulting code
will probably be faster than the current implementation if we cache
the fd.

If this doesn't work, perhaps someone can come up with another
approach?  Comments?

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Libiberty's snprintf for v3?

[DJ Delorie]

> But, on the upside we won't need vasprintf.c, and the resulting code
> will probably be faster than the current implementation if we cache
> the fd.

File I/O is very expensive on Microsoft platforms (DJGPP, Cygwin,
MinGW) compared to what you're used to under Unix platforms, even to
/dev/null (both djgpp and cygwin emulate /dev/null, I don't know about
mingw).

Re: Libiberty's snprintf for v3?

[Kaveh R. Ghazi]

[second try, this time with a Subject:]

> From: DJ Delorie <dj@redhat.com>
> 
> > But, on the upside we won't need vasprintf.c, and the resulting code
> > will probably be faster than the current implementation if we cache
> > the fd.
> 
> File I/O is very expensive on Microsoft platforms (DJGPP, Cygwin,
> MinGW) compared to what you're used to under Unix platforms, even to
> /dev/null (both djgpp and cygwin emulate /dev/null, I don't know about
> mingw).

Ok, but that's only a concern if these platforms are missing
[v]snprintf and therefore would rely on the libiberty copy.
E.g. I'd expect that cygwin has snprintf.  Don't know about the
others.

Can you please clarify the other platforms' situations?

--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Libiberty's snprintf for v3?

[Kaveh R. Ghazi]

 > From: DJ Delorie <dj@redhat.com>
 > 
 > > Ok, but that's only a concern if these platforms are missing
 > > [v]snprintf and therefore would rely on the libiberty copy.
 > > E.g. I'd expect that cygwin has snprintf.  Don't know about the
 > > others.
 > > 
 > > Can you please clarify the other platforms' situations?
 > 
 > [v]snprintf will be in djgpp 2.04, but isn't in the current 2.03.

Ok I'd be interested in concrete measurements.  For the sake of this
discussion, I've written a new vsnprintf.c which uses the strategy I
outlined previously.  (It's mixed with a patch to remove system header
includes, but ignore that for the moment.)

I'd be interested to know how fast or slow this new vsnprintf.c is
vs. the current one in libiberty on djgpp 2.03.

I tested the speed by wrapping the -DTEST main program with a large
loop and defining CLEAR(X) and VERIFY(X) to be empty so that it
isolates the calls to checkit which simply wraps vsnprintf.

On unix (sparc-solaris2.7) it's either 10% or 20% faster depending on
whether it has to call alloca for extra space, as in when the
resulting string would be longer than `n'.

		--Kaveh

diff -rcp orig/egcc-CVS20030421/libiberty/vsnprintf.c egcc-CVS20030421/libiberty/vsnprintf.c
*** orig/egcc-CVS20030421/libiberty/vsnprintf.c	Tue Apr 22 12:11:27 2003
--- egcc-CVS20030421/libiberty/vsnprintf.c	Tue Apr 22 16:35:27 2003
*************** system version of this function is used.
*** 39,62 ****
  
  */
  
- #include "config.h"
  #include "ansidecl.h"
  
  #ifdef ANSI_PROTOTYPES
  #include <stdarg.h>
  #else
  #include <varargs.h>
  #endif
! #ifdef HAVE_STRING_H
! #include <string.h>
! #endif
! #ifdef HAVE_STDLIB_H
! #include <stdlib.h>
  #endif
  
! #include "libiberty.h"
  
- /* This implementation relies on a working vasprintf.  */
  int
  vsnprintf (s, n, format, ap)
       char * s;
--- 39,79 ----
  
  */
  
  #include "ansidecl.h"
  
  #ifdef ANSI_PROTOTYPES
  #include <stdarg.h>
+ #include <stddef.h>
  #else
  #include <varargs.h>
+ #define size_t unsigned long
  #endif
! 
! PTR memcpy PARAMS ((PTR, const PTR, size_t));
! int atexit PARAMS ((void (*)(void)));
! 
! #ifndef va_copy
! # ifdef __va_copy
! #   define va_copy(d,s)  __va_copy((d),(s))
! # else
! #   define va_copy(d,s)  ((d) = (s))
! # endif
  #endif
  
! #include <stdio.h>
! 
! static FILE *nullstream = 0;
! 
! #if defined(HAVE_ATEXIT) || defined (HAVE_ON_EXIT)
! static void close_nullstream PARAMS ((void));
! static void
! close_nullstream()
! {
!   if (nullstream)
!     fclose (nullstream);
! }
! #endif
  
  int
  vsnprintf (s, n, format, ap)
       char * s;
*************** vsnprintf (s, n, format, ap)
*** 64,93 ****
       const char *format;
       va_list ap;
  {
!   char *buf = 0;
!   int result = vasprintf (&buf, format, ap);
  
!   if (!buf)
!     return -1;
!   if (result < 0)
      {
!       free (buf);
!       return -1;
      }
  
!   result = strlen (buf);
    if (n > 0)
      {
!       if ((long) n > result)
! 	memcpy (s, buf, result+1);
        else
          {
  	  memcpy (s, buf, n-1);
  	  s[n - 1] = 0;
  	}
      }
!   free (buf);
!   return result;
  }
  
  #ifdef TEST
--- 81,133 ----
       const char *format;
       va_list ap;
  {
!   int size;
!   va_list ap2;
  
!   /* Open a stream on /dev/null and arrange to close it at exit.  */
!   if (!nullstream)
      {
!       if ((nullstream = fopen ("/dev/null", "a")))
!         {
! 	  /* We don't want to rely on libiberty's atexit.c so that
!              this file can be used outside libiberty.  */
! #ifdef HAVE_ATEXIT
! 	  atexit (close_nullstream);
! #else
! # ifdef HAVE_ON_EXIT
! 	  on_exit ((void *)close_nullstream, 0);
! # endif
! #endif	
! 	}
!       else
! 	return -1;
      }
+   
+   /* Copy `ap' before using it.  */
+   va_copy (ap2, ap);
  
!   size = vfprintf (nullstream, format, ap);
!   if (size < 0)
!     return -1;
!   
    if (n > 0)
      {
!       /* If `size' fits in `n' then just print to the user supplied
!          buffer, otherwise print to a temporary space and copy `n'
!          bytes.  */
!       if ((long) n > size)
! 	vsprintf (s, format, ap2);
        else
          {
+ 	  char *const buf = alloca (size + 1);
+ 
+ 	  vsprintf (buf, format, ap2);
  	  memcpy (s, buf, n-1);
  	  s[n - 1] = 0;
  	}
      }
! 
!   return size;
  }
  
  #ifdef TEST
*************** vsnprintf (s, n, format, ap)
*** 96,101 ****
--- 136,145 ----
  /* For assertions.  */
  #define VERIFY(P) do { if (!(P)) abort(); } while (0)
  
+ void abort PARAMS ((void));
+ int memcmp PARAMS ((const PTR, const PTR, size_t));
+ PTR memset PARAMS ((PTR, int, size_t));
+ 
  static int ATTRIBUTE_PRINTF_3
  checkit VPARAMS ((char *s, size_t n, const char *format, ...))
  {

Re: Libiberty's snprintf for v3?

[Andreas Schwab]

"Kaveh R. Ghazi" <ghazi@caip.rutgers.edu> writes:

|> +   
|> +   /* Copy `ap' before using it.  */
|> +   va_copy (ap2, ap);

You need to call va_end on the va_alist copy before returning.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Libiberty's snprintf for v3?

[Kaveh R. Ghazi]

 > From: Andreas Schwab <schwab@suse.de>
 > 
 > "Kaveh R. Ghazi" <ghazi@caip.rutgers.edu> writes:
 > 
 > |> +   
 > |> +   /* Copy `ap' before using it.  */
 > |> +   va_copy (ap2, ap);
 > 
 > You need to call va_end on the va_alist copy before returning.
 > Andreas.

Oops.  Better?

diff -rcp orig/egcc-CVS20030425/libiberty/vsnprintf.c egcc-CVS20030425/libiberty/vsnprintf.c
*** orig/egcc-CVS20030425/libiberty/vsnprintf.c	Tue Apr 22 15:58:01 2003
--- egcc-CVS20030425/libiberty/vsnprintf.c	Sat Apr 26 10:21:07 2003
*************** system version of this function is used.
*** 39,62 ****
  
  */
  
- #include "config.h"
  #include "ansidecl.h"
  
  #ifdef ANSI_PROTOTYPES
  #include <stdarg.h>
  #else
  #include <varargs.h>
  #endif
! #ifdef HAVE_STRING_H
! #include <string.h>
! #endif
! #ifdef HAVE_STDLIB_H
! #include <stdlib.h>
! #endif
  
  #include "libiberty.h"
  
! /* This implementation relies on a working vasprintf.  */
  int
  vsnprintf (s, n, format, ap)
       char * s;
--- 39,80 ----
  
  */
  
  #include "ansidecl.h"
  
  #ifdef ANSI_PROTOTYPES
  #include <stdarg.h>
+ #include <stddef.h>
  #else
  #include <varargs.h>
+ #define size_t unsigned long
  #endif
! #include <stdio.h>
  
  #include "libiberty.h"
  
! PTR memcpy PARAMS ((PTR, const PTR, size_t));
! int atexit PARAMS ((void (*)(void)));
! 
! #ifndef va_copy
! # ifdef __va_copy
! #   define va_copy(d,s)  __va_copy((d),(s))
! # else
! #   define va_copy(d,s)  ((d) = (s))
! # endif
! #endif
! 
! static FILE *nullstream = 0;
! 
! #if defined(HAVE_ATEXIT) || defined (HAVE_ON_EXIT)
! static void close_nullstream PARAMS ((void));
! static void
! close_nullstream()
! {
!   if (nullstream)
!     fclose (nullstream);
! }
! #endif
! 
  int
  vsnprintf (s, n, format, ap)
       char * s;
*************** vsnprintf (s, n, format, ap)
*** 64,93 ****
       const char *format;
       va_list ap;
  {
!   char *buf = 0;
!   int result = vasprintf (&buf, format, ap);
  
!   if (!buf)
!     return -1;
!   if (result < 0)
      {
!       free (buf);
!       return -1;
      }
! 
!   result = strlen (buf);
    if (n > 0)
      {
!       if ((long) n > result)
! 	memcpy (s, buf, result+1);
        else
          {
  	  memcpy (s, buf, n-1);
  	  s[n - 1] = 0;
  	}
      }
!   free (buf);
!   return result;
  }
  
  #ifdef TEST
--- 82,136 ----
       const char *format;
       va_list ap;
  {
!   int size;
  
!   /* Open a stream on /dev/null and arrange to close it at exit.  */
!   if (!nullstream)
      {
!       if ((nullstream = fopen ("/dev/null", "a")))
!         {
! 	  /* We don't want to rely on libiberty's atexit.c so that
!              this file can be used outside libiberty.  */
! #ifdef HAVE_ATEXIT
! 	  atexit (close_nullstream);
! #else
! # ifdef HAVE_ON_EXIT
! 	  on_exit ((void *)close_nullstream, 0);
! # endif
! #endif	
! 	}
!       else
! 	return -1;
      }
!   
!   /* Copy `ap' since we use it twice, once here and once below.  */
!   {
!     va_list ap2;
!     va_copy (ap2, ap);
!     size = vfprintf (nullstream, format, ap2);
!     va_end (ap2);
!     if (size < 0)
!       return -1;
!   }
!   
    if (n > 0)
      {
!       /* If `size' fits in `n' then just print to the user supplied
!          buffer, otherwise print to a temporary space and copy `n'
!          bytes.  */
!       if ((long) n > size)
! 	vsprintf (s, format, ap);
        else
          {
+ 	  char *const buf = alloca (size + 1);
+ 
+ 	  vsprintf (buf, format, ap);
  	  memcpy (s, buf, n-1);
  	  s[n - 1] = 0;
  	}
      }
! 
!   return size;
  }
  
  #ifdef TEST
*************** vsnprintf (s, n, format, ap)
*** 96,101 ****
--- 139,148 ----
  /* For assertions.  */
  #define VERIFY(P) do { if (!(P)) abort(); } while (0)
  
+ void abort PARAMS ((void));
+ int memcmp PARAMS ((const PTR, const PTR, size_t));
+ PTR memset PARAMS ((PTR, int, size_t));
+ 
  static int ATTRIBUTE_PRINTF_3
  checkit VPARAMS ((char *s, size_t n, const char *format, ...))
  {

Re: Libiberty's snprintf for v3?

[Andreas Schwab]

"Kaveh R. Ghazi" <ghazi@caip.rutgers.edu> writes:

|> C.  I'm not sure if it's legal (portable) to traverse the va_list
|>     twice inside vsnprintf.c, once with vfprintf and once is vsprintf.

It is only legal when both traverses are either bracketed by
va_start/va_end, or you use va_copy to copy the va_alist object.  va_copy
is a C99 feature, if it isn't provided use __va_copy if defined, else a
simple assignment.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Libiberty's snprintf for v3?

[Kaveh R. Ghazi]

 > From: Andreas Schwab <schwab@suse.de>
 > 
 > "Kaveh R. Ghazi" <ghazi@caip.rutgers.edu> writes:
 > 
 > |> C.  I'm not sure if it's legal (portable) to traverse the va_list
 > |>     twice inside vsnprintf.c, once with vfprintf and once is vsprintf.
 > 
 > It is only legal when both traverses are either bracketed by
 > va_start/va_end, or you use va_copy to copy the va_alist object.  va_copy
 > is a C99 feature, if it isn't provided use __va_copy if defined, else a
 > simple assignment.
 > Andreas.

Thanks, I've snarfed the va_copy defaults from gcc/system.h.

--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Libiberty's snprintf for v3?

[Joe Buck]

On Mon, Apr 21, 2003 at 08:41:27PM -0400, Kaveh R. Ghazi wrote:
> I see two remedies.
> 
> 1.  Someone (not me, probably Paolo :-) ) needs to write to
>     assign@gnu.org, explain the situation and ask them to change the
>     license on vasprintf.c to GPL + special exception.  This isn't so
>     bad, I've found them to be reasonable and responsive.  Then you
>     can suck in that file too.

Better to ask RMS directly, cc-ing the SC list; RMS has been cooperative
lately on requests for license changes of this type, and he is the
decision maker.  As an SC member, you have more leverage than Paolo,
also it could be argued that by agreeing to be on the SC we've agreed
to fight such battles.  In any case, if Paolo asks assign@gnu.org, they'll
ask RMS, and then RMS will ask the SC list what it's about, so you're
just adding a few weeks to the process.

> 2.  Someone (probably me) needs to rewrite [v]snprintf.c to avoid
>     using vasprintf.

If we have perfectly good code, it seems that it's better to use the
relicensing approach, unless RMS rejects it.

Re: Libiberty's snprintf for v3?

[Kaveh R. Ghazi]

 > From: Joe Buck <jbuck@synopsys.com>
 > 
 > On Mon, Apr 21, 2003 at 08:41:27PM -0400, Kaveh R. Ghazi wrote:
 > > I see two remedies.
 > > 
 > > 1.  Someone (not me, probably Paolo :-) ) needs to write to
 > >     assign@gnu.org, explain the situation and ask them to change the
 > >     license on vasprintf.c to GPL + special exception.  This isn't so
 > >     bad, I've found them to be reasonable and responsive.  Then you
 > >     can suck in that file too.
 > 
 > Better to ask RMS directly, cc-ing the SC list; RMS has been cooperative
 > lately on requests for license changes of this type, and he is the
 > decision maker.  As an SC member, you have more leverage than Paolo,
 > also it could be argued that by agreeing to be on the SC we've agreed
 > to fight such battles.  In any case, if Paolo asks assign@gnu.org, they'll
 > ask RMS, and then RMS will ask the SC list what it's about, so you're
 > just adding a few weeks to the process.

Ok, fair enough.  Though I'd rather someone familiar with the code write
up the rationale and benefits.  E.g. would be be able to plug any
buffer overruns, etc?  If someone lights the torch, I'll carry it.

 > > 2.  Someone (probably me) needs to rewrite [v]snprintf.c to avoid
 > >     using vasprintf.
 > 
 > If we have perfectly good code, it seems that it's better to use the
 > relicensing approach, unless RMS rejects it.

Awww, it was so much more fun to write new code... :-)

--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu