Re: [PATCH] file-has-acl: avoid warning from bleeding-edge GCC

[Jim Meyering]

On Sun, May 28, 2023 at 10:44 AM Bruno Haible  wrote:
> How about adding -Wno-suggest-attribute to the list of warning options that
> are part of the GL_CFLAG_GNULIB_WARNINGS variable, no that such warnings
>   function might be candidate for attribute 'pure'
> or
>   function might be candidate for attribute 'const'
> no longer occur in gnulib source files, compiled as part of other packages?
>
> This would not only silence the warnings about static functions [1][2],
> but also about the global function definitions. Leading to slightly
> worse code generation by gcc in the long term.
>
> Paul, what do you think? Should we do that?
>
> Bruno
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914
> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109915

That would also solve the problem, but at a potential code-gen cost,
as you imply: code-gen for a few rare new and modified functions may
end up being degraded. I would endorse this approach if that GCC bug
is still unresolved in say October.

Re: [PATCH] file-has-acl: avoid warning from bleeding-edge GCC

[Jim Meyering]

On Sun, May 28, 2023 at 10:08 AM Bruno Haible  wrote:
>
> Hi Jim,
>
> Jim Meyering wrote:
> > FYI, just pushed, to avoid this:
> >
> >   lib/file-has-acl.c: In function 'have_xattr':
> >   lib/file-has-acl.c:54:1: error: function might be candidate for attribute 
> > 'pure' if it is known to return normally [-Werror=suggest-attribute=pure]
> >  54 | have_xattr (char const *attr, char const *listbuf, ssize_t 
> > listsize)
> >
>
> Just a week ago, Paul wrote [1]:
>   "it's not helpful for GCC to issue -Wsuggest-attribute diagnostics for 
> static
>functions. If GCC has already figured out that the function is pure or 
> const
>or whatever then that's good enough: GCC shouldn't badger the programmer to
>complicate the program to record something that GCC can easily calculate 
> for
>itself.
>
>This sounds like GCC bug 85734, which has been marked as fixed. Evidently
>it's rearing its ugly head again. We should fix the bug rather than pollute
>the code with compiler pacifications."
>
> and then I reported this precise warning as a GCC bug. [2]
>
> Bruno
>
> [1] https://lists.gnu.org/archive/html/bug-gnulib/2023-05/msg00139.html
> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

Hi Bruno,

I did see those, but in the meantime, it's a tiny change that lets us
continue to enable warnings when building on the latest systems. I'll
be happy to revert it the moment GCC's behavior changes.

Re: [PATCH] file-has-acl: avoid warning from bleeding-edge GCC

[Bruno Haible]

How about adding -Wno-suggest-attribute to the list of warning options that
are part of the GL_CFLAG_GNULIB_WARNINGS variable, no that such warnings
  function might be candidate for attribute 'pure'
or
  function might be candidate for attribute 'const'
no longer occur in gnulib source files, compiled as part of other packages?

This would not only silence the warnings about static functions [1][2],
but also about the global function definitions. Leading to slightly
worse code generation by gcc in the long term.

Paul, what do you think? Should we do that?

Bruno

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109915

warnings, manywarnings: Assume autoconf >= 2.64

[Bruno Haible]

Gnulib assumes autoconf >= 2.64 since August 2000.
Autoconf 2.64 added the macro AS_VAR_APPEND [1].
Thus, the gl_AS_VAR_APPEND macro, that is an alias of AS_VAR_APPEND, is
no longer needed.

Other packages than gnulib don't use this macro. [2]

Therefore I'm applying this simplification.

[1] https://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=NEWS#l960
[2] 
https://codesearch.debian.net/search?q=gl_AS_VAR_APPEND+-path%3A%2Fwarnings.m4+-path%3A%2Fmanywarnings.m4=1


2023-05-28  Bruno Haible  

warnings, manywarnings: Assume autoconf >= 2.64.
* m4/warnings.m4 (gl_AS_VAR_APPEND): Remove macro.
(gl_COMPILER_OPTION_IF, gl_WARN_ADD): Use AS_VAR_APPEND instead of
gl_AS_VAR_APPEND.
* m4/manywarnings.m4: Likewise.

diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 7792b4f3b4..a06f26f672 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -1,4 +1,4 @@
-# manywarnings.m4 serial 23
+# manywarnings.m4 serial 24
 dnl Copyright (C) 2008-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -6,6 +6,8 @@
 
 dnl From Simon Josefsson
 
+AC_PREREQ([2.64])
+
 # gl_MANYWARN_COMPLEMENT(OUTVAR, LISTVAR, REMOVEVAR)
 # --
 # Copy LISTVAR to OUTVAR except for the entries in REMOVEVAR.
@@ -21,7 +23,7 @@ AC_DEFUN([gl_MANYWARN_COMPLEMENT]
   *" $gl_warn_item "*)
 ;;
   *)
-gl_AS_VAR_APPEND([gl_warn_set], [" $gl_warn_item"])
+AS_VAR_APPEND([gl_warn_set], [" $gl_warn_item"])
 ;;
 esac
   done
@@ -148,51 +150,51 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC(C
 -Wwrite-strings \
 \
 ; do
-gl_AS_VAR_APPEND([$1], [" $gl_manywarn_item"])
+AS_VAR_APPEND([$1], [" $gl_manywarn_item"])
   done
 
   # gcc --help=warnings outputs an unusual form for these options; list
   # them here so that the above 'comm' command doesn't report a false match.
-  gl_AS_VAR_APPEND([$1], [' -Warray-bounds=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wattribute-alias=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wbidi-chars=any,ucn'])
-  gl_AS_VAR_APPEND([$1], [' -Wformat-overflow=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wformat=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wformat-truncation=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wimplicit-fallthrough=5'])
-  gl_AS_VAR_APPEND([$1], [' -Wshift-overflow=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wuse-after-free=3'])
-  gl_AS_VAR_APPEND([$1], [' -Wunused-const-variable=2'])
-  gl_AS_VAR_APPEND([$1], [' -Wvla-larger-than=4031'])
+  AS_VAR_APPEND([$1], [' -Warray-bounds=2'])
+  AS_VAR_APPEND([$1], [' -Wattribute-alias=2'])
+  AS_VAR_APPEND([$1], [' -Wbidi-chars=any,ucn'])
+  AS_VAR_APPEND([$1], [' -Wformat-overflow=2'])
+  AS_VAR_APPEND([$1], [' -Wformat=2'])
+  AS_VAR_APPEND([$1], [' -Wformat-truncation=2'])
+  AS_VAR_APPEND([$1], [' -Wimplicit-fallthrough=5'])
+  AS_VAR_APPEND([$1], [' -Wshift-overflow=2'])
+  AS_VAR_APPEND([$1], [' -Wuse-after-free=3'])
+  AS_VAR_APPEND([$1], [' -Wunused-const-variable=2'])
+  AS_VAR_APPEND([$1], [' -Wvla-larger-than=4031'])
 
   # These are needed for older GCC versions.
   if test -n "$GCC" && gl_gcc_version=`($CC --version) 2>/dev/null`; then
 case $gl_gcc_version in
   'gcc (GCC) '[[0-3]].* | \
   'gcc (GCC) '4.[[0-7]].*)
-gl_AS_VAR_APPEND([$1], [' -fdiagnostics-show-option'])
-gl_AS_VAR_APPEND([$1], [' -funit-at-a-time'])
+AS_VAR_APPEND([$1], [' -fdiagnostics-show-option'])
+AS_VAR_APPEND([$1], [' -funit-at-a-time'])
   ;;
 esac
 case $gl_gcc_version in
   'gcc (GCC) '[[0-9]].*)
-gl_AS_VAR_APPEND([$1], [' -fno-common'])
+AS_VAR_APPEND([$1], [' -fno-common'])
   ;;
 esac
   fi
 
   # Disable specific options as needed.
   if test "$gl_cv_cc_nomfi_needed" = yes; then
