MinGW compilation warnings in libiberty's xstrndup.c

MinGW compilation warnings in libiberty's xstrndup.c

[Eli Zaretskii]

When compiling libiberty (as part of GDB) with MinGW on MS-Windows, I
see the following warning:

     gcc -c -DHAVE_CONFIG_H -O2 -gdwarf-4 -g3 -D__USE_MINGW_ACCESS  -I. -I./../include   -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -pedantic  -D_GNU_SOURCE ./xstrndup.c -o xstrndup.o
     ./xstrndup.c: In function 'xstrndup':
     ./xstrndup.c:51:16: warning: implicit declaration of function 'strnlen' [-Wimplicit-function-declaration]
	size_t len = strnlen (s, n);
		     ^

This happens because libiberty.h uses incorrect guards for the
prototype of strnlen:

  #if defined (HAVE_DECL_STRNLEN) && !HAVE_DECL_STRNLEN
  extern size_t strnlen (const char *, size_t);
  #endif

It should use HAVE_STRNLEN instead, because that's the only
strnlen-related macro defined in config.g when strnlen is probed by
the configure script.

Thanks.

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Pedro Alves]

[Adding Thomas]

On 05/08/2017 04:30 PM, Eli Zaretskii wrote:
> When compiling libiberty (as part of GDB) with MinGW on MS-Windows, I
> see the following warning:
> 
>      gcc -c -DHAVE_CONFIG_H -O2 -gdwarf-4 -g3 -D__USE_MINGW_ACCESS  -I. -I./../include   -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -pedantic  -D_GNU_SOURCE ./xstrndup.c -o xstrndup.o
>      ./xstrndup.c: In function 'xstrndup':
>      ./xstrndup.c:51:16: warning: implicit declaration of function 'strnlen' [-Wimplicit-function-declaration]
> 	size_t len = strnlen (s, n);
> 		     ^
> 
> This happens because libiberty.h uses incorrect guards for the
> prototype of strnlen:
> 
>   #if defined (HAVE_DECL_STRNLEN) && !HAVE_DECL_STRNLEN
>   extern size_t strnlen (const char *, size_t);
>   #endif
> 
> It should use HAVE_STRNLEN instead, because that's the only
> strnlen-related macro defined in config.g when strnlen is probed by
> the configure script.

Looks like the declaration check got added to gcc's configure, but not
elsewhere, with:

commit d968efeac356364c01445013a1a3660e5087cb15
Author:     tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
AuthorDate: Tue Jun 10 09:45:00 2014 +0000

    [PR lto/61334] Declare prototype for strnlen, if needed.
    
        include/
        * libiberty.h [defined (HAVE_DECL_STRNLEN) &&
        !HAVE_DECL_STRNLEN] (strnlen): New prototype.
        gcc/
        * configure.ac: Use gcc_AC_CHECK_DECLS to check for strnlen
        prototype.
        * config.in: Regenerate.
        * configure: Likewise.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@211401 138bc75d-0d04-0410-961f-82ee72b054a4


But then, xstrndup.c has at the top:

#ifdef HAVE_STRING_H
#include <string.h>
#else
# ifdef HAVE_STRINGS_H
#  include <strings.h>
# endif
#endif

So I would expect your build to pick the strnlen declaration from
one of the string.h or strings.h mingw headers.  Why didn't it?

Thanks,
Pedro Alves

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org, Thomas Schwinge <thomas@codesourcery.com>
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 19 May 2017 16:22:55 +0100
> 
> But then, xstrndup.c has at the top:
> 
> #ifdef HAVE_STRING_H
> #include <string.h>
> #else
> # ifdef HAVE_STRINGS_H
> #  include <strings.h>
> # endif
> #endif
> 
> So I would expect your build to pick the strnlen declaration from
> one of the string.h or strings.h mingw headers.  Why didn't it?

Because MinGW doesn't have it, not unless you build a program that
will require one of the newer versions of the Windows C runtime
library.  That's why libiberty's strnlen is being compiled in the
MinGW build in the first place.

Specifically, the MinGW headers do provide a prototype for strnlen if
the program defines __MSVCRT_VERSION__ to be a high enough version, or
defines _POSIX_C_SOURCE >= 200809L, but none of these is set by
default, and is not a good idea, as explained above, for a program
that needs to run on a wide variety of OS versions.

IOW, libiberty shouldn't rely on the system headers to provide a
strnlen prototype when libiberty's strnlen is being included in the
library as a replacement.

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Pedro Alves]

