https://github.com/Sirraide created
https://github.com/llvm/llvm-project/pull/93077
Currently, if the argument to `__builtin_assume` and friends contains
side-effects, we issue the following diagnostic:
```
:1:34: warning: the argument to '__builtin_assume' has side effects
that will be discarded [-Wassume]
1 | void f(int x) { __builtin_assume(x++); }
|
```
The issue here is that this diagnostic misrepresents what is actually
happening: not only do we discard the side-effects of the expression, but we
also don’t even emit any assumption information at all because the backend is
not equipped to deal with eliminating side-effects in cases such as this.
This has caused some confusion (see #91612) beacuse the current wording of the
warning suggests that, sensibly, only the side-effects of the expression, and
not the assumption itself, will be discarded.
This pr updates the diagnostic to state what is actually happening: that the
assumption has no effect at all because its argument contains side-effects:
```
:1:34: warning: assumption is ignored because it contains (potential)
side-effects [-Wassume]
1 | void f(int x) { __builtin_assume(x++); }
|
```
I’ve deliberately included ‘(potential)’ here because even expressions that
only contain potential side-effects (e.g. `true ? x : x++` or a call to a
function that is pure, but we don’t know that it is) cause the assumption to be
discarded. This, too, has caused some confusion because it was erroneously
assumed that Clang would e.g. infer that a function call is pure and not
discard the assumption as a result when that isn’t the case.
This is intended to be temporary; we should revert back to the original
diagnostic once we have proper support for assumptions with side-effects in the
backend (in which case the side-effects will still be discarded, but the
assumption won’t)
This fixes #91612.
>From e5b11b59f3f8a0754d426ae9d3159dda62c31f06 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Wed, 22 May 2024 19:23:05 +0200
Subject: [PATCH] [Clang] [NFC] Clarify assume diagnostic
---
.../clang/Basic/DiagnosticSemaKinds.td| 2 +-
clang/test/Parser/MicrosoftExtensions.cpp | 2 +-
clang/test/Sema/builtin-assume.c | 12 +-
clang/test/Sema/stmtexprs.c | 2 +-
clang/test/SemaCXX/cxx23-assume.cpp | 22 +--
5 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 41a9745ddb570..e440e0a9c8507 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -855,7 +855,7 @@ def note_strncat_wrong_size : Note<
"the terminating null byte">;
def warn_assume_side_effects : Warning<
- "the argument to %0 has side effects that will be discarded">,
+ "assumption is ignored because it contains (potential) side-effects">,
InGroup>;
def warn_omp_assume_attribute_string_unknown : Warning<
"unknown assumption string '%0'; attribute is potentially ignored">,
diff --git a/clang/test/Parser/MicrosoftExtensions.cpp
b/clang/test/Parser/MicrosoftExtensions.cpp
index 6bf802a29ace3..9102bca8f6bb2 100644
--- a/clang/test/Parser/MicrosoftExtensions.cpp
+++ b/clang/test/Parser/MicrosoftExtensions.cpp
@@ -426,7 +426,7 @@ bool f(int);
template
struct A {
constexpr A(T t) {
-__assume(f(t)); // expected-warning{{the argument to '__assume' has side
effects that will be discarded}}
+__assume(f(t)); // expected-warning{{assumption is ignored because it
contains (potential) side-effects}}
}
constexpr bool g() { return false; }
};
diff --git a/clang/test/Sema/builtin-assume.c b/clang/test/Sema/builtin-assume.c
index 932fb5c973eb0..21d62d8fd06c1 100644
--- a/clang/test/Sema/builtin-assume.c
+++ b/clang/test/Sema/builtin-assume.c
@@ -8,20 +8,20 @@ int ispure(int) __attribute__((pure));
int foo(int *a, int i) {
#ifdef _MSC_VER
__assume(i != 4);
- __assume(++i > 2); //expected-warning {{the argument to '__assume' has side
effects that will be discarded}}
- __assume(nonconst() > 2); //expected-warning {{the argument to '__assume'
has side effects that will be discarded}}
+ __assume(++i > 2); //expected-warning {{assumption is ignored because it
contains (potential) side-effects}}
+ __assume(nonconst() > 2); //expected-warning {{assumption is ignored because
it contains (potential) side-effects}}
__assume(isconst() > 2);
__assume(ispure(i) > 2);
- __assume(ispure(++i) > 2); //expected-warning {{the argument to '__assume'
has side effects that will be discarded}}
+ __assume(ispure(++i) > 2); //expected-warning {{assumption is ignored
because it contains (potential) side-effects}}
int test = sizeof(struct{char qq[(__assume(i != 5), 7)];});
#else
__builtin_assume(i != 4);
- __builtin_assume(++
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (Sirraide)
Changes
Currently, if the argument to `__builtin_assume` and friends contains
side-effects, we issue the following diagnostic:
```
:1:34: warning: the argument to '__builtin_assume' has side
effects that will be discarded [-Wassume]
1 | void f(int x) { __builtin_assume(x++); }
|
```
The issue here is that this diagnostic misrepresents what is actually
happening: not only do we discard the side-effects of the expression, but we
also don’t even emit any assumption information at all because the backend is
not equipped to deal with eliminating side-effects in cases such as this.
This has caused some confusion (see #91612) beacuse the current wording
of the warning suggests that, sensibly, only the side-effects of the
expression, and not the assumption itself, will be discarded.
This pr updates the diagnostic to state what is actually happening: that the
assumption has no effect at all because its argument contains side-effects:
```
:1:34: warning: assumption is ignored because it contains
(potential) side-effects [-Wassume]
1 | void f(int x) { __builtin_assume(x++); }
|
```
I’ve deliberately included ‘(potential)’ here because even expressions that
only contain potential side-effects (e.g. `true ? x : x++` or a call to a
function that is pure, but we don’t know that it is) cause the assumption to be
discarded. This, too, has caused some confusion because it was erroneously
assumed that Clang would e.g. infer that a function call is pure and not
discard the assumption as a result when that isn’t the case.
This is intended to be temporary; we should revert back to the original
diagnostic once we have proper support for assumptions with side-effects in the
backend (in which case the side-effects will still be discarded, but the
assumption won’t)
This fixes #91612.
---
Full diff: https://github.com/llvm/llvm-project/pull/93077.diff
5 Files Affected:
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
- (modified) clang/test/Parser/MicrosoftExtensions.cpp (+1-1)
- (modified) clang/test/Sema/builtin-assume.c (+6-6)
- (modified) clang/test/Sema/stmtexprs.c (+1-1)
- (modified) clang/test/SemaCXX/cxx23-assume.cpp (+11-11)
``diff
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 41a9745ddb570..e440e0a9c8507 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -855,7 +855,7 @@ def note_strncat_wrong_size : Note<
"the terminating null byte">;
def warn_assume_side_effects : Warning<
- "the argument to %0 has side effects that will be discarded">,
+ "assumption is ignored because it contains (potential) side-effects">,
InGroup>;
def warn_omp_assume_attribute_string_unknown : Warning<
"unknown assumption string '%0'; attribute is potentially ignored">,
diff --git a/clang/test/Parser/MicrosoftExtensions.cpp
b/clang/test/Parser/MicrosoftExtensions.cpp
index 6bf802a29ace3..9102bca8f6bb2 100644
--- a/clang/test/Parser/MicrosoftExtensions.cpp
+++ b/clang/test/Parser/MicrosoftExtensions.cpp
@@ -426,7 +426,7 @@ bool f(int);
template
struct A {
constexpr A(T t) {
-__assume(f(t)); // expected-warning{{the argument to '__assume' has side
effects that will be discarded}}
+__assume(f(t)); // expected-warning{{assumption is ignored because it
contains (potential) side-effects}}
}
constexpr bool g() { return false; }
};
diff --git a/clang/test/Sema/builtin-assume.c b/clang/test/Sema/builtin-assume.c
index 932fb5c973eb0..21d62d8fd06c1 100644
--- a/clang/test/Sema/builtin-assume.c
+++ b/clang/test/Sema/builtin-assume.c
@@ -8,20 +8,20 @@ int ispure(int) __attribute__((pure));
int foo(int *a, int i) {
#ifdef _MSC_VER
__assume(i != 4);
- __assume(++i > 2); //expected-warning {{the argument to '__assume' has side
effects that will be discarded}}
- __assume(nonconst() > 2); //expected-warning {{the argument to '__assume'
has side effects that will be discarded}}
+ __assume(++i > 2); //expected-warning {{assumption is ignored because it
contains (potential) side-effects}}
+ __assume(nonconst() > 2); //expected-warning {{assumption is ignored because
it contains (potential) side-effects}}
__assume(isconst() > 2);
__assume(ispure(i) > 2);
- __assume(ispure(++i) > 2); //expected-warning {{the argument to '__assume'
has side effects that will be discarded}}
+ __assume(ispure(++i) > 2); //expected-warning {{assumption is ignored
because it contains (potential) side-effects}}
int test = sizeof(struct{char qq[(__assume(i != 5), 7)];});
#else
__builtin_assume(i != 4);
- __builtin_assume(++i > 2); //expected-warning {{the argument to
'__builtin_assume' has side effects that will be discarded}}
- __builtin_assume(n
danakj wrote:
Thanks, this is very good
https://github.com/llvm/llvm-project/pull/93077
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