-gl_AS_VAR_APPEND([$1], [' -Wno-missing-field-initializers'])
+AS_VAR_APPEND([$1], [' -Wno-missing-field-initializers'])
   fi
 
   if test "$gl_cv_cc_uninitialized_supported" = no; then
-gl_AS_VAR_APPEND([$1], [' -Wno-uninitialized'])
+AS_VAR_APPEND([$1], [' -Wno-uninitialized'])
   fi
 
   # This warning have too many false alarms in GCC 11.2.1.
   # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101713
-  gl_AS_VAR_APPEND([$1], [' -Wno-analyzer-malloc-leak'])
+  AS_VAR_APPEND([$1], [' -Wno-analyzer-malloc-leak'])
 
   AC_LANG_POP([C])
 ])
diff --git a/m4/warnings.m4 b/m4/warnings.m4
index 063bc5ca64..76e97907b5 100644
--- a/m4/warnings.m4
+++ b/m4/warnings.m4
@@ -1,4 +1,4 @@
-# warnings.m4 serial 16
+# warnings.m4 serial 17
 dnl Copyright (C) 2008-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -6,14 +6,7 @@
 
 dnl From Simon Josefsson
 
-# gl_AS_VAR_APPEND(VAR, VALUE)
-# 
-# Provide the functionality of AS_VAR_APPEND if Autoconf does not have it.

Re: [PATCH] file-has-acl: avoid warning from bleeding-edge GCC

[Bruno Haible]

Hi Jim,

Jim Meyering wrote:
> FYI, just pushed, to avoid this:
> 
>   lib/file-has-acl.c: In function 'have_xattr':
>   lib/file-has-acl.c:54:1: error: function might be candidate for attribute 
> 'pure' if it is known to return normally [-Werror=suggest-attribute=pure]
>  54 | have_xattr (char const *attr, char const *listbuf, ssize_t listsize)
> 

Just a week ago, Paul wrote [1]:
  "it's not helpful for GCC to issue -Wsuggest-attribute diagnostics for static
   functions. If GCC has already figured out that the function is pure or const
   or whatever then that's good enough: GCC shouldn't badger the programmer to
   complicate the program to record something that GCC can easily calculate for
   itself.

   This sounds like GCC bug 85734, which has been marked as fixed. Evidently
   it's rearing its ugly head again. We should fix the bug rather than pollute
   the code with compiler pacifications."

and then I reported this precise warning as a GCC bug. [2]

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2023-05/msg00139.html
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

[PATCH] file-has-acl: avoid warning from bleeding-edge GCC

[Jim Meyering]

FYI, just pushed, to avoid this:

  lib/file-has-acl.c: In function 'have_xattr':
  lib/file-has-acl.c:54:1: error: function might be candidate for attribute 
'pure' if it is known to return normally [-Werror=suggest-attribute=pure]
 54 | have_xattr (char const *attr, char const *listbuf, ssize_t listsize)

>From 2bd4c8dc13de3121d76dad755a255c8e92fd46a7 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Sun, 28 May 2023 09:41:08 -0700
Subject: [PATCH] file-has-acl: avoid warning from bleeding-edge GCC

* lib/file-has-acl.c: Include attribute.h.
(have_xattr): Declare with ATTRIBUTE_PURE,
to avoid new warning from GCC14-to-be.
* modules/file-has-acl (Depends-on): Add attribute.
Spotted while building coreutils with this:
gcc version 14.0.0 20230526 (experimental)
---
 ChangeLog| 10 ++
 lib/file-has-acl.c   |  4 ++--
 modules/file-has-acl |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e3a2507c47..93682f526b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2023-05-28  Jim Meyering  
+
+	file-has-acl: avoid warning from bleeding-edge GCC
+	* lib/file-has-acl.c: Include attribute.h.
+	(have_xattr): Declare with ATTRIBUTE_PURE,
+	to avoid new warning from GCC14-to-be.
+	* modules/file-has-acl (Depends-on): Add attribute.
+	Spotted while building coreutils with this:
+	gcc version 14.0.0 20230526 (experimental)
+
 2023-05-28  Bruno Haible  

 	error: Avoid -Wint-in-bool-context warning.
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 4cddc80bd1..13f08c3055 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -28,7 +28,7 @@
 #include "acl.h"

 #include "acl-internal.h"
