[patch] basename vs. __gnu_basename fix

[Craig Howland]

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