On 05/19/2017 04:40 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org, Thomas Schwinge <thomas@codesourcery.com>
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 19 May 2017 16:22:55 +0100
>>
>> But then, xstrndup.c has at the top:
>>
>> #ifdef HAVE_STRING_H
>> #include <string.h>
>> #else
>> # ifdef HAVE_STRINGS_H
>> #  include <strings.h>
>> # endif
>> #endif
>>
>> So I would expect your build to pick the strnlen declaration from
>> one of the string.h or strings.h mingw headers.  Why didn't it?
> 
> Because MinGW doesn't have it, not unless you build a program that
> will require one of the newer versions of the Windows C runtime
> library.  That's why libiberty's strnlen is being compiled in the
> MinGW build in the first place.

OK, I didn't realize there was a strnlen replacement too.

> 
> Specifically, the MinGW headers do provide a prototype for strnlen if
> the program defines __MSVCRT_VERSION__ to be a high enough version, or
> defines _POSIX_C_SOURCE >= 200809L, but none of these is set by
> default, and is not a good idea, as explained above, for a program
> that needs to run on a wide variety of OS versions.
> 
> IOW, libiberty shouldn't rely on the system headers to provide a
> strnlen prototype when libiberty's strnlen is being included in the
> library as a replacement.

OK, I guess then we're up to figuring out which direction to go.
Either an AC_CHECK_DECL is missing on libiberty's configure,
or the original patch really wanted AC_CHECK_FUNC instead of 
AC_CHECK_DECL.  Or something else, I only look at libiberty's
configury every couple of years and forget how this is all
supposed to work in between.

Thanks,
Pedro Alves

Re: MinGW compilation warnings in libiberty's xstrndup.c

[DJ Delorie]

Eli Zaretskii <eliz@gnu.org> writes:
> It should use HAVE_STRNLEN instead, because that's the only
> strnlen-related macro defined in config.g when strnlen is probed by
> the configure script.

Ah, but gcc's configure defines HAVE_DECL_STRNLEN.  Header guards need
to be coordinated across all the users, not just libiberty.

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Pedro Alves]

On 05/19/2017 10:56 PM, DJ Delorie wrote:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
>> It should use HAVE_STRNLEN instead, because that's the only
>> strnlen-related macro defined in config.g when strnlen is probed by
>> the configure script.
> 
> Ah, but gcc's configure defines HAVE_DECL_STRNLEN.  Header guards need
> to be coordinated across all the users, not just libiberty.
> 

The "user" in this case is libiberty itself.

Thanks,
Pedro Alves

Re: MinGW compilation warnings in libiberty's xstrndup.c

[DJ Delorie]

Right, I meant, libiberty's configure, gcc's configure, binutils'
configure, and gdb's configure, all need to agree on whether strnlen is
a HAVE or a HAVE_DECL type symbol.  If they don't, the header can't
provide "one" working solution.

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Pedro Alves]

On 05/19/2017 11:31 PM, DJ Delorie wrote:
> 
> Right, I meant, libiberty's configure, gcc's configure, binutils'
> configure, and gdb's configure, all need to agree on whether strnlen is
> a HAVE or a HAVE_DECL type symbol.  If they don't, the header can't
> provide "one" working solution.
> 

Ah, yeah.  AFAICS, all the declaration checks in libiberty.h are 
HAVE_DECL checks.  This suggests to me that this declaration guard 
should be HAVE_DECL too [1].

BTW, I once proposed a new libiberty.m4 file that all libiberty
clients would source so that these checks are all centralized.

Here:
 https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00580.html

And follow up here:
 https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01712.html

Leading to (as, gold, ld, gdb and libiberty/ itself converted):
 https://github.com/palves/gdb/commits/palves/libiberty_m4

I never tried adjusting gcc, but even if it wouldn't
work there, it'd still be a net win.

Wonder what others think of that approach.

Thanks,
Pedro Alves

Re: MinGW compilation warnings in libiberty's xstrndup.c

[DJ Delorie]

Pedro Alves <palves@redhat.com> writes:
> Ah, yeah.  AFAICS, all the declaration checks in libiberty.h are 
> HAVE_DECL checks.  This suggests to me that this declaration guard 
> should be HAVE_DECL too [1].

Except the ones in the $funcs list, which includes strnlen.  I think in
the old days, we didn't put in declarations at all... until "char *"
became a different size than "int" and we started needing them.

So some functions in libiberty are HAVE_DECL and others are still HAVE.

Ah, found it, this commit is incomplete:

https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00784.html

It changes gcc's configure but nobody else's (and now we have an answer
to the three-year-old question "why don't we have a more liberal commit
policy?" ;) which breaks both libiberty and libgfortran.

> BTW, I once proposed a new libiberty.m4 file that all libiberty
> clients would source so that these checks are all centralized.

I have no philosophical problem with that type of change, but I have the
usual fear of touching anything in libiberty that's been around this
long ;-)

