[CCing bug-gnulib.] Paul Eggert wrote in <https://lists.gnu.org/archive/html/emacs-devel/2020-08/msg00838.html>: Mattias Engdegård wrote: > > The latest assume reformulation caused several warnings like the following > > when building with Clang: > > > > ../../emacs/src/xwidget.h:171:72: warning: control reaches end of non-void > > function [-Wreturn-type] > > INLINE struct xwidget *lookup_xwidget (Lisp_Object obj) { eassume (0); } > > ^ > > ../../emacs/src/eval.c:1571:1: warning: function declared 'noreturn' should > > not > > return [-Winvalid-noreturn] > > } > > ^ > > > > This is with Apple clang version 11.0.0 (clang-1100.0.33.17) but it looks > > like the same applies to clang trunk. > > Thanks for reporting that. I can reproduce the problem with Clang 9.0.1 on > Fedora 31. For the program at the end of this message, 'clang -O2 -Wall' > incorrectly complains "warning: function declared 'noreturn' should not > return > [-Winvalid-noreturn]".
The patch below fixes this. Thanks for the report. > Bruno, how about if we go back to how verify.h did 'assume' before the recent > clang-related changes? That would avoid the complaints. It might cost us a > few > nanoseconds of performance with clang-generated code but that's no big deal. Since the patch below fixes it without giving up on the optimizations for clang <= 8, I see no reason to give up on the approach with __builtin_assume. > And anyway, clang ought to get fixed to generate the slightly-better code > with > 'verify.h' just as it was. It has already been fixed: clang >= 9 optimizes well with just __builtin_unreachable and no use of __builtin_assume. 2020-08-25 Bruno Haible <[email protected]> verify: Avoid warnings when assume(0) is used. Reported by Mattias Engdegård <[email protected]> via Paul Eggert in <https://lists.gnu.org/archive/html/emacs-devel/2020-08/msg00838.html>. * lib/verify.h (assume): Use __builtin_unreachable if the argument is the constant 0. * tests/test-verify.c (f): New function. (state): New type. (test_assume_expressions, test_assume_optimization, test_assume_noreturn): New functions. diff --git a/lib/verify.h b/lib/verify.h index 6d7b961..ca2a154 100644 --- a/lib/verify.h +++ b/lib/verify.h @@ -320,7 +320,9 @@ template <int w> based on __builtin_unreachable does not. (GCC so far has only __builtin_unreachable.) */ #if _GL_HAS_BUILTIN_ASSUME -/* Use a temporary variable, to avoid a clang warning +/* Use __builtin_constant_p to help clang's data-flow analysis for the case + assume (0). + Use a temporary variable, to avoid a clang warning "the argument to '__builtin_assume' has side effects that will be discarded" if R contains invocations of functions not marked as 'const'. The type of the temporary variable can't be __typeof__ (R), because that @@ -328,12 +330,16 @@ template <int w> instead. */ # if defined __cplusplus # define assume(R) \ - ((void) ({ bool _gl_verify_temp = (R); \ - __builtin_assume (_gl_verify_temp); })) + (__builtin_constant_p (R) && !(R) \ + ? (void) __builtin_unreachable () \ + : (void) ({ bool _gl_verify_temp = (R); \ + __builtin_assume (_gl_verify_temp); })) # else # define assume(R) \ - ((void) ({ _Bool _gl_verify_temp = (R); \ - __builtin_assume (_gl_verify_temp); })) + (__builtin_constant_p (R) && !(R) \ + ? (void) __builtin_unreachable () \ + : (void) ({ _Bool _gl_verify_temp = (R); \ + __builtin_assume (_gl_verify_temp); })) # endif #elif _GL_HAS_BUILTIN_UNREACHABLE # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ()) diff --git a/tests/test-verify.c b/tests/test-verify.c index 498dd2f..7201ca1 100644 --- a/tests/test-verify.c +++ b/tests/test-verify.c @@ -25,6 +25,8 @@ # define EXP_FAIL 0 #endif +/* ======================= Test verify, verify_expr ======================= */ + int x; enum { a, b, c }; @@ -67,3 +69,43 @@ main (void) { return !(function (0) == 0 && function (1) == 8); } + +/* ============================== Test assume ============================== */ + +static int +f (int a) +{ + return a; +} + +typedef struct { unsigned int context : 4; unsigned int halt : 1; } state; + +void +test_assume_expressions (state *s) +{ + /* Check that 'assume' accepts a function call, even of a non-const + function. */ + assume (f (1)); + /* Check that 'assume' accepts a bit-field expression. */ + assume (s->halt); +} + +int +test_assume_optimization (int x) +{ + /* Check that the compiler uses 'assume' for optimization. + This function, when compiled with optimization, should have code + equivalent to + return x + 3; + Use 'objdump --disassemble test-verify.o' to verify this. */ + assume (x >= 4); + return (x > 1 ? x + 3 : 2 * x + 10); +} + +_Noreturn void +test_assume_noreturn (void) +{ + /* Check that the compiler's data-flow analysis recognizes 'assume (0)'. + This function should not elicit a warning. */ + assume (0); +}
