Re: manywarnings and -f options

[Bruno Haible]

Eric Blake wrote:
> > Should gl_MANYWARN_ALL_GCC be adding some gcc -f options?  For example,
> > at least gcc 4.3.4 -Wdisabled-optimization would emit a warning that
> > several other -W warnings are useless without -funit-at-a-time also in
> > place (see coreutils commit 5e361387d).  Also, according to 'info gcc,
> > -Wsuggest-attribute=pure only works if -fipa-pure-const is turned on
> > (true by default for -O compilation, but apparently the -W option can
> > also catch a few cases even without -O if -fipa-pure-const is manually
> > turned on).

-funit-at-a-time is an option which changes the code generation. IMO
it should not be enabled by a warnings module. For the same reason,
-Wdisabled-optimization is useless for our purposes - it is useful
for people who want maximum optimization, but the Autoconf default
is -O2 (for a good reason: Avoid less reliable optimizations).

So, treat -Wdisabled-optimization like -Wno-unsuffixed-float-constants.

> I also meant to add that -fdiagnostics-show-option is a must for
> determining which warnings are firing, when deciding which warnings to
> avoid.  Having these -f options added by default instead of making each
> client add them would make maintenance easier.

This option influences diagnostics, so IMO it makes sense to add it
whenever the compiler supports it.

Bruno
-- 
In memoriam Rudolf Slánský 

Re: spurious message about include files from `bootstrap' script

[Bruno Haible]

Hi Stefano,

>   You may need to add #include directives for the following .h files.
> #include 
> ... [SNIP]
> #include 
> #include 
> /* Include only after all system include files.  */
> GPL
> License
> #include "acl.h"
> #include "alignof.h"
> # ... [SNIP]
> #include "yesno.h"
> 
>   You may need to use the following Makefile variables when linking.
>   ...

Thanks for the report. This should fix it.

Jim, these modules came from codeutils. The same bug is also present in
the coreutils files

  gl/modules/heap
  gl/modules/randint
  gl/modules/randperm
  gl/modules/randread

If you start a new module by making a copy of modules/TEMPLATE, this mistake
can't happen.


2011-12-03  Bruno Haible  

Fix module descriptions syntax.
* modules/argv-iter (License): Fix syntax.
* modules/di-set (License): Likewise.
* modules/ino-map (License): Likewise.
Reported by Stefano Lattarini .

--- modules/argv-iter.orig  Sat Dec  3 13:06:54 2011
+++ modules/argv-iter   Sat Dec  3 13:04:41 2011
@@ -18,7 +18,7 @@
 Include:
 "argv-iter.h"
 
-License
+License:
 GPL
 
 Maintainer:
--- modules/di-set.orig Sat Dec  3 13:06:54 2011
+++ modules/di-set  Sat Dec  3 13:04:41 2011
@@ -17,7 +17,7 @@
 Include:
 "di-set.h"
 
-License
+License:
 GPL
 
 Maintainer:
--- modules/ino-map.origSat Dec  3 13:06:54 2011
+++ modules/ino-map Sat Dec  3 13:04:39 2011
@@ -17,7 +17,7 @@
 Include:
 "ino-map.h"
 
-License
+License:
 GPL
 
 Maintainer:
-- 
In memoriam Rudolf Slánský 

Re: [PATCH 1/4] Split the HOST_NAME_MAX detection into a separate m4 macro

[Bruno Haible]

Ben Walton wrote:
> The sethostname module will rely on this code too, so make it a
> separate function.

Thanks, I'm applying this patch, tweaking the ChangeLog entry to contain
a one-line summary and tweaking the git commit message to contain the
ChangeLog entry.

And this trivial whitespace+comments change.


diff --git a/m4/gethostname.m4 b/m4/gethostname.m4
--- a/m4/gethostname.m4
+++ b/m4/gethostname.m4
@@ -43,9 +43,8 @@ AC_DEFUN([gl_FUNC_GETHOSTNAME],
   gl_PREREQ_HOST_NAME_MAX
 ])
 
-dnl Also provide HOST_NAME_MAX when  lacks it.
+# Provide HOST_NAME_MAX when  lacks it.
 AC_DEFUN([gl_PREREQ_HOST_NAME_MAX], [
-
   dnl - On most Unix systems, use MAXHOSTNAMELEN from  instead.
   dnl - On Solaris, Cygwin, BeOS, use MAXHOSTNAMELEN from  instead.
   dnl - On mingw, use 256, because
-- 
In memoriam Rudolf Slánský 

Re: spurious message about include files from `bootstrap' script

[Jim Meyering]

Bruno Haible wrote:
> Hi Stefano,
>>   You may need to add #include directives for the following .h files.
>> #include 
>> ... [SNIP]
>> #include 
>> #include 
>> /* Include only after all system include files.  */
>> GPL
>> License
>> #include "acl.h"
>> #include "alignof.h"
>> # ... [SNIP]
>> #include "yesno.h"
>>
>>   You may need to use the following Makefile variables when linking.
>>   ...
>
> Thanks for the report. This should fix it.
>
> Jim, these modules came from codeutils. The same bug is also present in
> the coreutils files
>
>   gl/modules/heap
>   gl/modules/randint
>   gl/modules/randperm
>   gl/modules/randread
>
> If you start a new module by making a copy of modules/TEMPLATE, this mistake
> can't happen.

Thanks to both of you.

Bruno, what do you think about making gnulib-tool warn about
that syntax error?

Fixed with this:

  perl -pi -e 's/^(License)$/$1:/' $(git grep -l '^License$' gl)


>From 79c5fcc681b823a4e5fbb8fd1baa47db4eccd86c Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Sat, 3 Dec 2011 14:04:17 +0100
Subject: [PATCH] maint: add missing ":" after "License" in local gnulib
 module files