(this bug being a prime example of how subtle an incorrect change can be)

(and honestly, my upstream attention is elsewhere these days)

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Pedro Alves]

On 05/20/2017 01:38 AM, DJ Delorie wrote:
> 
> Pedro Alves <palves@redhat.com> writes:
>> Ah, yeah.  AFAICS, all the declaration checks in libiberty.h are 
>> HAVE_DECL checks.  This suggests to me that this declaration guard 
>> should be HAVE_DECL too [1].
> 
> Except the ones in the $funcs list, which includes strnlen.  I think in
> the old days, we didn't put in declarations at all... until "char *"
> became a different size than "int" and we started needing them.

Running:

$ grep HAVE_ libiberty/config.h | sed 's/DECL_//g'| sort | uniq -c | sort -n

on the build I have handy shows:
...
      2 #define HAVE_ASPRINTF 1
      2 #define HAVE_BASENAME 1
      2 #define HAVE_CALLOC 1
      2 #define HAVE_FFS 1
      2 #define HAVE_SBRK 1
      2 #define HAVE_SNPRINTF 1
      2 #define HAVE_STRTOL 1
      2 #define HAVE_STRTOLL 1
      2 #define HAVE_STRTOUL 1
      2 #define HAVE_STRTOULL 1
      2 #define HAVE_STRVERSCMP 1
      2 #define HAVE_VASPRINTF 1

"2" means above means each FOO symbol above has both HAVE_FOO
and HAVE_DECL_FOO defines:

 $ grep "HAVE.*_SNPRINTF" config.h
 #define HAVE_DECL_SNPRINTF 1
 #define HAVE_SNPRINTF 1

> 
> So some functions in libiberty are HAVE_DECL and others are still HAVE.

But I don't see any HAVE check in libiberty.h (for function symbols),
only HAVE_DECL ones:

$ grep HAVE libiberty.h 
/* HAVE_DECL_* is a three-state macro: undefined, 0 or 1.  If it is
#if !HAVE_DECL_BASENAME
 || defined (__DragonFly__) || defined (HAVE_DECL_BASENAME) 
   autoconf which would result in HAVE_DECL_BASENAME being set.  */
#if defined (HAVE_DECL_FFS) && !HAVE_DECL_FFS
#if defined(HAVE_DECL_ASPRINTF) && !HAVE_DECL_ASPRINTF
#if !HAVE_DECL_VASPRINTF
#if defined(HAVE_DECL_SNPRINTF) && !HAVE_DECL_SNPRINTF
#if defined(HAVE_DECL_VSNPRINTF) && !HAVE_DECL_VSNPRINTF
#if defined (HAVE_DECL_STRNLEN) && !HAVE_DECL_STRNLEN
#if defined(HAVE_DECL_STRVERSCMP) && !HAVE_DECL_STRVERSCMP
#if defined(HAVE_DECL_STRTOL) && !HAVE_DECL_STRTOL
#if defined(HAVE_DECL_STRTOUL) && !HAVE_DECL_STRTOUL
#if defined(HAVE_LONG_LONG) && defined(HAVE_DECL_STRTOLL) && !HAVE_DECL_STRTOLL
#if defined(HAVE_LONG_LONG) && defined(HAVE_DECL_STRTOULL) && !HAVE_DECL_STRTOULL
#if defined(HAVE_DECL_STRVERSCMP) && !HAVE_DECL_STRVERSCMP

Nor in other headers under include/, while at it.
Are you looking elsewhere perhaps?  Based on the above, it looks to
me like the non-HAVE_DECL HAVE symbols are implementation detail
to libiberty, side effect of the checks used to determine whether
a replacement is necessary.

> 
> Ah, found it, this commit is incomplete:
> 
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00784.html
> 
> It changes gcc's configure but nobody else's (and now we have an answer
> to the three-year-old question "why don't we have a more liberal commit
> policy?" ;) which breaks both libiberty and libgfortran.

Yeah, that exactly the sort of thing that gets fixed by design by having
a centralized libiberty.m4 file.

> 
>> BTW, I once proposed a new libiberty.m4 file that all libiberty
>> clients would source so that these checks are all centralized.
> 
> I have no philosophical problem with that type of change, but I have the
> usual fear of touching anything in libiberty that's been around this
> long ;-)
> 
> (this bug being a prime example of how subtle an incorrect change can be)
> 
> (and honestly, my upstream attention is elsewhere these days)
> 

Thanks,
Pedro Alves

Re: MinGW compilation warnings in libiberty's xstrndup.c

[DJ Delorie]

Please try this patch:

