* Re: more macOS Clang assume warnings
2020-08-25 0:23 ` Paul Eggert
@ 2020-08-26 0:01 ` Bruno Haible
2020-08-26 0:44 ` Paul Eggert
0 siblings, 1 reply; 5+ messages in thread
From: Bruno Haible @ 2020-08-26 0:01 UTC (permalink / raw)
To: Paul Eggert; +Cc: Mattias Engdegård, bug-gnulib, emacs-devel
[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);
+}
^ permalink raw reply related [flat|nested] 5+ messages in thread