This is the mail archive of the libc-alpha@sources.redhat.com mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Portability fix for argp/argp.h.


Ulrich Drepper <drepper@redhat.com> writes:

> I don't think half of this patch is needed.  USE_IN_LIBIO is defined
> only inside libc.  We need no #ifs for HAVE_STRERROR_R in that branch.

Ah, thanks.

>> +#if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME
>> +      program_invocation_short_name = state->name;
>> +#endif
...
>
> You cannot remove program_invocation_short_name for _LIBC.  This
> variable is part of the libc ABI.  After your change it seems not to be
> initialized anymore.  And in any case, since it is a separate variable
> it might get a value other than basename(program_invocation_name)
> assigned at any time.  You cannot mess with it.
>
> I.e., leave the code as is for libc.

That was an oversight, it should have had a '|| defined _LIBC' too.

In the revised the patch below, the code should amount to the same
thing in libc (it doesn't even use basename), and should also work on
platforms that have any, or both, of program_invocation_{short_,}name.

Since the patch is slightly hard to read, I'm including the source as
it was before and how it is after the patch in case you'd like to
verify it behave the same for libc.  I've verified that the function
further down doesn't rely on the value of 'arg', since the patch
modify it's contents.

Before:

    case OPT_PROGNAME:		/* Set the program name.  */
      program_invocation_name = arg;

      /* [Note that some systems only have PROGRAM_INVOCATION_SHORT_NAME (aka
	 __PROGNAME), in which case, PROGRAM_INVOCATION_NAME is just defined
	 to be that, so we have to be a bit careful here.]  */
      arg = strrchr (arg, '/');
      if (arg)
	program_invocation_short_name = arg + 1;
      else
	program_invocation_short_name = program_invocation_name;

      /* Update what we use for messages.  */
      state->name = program_invocation_short_name;

      if ((state->flags & (ARGP_PARSE_ARGV0 | ARGP_NO_ERRS))
	  == ARGP_PARSE_ARGV0)
	/* Update what getopt uses too.  */
	state->argv[0] = program_invocation_name;

      break;

After:

    case OPT_PROGNAME:		/* Set the program name.  */
#if defined _LIBC || HAVE_DECL_PROGRAM_INVOCATION_NAME
      program_invocation_name = arg;
#endif
      /* [Note that some systems only have PROGRAM_INVOCATION_SHORT_NAME (aka
	 __PROGNAME), in which case, PROGRAM_INVOCATION_NAME is just defined
	 to be that, so we have to be a bit careful here.]  */

      /* Update what we use for messages.  */
      state->name = strrchr (arg, '/');
      if (state->name)
	state->name++;
      else
	state->name = arg;

#if defined _LIBC || HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME
      program_invocation_short_name = state->name;
#endif

      if ((state->flags & (ARGP_PARSE_ARGV0 | ARGP_NO_ERRS))
	  == ARGP_PARSE_ARGV0)
	/* Update what getopt uses too.  */
	state->argv[0] = arg;

      break;

> If necessary, add wrappers for other uses although I still don't
> understand why it is needed.

Some platforms may have either/both program_invocation_{short_,}name,
and gnulib could be enhanced to contain a module that provide it even
on platforms that doesn't have them (this is discussed on the gnulib
list now).  So making the code check for this and use the variables
where available, even outside _LIBC, could be useful.  Does it make
sense now?  These are still unimplemented ideas, so there may be flaws
in them.

See <http://josefsson.org/autobuild/shishi.html> for a list of
platforms on which the code below compile on (the platforms that fail
does so for non-argp reasons, the new code is only in the shishi-0.0.3
section).

Some other minor tweaks discovered since the last message has been
added to the patch too.

Thanks.