This avoids spurious diagnostics when running our "bootstrap" script.
* gl/modules/heap: Append colon after "License".
* gl/modules/randint: Likewise.
* gl/modules/randperm: Likewise.
* gl/modules/randread: Likewise.
Reported by Stefano Lattarini.  Diagnosed by Bruno Haible.
---
 gl/modules/heap |2 +-
 gl/modules/randint  |2 +-
 gl/modules/randperm |2 +-
 gl/modules/randread |2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gl/modules/heap b/gl/modules/heap
index cd97e29..97e9df2 100644
--- a/gl/modules/heap
+++ b/gl/modules/heap
@@ -17,7 +17,7 @@ lib_SOURCES += heap.c heap.h
 Include:
 "heap.h"

-License
+License:
 GPL

 Maintainer:
diff --git a/gl/modules/randint b/gl/modules/randint
index 4485581..37025b7 100644
--- a/gl/modules/randint
+++ b/gl/modules/randint
@@ -17,7 +17,7 @@ lib_SOURCES += randint.c randint.h
 Include:
 "randint.h"

-License
+License:
 GPL

 Maintainer:
diff --git a/gl/modules/randperm b/gl/modules/randperm
index daf9e32..1fa2612 100644
--- a/gl/modules/randperm
+++ b/gl/modules/randperm
@@ -18,7 +18,7 @@ lib_SOURCES += randperm.c randperm.h
 Include:
 "randperm.h"

-License
+License:
 GPL

 Maintainer:
diff --git a/gl/modules/randread b/gl/modules/randread
index 33485c5..14af00e 100644
--- a/gl/modules/randread
+++ b/gl/modules/randread
@@ -28,7 +28,7 @@ lib_SOURCES += randread.c randread.h rand-isaac.c rand-isaac.h
 Include:
 "randread.h"

-License
+License:
 GPL

 Maintainer:
--
1.7.8.rc4

Re: [PATCH 2/4] Add a new sethostname module

[Bruno Haible]

Ben Walton wrote:
> Define sethostname on platforms that do not provide the declaration.
> Provide a function for platforms that lack it.  The general handling
> of the provided function is to simply return -1 and set errno to
> ENOSYS.  A specific handler is provided for Minix.

Thanks, I'm applying this for you. I trust that you will follow through
with the paperwork.

I'm applying a couple of followup tweaks.

> +@item
> +On some platforms the Gnulib replacement always fails with ENOSYS.

The ENOSYS failure is not a problem the Gnulib fixes, but it is more a
limitation that Gnulib has, when fixing the "missing function" problem.

> +  /* NAME does not need to be null terminated so leave room to terminate
> + regardless of input. */
> +  char hostname[HOST_NAME_MAX + 1];
> +  memcpy ((void *) hostname, (const void *) name, len);
> +  hostname[len] = '\0';

Copying the host name argument into a stack allocated array is actually
not needed, if we don't use functions like strlen(). The fprintf()
call below can be changed to not assume a NUL terminated string.