-
+#include "attribute.h"
 #include "minmax.h"

 #if USE_ACL && HAVE_LINUX_XATTR_H && HAVE_LISTXATTR
@@ -50,7 +50,7 @@ enum {
 /* Return true if ATTR is in the set represented by the NUL-terminated
strings in LISTBUF, which is of size LISTSIZE.  */

-static bool
+ATTRIBUTE_PURE static bool
 have_xattr (char const *attr, char const *listbuf, ssize_t listsize)
 {
   char const *blim = listbuf + listsize;
diff --git a/modules/file-has-acl b/modules/file-has-acl
index 199fccd6ca..ec93313dfd 100644
--- a/modules/file-has-acl
+++ b/modules/file-has-acl
@@ -8,6 +8,7 @@ m4/acl.m4

 Depends-on:
 acl-permissions
+attribute
 extern-inline
 minmax
 free-posix
-- 
2.41.0.rc2

Re: error: Support the compiler's control flow analysis better

[Bruno Haible]

Pádraig Brady wrote:
> >> We might need to cast STATUS to bool to avoid the
> >> following failure from coreutils CI:

What you call a failure is in fact a warning. Adding -Werror is your
responsibility.

> >> In file included from src/die.h:22,
> >> from src/chroot.c:27:
> >> src/chroot.c: In function 'main':
> >> src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
> >> [-Werror=int-in-bool-context]
> >>  362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
> >> ./lib/error.h:422:33: note: in definition of macro 'error'
> >>  422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : 
> >> (void)0)
> >>  | ^~
> > 
> > Actually casting with (bool), or using !! does NOT help here.
> To avoid this one can use `(status) != 0`.

What this code is meant to do is to test status against 0. Writing it
like this is the obvious fix. Done as shown below.

> There still is a gotcha (hit in dd.c in coreutils)
> where if you define an error macro yourself
> you get a macro redefinition error,

You can #define _GL_NO_INLINE_ERROR if you don't like gnulib's override.


2023-05-28  Bruno Haible  

error: Avoid -Wint-in-bool-context warning.
Reported by Pádraig Brady in
.
* lib/error.in.h (error, error_at_line): Use 'status != 0', since status
is expected to be an int, not a bool value.

diff --git a/lib/error.in.h b/lib/error.in.h
index 4bf191e102..70fb132133 100644
--- a/lib/error.in.h
+++ b/lib/error.in.h
@@ -69,7 +69,7 @@ _GL_CXXALIAS_RPL (error, void,
 # ifndef _GL_NO_INLINE_ERROR
 #  undef error
 #  define error(status, ...) \
- ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+ ((rpl_error)(0, __VA_ARGS__), (status) != 0 ? exit (status) : (void)0)
 # endif
 #else
 # if ! @HAVE_ERROR@
@@ -81,7 +81,7 @@ _GL_CXXALIAS_SYS (error, void,
   (int __status, int __errnum, const char *__format, ...));
 # ifndef _GL_NO_INLINE_ERROR
 #  define error(status, ...) \
- ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+ ((error)(0, __VA_ARGS__), (status) != 0 ? exit (status) : (void)0)
 # endif
 #endif
 #if __GLIBC__ >= 2
@@ -105,7 +105,7 @@ _GL_CXXALIAS_RPL (error_at_line, void,
 # ifndef _GL_NO_INLINE_ERROR
 #  undef error_at_line
 #  define error_at_line(status, ...) \
- ((rpl_error_at_line)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+ ((rpl_error_at_line)(0, __VA_ARGS__), (status) != 0 ? exit (status) : 
(void)0)
 # endif
 #else
 # if ! @HAVE_ERROR_AT_LINE@
@@ -119,7 +119,7 @@ _GL_CXXALIAS_SYS (error_at_line, void,
unsigned int __lineno, const char *__format, ...));
 # ifndef _GL_NO_INLINE_ERROR
 #  define error_at_line(status, ...) \
- ((error_at_line)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+ ((error_at_line)(0, __VA_ARGS__), (status) != 0 ? exit (status) : (void)0)
 # endif
 #endif
 _GL_CXXALIASWARN (error_at_line);

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 28/05/2023 13:50, Pádraig Brady wrote:

On 28/05/2023 13:31, Pádraig Brady wrote:

On 27/05/2023 21:53, Bruno Haible wrote:

+# ifndef _GL_NO_INLINE_ERROR
+#  undef error
+#  define error(status, ...) \
+ ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+# endif


We might need to cast STATUS to bool to avoid the
following failure from coreutils CI:

In file included from src/die.h:22,
from src/chroot.c:27:
src/chroot.c: In function 'main':
src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
[-Werror=int-in-bool-context]
 362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
./lib/error.h:422:33: note: in definition of macro 'error'
 422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
 | ^~


Actually casting with (bool), or using !! does NOT help here.

It looks to be due to the nested use of ?: that's triggering the issue.

$ gcc --version
gcc (Debian 10.2.1-6) 10.2.1 20210110


To avoid this one can use `(status) != 0`.

There still is a gotcha (hit in dd.c in coreutils)
where if you define an error macro yourself
you get a macro redefinition error,
but that's something we can/should handle on the coreutils side anyway.

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 28/05/2023 13:31, Pádraig Brady wrote:

On 27/05/2023 21:53, Bruno Haible wrote:

+# ifndef _GL_NO_INLINE_ERROR
+#  undef error
+#  define error(status, ...) \
+ ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+# endif


We might need to cast STATUS to bool to avoid the
following failure from coreutils CI:

In file included from src/die.h:22,
   from src/chroot.c:27:
src/chroot.c: In function 'main':
src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
[-Werror=int-in-bool-context]
362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
./lib/error.h:422:33: note: in definition of macro 'error'
422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
| ^~


Actually casting with (bool), or using !! does NOT help here.

It looks to be due to the nested use of ?: that's triggering the issue.

$ gcc --version
gcc (Debian 10.2.1-6) 10.2.1 20210110

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 27/05/2023 21:53, Bruno Haible wrote:

Paul Eggert wrote:

Maybe by defining
error and error_at_line as inline functions


They're defined by glibc, no? The definitions might collide.


Yes, they are defined in glibc. Overriding functions without causing
collisions is our core expertise here :)


Also, I suppose the compiler might not inline them


Indeed, when I attempt to define
   void inline_error (int status, int errnum, const char *fmt, ...) { ... }
gcc does not inline the function, because it never inlines varargs functions.

An alternative would be to define
   void inline_error (int status, int errnum, const char *message) { ... }
and pass xasprintf (fmt, ...) as message argument. But the out-of-memory
handling or the LGPLv2+ / GPL license difference causes problems.

Fortunately, we don't need an inline function: Jim's die() implementation
shows that it can be done with just a macro definition.


The basic problem is that the old 'error' API doesn't mix well with
[[noreturn]] and the like. We could write a new function, "eexit" say,
that behaves like "error" except it never returns. (I chose the name
"eexit" so as to not mess up the indenting of existing code.)

Or we could just live with "die", as it works.


While this is definitely working, it has the problem that it pulls
developers away from the glibc API. Things are easier for developers
if they can use the symbols from POSIX or glibc rather than gnulib-
invented symbols. And that is possible here.

Also, the name 'die' has the problem that it may invoke traumatic
memories in some people (like the verb 'kill', but we can't get rid
of this one since it's standardized).



+# ifndef _GL_NO_INLINE_ERROR
+#  undef error
+#  define error(status, ...) \
+ ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+# endif


We might need to cast STATUS to bool to avoid the
following failure from coreutils CI:

In file included from src/die.h:22,
 from src/chroot.c:27:
src/chroot.c: In function 'main':
src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
[-Werror=int-in-bool-context]
  362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
./lib/error.h:422:33: note: in definition of macro 'error'
  422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
  | ^~


cheers,
Pádraig