2003-08-22  Simon Josefsson  <jas@extundo.com>

	* argp/argp-fmtstream.h [!__attribute__]: Define to nothing.

	* argp/argp-help.c: Don't include malloc.h, some platforms
	complain and it doesn't appear to be used.
	[!_LIBC && HAVE_STRERROR_R && !HAVE_DECL_STRERROR_R]: Declare
	strerror_r.
	[!_LIBC && !HAVE_STRERROR_R && !HAVE_DECL_STRERROR]: Declare
	strerror.
	(hol_entry_long_iterate): Change __attribute to __attribute__.
	(_help, __argp_error, __argp_failure) [!_LIBC && (HAVE_FLOCKFILE
	&& HAVE_FUNLOCKFILE)]: Protect call to flockfile and funlockfile.
	(__argp_basename) [!_LIBC]: New. Taken from LSH, by Niels Möller,
	modifed after comments from Ulrich Drepper.
	(__argp_short_program_name): Ditto.
	(__argp_state_help, __argp_error, __argp_failure): Use it.
	(__argp_failure): Use strerror when necessary.

	* argp/argp-namefrob.h (__flockfile, __funlockfile, __mempcpy)
	(__strchrnul, __strerror_r, __strndup) [!_LIBC]: Remove __-prefix.
	(clearerr_unlocked, feof_unlocked, ferror_unlocked)
	(fflush_unlocked, fgets_unlocked, fputc_unlocked, fputs_unlocked)
	(fread_unlocked, fwrite_unlocked, getc_unlocked, getchar_unlocked)
	(putc_unlocked, putchar_unlocked) [!_LIBC && !HAVE_DECL_*]: Map to
	non-unlocked functions.
	[!_LIBC]: Add prototypes for __argp_basename and
	__argp_short_program_name.

	* argp/argp-parse.c (argp_default_parser): Only use
	program_invocation{_short,}_name if declared.
	(parser_init): Use __argp_short_program_name.

	* argp/argp-xinl.c [_LIBC || HAVE_FEATURES_H]: Add CPP check for
	'#include features.h'.

	* argp/argp.h [!__attribute__]: Define to nothing.

Index: argp-fmtstream.h
===================================================================
RCS file: /cvs/glibc/libc/argp/argp-fmtstream.h,v
retrieving revision 1.4
diff -u -p -r1.4 argp-fmtstream.h
--- argp-fmtstream.h	6 Jul 2001 04:54:44 -0000	1.4
+++ argp-fmtstream.h	22 Aug 2003 22:07:06 -0000
@@ -34,6 +34,19 @@
 #include <string.h>
 #include <unistd.h>
 
+#ifndef __attribute__
+/* This feature is available in gcc versions 2.5 and later.  */
+# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5) || __STRICT_ANSI__
+#  define __attribute__(Spec) /* empty */
+# endif
+/* The __-protected variants of `format' and `printf' attributes
+   are accepted by gcc versions 2.6.4 (effectively 2.7) and later.  */
+# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 7) || __STRICT_ANSI__
+#  define __format__ format
+#  define __printf__ printf
+# endif
+#endif
+
 #if    (_LIBC - 0 && !defined (USE_IN_LIBIO)) \
     || (defined (__GNU_LIBRARY__) && defined (HAVE_LINEWRAP_H))
 /* line_wrap_stream is available, so use that.  */
Index: argp-help.c
===================================================================
RCS file: /cvs/glibc/libc/argp/argp-help.c,v
retrieving revision 1.37
diff -u -p -r1.37 argp-help.c
--- argp-help.c	13 Jun 2003 20:38:13 -0000	1.37
+++ argp-help.c	22 Aug 2003 22:07:06 -0000
@@ -50,7 +50,6 @@ char *alloca ();
 #include <string.h>
 #include <assert.h>
 #include <stdarg.h>
-#include <malloc.h>
 #include <ctype.h>
 #ifdef USE_IN_LIBIO
 # include <wchar.h>
@@ -70,6 +69,18 @@ char *alloca ();
 # endif
 #endif
 
+#ifndef _LIBC
+# if HAVE_STRERROR_R
+#  if !HAVE_DECL_STRERROR_R
+char *strerror_r (int errnum, char *buf, size_t buflen);
+#  endif
+# else
+#  if !HAVE_DECL_STRERROR
+char *strerror (int errnum);
+#  endif
+# endif
+#endif
+
 #include "argp.h"
 #include "argp-fmtstream.h"
 #include "argp-namefrob.h"
@@ -547,7 +558,7 @@ hol_entry_short_iterate (const struct ho
 }
 
 static inline int