Index: config.in
===================================================================
--- config.in	(revision 248482)
+++ config.in	(working copy)
@@ -76,12 +76,16 @@
 #undef HAVE_DECL_SBRK
 
 /* Define to 1 if you have the declaration of `snprintf', and to 0 if you
    don't. */
 #undef HAVE_DECL_SNPRINTF
 
+/* Define to 1 if you have the declaration of `strnlen', and to 0 if you
+   don't. */
+#undef HAVE_DECL_STRNLEN
+
 /* Define to 1 if you have the declaration of `strtol', and to 0 if you don't.
    */
 #undef HAVE_DECL_STRTOL
 
 /* Define to 1 if you have the declaration of `strtoll', and to 0 if you
    don't. */
Index: configure
===================================================================
--- configure	(revision 248482)
+++ configure	(working copy)
@@ -5861,12 +5861,22 @@ else
   ac_have_decl=0
 fi
 
 cat >>confdefs.h <<_ACEOF
 #define HAVE_DECL_STRTOULL $ac_have_decl
 _ACEOF
+ac_fn_c_check_decl "$LINENO" "strnlen" "ac_cv_have_decl_strnlen" "$ac_includes_default"
+if test "x$ac_cv_have_decl_strnlen" = x""yes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_STRNLEN $ac_have_decl
+_ACEOF
 
 
 $as_echo "#define HAVE_SYS_ERRLIST 1" >>confdefs.h
 
 
 $as_echo "#define HAVE_SYS_NERR 1" >>confdefs.h
@@ -7002,12 +7012,23 @@ else
 fi
 
 cat >>confdefs.h <<_ACEOF
 #define HAVE_DECL_STRVERSCMP $ac_have_decl
 _ACEOF
 
+  ac_fn_c_check_decl "$LINENO" "strnlen" "ac_cv_have_decl_strnlen" "$ac_includes_default"
+if test "x$ac_cv_have_decl_strnlen" = x""yes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_STRNLEN $ac_have_decl
+_ACEOF
+
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether canonicalize_file_name must be declared" >&5
 $as_echo_n "checking whether canonicalize_file_name must be declared... " >&6; }
 if test "${libiberty_cv_decl_needed_canonicalize_file_name+set}" = set; then :
   $as_echo_n "(cached) " >&6
 else
   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
Index: configure.ac
===================================================================
--- configure.ac	(revision 248482)
+++ configure.ac	(working copy)
@@ -413,13 +413,13 @@ if test "x" = "y"; then
      stpcpy stpncpy strcasecmp strchr strdup \
      strerror strncasecmp strndup strnlen strrchr strsignal strstr strtod \
      strtol strtoul strtoll strtoull strverscmp sysconf sysctl sysmp \
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid)
-  AC_CHECK_DECLS([basename(char *), ffs, asprintf, vasprintf, snprintf, vsnprintf, strtol, strtoul, strtoll, strtoull])
+  AC_CHECK_DECLS([basename(char *), ffs, asprintf, vasprintf, snprintf, vsnprintf, strtol, strtoul, strtoll, strtoull, strnlen])
   AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
   AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
   AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
 fi
 
 # For each of these functions, if the host does not provide the
@@ -686,12 +686,13 @@ if test -z "${setobjs}"; then
 
   AC_CHECK_FUNCS($checkfuncs)
   AC_CHECK_DECLS([basename(char *), ffs, asprintf, vasprintf, snprintf, vsnprintf])
   AC_CHECK_DECLS([calloc, getenv, getopt, malloc, realloc, sbrk])
   AC_CHECK_DECLS([strtol, strtoul, strtoll, strtoull])
   AC_CHECK_DECLS([strverscmp])
+  AC_CHECK_DECLS([strnlen])
   libiberty_NEED_DECLARATION(canonicalize_file_name)
 fi
 
 # Figure out which version of pexecute to use.
 case "${host}" in
      *-*-mingw* | *-*-winnt*)	pexecute=pex-win32  ;;

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Eli Zaretskii]

> From: DJ Delorie <dj@redhat.com>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sourceware.org
> Date: Fri, 26 May 2017 17:34:12 -0400
> 
> 
> Please try this patch:

Seems to work fine, thanks.

Re: MinGW compilation warnings in libiberty's xstrndup.c

[DJ Delorie]

Eli Zaretskii <eliz@gnu.org> writes:
> Seems to work fine, thanks.

Checked into gcc trunk then :-)

Re: MinGW compilation warnings in libiberty's xstrndup.c

[Eli Zaretskii]

> From: DJ Delorie <dj@redhat.com>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sourceware.org
> Date: Wed, 31 May 2017 00:17:14 -0400
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > Seems to work fine, thanks.
> 
> Checked into gcc trunk then :-)

Thanks!