Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
I did:
> (error): Define as a macro that explicitly invokes exit().
> (error_at_line): Likewise.
Oops. That patch broke the library namespacing via config.h. Namely,
when a package uses a config.h definition such as
#define error libgettextpo_error
the intent is that the Gnulib module defines a function named
'libgettextpo_error', not 'error'.
With the error.in.h change, I now get redefinition warnings and compilation
errors (on platforms other than glibc and cygwin) such as
./error.h:458:11: warning: 'error' macro redefined [-Wmacro-redefined]
# define error(status, ...) \
^
./config.h:51:9: note: previous definition is here
#define error libgettextpo_error
^
...
../../../gettext-tools/libgettextpo/gettext-po.c:1326:5: error: use of
undeclared identifier 'error'
error (EXIT_FAILURE, 0, _("memory exhausted"));
^
./error.h:459:23: note: expanded from macro 'error'
__gl_error_call (error, status, __VA_ARGS__)
^
The fix is:
1) Capture the name of the error function through an static function.
2) Since this static function is a varargs function and needs to call a
varargs function, it needs to invoke __builtin_va_arg_pack().
(The alternative would be to add a dependency from 'error' to the
'verror' module. Which is not really better...)
3) Since __builtin_va_arg_pack is only allowed in always-inline function,
we need to mark it as _GL_ATTRIBUTE_ALWAYS_INLINE.
4) The compiler then warns that the inlining may fail. To silence this
warning, we need a '#pragma GCC diagnostic ignored "-Wattributes"'.
That's more involved than what I had expected.
2023-06-04 Bruno Haible
error: Fix support for library namespacing (regression 2023-05-27).
* lib/error.in.h (error): If error is defined as a macro, define a
static inline function _gl_inline_error that invokes it, and let the
new error macro invoke that function.
(error_at_line): If error_at_line is defined as a macro, define a static
inline function _gl_inline_error_at_line that invokes it, and let the
new error_at_line macro invoke that function.
diff --git a/lib/error.in.h b/lib/error.in.h
index ef4b3c3815..94477fde08 100644
--- a/lib/error.in.h
+++ b/lib/error.in.h
@@ -30,7 +30,8 @@
#ifndef _@GUARD_PREFIX@_ERROR_H
#define _@GUARD_PREFIX@_ERROR_H
-/* This file uses _GL_ATTRIBUTE_FORMAT. */
+/* This file uses _GL_ATTRIBUTE_ALWAYS_INLINE, _GL_ATTRIBUTE_FORMAT,
+ _GL_ATTRIBUTE_MAYBE_UNUSED. */
#if !_GL_CONFIG_H_INCLUDED
#error "Please include config.h first."
#endif
@@ -108,8 +109,28 @@ _GL_FUNCDECL_SYS (error, void,
_GL_CXXALIAS_SYS (error, void,
(int __status, int __errnum, const char *__format, ...));
# ifndef _GL_NO_INLINE_ERROR
-# define error(status, ...) \
- __gl_error_call (error, status, __VA_ARGS__)
+# ifdef error
+/* Only gcc ≥ 4.7 has __builtin_va_arg_pack. */
+# if _GL_GNUC_PREREQ (4, 7)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+_GL_ATTRIBUTE_MAYBE_UNUSED
+static void
+_GL_ATTRIBUTE_ALWAYS_INLINE
+_GL_ATTRIBUTE_FORMAT ((_GL_ATTRIBUTE_SPEC_PRINTF_ERROR, 3, 4))
+_gl_inline_error (int __status, int __errnum, const char *__format, ...)
+{
+ return error (__status, __errnum, __format, __builtin_va_arg_pack ());
+}
+#pragma GCC diagnostic pop
+#undef error
+#define error(status, ...) \
+ __gl_error_call (_gl_inline_error, status, __VA_ARGS__)
+# endif
+# else
+# define error(status, ...) \
+ __gl_error_call (error, status, __VA_ARGS__)
+# endif
# endif
#endif
#if __GLIBC__ >= 2
@@ -146,8 +167,30 @@ _GL_CXXALIAS_SYS (error_at_line, void,
(int __status, int __errnum, const char *__filename,
unsigned int __lineno, const char *__format, ...));
# ifndef _GL_NO_INLINE_ERROR
-# define error_at_line(status, ...) \
- __gl_error_call (error_at_line, status, __VA_ARGS__)
+# ifdef error_at_line
+/* Only gcc ≥ 4.7 has __builtin_va_arg_pack. */
+# if _GL_GNUC_PREREQ (4, 7)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+_GL_ATTRIBUTE_MAYBE_UNUSED
+static void
+_GL_ATTRIBUTE_ALWAYS_INLINE
+_GL_ATTRIBUTE_FORMAT ((_GL_ATTRIBUTE_SPEC_PRINTF_ERROR, 5, 6))
+_gl_inline_error_at_line (int __status, int __errnum, const char *__filename,
+ unsigned int __lineno, const char *__format, ...)
+{
+ return error_at_line (__status, __errnum, __filename, __lineno, __format,
+__builtin_va_arg_pack ());
+}
+#pragma GCC diagnostic pop
+#undef error_at_line
+#define error_at_line(status, ...) \
+ __gl_error_call (_gl_inline_error_at_line, status, __VA_ARGS__)
+# endif
+# else
+# define error_at_line(status, ...) \
+ __gl_error_call (error_at_line, status, __VA_ARGS__)
+# endif
# endif
#endif
_GL_CXXALIASWARN (error_at_line);
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
Paul Eggert wrote: > > How about changing this to use __builtin_constant_p instead of > > __OPTIMIZE__? I.e., use the comma expression if __builtin_constant_p > > (status), and use the statement expression otherwise. > > I gave that a shot by installing the attached. Oh, I see now what you meant. That's clever. So clever that it deserves comments :-) 2023-06-03 Bruno Haible error: Improve comments. * lib/error.in.h (__gl_error_call): Add more comments. diff --git a/lib/error.in.h b/lib/error.in.h index 279258f63e..ef4b3c3815 100644 --- a/lib/error.in.h +++ b/lib/error.in.h @@ -50,13 +50,21 @@ #endif /* Helper macro for supporting the compiler's control flow analysis better. - It evaluates its arguments only once. It uses __builtin_constant_p - and comma expressions to work around GCC false positives. + It evaluates its arguments only once. Test case: Compile copy-file.c with "gcc -Wimplicit-fallthrough". */ #ifdef __GNUC__ +/* Use 'unreachable' to tell the compiler when the function call does not + return. */ # define __gl_error_call1(function, status, ...) \ ((function) (status, __VA_ARGS__), \ (status) != 0 ? unreachable () : (void) 0) +/* If STATUS is a not a constant, the function call may or may not return; + therefore -Wimplicit-fallthrough will produce a warning. Use a compound + statement in order to evaluate STATUS only once. + If STATUS is a constant, we don't use a compound statement, because that + would trigger a -Wimplicit-fallthrough warning even when STATUS is != 0, + when not optimizing. This causes STATUS to be evaluated twice, but + that's OK since it does not have side effects. */ # define __gl_error_call(function, status, ...) \ (__builtin_constant_p (status) \ ? __gl_error_call1 (function, status, __VA_ARGS__) \
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 2023-06-02 15:03, Paul Eggert wrote:
How about changing this to use __builtin_constant_p instead of
__OPTIMIZE__? I.e., use the comma expression if __builtin_constant_p
(status), and use the statement expression otherwise.
I gave that a shot by installing the attached.From cfd41b3ac199a4f95fb78ee251e8c8a7c6f86ad1 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Fri, 2 Jun 2023 22:30:52 -0700
Subject: [PATCH] error: do not evaluate status twice
Do this in a different way, so that the status is evaluated
once even when not optimizing and when using GCC.
* lib/error.in.h (__gl_error_call1) [__GNUC__]: New macro.
(__gl_error_call) [__GNUC__]: Use it.
---
ChangeLog | 8
lib/error.in.h | 27 ---
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 3187128c1f..fda2ed0e3f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2023-06-02 Paul Eggert
+
+ error: do not evaluate status twice
+ Do this in a different way, so that the status is evaluated
+ once even when not optimizing and when using GCC.
+ * lib/error.in.h (__gl_error_call1) [__GNUC__]: New macro.
+ (__gl_error_call) [__GNUC__]: Use it.
+
2023-06-02 Bruno Haible
warnings: Add ability to inhibit all warnings.
diff --git a/lib/error.in.h b/lib/error.in.h
index f37dba170c..279258f63e 100644
--- a/lib/error.in.h
+++ b/lib/error.in.h
@@ -50,23 +50,20 @@
#endif
/* Helper macro for supporting the compiler's control flow analysis better.
+ It evaluates its arguments only once. It uses __builtin_constant_p
+ and comma expressions to work around GCC false positives.
Test case: Compile copy-file.c with "gcc -Wimplicit-fallthrough". */
#ifdef __GNUC__
-/* Avoid evaluating STATUS twice, if this is possible without making the
- "warning: this statement may fall through" reappear. */
-# if __OPTIMIZE__
-# define __gl_error_call(function, status, ...) \
- ({ \
-int const __errstatus = (status); \
-(function) (__errstatus, __VA_ARGS__); \
-__errstatus != 0 ? unreachable () : (void) 0; \
- })
-# else
-# define __gl_error_call(function, status, ...) \
- ((function) (status, __VA_ARGS__), \
- (status) != 0 ? unreachable () : (void)0 \
- )
-# endif
+# define __gl_error_call1(function, status, ...) \
+((function) (status, __VA_ARGS__), \
+ (status) != 0 ? unreachable () : (void) 0)
+# define __gl_error_call(function, status, ...) \
+(__builtin_constant_p (status) \
+ ? __gl_error_call1 (function, status, __VA_ARGS__) \
+ : ({ \
+ int const __errstatus = status; \
+ __gl_error_call1 (function, __errstatus, __VA_ARGS__); \
+ }))
#else
# define __gl_error_call(function, status, ...) \
(function) (status, __VA_ARGS__)
--
2.39.2
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 6/2/23 11:10, Bruno Haible wrote: +/* Avoid evaluating STATUS twice, if this is possible without making the + "warning: this statement may fall through" reappear. */ +# if __OPTIMIZE__ +# define __gl_error_call(function, status, ...) \ That means semantics are different when optimizing; generally speaking optimization should affect only performance, not correctness. How about changing this to use __builtin_constant_p instead of __OPTIMIZE__? I.e., use the comma expression if __builtin_constant_p (status), and use the statement expression otherwise.
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
Paul Eggert wrote on 2023-05-30:
> > +# define error(status, ...) \
> > + ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
> ...
> Also, it's better to not evaluate 'status' twice. Not that I think
> 'status' should have side effects or even that it does have side effects
> in any Gnulib-using code, just that it's more hygienic in case some
> caller foolishly puts side effects there.
Unfortunately, this change makes the warning in copy-file.c reappear,
when compiling with gcc 13.1.0 and no optimization.
How to reproduce:
$ rm -rf ../testdir2; ./gnulib-tool --create-testdir --dir=../testdir2
--single-configure copy-file
$ cd ../testdir2
$ ./configure CPPFLAGS=-ggdb CFLAGS=-Wimplicit-fallthrough
$ make
...
/inst-gcc/13.1.0/bin/gcc -ftrapv -DHAVE_CONFIG_H -I. -I..
-DGNULIB_STRICT_CHECKING=1 -ggdb -Wimplicit-fallthrough -MT copy-file.o -MD
-MP -MF .deps/copy-file.Tpo -c -o copy-file.o copy-file.c
In file included from error.h:28,
from copy-file.c:31:
copy-file.c: In function ‘copy_file_preserving’:
./error.h:392:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
392 | ({ \
| ~^~~
393 |int const __errstatus = status; \
|~
394 |(function) (__errstatus, __VA_ARGS__); \
|
395 |__errstatus != 0 ? unreachable () : (void) 0; \
|~~~
396 | })
| ~~
./error.h:434:6: note: in expansion of macro ‘__gl_error_call’
434 | __gl_error_call (error, status, __VA_ARGS__)
| ^~~
copy-file.c:192:7: note: in expansion of macro ‘error’
192 | error (EXIT_FAILURE, errno, _("error while opening %s for
reading"),
| ^
copy-file.c:195:5: note: here
195 | case GL_COPY_ERR_OPEN_BACKUP_WRITE:
| ^~~~
./error.h:392:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
392 | ({ \
| ~^~~
393 |int const __errstatus = status; \
|~
394 |(function) (__errstatus, __VA_ARGS__); \
|
395 |__errstatus != 0 ? unreachable () : (void) 0; \
|~~~
396 | })
| ~~
./error.h:434:6: note: in expansion of macro ‘__gl_error_call’
434 | __gl_error_call (error, status, __VA_ARGS__)
| ^~~
copy-file.c:196:7: note: in expansion of macro ‘error’
196 | error (EXIT_FAILURE, errno, _("cannot open backup file %s for
writing"),
| ^
copy-file.c:199:5: note: here
199 | case GL_COPY_ERR_READ:
| ^~~~
./error.h:392:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
392 | ({ \
| ~^~~
393 |int const __errstatus = status; \
|~
394 |(function) (__errstatus, __VA_ARGS__); \
|
395 |__errstatus != 0 ? unreachable () : (void) 0; \
|~~~
396 | })
| ~~
./error.h:434:6: note: in expansion of macro ‘__gl_error_call’
434 | __gl_error_call (error, status, __VA_ARGS__)
| ^~~
copy-file.c:200:7: note: in expansion of macro ‘error’
200 | error (EXIT_FAILURE, errno, _("error reading %s"),
| ^
copy-file.c:203:5: note: here
203 | case GL_COPY_ERR_WRITE:
| ^~~~
./error.h:392:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
392 | ({ \
| ~^~~
393 |int const __errstatus = status; \
|~
394 |(function) (__errstatus, __VA_ARGS__); \
|
395 |__errstatus != 0 ? unreachable () : (void) 0; \
|~~~
396 | })
| ~~
./error.h:434:6: note: in expansion of macro ‘__gl_error_call’
434 | __gl_error_call (error, status, __VA_ARGS__)
| ^~~
copy-file.c:204:7: note: in expansion of macro ‘error’
204 | error (EXIT_FAILURE, errno, _("error writing %s"),
| ^
copy-file.c:207:5: note: here
207 | case GL_COPY_ERR_AFTER_READ:
| ^~~~
./error.h:392:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
392 | ({ \
| ~^~~
393 |int const __errstatus = status; \
|~
394 |(function) (__errstatus, __VA_ARGS__); \
|
395 |__errstatus != 0 ? unreachable () : (void) 0; \
|~
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 01/06/2023 13:10, Bruno Haible wrote:
Pádraig Brady wrote:
Was there a reason to prefer curly braces there, rather than the more
conventional parentheses?
'$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
I had a slight preference for the curly braces since it was used in a shell
pipeline
and so it's immediately obvious it's a ${variable interpolation}
rather than being misread as a $(command substitution).
Sorry, but I find it as confusing as Jim. Inside the commands of a Makefile
rule,
$$x or $${x} references a shell variable
$$(x) does a command substitution
$(x) or ${x) references a Makefile variable
$(x y) or ${x y} does a GNU make function call
For the latter two, the customary notation is with parentheses.
So, if someone writes ${x) or ${x y}, the reader immediately wonders:
What is the point of the braces? Was it a typo, and they mean $${x} instead?
Fair point.
I've adjusted to consistent $() interpolation now.
cheers,
Pádraig
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
Pádraig Brady wrote:
> > Was there a reason to prefer curly braces there, rather than the more
> > conventional parentheses?
> >
> > '$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
>
> I had a slight preference for the curly braces since it was used in a shell
> pipeline
> and so it's immediately obvious it's a ${variable interpolation}
> rather than being misread as a $(command substitution).
Sorry, but I find it as confusing as Jim. Inside the commands of a Makefile
rule,
$$x or $${x} references a shell variable
$$(x) does a command substitution
$(x) or ${x) references a Makefile variable
$(x y) or ${x y} does a GNU make function call
For the latter two, the customary notation is with parentheses.
So, if someone writes ${x) or ${x y}, the reader immediately wonders:
What is the point of the braces? Was it a typo, and they mean $${x} instead?
Bruno
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 31/05/2023 18:46, Jim Meyering wrote:
On Wed, May 31, 2023 at 9:12 AM Pádraig Brady wrote:
- 'error \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
+ '${error_fns} \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
Thanks!
Was there a reason to prefer curly braces there, rather than the more
conventional parentheses?
'$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
I had a slight preference for the curly braces since it was used in a shell
pipeline
and so it's immediately obvious it's a ${variable interpolation}
rather than being misread as a $(command substitution).
cheers,
Pádraig
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On Wed, May 31, 2023 at 9:12 AM Pádraig Brady wrote:
> On 30/05/2023 22:29, Paul Eggert wrote:
> > On 5/28/23 06:07, Pádraig Brady wrote:
> >> 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,
> >
> > I see you fixed that by adding a quick "#define _GL_NO_INLINE_ERROR" to
> > coreutils/src/dd.c. It's a bit cleaner to fix the underlying naming
> > problem instead, so that dd.c need not define the Gnulib internals macro
> > (or its own quirky error macro), so I installed the attached to
> > coreutils to do that.
>
> I was debating that option but decided against it
> as we'd then lose some of the syntax checks on error(args).
> But we can augment the syntax checks to cater for this class of function,
> which I'm doing as follows.
>
> cheers,
> Pádraig
>
> diff --git a/cfg.mk b/cfg.mk
> index 263bc0cfd..64db2bec4 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -189,12 +189,15 @@ sc_prohibit_quotes_notation:
> exit 1; } \
>|| :
>
> +error_fns = (error|die|diagnose)
> +
> # Files in src/ should quote all strings in error() output, so that
> # unexpected input chars like \r etc. don't corrupt the error.
> # In edge cases this can be avoided by putting the format string
> # on a separate line to the arguments, or the arguments in parenthesis.
> sc_error_quotes:
> - @cd $(srcdir)/src && GIT_PAGER= git grep -n 'error *(.*%s.*,
> [^(]*);$$'\
> + @cd $(srcdir)/src \
> + && GIT_PAGER= git grep -E -n '${error_fns} *\(.*%s.*, [^(]*\);$$' \
>*.c | grep -v ', q' \
>&& { echo '$(ME): '"Use quote() for error string arguments" 1>&2; \
> exit 1; } \
> @@ -206,7 +209,7 @@ sc_error_quotes:
> sc_error_shell_quotes:
> @cd $(srcdir)/src && \
>{ GIT_PAGER= git grep -E \
> - 'error \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
> + '${error_fns} \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
Thanks!
Was there a reason to prefer curly braces there, rather than the more
conventional parentheses?
'$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 30/05/2023 22:29, Paul Eggert wrote:
On 5/28/23 06:07, Pádraig Brady wrote:
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,
I see you fixed that by adding a quick "#define _GL_NO_INLINE_ERROR" to
coreutils/src/dd.c. It's a bit cleaner to fix the underlying naming
problem instead, so that dd.c need not define the Gnulib internals macro
(or its own quirky error macro), so I installed the attached to
coreutils to do that.
I was debating that option but decided against it
as we'd then lose some of the syntax checks on error(args).
But we can augment the syntax checks to cater for this class of function,
which I'm doing as follows.
cheers,
Pádraig
diff --git a/cfg.mk b/cfg.mk
index 263bc0cfd..64db2bec4 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -189,12 +189,15 @@ sc_prohibit_quotes_notation:
exit 1; } \
|| :
+error_fns = (error|die|diagnose)
+
# Files in src/ should quote all strings in error() output, so that
# unexpected input chars like \r etc. don't corrupt the error.
# In edge cases this can be avoided by putting the format string
# on a separate line to the arguments, or the arguments in parenthesis.
sc_error_quotes:
- @cd $(srcdir)/src && GIT_PAGER= git grep -n 'error *(.*%s.*, [^(]*);$$'\
+ @cd $(srcdir)/src \
+ && GIT_PAGER= git grep -E -n '${error_fns} *\(.*%s.*, [^(]*\);$$' \
*.c | grep -v ', q' \
&& { echo '$(ME): '"Use quote() for error string arguments" 1>&2; \
exit 1; } \
@@ -206,7 +209,7 @@ sc_error_quotes:
sc_error_shell_quotes:
@cd $(srcdir)/src && \
{ GIT_PAGER= git grep -E \
- 'error \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
+ '${error_fns} \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
GIT_PAGER= git grep -E \
' quote[ _].*file' *.c; } \
| grep -Ev '(quotef|q[^ ]*name)' \
@@ -220,13 +223,13 @@ sc_error_shell_quotes:
# to provide better support for copy and paste.
sc_error_shell_always_quotes:
@cd $(srcdir)/src && GIT_PAGER= git grep -E \
- 'error \(.*[^:] %s[ "].*, .*(name|file)[^"]*\);$$' \
+ '${error_fns} \(.*[^:] %s[ "].*, .*(name|file)[^"]*\);$$' \
*.c | grep -Ev '(quoteaf|q[^ ]*name)' \
&& { echo '$(ME): '"Use quoteaf() for space delimited names" 1>&2; \
exit 1; } \
|| :
@cd $(srcdir)/src && GIT_PAGER= git grep -E -A1 \
- 'error \([^%]*[^:] %s[ "]' *.c | grep 'quotef' \
+ '${error_fns} \([^%]*[^:] %s[ "]' *.c | grep 'quotef' \
&& { echo '$(ME): '"Use quoteaf() for space delimited names" 1>&2; \
exit 1; } \
|| :
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 5/30/23 15:06, Bruno Haible wrote: In terms of functions calls, I don't think it makes a difference, whether exit() gets called from within the error() invocation [1] or after the error() invocation. It makes the calling code a bit smaller, e.g., it avoids GCC having to generate code to save and restore registers in the caller, along with issuing two calls instead of one. Admittedly not a huge deal. I also vaguely think that perhaps GCC will find the resulting code easier to analyze. Although this is just a guess, GCC is kinda buggy in this area and I hope the smaller code avoid bugs. + __gl_error_call (rpl_error, status, __VA_ARGS) What is __VA_ARGS ? Did you mean __VA_ARGS__ ? Yes I did. Thanks for catching that typo. I installed a fix.
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
Paul Eggert wrote: > > +# define error(status, ...) \ > > + ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0) > > We can do a bit better than that by using 'unreachable ()' instead of > 'exit (status)', and passing 'status' (instead of 0) to the underlying > error function. This saves a function call In terms of functions calls, I don't think it makes a difference, whether exit() gets called from within the error() invocation [1] or after the error() invocation. [1] lib/error.c:285 > Also, it's better to not evaluate 'status' twice. Not that I think > 'status' should have side effects or even that it does have side effects > in any Gnulib-using code, just that it's more hygienic in case some > caller foolishly puts side effects there. Indeed, having it evaluate only once is better. Thanks. > + __gl_error_call (rpl_error, status, __VA_ARGS) What is __VA_ARGS ? Did you mean __VA_ARGS__ ? Bruno
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 5/27/23 13:53, Bruno Haible wrote:
+# define error(status, ...) \
+ ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
We can do a bit better than that by using 'unreachable ()' instead of
'exit (status)', and passing 'status' (instead of 0) to the underlying
error function. This saves a function call and should still pacify GCC.
Also, it's better to not evaluate 'status' twice. Not that I think
'status' should have side effects or even that it does have side effects
in any Gnulib-using code, just that it's more hygienic in case some
caller foolishly puts side effects there.
I installed the attached further patches into Gnulib to try to address
these issues.From 8b21ff255996a518244a5635e36ea58e21f818be Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Tue, 30 May 2023 12:49:20 -0700
Subject: [PATCH 1/2] =?UTF-8?q?error:=20don=E2=80=99t=20evaluate=20status?=
=?UTF-8?q?=20arg=20twice?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This avoids potential issues if the first argument has a side effect.
* lib/error.in.h (__gl_error_call): New macro, which evaluates its
status arg only once, by using a statement expression if GNU C -
the only platform we need to worry about pacifying - and by simply
calling ‘error’ otherwise.
(error, error_at_line): Use it.
---
ChangeLog | 10 ++
lib/error.in.h | 20
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 91a6135f8a..f70eeef893 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2023-05-30 Paul Eggert
+
+ error: don’t evaluate status arg twice
+ This avoids potential issues if the first argument has a side effect.
+ * lib/error.in.h (__gl_error_call): New macro, which evaluates its
+ status arg only once, by using a statement expression if GNU C -
+ the only platform we need to worry about pacifying - and by simply
+ calling ‘error’ otherwise.
+ (error, error_at_line): Use it.
+
2023-05-28 Bruno Haible
warnings, manywarnings: Assume autoconf >= 2.64.
diff --git a/lib/error.in.h b/lib/error.in.h
index 70fb132133..a5f49e4143 100644
--- a/lib/error.in.h
+++ b/lib/error.in.h
@@ -49,6 +49,18 @@
# define _GL_ATTRIBUTE_SPEC_PRINTF_ERROR _GL_ATTRIBUTE_SPEC_PRINTF_SYSTEM
#endif
+#ifdef __GNUC__
+# define __gl_error_call(function, status, ...) \
+({ \
+ int const __errstatus = status; \
+ (function) (0, __VA_ARGS__); \
+ __errstatus != 0 ? exit (__errstatus) : (void) 0; \
+})
+#else
+# define __gl_error_call(function, status, ...) \
+(function) (status, __VA_ARGS__)
+#endif
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -69,7 +81,7 @@ _GL_CXXALIAS_RPL (error, void,
# ifndef _GL_NO_INLINE_ERROR
# undef error
# define error(status, ...) \
- ((rpl_error)(0, __VA_ARGS__), (status) != 0 ? exit (status) : (void)0)
+ __gl_error_call (rpl_error, status, __VA_ARGS)
# endif
#else
# if ! @HAVE_ERROR@
@@ -81,7 +93,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) != 0 ? exit (status) : (void)0)
+ __gl_error_call (error, status, __VA_ARGS__)
# endif
#endif
#if __GLIBC__ >= 2
@@ -105,7 +117,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) != 0 ? exit (status) : (void)0)
+ __gl_error_call (rpl_error_at_line, status, __VA_ARGS__)
# endif
#else
# if ! @HAVE_ERROR_AT_LINE@
@@ -119,7 +131,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) != 0 ? exit (status) : (void)0)
+ __gl_error_call (error_at_line, status, __VA_ARGS__)
# endif
#endif
_GL_CXXALIASWARN (error_at_line);
--
2.40.1
From 47811d1ac24e38e8842dc37e2e3cd3b4120338ad Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Tue, 30 May 2023 14:38:41 -0700
Subject: [PATCH 2/2] =?UTF-8?q?error:=20don=E2=80=99t=20call=20=E2=80=98ex?=
=?UTF-8?q?it=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Let the underlying functions call ‘exit’, instead of having the
Gnulib replacement macros do it. Use ‘unreachable’ to tell the
compiler that those functions exit when the status is nonzero.
This saves a function call.
* lib/error.in.h: Include stddef.h, not stdlib.h.
(__gl_error_call): Rely on the function to exit, using
‘unreachable’ to tell the compiler that the function does not return.
* modules/error (Depends-on): Add stddef.
---
ChangeLog | 10 ++
lib/error.in.h | 10 +-
modules/error | 1 +
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
On 5/28/23 06:07, Pádraig Brady wrote:
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,
I see you fixed that by adding a quick "#define _GL_NO_INLINE_ERROR" to
coreutils/src/dd.c. It's a bit cleaner to fix the underlying naming
problem instead, so that dd.c need not define the Gnulib internals macro
(or its own quirky error macro), so I installed the attached to
coreutils to do that.
From 48f5a39872acbf3f23f02cf3216a6e30d00db671 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Tue, 30 May 2023 14:24:46 -0700
Subject: [PATCH] =?UTF-8?q?dd:=20fix=20=E2=80=98error=E2=80=99=20name=20is?=
=?UTF-8?q?sue=20without=20macros?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* src/dd.c (_GL_NO_INLINE_ERROR): Remove; no longer needed.
(diagnose): Rename from nl_error and omit first arg since it is
always zero. All uses changed.
(error): Remove macro.
---
src/dd.c | 109 ++-
1 file changed, 52 insertions(+), 57 deletions(-)
diff --git a/src/dd.c b/src/dd.c
index f41966969..8fcfdd3db 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -17,7 +17,6 @@
/* Written by Paul Rubin, David MacKenzie, and Stuart Kemp. */
#include
-#define _GL_NO_INLINE_ERROR /* Avoid gnulib's error macro. */
#include
#include
@@ -515,11 +514,12 @@ maybe_close_stdout (void)
_exit (EXIT_FAILURE);
}
-/* Like the 'error' function but handle any pending newline. */
+/* Like the 'error' function but handle any pending newline,
+ and do not exit. */
-ATTRIBUTE_FORMAT ((__printf__, 3, 4))
+ATTRIBUTE_FORMAT ((__printf__, 2, 3))
static void
-nl_error (int status, int errnum, char const *fmt, ...)
+diagnose (int errnum, char const *fmt, ...)
{
if (0 < progress_len)
{
@@ -529,12 +529,10 @@ nl_error (int status, int errnum, char const *fmt, ...)
va_list ap;
va_start (ap, fmt);
- verror (status, errnum, fmt, ap);
+ verror (0, errnum, fmt, ap);
va_end (ap);
}
-#define error nl_error
-
void
usage (int status)
{
@@ -1138,12 +1136,12 @@ iread (int fd, char *buf, idx_t size)
{
idx_t prev = prev_nread;
if (status_level != STATUS_NONE)
-error (0, 0, ngettext (("warning: partial read (%td byte); "
+diagnose (0, ngettext (("warning: partial read (%td byte); "
"suggest iflag=fullblock"),
("warning: partial read (%td bytes); "
"suggest iflag=fullblock"),
select_plural (prev)),
- prev);
+ prev);
warn_partial_read = false;
}
}
@@ -1188,8 +1186,8 @@ iwrite (int fd, char const *buf, idx_t size)
int old_flags = fcntl (STDOUT_FILENO, F_GETFL);
if (fcntl (STDOUT_FILENO, F_SETFL, old_flags & ~O_DIRECT) != 0
&& status_level != STATUS_NONE)
-error (0, errno, _("failed to turn off O_DIRECT: %s"),
- quotef (output_file));
+diagnose (errno, _("failed to turn off O_DIRECT: %s"),
+ quotef (output_file));
/* Since we have just turned off O_DIRECT for the final write,
we try to preserve some of its semantics. */
@@ -1263,7 +1261,7 @@ write_output (void)
w_bytes += nwritten;
if (nwritten != output_blocksize)
{
- error (0, errno, _("writing to %s"), quoteaf (output_file));
+ diagnose (errno, _("writing to %s"), quoteaf (output_file));
if (nwritten != 0)
w_partial++;
quit (EXIT_FAILURE);
@@ -1392,8 +1390,9 @@ parse_symbols (char const *str, struct symbol_value const *table,
if (! entry->symbol[0])
{
idx_t slen = strcomma ? strcomma - str : strlen (str);
- error (0, 0, "%s: %s", _(error_msgid),
- quotearg_n_style_mem (0, locale_quoting_style, str, slen));
+ diagnose (0, "%s: %s", _(error_msgid),
+quotearg_n_style_mem (0, locale_quoting_style,
+ str, slen));
usage (EXIT_FAILURE);
}
}
@@ -1456,10 +1455,9 @@ parse_integer (char const *str, strtol_error *invalid)
else
{
if (result == 0 && STRPREFIX (str, "0x"))
-error (0, 0,
- _("warning: %s is a zero multiplier; "
- "use %s if that is intended"),
- quote_n (0, "0x"), quote_n (1, "00x"));
+diagnose (0, _("warning: %s is a zero multiplier; "
+ "use %s if that is intended"),
+ quote_n (0, "0x"), quote_n (1, "00x"));
e = LONGINT_OK;
}
}
@@ -1500,8 +1498,7 @@ scanargs (int argc, char *const *argv)
if (val == NULL)
{
- error (0, 0, _("unr
Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)
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)
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)
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
