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

[Bruno Haible]

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; \
      |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  396 |     })
      |     ~~
./error.h:434:6: note: in expansion of macro ‘__gl_error_call’
  434 |      __gl_error_call (error, status, __VA_ARGS__)
      |      ^~~~~~~~~~~~~~~
copy-file.c:208:7: note: in expansion of macro ‘error’
  208 |       error (EXIT_FAILURE, errno, _("error after reading %s"),
      |       ^~~~~
copy-file.c:211:5: note: here
  211 |     case GL_COPY_ERR_GET_ACL:
      |     ^~~~
./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:212:7: note: in expansion of macro ‘error’
  212 |       error (EXIT_FAILURE, errno, "%s", quote (src_filename));
      |       ^~~~~
copy-file.c:214:5: note: here
  214 |     case GL_COPY_ERR_SET_ACL:
      |     ^~~~
./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:215:7: note: in expansion of macro ‘error’
  215 |       error (EXIT_FAILURE, errno, _("preserving permissions for %s"),
      |       ^~~~~
copy-file.c:218:5: note: here
  218 |     default:
      |     ^~~~~~~
...

When no optimization is in effect, it is more important to have the
"this statement may fall through" warning be silenced, than to avoid
evaluating 'status' twice. (I build GNU gettext with CFLAGS="-ggdb"
about half of the time, and it would be annoying to have a warning
reappear that was already silenced.)

I'm therefore applying this patch, which fixes it by using a comma
expression instead of a compound statement.


2023-06-02  Bruno Haible  <bruno@clisp.org>

        error: Avoid implicit-fallthrough warnings with -O0 (regr. 2023-05-30).
        * lib/error.in.h (__gl_error_call): Parenthesize status. When not
        optimizing, expand to code without compound statements.

diff --git a/lib/error.in.h b/lib/error.in.h
index f1ba94577b..f37dba170c 100644
--- a/lib/error.in.h
+++ b/lib/error.in.h
@@ -49,13 +49,24 @@
 # define _GL_ATTRIBUTE_SPEC_PRINTF_ERROR _GL_ATTRIBUTE_SPEC_PRINTF_SYSTEM
 #endif
 
+/* Helper macro for supporting the compiler's control flow analysis better.
+   Test case: Compile copy-file.c with "gcc -Wimplicit-fallthrough".  */
 #ifdef __GNUC__
-# define __gl_error_call(function, status, ...) \
-    ({ \
-       int const __errstatus = status; \
-       (function) (__errstatus, __VA_ARGS__); \
-       __errstatus != 0 ? unreachable () : (void) 0; \
-    })
+/* 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
 #else
 # define __gl_error_call(function, status, ...) \
     (function) (status, __VA_ARGS__)