This is the mail archive of the newlib@sourceware.org mailing list for the newlib 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: [patch] basename vs. __gnu_basename fix




On 04/21/2015 11:50 AM, Corinna Vinschen wrote:
Hi,

I'd like to propose the following patch.  While running some autoconf
script under a test version of Cygwin, the following problem turned up:

In file included from conftest.c:429:0:
/usr/include/libgen.h:18:14: error: conflicting types for 'basename'
  extern char *basename (char *path);
               ^
In file included from /usr/include/stdio.h:29:0,
                  from conftest.c:396:
/usr/include/string.h:172:7: note: previous declaration of 'basename' was here
  char *_EXFUN(basename,(const char *))
        ^

It turns out that the current method to define basename in libgen.h
(POSIX) as well as in string.h (GNU) is prone to an ordering problem.  I
checked that this problem doesn't exist with glibc, and that the glibc
method prefers the POSIX version over the GNU version if libgen.h is
included, independently of the order of inclusion.

However, this is a bit simpler in glibc because the symbol "basename"
is the GNU version and "__xpg_basename" is the POSIX version and
__xpg_basename is preferred by simply having

   #define basename __xpg_basename

in libgen.h.

The below patch emulates the way glibc does it.  I tested it locally
on a Cygwin installation, but I wouldn't be too unhappy for some
scrutinizing look.
Looks nominally sound, but some suggested tweaks, below.


Thanks,
Corinna


	* libc/include/libgen.h: Drop defining _BASENAME_DEFINED.  Always
	define macro basename.  Add comment to explain why.
	* libc/include/string.h: Check for basename instead of
	_BASENAME_DEFINED.  Drop __GNUC__ branch and always use basename
	macro instead.  Change comment to explain why.


diff --git a/newlib/libc/include/libgen.h b/newlib/libc/include/libgen.h
index 8360a22..292d585 100644
--- a/newlib/libc/include/libgen.h
+++ b/newlib/libc/include/libgen.h
@@ -12,8 +12,14 @@
  extern "C" {
  #endif
+/* There are two common basename variants. If you #include <libgen.h> you get
+   the POSIX version; otherwise, if you define _GNU_SOURCE, you get the GNU
+   version via <string.h>.  POSIX requires that #undef basename will still let
+   you invoke the underlying function.  However, this also implies that the
+   POSIX version is used in this case.  That's made sure here. */
+#undef basename
+#define basename basename
  char      *_EXFUN(basename,     (char *));
-#define _BASENAME_DEFINED
  char      *_EXFUN(dirname,     (char *));
#ifdef __cplusplus
diff --git a/newlib/libc/include/string.h b/newlib/libc/include/string.h
index 9e11e5c..cee9bd9 100644
--- a/newlib/libc/include/string.h
+++ b/newlib/libc/include/string.h
@@ -163,18 +163,14 @@ int	_EXFUN(strtosigno, (const char *__name));
  			 (char *) memcpy (__out, __in, __len-1);}))
  #endif /* _GNU_SOURCE && __GNUC__ */
-/* There are two common basename variants. If you #include <libgen.h>
-   first, you get the POSIX version; otherwise you get the GNU version.
-   POSIX requires that #undef basename will still let you
-   invoke the underlying function, but that requires gcc support.  */
-#if __GNU_VISIBLE && !defined(_BASENAME_DEFINED)
-# ifdef __GNUC__
-char	*_EXFUN(basename,(const char *))
-             __asm__ (__ASMNAME ("__gnu_basename")) __nonnull(1);
-# else
+/* There are two common basename variants.  If you #include <libgen.h> you get
+   the POSIX version; otherwise, if you define _GNU_SOURCE, you get the GNU
+   version via <string.h>.  POSIX requires that #undef basename will still let
+   you invoke the underlying function.  However, this also implies that the
+   POSIX version is used in this case.  That's made sure here. */
+#if __GNU_VISIBLE && !defined(basename)
  char	*_EXFUN(__gnu_basename,(const char *));
  # define basename __gnu_basename
-# endif
  #endif
The prototype should not be skipped if basename is defined (which I'm guessing is an unintended change). In addition, to help make use more clear (particularly for users--maintainers too, but not so much) the comment and #if condition should perhaps be changed a little. I suggest it should end up being more like the following. (Comment same in both files, as you have it, but only shown once in the context of string.h.)

I also added the nonnull attribute to the prototype, to avoid a possible change in compiler warnings due to the GNUC part being dropped.

/* There are two common basename variants.  If you do NOT #include <libgen.h>
 * and you do
 * 	#define _GNU_SOURCE
 * 	#include <string.h>
 * you get the GNU version.  Otherwise, you get the POSIX version, for which
 * you should #include <libgen.h> for the function prototype.  POSIX requires
 * that #undef basename will still let you invoke the underlying function.
 * However, this also implies that the POSIX version is used in this case.
 * That's made sure here. */
#if defined(_GNU_SOURCE)
char	*_EXFUN(__nonnull(1) __gnu_basename,(const char *));
# if !defined(basename)
#  define basename __gnu_basename
# endif
#endif
(I suggest #if defined(_GNU_SOURCE) instead of # if __GNU_VISIBLE because the former is the method by which the user is supposed to request it. Yes, given cdefs.h the two are equivalent, but it seems that _GNU_SOURCE would be more clear. (No need to spend time figuring out the __GNU_VISIBLE indirection.))

Craig


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