Re: more macOS Clang assume warnings

[Bruno Haible <bruno@clisp.org>]

[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  <bruno@clisp.org>

	verify: Avoid warnings when assume(0) is used.
	Reported by Mattias Engdegård <mattiase@acm.org> 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);
+}