> +#ifdef __minix /* Minix */
> +  {
> +FILE *hostf;
> +int r = 0;
> +
> +/* glibc returns EFAULT, EINVAL, and EPERM on error.  None of
> +   these are appropriate for us to set, even if they may match the
> +   situation, during failed open/write/close operations, so we
> +   leave errno alone and rely on what the system sets up. */
> +hostf = fopen ("/etc/hostname.file", "w");
> +if (hostf == NULL)
> +  r = -1;
> +else
> +  {
> +fprintf (hostf, "%s\n", hostname);
> +if (ferror (hostf))
> +  {
> +clearerr (hostf);
> +r = -1;

Here the clearerr call is unnecessary, because the next call is fclose (hostf),
which doesn't care about the stream's error flag.

But here you need to save errno. Otherwise, if fprintf failed but the fclose
then succeeds, you end up with whatever error code the fclose call may have
put into errno. If fclose succeeds you have to ignore the value that it
may have put into errno - that is a general rule about POSIX functions and
errno.

> +  }
> +
> +/* use return value of fclose for function return value as it
> +   matches our needs.  fclose will also set errno on
> +   failure */
> +r = fclose (hostf);

Using the same return value here is not portable. When fclose fails, it
returns EOF. Whereas when sethostname fails, it should return a negative
value. There is no guarantee that EOF is negative. While EOF happens to be
-1 on most systems, it could also be defined to 0x1000 on some systems.

> diff --git a/m4/sethostname.m4 b/m4/sethostname.m4
> new file mode 100644
> index 000..be5b036
> --- /dev/null
> +++ b/m4/sethostname.m4
> @@ -0,0 +1,25 @@
> +# sethostname.m4 serial 1
> +dnl Copyright (C) 2011 Free Software Foundation, Inc.
> +dnl This file is free software; the Free Software Foundation
> +dnl gives unlimited permission to copy and/or distribute it,
> +dnl with or without modifications, as long as this notice is preserved.
> +
> +# Ensure
> +# - the sethostname() function,

I updated the comment here, to mention HOST_NAME_MAX.

> +AC_DEFUN([gl_FUNC_SETHOSTNAME],
> +[
> +  AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
> +
> +  gl_PREREQ_HOST_NAME_MAX
> +
> +  AC_REPLACE_FUNCS([sethostname])
> +  AC_CHECK_FUNCS([sethostname])

AC_REPLACE_FUNCS([sethostname]) is equivalent to
 AC_CHECK_FUNCS([sethostname])
 if test $ac_cv_func_sethostname = no; then
   AC_LIBOBJ([sethostname])
 fi

But in gnulib we now avoid to put AC_LIBOBJ calls into *.m4 files; we put them
into the module description instead.

> +Include:
> +
> +
> +Link:
> +

An empty 'Link' section can be removed. Just a matter of style. The
modules/TEMPLATE file contains a skeleton of the fields that should
be present, even when empty.


2011-12-03  Bruno Haible  

Tweak last commit.
* lib/sethostname.c: Don't include .
(sethostname): No need to copy the argument string to the stack. Don't
call clearerr. Preserve errno when fprintf failed.
* m4/sethostname.m4 (gl_FUNC_SETHOSTNAME): Comment about HOST_NAME_MAX.
Don't invoke AC_REPLACE_FUNCS.
* modules/sethostname (Link): Remove empty section.
* doc/glibc-functions/sethostname.texi: Gnulib does not fix the ENOSYS
failure problem.

--- doc/glibc-functions/sethostname.texi.orig   Sat Dec  3 13:54:54 2011
+++ doc/glibc-functions/sethostname.texiSat Dec  3 13:54:31 2011
@@ -9,6 +9,7 @@
 @item
 This function is missing on some platforms:
 Minix 3.1.8, Cygwin, mingw, MSVC 9, Interix 3.5, BeOS.
+Note that the Gnulib replacement may fail with ENOSYS on some platforms.
 @item
 This function is not declared on some platforms:
 AIX 7.1, OSF/1 5.1, Solaris 10.
@@ -16,8 +17,6 @@
 On Solaris 10, the first argument is @code{char *} instead of
 @code{const char *} and the second parameter is @code{int} instead o

Re: [PATCH 3/4] Integrate the sethostname module into unistd

[Bruno Haible]

Ben Walton wrote:
> Ensure that sethostname is accounted for within the unistd module.

Thanks, I'm applying this patch as well.

>  2011-12-01  Ben Walton  
>  
> + * lib/unistd.in.h: Integrate the SETHOSTNAME preprocessor handling
> +  into the unistd.h header.
> + * m4/unistd_h.m4: Setup the autoconf handling for the SETHOSTNAME
> +  preprocessor directives.
> + * modules/unistd: Setup the Makefile substitutions of the
> +  SETHOSTNAME preprocessor directives.

I removed the extra spaces, for consistency with other ChangeLog entries.

> +#if @GNULIB_SETHOSTNAME@
> +/* Set the host name of the machine.
> +   The host name may or may not be fully qualified.
> +
> +   Put LEN bytes of NAME into the host name.
> +   Return 0 if successful, otherwise, set errno and return -1
> +
> +   Platforms with no ability to set the hostname return -1 and set
> +   errno = ENOSYS. */
> +#  if !@HAVE_SETHOSTNAME@ || !@HAVE_DECL_SETHOSTNAME@

This preprocessor directive line has too much indentation.

> +#elif defined GNULIB_POSIXCHECK
> +# undef gethostname

Should be sethostname, not gethostname.

> +# if HAVE_RAW_DECL_GETHOSTNAME

Likewise.

> +_GL_WARN_ON_USE (sethostname, "sethostname is unportable - "
> + "use gnulib module sethostname for portability");

Also, in this file, the chunks are ordered alphabetically according to the
function name. I don't like this, but that's the way it is now.

> @@ -95,6 +95,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
>GNULIB_READLINKAT=0;   AC_SUBST([GNULIB_READLINKAT])
>GNULIB_RMDIR=0;AC_SUBST([GNULIB_RMDIR])
>GNULIB_SLEEP=0;AC_SUBST([GNULIB_SLEEP])
> +  GNULIB_SETHOSTNAME=0;  AC_SUBST([GNULIB_SETHOSTNAME])
>GNULIB_SYMLINK=0;  AC_SUBST([GNULIB_SYMLINK])
>GNULIB_SYMLINKAT=0;AC_SUBST([GNULIB_SYMLINKAT])
>GNULIB_TTYNAME_R=0;AC_SUBST([GNULIB_TTYNAME_R])
> @@ -131,6 +132,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
>HAVE_READLINK=1;AC_SUBST([HAVE_READLINK])
>HAVE_READLINKAT=1;  AC_SUBST([HAVE_READLINKAT])
>HAVE_SLEEP=1;   AC_SUBST([HAVE_SLEEP])
> +  HAVE_SETHOSTNAME=1; AC_SUBST([HAVE_SETHOSTNAME])
>HAVE_SYMLINK=1; AC_SUBST([HAVE_SYMLINK])
>HAVE_SYMLINKAT=1;   AC_SUBST([HAVE_SYMLINKAT])
>HAVE_UNLINKAT=1;AC_SUBST([HAVE_UNLINKAT])

Here the lists are apparently sorted alphabetically as well.
'sethostname' ought to come before 'sleep'.

> -e 's|@''HAVE_READLINK''@|$(HAVE_READLINK)|g' \
> -e 's|@''HAVE_READLINKAT''@|$(HAVE_READLINKAT)|g' \
> -e 's|@''HAVE_SLEEP''@|$(HAVE_SLEEP)|g' \
> +   -e 's|@''HAVE_SETHOSTNAME''@|$(HAVE_SETHOSTNAME)|g' \
> -e 's|@''HAVE_SYMLINK''@|$(HAVE_SYMLINK)|g' \
> -e 's|@''HAVE_SYMLINKAT''@|$(HAVE_SYMLINKAT)|g' \
> -e 's|@''HAVE_UNLINKAT''@|$(HAVE_UNLINKAT)|g' \

Likewise.


The complete set of tweaks that I'm adding:

2011-12-03  Bruno Haible  

Tweak last commit.
* lib/unistd.in.h (sethostname): Keep declarations in alphabetic order.
Fix preprocessor directives indentation. Fix typos.
* m4/unistd_h.m4 (gl_UNISTD_H_DEFAULTS): Keep alphabetic order.
* modules/unistd (Makefile): Likewise.

--- lib/unistd.in.h.origSat Dec  3 14:19:50 2011
+++ lib/unistd.in.h Sat Dec  3 14:17:26 2011
@@ -683,31 +683,6 @@
 # endif
 #endif
 
-#if @GNULIB_SETHOSTNAME@
-/* Set the host name of the machine.
-   The host name may or may not be fully qualified.
-
-   Put LEN bytes of NAME into the host name.
-   Return 0 if successful, otherwise, set errno and return -1
-
-   Platforms with no ability to set the hostname return -1 and set
-   errno = ENOSYS. */
-#  if !@HAVE_SETHOSTNAME@ || !@HAVE_DECL_SETHOSTNAME@
-_GL_FUNCDECL_SYS (sethostname, int, (const char *name, size_t len)
-_GL_ARG_NONNULL ((1)));
-#  endif
-/* Need to cast, because on Solaris 11 2011-10, MacOS X 10.5, IRIX 6.5
-   and FreeBSD 6.4 the second parameter is int.  On Solaris 11
-   2011-10, the first parameter is not const. */
-_GL_CXXALIAS_SYS_CAST (sethostname, int, (const char *name, size_t len));
-_GL_CXXALIASWARN (sethostname);
-#elif defined GNULIB_POSIXCHECK
-# undef gethostname
-# if HAVE_RAW_DECL_GETHOSTNAME
-_GL_WARN_ON_USE (sethostname, "sethostname is unportable - "
- "use gnulib module sethostname for portability");
-# endif
-#endif
 
 #if @GNULIB_GETHOSTNAME@
 /* Return the standard host name of the machine.
@@ -1292,6 +1267,33 @@
 # endif
 #endif
 
+
+#if @GNULIB_SETHOSTNAME@
+/* Set the host name of the machine.
+   The host name may or may not be fully qualified.
+
+   Put LEN bytes of NAME into the host name.
+   Return 0 if successful, otherwise, set errno and return -1.
+
+   Platforms with no ability to set the hostname return -1 and set
+   errno = ENOSYS.  */
+# if !@HAVE_SETHOSTNAME@ || !@HAVE_DECL_SETHOSTNAME@
+_GL_

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> Provide a module that tests the functionality of sethostname().

Thanks, I'm applying this as well.

> Signed-off-by: Ben Walton 
> ---
>  ChangeLog |6 ++
>  modules/sethostname-tests |   13 +
>  tests/test-sethostname.c  |  117 
> +
>  3 files changed, 136 insertions(+), 0 deletions(-)
>  create mode 100644 modules/sethostname-tests
>  create mode 100644 tests/test-sethostname.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 67f7f42..2e85f16 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,11 @@
>  2011-12-01  Ben Walton  
>  
> + * modules/sethostname-tests: New file.  A test program
> + for the sethostname module.
> + * tests/test-sethostname.c: Likewise.
> + 

A trailing blank here (in place of a blank line) disturbs the 
git-merge-changelog
program.

> +#define TESTHOSTNAME "gnulib-hostname"
> +
> +/* mingw and MSVC 9 lack geteuid, so setup a value that will indicate
> +   we don't have root privilege since we wouldn't know whether to
> +   expect success or failure when setting a name anyway*/
> +#if !HAVE_GETEUID
> +# define geteuid() ((uid_t) -1)
> +#endif

Wow! You have thought at many things. You did it better than if I had
written this test.

Just two things:
  - If a test fails, it's most conventional (in Gnulib) to print the cause
to stderr, not to stdout. 
  - When we have to skip the major part of the test, by convention it should
return 77 (so that the Automake generated Makefile rule prints "SKIP:"
instead of "PASS:") and - by Gnulib convention - also print an explanation
why the test was skipped. This helps detecting bugs.

> +  if (rcs != 0)
> +{
> +  if (rcs == -1 && errno == ENOSYS)
> + return 0;

Oops, that looks like a tab again.

> +  rcs = sethostname ((const char *) longname, (HOST_NAME_MAX + 1));
> +
> +  if (rcs != -1)
> +{
> +  /* attempt to restore the original name. */
> +  sethostname (origname, strlen (origname));
> +  printf ("expected failure when setting very long hostname.\n");

Error messages should not state in the first place what the program
expected, but what events occurred or what data actually appeared,
optionally with a small hint to what the program expected.


I'm applying these tweaks:


2011-12-03  Bruno Haible  

Tweak last commit.
* modules/sethostname-tests (Files): Sort by decreasing importance.
(configure.ac): Check for geteuid.
* tests/test-sethostname.c (main): Emit error messages to stderr. Skip
the test when there's nothing to test. Drop an unnecessary cast.
Improve an error message. Verify that the final sethostname() call
succeeds.

--- modules/sethostname-tests.orig  Sat Dec  3 14:38:36 2011
+++ modules/sethostname-tests   Sat Dec  3 14:32:34 2011
@@ -1,12 +1,13 @@
 Files:
-tests/signature.h
 tests/test-sethostname.c
+tests/signature.h
 tests/macros.h
 
 Depends-on:
 sys_types
 
 configure.ac:
+AC_CHECK_FUNCS_ONCE([geteuid])
 
 Makefile.am:
 TESTS += test-sethostname
--- tests/test-sethostname.c.orig   Sat Dec  3 14:38:36 2011
+++ tests/test-sethostname.cSat Dec  3 14:38:16 2011
@@ -55,13 +55,16 @@
  consider things like CAP_SYS_ADMIN (linux) or PRIV_SYS_ADMIN
  (solaris), etc.  systems without a working geteuid (mingw, MSVC
  9) will always skip this test. */
-  if (geteuid() != 0)
-return 0;
+  if (geteuid () != 0)
+{
+  fprintf (stderr, "Skipping test: insufficient permissions.\n");
+  return 77;
+}
 
   /* we want to ensure we can do a get/set/get check to ensure the
  change is accepted. record the current name so it can be restored
  later */
-  ASSERT(gethostname (origname, sizeof (origname)) == 0);
+  ASSERT (gethostname (origname, sizeof (origname)) == 0);
 
   /* try setting a valid hostname.  if it fails -1/ENOSYS, we will
  skip the test for long names as this is an indication we're using
@@ -71,10 +74,14 @@
   if (rcs != 0)
 {
   if (rcs == -1 && errno == ENOSYS)
-   return 0;
+{
+  fprintf (stderr,
+   "Skipping test: sethostname is not really implemented.\n");
+  return 77;
+}
   else
{
- printf ("error setting valid hostname.\n");
+ fprintf (stderr, "error setting valid hostname.\n");
  return 1;
}
 }
@@ -87,7 +94,7 @@
 properly changed. */
   if (strcmp (newname, TESTHOSTNAME) != 0)
{
- printf ("set/get comparison failed.\n");
+ fprintf (stderr, "set/get comparison failed.\n");
  return 1;
}
 }
@@ -100,18 +107,18 @@
 
   longname[i] = '\0';
 
-  rcs = sethostname ((const char *) longname, (HOST_NAME_MAX + 1));
+  rcs = sethostname (longname, (HOST_NAME_MAX + 1));
 
   if (rcs != -1)
 {
   /* attempt to restore the original name. */
-  sethostname (origname, strlen (origname));
-  printf ("

Re: spurious message about include files from `bootstrap' script

[Bruno Haible]

Hi Jim,

> Bruno, what do you think about making gnulib-tool warn about
> that syntax error?

You can not anticipate all kinds of typos that can occur. Similar things
would have happened if someone wrote

  Licence:

instead of

  License:

IMO warnings that rely on heuristics belong more into 'syntax-check' rules
of every project, rather than in gnulib-tool.

Bruno
-- 
In memoriam Rudolf Slánský 

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> +  int rcg, rcs, i;

I'm getting a warning

test-sethostname.c: In function ‘main’:
test-sethostname.c:52:7: warning: unused variable ‘rcg’ [-Wunused-variable]

The fix is obvious.


2011-12-03  Bruno Haible  

sethostname tests: Avoid a gcc warning.
* tests/test-sethostname.c (main): Remove an unused variable.

--- tests/test-sethostname.c.orig   Sat Dec  3 15:13:31 2011
+++ tests/test-sethostname.cSat Dec  3 15:05:32 2011
@@ -49,7 +49,7 @@
   char origname[HOST_NAME_MAX];
   char newname[HOST_NAME_MAX];
   char longname[HOST_NAME_MAX + 2];
-  int rcg, rcs, i;
+  int rcs, i;
 
   /* skip the tests if we don't have root privilege.  this does not
  consider things like CAP_SYS_ADMIN (linux) or PRIV_SYS_ADMIN
-- 
In memoriam Rudolf Slánský 

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> +#if !HAVE_GETEUID
> +# define geteuid() ((uid_t) -1)
> +#endif

On mingw, I'm getting a compilation error:

test-sethostname.c: In function `main':
test-sethostname.c:58: error: `uid_t' undeclared (first use in this function)
test-sethostname.c:58: error: (Each undeclared identifier is reported only once
test-sethostname.c:58: error: for each function it appears in.)

This fixes it, and enables testing sethostname() also on mingw.


2011-12-03  Bruno Haible  

sethostname tests: Fix compilation error on mingw.
* tests/test-sethostname.c: Don't include .
(geteuid): Use a dummy value without uid_t.

--- tests/test-sethostname.c.orig   Sat Dec  3 15:26:00 2011
+++ tests/test-sethostname.cSat Dec  3 15:25:57 2011
@@ -24,8 +24,6 @@
 
 /* for HOST_NAME_MAX */
 #include 
-/* for uid_t */
-#include 
 /* for strlen */
 #include 
 
@@ -36,11 +34,9 @@
 
 #define TESTHOSTNAME "gnulib-hostname"
 
-/* mingw and MSVC 9 lack geteuid, so setup a value that will indicate
-   we don't have root privilege since we wouldn't know whether to
-   expect success or failure when setting a name anyway*/
+/* mingw and MSVC 9 lack geteuid, so setup a dummy value.  */
 #if !HAVE_GETEUID
-# define geteuid() ((uid_t) -1)
+# define geteuid() 0
 #endif
 
 int

-- 
In memoriam Rudolf Slánský 

Re: spurious message about include files from `bootstrap' script

[Jim Meyering]

Bruno Haible wrote:

> Hi Jim,
>
>> Bruno, what do you think about making gnulib-tool warn about
>> that syntax error?
>
> You can not anticipate all kinds of typos that can occur. Similar things
> would have happened if someone wrote
>
>   Licence:
>
> instead of
>
>   License:
>
> IMO warnings that rely on heuristics belong more into 'syntax-check' rules
> of every project, rather than in gnulib-tool.

Hi Bruno,

Let me put it another way.
How about diagnosing a missing License: when
the module is not a -test one?

If you'd prefer not to do that, I can easily convert
this into a syntax-check:

$ git grep -L '^License:' modules|grep -ve '-tests$'
modules/.gitattributes
modules/COPYING
modules/README
modules/TEMPLATE-TESTS

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> +  ASSERT(gethostname (origname, sizeof (origname)) == 0);

On mingw, I'm seeing a warning and link error:

test-sethostname.c: In function `main':
test-sethostname.c:62: warning: implicit declaration of function `gethostname'
...
test-sethostname.o: In function `main':
/home/bruno/multibuild-1547/mingw2009/testdir1/gltests/test-sethostname.c:62: 
undefined reference to `_gethostname'
/home/bruno/multibuild-1547/mingw2009/testdir1/gltests/test-sethostname.c:85: 
undefined reference to `_gethostname'
collect2: ld returned 1 exit status

The trivial change would be to simply require the 'gethostname' module and
link with $(GETHOSTNAME_LIB). But then the test suite would not verify any more
that sethostname can be used without any -l options. The best way to continue
verifying this is through two test programs, one linked without options and one
linked with $(GETHOSTNAME_LIB).


2011-12-03  Bruno Haible  

sethostname tests: Fix link error on mingw.
* tests/test-sethostname1.c: New file, extracted from
tests/test-sethostname.c.
* tests/test-sethostname2.c: New file, extracted from
tests/test-sethostname.c.
* tests/test-sethostname.c: Remove file.
* modules/sethostname-tests (Files): Add tests/test-sethostname1.c,
tests/test-sethostname2.c. Remove tests/test-sethostname.c.
(Depends-on): Add gethostname.
(Makefile.am): Compile both test-sethostname1 and test-sethostname2.
Link the latter with $(GETHOSTNAME_LIB).

--- modules/sethostname-tests.orig  Sat Dec  3 15:52:10 2011
+++ modules/sethostname-tests   Sat Dec  3 15:42:48 2011
@@ -1,13 +1,16 @@
 Files:
-tests/test-sethostname.c
+tests/test-sethostname1.c
+tests/test-sethostname2.c
 tests/signature.h
 tests/macros.h
 
 Depends-on:
+gethostname
 
 configure.ac:
 AC_CHECK_FUNCS_ONCE([geteuid])
 
 Makefile.am:
-TESTS += test-sethostname
-check_PROGRAMS += test-sethostname
+TESTS += test-sethostname1 test-sethostname2
+check_PROGRAMS += test-sethostname1 test-sethostname2
+test_sethostname2_LDADD = $(LDADD) @GETHOSTNAME_LIB@

-- 
In memoriam Rudolf Slánský 

Re: spurious message about include files from `bootstrap' script

[Bruno Haible]

Jim Meyering wrote:
> How about diagnosing a missing License: when
> the module is not a -test one?

Neat idea. This one can be implemented without a heuristic. Done as follows:


2011-12-03  Bruno Haible  

gnulib-tool: Verify that the License field is present and non-empty.
* gnulib-tool (func_get_license_raw): New function, extracted from
func_get_license.
(func_get_license): Use it. Warn if the module is not a test module and
has no license.
Suggested by Jim Meyering.

--- gnulib-tool.origSat Dec  3 16:16:52 2011
+++ gnulib-tool Sat Dec  3 16:14:34 2011
@@ -2375,31 +2375,50 @@
   fi
 }
 
-# func_get_license module
+# func_get_license_raw module
 # Input:
 # - local_gnulib_dir  from --local-dir
 # - modcache  true or false, from --cache-modules/--no-cache-modules
-func_get_license ()
+func_get_license_raw ()
 {
-  {
-if ! $modcache; then
-  func_lookup_file "modules/$1"
-  sed -n -e "/^License$sed_extract_prog" < "$lookedup_file"
+  if ! $modcache; then
+func_lookup_file "modules/$1"
+sed -n -e "/^License$sed_extract_prog" < "$lookedup_file"
+  else
+func_cache_lookup_module "$1"
+# Output the field's value, including the final newline (if any).
+if $have_associative; then
+  if eval 'test -n "${modcache_license[$1]+set}"'; then
+eval 'echo "${modcache_license[$1]}"'
+  fi
 else
-  func_cache_lookup_module "$1"
-  # Output the field's value, including the final newline (if any).
-  if $have_associative; then
-if eval 'test -n "${modcache_license[$1]+set}"'; then
-  eval 'echo "${modcache_license[$1]}"'
-fi
-  else
-eval "field_set=\"\$${cachevar}_license_set\""
-if test -n "$field_set"; then
-  eval "field_value=\"\$${cachevar}_license\""
-  echo "${field_value}"
-fi
+  eval "field_set=\"\$${cachevar}_license_set\""
+  if test -n "$field_set"; then
+eval "field_value=\"\$${cachevar}_license\""
+echo "${field_value}"
   fi
 fi
+  fi
+}
+
+# func_get_license module
+# Input:
+# - local_gnulib_dir  from --local-dir
+# - modcache  true or false, from --cache-modules/--no-cache-modules
+func_get_license ()
+{
+  # Warn if the License field is missing.
+  case "$1" in
+*-tests ) ;;
+* )
+  license=`func_get_license_raw "$1"`
+  if test -z "$license"; then
+func_warning "module $1 lacks a License"
+  fi
+  ;;
+  esac
+  {
+func_get_license_raw "$1"
 # The default is GPL.
 echo "GPL"
   } | sed -e 's,^ *$,,' | sed -e 1q
-- 
In memoriam Rudolf Slánský 

arpg-help: Warning with NetBSD and amd64.

[Mats Erik Andersson]

Dear all,

I observe a warning for NetBSD 5.1 on amd64, a warning which is
absent for i386. Its effect is to disable two assertion statements.

  assert (num_entries <= SIZE_MAX / sizeof (struct hol_entry));

  CC argp-help.o
argp-help.c: In function 'make_hol':
argp-help.c:467: warning: comparison is always true due to limited range of 
data type
argp-help.c: In function 'hol_append':
argp-help.c:883: warning: comparison is always true due to limited range of 
data type
  CC argp-parse.o

A corresponding message does not appear for GNU/kFreeBSD, flavour amd64.


Best regards,
  Mats Erik Andersson, on behalf of GNU Inetutils.

Re: manywarnings and -f options

[Simon Josefsson]

Eric Blake  writes:

> On 12/02/2011 11:05 AM, Eric Blake wrote:
>> Should gl_MANYWARN_ALL_GCC be adding some gcc -f options?  For example,
>> at least gcc 4.3.4 -Wdisabled-optimization would emit a warning that
>> several other -W warnings are useless without -funit-at-a-time also in
>> place (see coreutils commit 5e361387d).  Also, according to 'info gcc,
>> -Wsuggest-attribute=pure only works if -fipa-pure-const is turned on
>> (true by default for -O compilation, but apparently the -W option can
>> also catch a few cases even without -O if -fipa-pure-const is manually
>> turned on).
>
> I also meant to add that -fdiagnostics-show-option is a must for
> determining which warnings are firing, when deciding which warnings to
> avoid.  Having these -f options added by default instead of making each
> client add them would make maintenance easier.

I already add -fdiagnostics-show-option to all my projects, so it seems
like a good idea to add to manywarnings.m4.

What does -funit-at-a-time really do?  My gcc 4.4 manual says:

`-funit-at-a-time'
 This option is left for compatibility reasons. `-funit-at-a-time'
 has no effect, while `-fno-unit-at-a-time' implies
 `-fno-toplevel-reorder' and `-fno-section-anchors'.

 Enabled by default.

The manual for -ipa-pure-const says:

`-fipa-pure-const'
 Discover which functions are pure or constant.  Enabled by default
 at `-O' and higher.

That seems harmless and as far I understand would not modify code
generation.

Considering this, I think that one could say generally that any -f*
parameter that does not modify code generation and may lead to more
warnings being emited are appropriate in manywarnings.m4.

/Simon

-Wvla vs dcnpgettext_expr's VLA decl

[Jim Meyering]

Hi Bruno,

This is in the context of manywarnings, so I'll write to bug-gnulib,
of course, you're welcome to add bug-gettext, if desired.

I tried enabling -Wvla for coreutils and found that it provoked hundreds
of warnings, all due to this:

../lib/gettext.h:262:3: error: variable length array 'msg_ctxt_id'\
is used [-Werror=vla]

That comes from this inline function from gettext.h:

static const char *
dcnpgettext_expr (const char *domain,
  const char *msgctxt, const char *msgid,
  const char *msgid_plural, unsigned long int n,
  int category)
{
  size_t msgctxt_len = strlen (msgctxt) + 1;
  size_t msgid_len = strlen (msgid) + 1;
  const char *translation;
#if _LIBGETTEXT_HAVE_VARIABLE_SIZE_ARRAYS
  char msg_ctxt_id[msgctxt_len + msgid_len];
#else
...

Do we have a guarantee that that array dimension is reasonable?

Have you compared the trade-offs of VLA-vs-malloc here?

Re: arpg-help: Warning with NetBSD and amd64.

[Paul Eggert]

On 12/03/11 07:35, Mats Erik Andersson wrote:
> argp-help.c:467: warning: comparison is always true due to limited range of 
> data type

Thanks, but in general we ignore warnings like that,
since they're almost always (as in this case) false alarms
when they appear in gnulib code.

Come to think of it, we can simplify the code in question
(at the cost of possibly generating more of these bogus warnings,
which is OK) like this:

diff --git a/lib/argp-help.c b/lib/argp-help.c
index ec7fcda..97557ea 100644
--- a/lib/argp-help.c
+++ b/lib/argp-help.c
@@ -463,8 +463,7 @@ make_hol (const struct argp *argp, struct hol_cluster 
*cluster)
   hol->short_options = malloc (num_short_options + 1);
 
   assert (hol->entries && hol->short_options);
-  if (SIZE_MAX <= UINT_MAX)
-assert (hol->num_entries <= SIZE_MAX / sizeof (struct hol_entry));
+  assert (hol->num_entries <= SIZE_MAX / sizeof (struct hol_entry));
 
   /* Fill in the entries.  */
   so = hol->short_options;
@@ -879,8 +878,7 @@ hol_append (struct hol *hol, struct hol *more)
 malloc (hol_so_len + strlen (more->short_options) + 1);
 
   assert (entries && short_options);
-  if (SIZE_MAX <= UINT_MAX)
-assert (num_entries <= SIZE_MAX / sizeof (struct hol_entry));
+  assert (num_entries <= SIZE_MAX / sizeof (struct hol_entry));
 
   __mempcpy (__mempcpy (entries, hol->entries,
 hol->num_entries * sizeof (struct hol_entry)),

Re: -Wvla vs dcnpgettext_expr's VLA decl

[Bruno Haible]

Hi Jim,

> I tried enabling -Wvla for coreutils and found that it provoked hundreds
> of warnings, all due to this:
> 
> ../lib/gettext.h:262:3: error: variable length array 'msg_ctxt_id'\
> is used [-Werror=vla]
> 
> That comes from this inline function from gettext.h:
> 
> static const char *
> dcnpgettext_expr (const char *domain,
>   const char *msgctxt, const char *msgid,
>   const char *msgid_plural, unsigned long int n,
>   int category)
> {
>   size_t msgctxt_len = strlen (msgctxt) + 1;
>   size_t msgid_len = strlen (msgid) + 1;
>   const char *translation;
> #if _LIBGETTEXT_HAVE_VARIABLE_SIZE_ARRAYS
>   char msg_ctxt_id[msgctxt_len + msgid_len];
> #else
> ...

Yes, as you can see this code uses variable-length arrays only
when the compiler supports it.

> Do we have a guarantee that that array dimension is reasonable?

Yes. While msgctxt and msgid normally rarely exceed 1 KB (because
of the principle to split at paragraph boundaries, so that translators
can compare an old and a new msgid, for msgids with plural, the
string is most often only a single sentence, that is, rarely larger
than 200 bytes.

> Have you compared the trade-offs of VLA-vs-malloc here?

Yes I did.

The problem is that the compiler does not know that I did.

You may want to propose a #pragma diagnostic ignore patch.

Bruno
-- 
In memoriam Rudolf Slánský 

Re: spurious message about include files from `bootstrap' script

[Jim Meyering]

Bruno Haible wrote:
> Jim Meyering wrote:
>> How about diagnosing a missing License: when
>> the module is not a -test one?
>
> Neat idea. This one can be implemented without a heuristic. Done as follows:

Thanks!
>
> 2011-12-03  Bruno Haible  
>
>   gnulib-tool: Verify that the License field is present and non-empty.
>   * gnulib-tool (func_get_license_raw): New function, extracted from
>   func_get_license.
>   (func_get_license): Use it. Warn if the module is not a test module and
>   has no license.
>   Suggested by Jim Meyering.

Re: manywarnings and -f options

[Eric Blake]

On 12/03/2011 09:00 AM, Simon Josefsson wrote:
> What does -funit-at-a-time really do?  My gcc 4.4 manual says:
> 
> `-funit-at-a-time'
>  This option is left for compatibility reasons. `-funit-at-a-time'
>  has no effect, while `-fno-unit-at-a-time' implies
>  `-fno-toplevel-reorder' and `-fno-section-anchors'.
> 
>  Enabled by default.

That's the case for 4.4 and later.  But in gcc 4.3, it was not
unconditionally enabled, and as I said earlier, at least coreutils ran
into a situation where gcc 4.3. failed to compile at -Werror because
-Wdisabled-optimization warned that -fno-unit-at-a-time was required,
which warning turned into an error.

At this point, gcc 4.3 is slowly phasing out; most Linux distros and
Cygwin have moved on to newer compilers, where the problem is less
likely to happen.

> 
> The manual for -ipa-pure-const says:
> 
> `-fipa-pure-const'
>  Discover which functions are pure or constant.  Enabled by default
>  at `-O' and higher.
> 
> That seems harmless and as far I understand would not modify code
> generation.

It doesn't modify code generation, but it DOES modify warning
generation, and in a way that negatively interacts with libtool:
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10197

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: -Wvla vs dcnpgettext_expr's VLA decl

[Jim Meyering]

Bruno Haible wrote:

> Hi Jim,
>
>> I tried enabling -Wvla for coreutils and found that it provoked hundreds
>> of warnings, all due to this:
>>
>> ../lib/gettext.h:262:3: error: variable length array 'msg_ctxt_id'\
>> is used [-Werror=vla]
>>
>> That comes from this inline function from gettext.h:
>>
>> static const char *
>> dcnpgettext_expr (const char *domain,
>>   const char *msgctxt, const char *msgid,
>>   const char *msgid_plural, unsigned long int n,
>>   int category)
>> {
>>   size_t msgctxt_len = strlen (msgctxt) + 1;
>>   size_t msgid_len = strlen (msgid) + 1;
>>   const char *translation;
>> #if _LIBGETTEXT_HAVE_VARIABLE_SIZE_ARRAYS
>>   char msg_ctxt_id[msgctxt_len + msgid_len];
>> #else
>> ...
>
> Yes, as you can see this code uses variable-length arrays only
> when the compiler supports it.
>
>> Do we have a guarantee that that array dimension is reasonable?
>
> Yes. While msgctxt and msgid normally rarely exceed 1 KB (because

That sounds like convention.  Is there a guarantee?

> of the principle to split at paragraph boundaries, so that translators
> can compare an old and a new msgid, for msgids with plural, the
> string is most often only a single sentence, that is, rarely larger
> than 200 bytes.

What I was wondering is whether there were some guard, say
in all callers, that would prevent VLA abuse.

>> Have you compared the trade-offs of VLA-vs-malloc here?
>
> Yes I did.
>
> The problem is that the compiler does not know that I did.
>
> You may want to propose a #pragma diagnostic ignore patch.

Such a #pragma affects a compilation unit, so putting it
in the .h file would affect every .c file that includes it.
Putting such a #pragma in every file from which gettext.h
is included is not really an option.

I can live with the status quo, but it would make -Wvla more useful
not to put such constructs in commonly-included header files.

Re: manywarnings and -f options

[Jim Meyering]

Eric Blake wrote:
> On 12/03/2011 09:00 AM, Simon Josefsson wrote:
>> What does -funit-at-a-time really do?  My gcc 4.4 manual says:
>>
>> `-funit-at-a-time'
>>  This option is left for compatibility reasons. `-funit-at-a-time'
>>  has no effect, while `-fno-unit-at-a-time' implies
>>  `-fno-toplevel-reorder' and `-fno-section-anchors'.
>>
>>  Enabled by default.
>
> That's the case for 4.4 and later.  But in gcc 4.3, it was not
> unconditionally enabled, and as I said earlier, at least coreutils ran
> into a situation where gcc 4.3. failed to compile at -Werror because
> -Wdisabled-optimization warned that -fno-unit-at-a-time was required,
> which warning turned into an error.
>
> At this point, gcc 4.3 is slowly phasing out; most Linux distros and
> Cygwin have moved on to newer compilers, where the problem is less
> likely to happen.

IMHO, we should treat --enable-gcc-warnings as something that must work
well with the latest stable version of gcc (currently 4.6) and recent
glibc headers.  Trying to accommodate older versions of gcc does not seem
worthwhile.  Just tell people who use old versions of gcc not to use
--enable-gcc-warnings, or even detect that and turn it off automatically.