-__attribute ((always_inline))
+__attribute__ ((always_inline))
 hol_entry_long_iterate (const struct hol_entry *entry,
 			int (*func)(const struct argp_option *opt,
 				    const struct argp_option *real,
@@ -1530,7 +1541,9 @@ _help (const struct argp *argp, const st
   if (! stream)
     return;
 
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
   __flockfile (stream);
+#endif
 
   if (! uparams.valid)
     fill_in_uparams (state);
@@ -1538,7 +1551,9 @@ _help (const struct argp *argp, const st
   fs = __argp_make_fmtstream (stream, 0, uparams.rmargin, 0);
   if (! fs)
     {
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
       __funlockfile (stream);
+#endif
       return;
     }
 
@@ -1646,7 +1661,9 @@ Try `%s --help' or `%s --usage' for more
       anything = 1;
     }
 
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
   __funlockfile (stream);
+#endif
 
   if (hol)
     hol_free (hol);
@@ -1665,6 +1682,32 @@ void __argp_help (const struct argp *arg
 weak_alias (__argp_help, argp_help)
 #endif
 
+#ifndef _LIBC
+char *__argp_basename (char *name)
+{
+  char *short_name = strrchr (name, '/');
+  return short_name ? short_name + 1 : name;
+}
+#endif
+
+char *
+__argp_short_program_name (void)
+{
+#if defined _LIBC || HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME
+  return program_invocation_short_name;
+#elif HAVE_DECL_PROGRAM_INVOCATION_NAME
+  return __argp_basename (program_invocation_name);
+#else
+  /* FIXME: What now? Miles suggests that it is better to use NULL,
+     but currently the value is passed on directly to fputs_unlocked,
+     so that requires more changes. */
+#if __GNUC__
+# warning No reasonable value to return
+#endif /* __GNUC__ */
+  return "";
+#endif
+}
+
 /* Output, if appropriate, a usage message for STATE to STREAM.  FLAGS are
    from the set ARGP_HELP_*.  */
 void
@@ -1676,7 +1719,7 @@ __argp_state_help (const struct argp_sta
 	flags |= ARGP_HELP_LONG_ONLY;
 
       _help (state ? state->root_argp : 0, state, stream, flags,
-	     state ? state->name : program_invocation_short_name);
+	     state ? state->name : __argp_short_program_name ());
 
       if (!state || ! (state->flags & ARGP_NO_EXIT))
 	{
@@ -1705,7 +1748,9 @@ __argp_error (const struct argp_state *s
 	{
 	  va_list ap;
 
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
 	  __flockfile (stream);
+#endif
 
 	  va_start (ap, fmt);
 
@@ -1717,7 +1762,7 @@ __argp_error (const struct argp_state *s
 	      __asprintf (&buf, fmt, ap);
 
 	      __fwprintf (stream, L"%s: %s\n",
-			  state ? state->name : program_invocation_short_name,
+			  state ? state->name : __argp_short_program_name (),
 			  buf);
 
 	      free (buf);
@@ -1726,7 +1771,7 @@ __argp_error (const struct argp_state *s
 #endif
 	    {
 	      fputs_unlocked (state
-			      ? state->name : program_invocation_short_name,
+			      ? state->name : __argp_short_program_name (),
 			      stream);
 	      putc_unlocked (':', stream);
 	      putc_unlocked (' ', stream);
@@ -1740,7 +1785,9 @@ __argp_error (const struct argp_state *s
 
 	  va_end (ap);
 
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
 	  __funlockfile (stream);
+#endif
 	}
     }
 }
@@ -1766,16 +1813,18 @@ __argp_failure (const struct argp_state 
 
       if (stream)
 	{
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
 	  __flockfile (stream);
+#endif
 
 #ifdef USE_IN_LIBIO
 	  if (_IO_fwide (stream, 0) > 0)
 	    __fwprintf (stream, L"%s",
-			state ? state->name : program_invocation_short_name);
+			state ? state->name : __argp_short_program_name ());
 	  else
 #endif
 	    fputs_unlocked (state
-			    ? state->name : program_invocation_short_name,
+			    ? state->name : __argp_short_program_name (),
 			    stream);
 
 	  if (fmt)
@@ -1819,7 +1868,11 @@ __argp_failure (const struct argp_state 
 		{
 		  putc_unlocked (':', stream);
 		  putc_unlocked (' ', stream);
+#if defined _LIBC || defined HAVE_STRERROR_R
 		  fputs (__strerror_r (errnum, buf, sizeof (buf)), stream);
+#else
+		  fputs (strerror (errnum), stream);
+#endif
 		}
 	    }
 
@@ -1830,7 +1883,9 @@ __argp_failure (const struct argp_state 
 #endif
 	    putc_unlocked ('\n', stream);
 
+#if _LIBC || (HAVE_FLOCKFILE && HAVE_FUNLOCKFILE)
 	  __funlockfile (stream);
+#endif
 
 	  if (status && (!state || !(state->flags & ARGP_NO_EXIT)))
 	    exit (status);
Index: argp-namefrob.h
===================================================================
RCS file: /cvs/glibc/libc/argp/argp-namefrob.h,v
retrieving revision 1.3
diff -u -p -r1.3 argp-namefrob.h
--- argp-namefrob.h	6 Jul 2001 04:54:44 -0000	1.3
+++ argp-namefrob.h	22 Aug 2003 22:07:06 -0000
@@ -78,12 +78,67 @@
 #define __argp_fmtstream_wmargin argp_fmtstream_wmargin
 
 /* normal libc functions we call */
+#undef __flockfile
+#define __flockfile flockfile
+#undef __funlockfile
+#define __funlockfile funlockfile
+#undef __mempcpy
+#define __mempcpy mempcpy
 #undef __sleep
 #define __sleep sleep
 #undef __strcasecmp
 #define __strcasecmp strcasecmp
+#undef __strchrnul
+#define __strchrnul strchrnul
+#undef __strerror_r
+#define __strerror_r strerror_r
+#undef __strndup
+#define __strndup strndup
 #undef __vsnprintf
 #define __vsnprintf vsnprintf
+
+#if defined(HAVE_DECL_CLEARERR_UNLOCKED) && !HAVE_DECL_CLEARERR_UNLOCKED
+# define clearerr_unlocked(x) clearerr (x)
+#endif
+#if defined(HAVE_DECL_FEOF_UNLOCKED) && !HAVE_DECL_FEOF_UNLOCKED
+# define feof_unlocked(x) feof (x)
+# endif
+#if defined(HAVE_DECL_FERROR_UNLOCKED) && !HAVE_DECL_FERROR_UNLOCKED
+# define ferror_unlocked(x) ferror (x)
+# endif
+#if defined(HAVE_DECL_FFLUSH_UNLOCKED) && !HAVE_DECL_FFLUSH_UNLOCKED
+# define fflush_unlocked(x) fflush (x)
+# endif
+#if defined(HAVE_DECL_FGETS_UNLOCKED) && !HAVE_DECL_FGETS_UNLOCKED
+# define fgets_unlocked(x,y,z) fgets (x,y,z)
+# endif
+#if defined(HAVE_DECL_FPUTC_UNLOCKED) && !HAVE_DECL_FPUTC_UNLOCKED
+# define fputc_unlocked(x,y) fputc (x,y)
+# endif
+#if defined(HAVE_DECL_FPUTS_UNLOCKED) && !HAVE_DECL_FPUTS_UNLOCKED
+# define fputs_unlocked(x,y) fputs (x,y)
+# endif
+#if defined(HAVE_DECL_FREAD_UNLOCKED) && !HAVE_DECL_FREAD_UNLOCKED
+# define fread_unlocked(w,x,y,z) fread (w,x,y,z)
+# endif
+#if defined(HAVE_DECL_FWRITE_UNLOCKED) && !HAVE_DECL_FWRITE_UNLOCKED
+# define fwrite_unlocked(w,x,y,z) fwrite (w,x,y,z)
+# endif
+#if defined(HAVE_DECL_GETC_UNLOCKED) && !HAVE_DECL_GETC_UNLOCKED
+# define getc_unlocked(x) getc (x)
+# endif
+#if defined(HAVE_DECL_GETCHAR_UNLOCKED) && !HAVE_DECL_GETCHAR_UNLOCKED
+#  define getchar_unlocked() getchar ()
+# endif
+#if defined(HAVE_DECL_PUTC_UNLOCKED) && !HAVE_DECL_PUTC_UNLOCKED
+# define putc_unlocked(x,y) putc (x,y)
+# endif
+#if defined(HAVE_DECL_PUTCHAR_UNLOCKED) && !HAVE_DECL_PUTCHAR_UNLOCKED
+# define putchar_unlocked(x) putchar (x)
+# endif
+
+extern char *__argp_basename (char *name);
+extern char *__argp_short_program_name (void);
 
 #endif /* !_LIBC */
 
Index: argp-parse.c
===================================================================
RCS file: /cvs/glibc/libc/argp/argp-parse.c,v
retrieving revision 1.17
diff -u -p -r1.17 argp-parse.c
--- argp-parse.c	8 Apr 2002 08:38:33 -0000	1.17
+++ argp-parse.c	22 Aug 2003 22:07:06 -0000
@@ -119,24 +119,28 @@ argp_default_parser (int key, char *arg,
       break;
 
     case OPT_PROGNAME:		/* Set the program name.  */
+#if defined _LIBC || HAVE_DECL_PROGRAM_INVOCATION_NAME
       program_invocation_name = arg;
-
+#endif
       /* [Note that some systems only have PROGRAM_INVOCATION_SHORT_NAME (aka
 	 __PROGNAME), in which case, PROGRAM_INVOCATION_NAME is just defined
 	 to be that, so we have to be a bit careful here.]  */
-      arg = strrchr (arg, '/');
-      if (arg)
-	program_invocation_short_name = arg + 1;
-      else
-	program_invocation_short_name = program_invocation_name;
 
       /* Update what we use for messages.  */
-      state->name = program_invocation_short_name;
+      state->name = strrchr (arg, '/');
+      if (state->name)
+	state->name++;
+      else
+	state->name = arg;
+
+#if defined _LIBC || HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME
+      program_invocation_short_name = state->name;
+#endif
 
       if ((state->flags & (ARGP_PARSE_ARGV0 | ARGP_NO_ERRS))
 	  == ARGP_PARSE_ARGV0)
 	/* Update what getopt uses too.  */
-	state->argv[0] = program_invocation_name;
+	state->argv[0] = arg;
 
       break;
 
@@ -599,7 +603,7 @@ parser_init (struct parser *parser, cons
       parser->state.name = short_name ? short_name + 1 : argv[0];
     }
   else
-    parser->state.name = program_invocation_short_name;
+    parser->state.name = __argp_short_program_name ();
 
   return 0;
 }
Index: argp-xinl.c
===================================================================
RCS file: /cvs/glibc/libc/argp/argp-xinl.c,v
retrieving revision 1.3
diff -u -p -r1.3 argp-xinl.c
--- argp-xinl.c	6 Jul 2001 04:54:44 -0000	1.3
+++ argp-xinl.c	22 Aug 2003 22:07:06 -0000
@@ -22,7 +22,9 @@
 #include <config.h>
 #endif
 
-#include <features.h>
+#if defined _LIBC || defined HAVE_FEATURES_H
+# include <features.h>
+#endif
 
 #ifndef __USE_EXTERN_INLINES
 # define __USE_EXTERN_INLINES	1
Index: argp.h
===================================================================
RCS file: /cvs/glibc/libc/argp/argp.h,v
retrieving revision 1.25
diff -u -p -r1.25 argp.h
--- argp.h	12 Jun 2003 18:07:27 -0000	1.25
+++ argp.h	22 Aug 2003 22:07:06 -0000
@@ -36,6 +36,19 @@
 # define __THROW
 #endif
 
+#ifndef __attribute__
+/* This feature is available in gcc versions 2.5 and later.  */
+# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5) || __STRICT_ANSI__
+#  define __attribute__(Spec) /* empty */
+# endif
+/* The __-protected variants of `format' and `printf' attributes
+   are accepted by gcc versions 2.6.4 (effectively 2.7) and later.  */
+# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 7) || __STRICT_ANSI__
+#  define __format__ format
+#  define __printf__ printf
+# endif
+#endif
+
 #ifndef __error_t_defined
 typedef int error_t;
 # define __error_t_defined


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]