Skip to content

Commit

Permalink
[Clang] [NFC] Clarify assume diagnostic (#93077)
Browse files Browse the repository at this point in the history
Currently, if the argument to `__builtin_assume` and friends contains
side-effects, we issue the following diagnostic:
```
<source>: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:
```
<source>: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.
  • Loading branch information
Sirraide authored May 27, 2024
1 parent 6702594 commit 5e140c8
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 20 deletions.
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -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<DiagGroup<"assume">>;
def warn_omp_assume_attribute_string_unknown : Warning<
"unknown assumption string '%0'; attribute is potentially ignored">,
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Parser/MicrosoftExtensions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ bool f(int);
template <typename T>
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; }
};
Expand Down
12 changes: 6 additions & 6 deletions clang/test/Sema/builtin-assume.c
Original file line number Diff line number Diff line change
Expand Up @@ -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(nonconst() > 2); //expected-warning {{the argument to '__builtin_assume' has side effects that will be discarded}}
__builtin_assume(++i > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
__builtin_assume(nonconst() > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
__builtin_assume(isconst() > 2);
__builtin_assume(ispure(i) > 2);
__builtin_assume(ispure(++i) > 2); //expected-warning {{the argument to '__builtin_assume' has side effects that will be discarded}}
__builtin_assume(ispure(++i) > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}

int test = sizeof(struct{char qq[(__builtin_assume(i != 5), 7)];}); // expected-warning {{variable length array}}
#endif
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Sema/stmtexprs.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ int stmtexpr_fn(void);
void stmtexprs(int i) {
__builtin_assume( ({ 1; }) ); // no warning about "side effects"
__builtin_assume( ({ if (i) { (void)0; }; 42; }) ); // no warning about "side effects"
// expected-warning@+1 {{the argument to '__builtin_assume' has side effects that will be discarded}}
// expected-warning@+1 {{assumption is ignored because it contains (potential) side-effects}}
__builtin_assume( ({ if (i) ({ stmtexpr_fn(); }); 1; }) );
}
22 changes: 11 additions & 11 deletions clang/test/SemaCXX/cxx23-assume.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ bool f2();

template <typename T>
constexpr void f3() {
[[assume(T{})]]; // expected-error {{not contextually convertible to 'bool'}} expected-warning {{has side effects that will be discarded}} ext-warning {{C++23 extension}}
[[assume(T{})]]; // expected-error {{not contextually convertible to 'bool'}} expected-warning {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
}

void g(int x) {
Expand All @@ -38,13 +38,13 @@ void g(int x) {
S<false>{}.f();
S<true>{}.g<char>();
S<true>{}.g<int>();
[[assume(f2())]]; // expected-warning {{side effects that will be discarded}} ext-warning {{C++23 extension}}
[[assume(f2())]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}

[[assume((x = 3))]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
[[assume(x++)]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
[[assume(++x)]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
[[assume([]{ return true; }())]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
[[assume(B{})]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
[[assume((x = 3))]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
[[assume(x++)]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
[[assume(++x)]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
[[assume([]{ return true; }())]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
[[assume(B{})]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
[[assume((1, 2))]]; // expected-warning {{has no effect}} // ext-warning {{C++23 extension}}

f3<A>(); // expected-note {{in instantiation of}}
Expand Down Expand Up @@ -91,7 +91,7 @@ static_assert(S<false>{}.g<A>()); // expected-error {{not an integral constant e

template <typename T>
constexpr bool f4() {
[[assume(!T{})]]; // expected-error {{invalid argument type 'D'}} // expected-warning 2 {{side effects}} ext-warning {{C++23 extension}}
[[assume(!T{})]]; // expected-error {{invalid argument type 'D'}} // expected-warning 2 {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
return sizeof(T) == sizeof(int);
}

Expand Down Expand Up @@ -137,8 +137,8 @@ static_assert(f5<F>() == 2); // expected-note {{while checking constraint satisf
// Do not validate assumptions whose evaluation would have side-effects.
constexpr int foo() {
int a = 0;
[[assume(a++)]] [[assume(++a)]]; // expected-warning 2 {{has side effects that will be discarded}} ext-warning 2 {{C++23 extension}}
[[assume((a+=1))]]; // expected-warning {{has side effects that will be discarded}} ext-warning {{C++23 extension}}
[[assume(a++)]] [[assume(++a)]]; // expected-warning 2 {{assumption is ignored because it contains (potential) side-effects}} ext-warning 2 {{C++23 extension}}
[[assume((a+=1))]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
return a;
}

Expand All @@ -154,7 +154,7 @@ int
foo (int x, int y)
{
__attribute__((assume(x == 42)));
__attribute__((assume(++y == 43))); // expected-warning {{has side effects that will be discarded}}
__attribute__((assume(++y == 43))); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
return x + y;
}
}

0 comments on commit 5e140c8

Please sign in to comment.