[PATCH] D91348: [OpenCL] Warn about side effects for unevaluated vec_step arg

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e4d2361b817: [OpenCL] Warn about side effects for 
unevaluated vec_step arg (authored by svenvh).

Changed prior to commit:
  https://reviews.llvm.org/D91348?vs=305058=314559#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91348/new/

https://reviews.llvm.org/D91348

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaOpenCL/vec_step.cl


Index: clang/test/SemaOpenCL/vec_step.cl
===
--- clang/test/SemaOpenCL/vec_step.cl
+++ clang/test/SemaOpenCL/vec_step.cl
@@ -29,4 +29,6 @@
   int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires 
built-in scalar or vector type, '__private struct S' invalid}}
   int res14 = vec_step(int16*); // expected-error {{'vec_step' requires 
built-in scalar or vector type, '__private int16 *' invalid}}
   int res15 = vec_step(void(void)); // expected-error {{'vec_step' requires 
built-in scalar or vector type, 'void (void)' invalid}}
+
+  int res_no_effect = vec_step(auto3++); // expected-warning {{expression with 
side effects has no effect in an unevaluated context}}
 }
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -328,6 +328,7 @@
 // expected-warning@-1 {{declaration does not declare 
anything}}
   sizeof(co_await a); // expected-error {{'co_await' cannot be used in an 
unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' 
to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects 
has no effect in an unevaluated context}}
   typeid(co_await a); // expected-error {{'co_await' cannot be used in an 
unevaluated context}}
   // expected-warning@-1 {{expression with side effects 
has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
@@ -335,6 +336,7 @@
 // expected-warning@-1 {{declaration does not declare 
anything}}
   sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an 
unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' 
to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects 
has no effect in an unevaluated context}}
   typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an 
unevaluated context}}
   // expected-warning@-1 {{expression with side effects 
has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4089,7 +4089,7 @@
 
   bool IsUnevaluatedOperand =
   (ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
-   ExprKind == UETT_PreferredAlignOf);
+   ExprKind == UETT_PreferredAlignOf || ExprKind == UETT_VecStep);
   if (IsUnevaluatedOperand) {
 ExprResult Result = CheckUnevaluatedOperand(E);
 if (Result.isInvalid())
@@ -4097,6 +4097,16 @@
 E = Result.get();
   }
 
+  // The operand for sizeof and alignof is in an unevaluated expression 
context,
+  // so side effects could result in unintended consequences.
+  // Exclude instantiation-dependent expressions, because 'sizeof' is sometimes
+  // used to build SFINAE gadgets.
+  // FIXME: Should we consider instantiation-dependent operands to 'alignof'?
+  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
+  !E->isInstantiationDependent() &&
+  E->HasSideEffects(Context, false))
+Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
+
   if (ExprKind == UETT_VecStep)
 return CheckVecStepTraitOperandType(*this, ExprTy, E->getExprLoc(),
 E->getSourceRange());
@@ -4133,16 +4143,6 @@
 return true;
   }
 
-  // The operand for sizeof and alignof is in an unevaluated expression 
context,
-  // so side effects could result in unintended consequences.
-  // Exclude instantiation-dependent expressions, because 'sizeof' is sometimes
-  // used to build SFINAE gadgets.
-  // FIXME: Should we consider instantiation-dependent operands to 'alignof'?
-  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
-  !E->isInstantiationDependent() &&
-  E->HasSideEffects(Context, false))
-Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
-
   if (CheckObjCTraitOperandConstraints(*this, ExprTy, E->getExprLoc(),

[PATCH] D91348: [OpenCL] Warn about side effects for unevaluated vec_step arg

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

In D91348#2475646 , @Anastasia wrote:

> LGTM, the change seems reasonable. Thanks!
>
>> A minor side-effect of this change is that it also produces the
>> warning for co_await and co_yield now.
>
> This warning is produced for **sizeof** and it seems like a duplication of 
> another diagnostic that occurs for this case.

It aligns the note diagnostics for `sizeof` and `vec_step` for this case indeed.

> Btw just a suggestion - it is better to upload a full diff because the 
> regular diff is not reliable in the Phabricator as line numbers change 
> quickly in the repository and there is no information on the parent commit 
> for the patch.

Apologies, I normally do so but forgot to provide the full context this time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91348/new/

https://reviews.llvm.org/D91348

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91348: [OpenCL] Warn about side effects for unevaluated vec_step arg

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: lxfind.

LGTM, the change seems reasonable. Thanks!

> A minor side-effect of this change is that it also produces the
> warning for co_await and co_yield now.

This warning is produced for **sizeof** and it seems like a duplication of 
another diagnostic that occurs for this case. But this happens elsewhere and it 
is not harmful so it seems acceptable. Alternatively, we could try to move code 
that contains `CheckVecStepTraitOperandType` some line below instead of moving 
the warning to the top. But I don't find it necessary.

Btw just a suggestion - it is better to upload a full diff because the regular 
diff is not reliable in the Phabricator as line numbers change quickly in the 
repository and there is no information on the parent commit for the patch.




Comment at: clang/lib/Sema/SemaExpr.cpp:4038
 
+  // The operand for sizeof and alignof is in an unevaluated expression 
context,
+  // so side effects could result in unintended consequences.

The comment could be updated to add vec_step. Although I think the comment has 
already changed in the repo so a rebase would be necessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91348/new/

https://reviews.llvm.org/D91348

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91348: [OpenCL] Warn about side effects for unevaluated vec_step arg

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 305058.
svenvh added a comment.

Add test case for OpenCL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91348/new/

https://reviews.llvm.org/D91348

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaOpenCL/vec_step.cl


Index: clang/test/SemaOpenCL/vec_step.cl
===
--- clang/test/SemaOpenCL/vec_step.cl
+++ clang/test/SemaOpenCL/vec_step.cl
@@ -29,4 +29,6 @@
   int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires 
built-in scalar or vector type, '__private struct S' invalid}}
   int res14 = vec_step(int16*); // expected-error {{'vec_step' requires 
built-in scalar or vector type, '__private int16 *' invalid}}
   int res15 = vec_step(void(void)); // expected-error {{'vec_step' requires 
built-in scalar or vector type, 'void (void)' invalid}}
+
+  int res_no_effect = vec_step(auto3++); // expected-warning {{expression with 
side effects has no effect in an unevaluated context}}
 }
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -328,6 +328,7 @@
 // expected-warning@-1 {{declaration does not declare 
anything}}
   sizeof(co_await a); // expected-error {{'co_await' cannot be used in an 
unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' 
to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects 
has no effect in an unevaluated context}}
   typeid(co_await a); // expected-error {{'co_await' cannot be used in an 
unevaluated context}}
   // expected-warning@-1 {{expression with side effects 
has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
@@ -335,6 +336,7 @@
 // expected-warning@-1 {{declaration does not declare 
anything}}
   sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an 
unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' 
to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects 
has no effect in an unevaluated context}}
   typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an 
unevaluated context}}
   // expected-warning@-1 {{expression with side effects 
has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4027,7 +4027,7 @@
 
   bool IsUnevaluatedOperand =
   (ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
-   ExprKind == UETT_PreferredAlignOf);
+   ExprKind == UETT_PreferredAlignOf || ExprKind == UETT_VecStep);
   if (IsUnevaluatedOperand) {
 ExprResult Result = CheckUnevaluatedOperand(E);
 if (Result.isInvalid())
@@ -4035,6 +4035,12 @@
 E = Result.get();
   }
 
+  // The operand for sizeof and alignof is in an unevaluated expression 
context,
+  // so side effects could result in unintended consequences.
+  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
+  E->HasSideEffects(Context, false))
+Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
+
   if (ExprKind == UETT_VecStep)
 return CheckVecStepTraitOperandType(*this, ExprTy, E->getExprLoc(),
 E->getSourceRange());
@@ -4071,12 +4077,6 @@
 return true;
   }
 
-  // The operand for sizeof and alignof is in an unevaluated expression 
context,
-  // so side effects could result in unintended consequences.
-  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
-  E->HasSideEffects(Context, false))
-Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
-
   if (CheckObjCTraitOperandConstraints(*this, ExprTy, E->getExprLoc(),
E->getSourceRange(), ExprKind))
 return true;


Index: clang/test/SemaOpenCL/vec_step.cl
===
--- clang/test/SemaOpenCL/vec_step.cl
+++ clang/test/SemaOpenCL/vec_step.cl
@@ -29,4 +29,6 @@
   int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private struct S' invalid}}
   int res14 = vec_step(int16*); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private int16 *' invalid}}
   int res15 = vec_step(void(void)); // expected-error {{'vec_step' requires built-in scalar or vector type, 'void (void)' invalid}}
+
+  int res_no_effect = 

[PATCH] D91348: [OpenCL] Warn about side effects for unevaluated vec_step arg

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh created this revision.
svenvh added a reviewer: Anastasia.
Herald added a subscriber: yaxunl.
Herald added a project: clang.
svenvh requested review of this revision.

The argument to the `vec_step` builtin is not evaluated.  Hoist the
diagnostic for this in `Sema::CheckUnaryExprOrTypeTraitOperand` such
that it comes before `Sema::CheckVecStepTraitOperandType`.

A minor side-effect of this change is that it also produces the
warning for `co_await` and `co_yield` now.

Associated spec clarification: 
https://github.com/KhronosGroup/OpenCL-Docs/pull/481


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91348

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/coroutines.cpp


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -328,6 +328,7 @@
 // expected-warning@-1 {{declaration does not declare 
anything}}
   sizeof(co_await a); // expected-error {{'co_await' cannot be used in an 
unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' 
to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects 
has no effect in an unevaluated context}}
   typeid(co_await a); // expected-error {{'co_await' cannot be used in an 
unevaluated context}}
   // expected-warning@-1 {{expression with side effects 
has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
@@ -335,6 +336,7 @@
 // expected-warning@-1 {{declaration does not declare 
anything}}
   sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an 
unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' 
to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects 
has no effect in an unevaluated context}}
   typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an 
unevaluated context}}
   // expected-warning@-1 {{expression with side effects 
has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4027,7 +4027,7 @@
 
   bool IsUnevaluatedOperand =
   (ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
-   ExprKind == UETT_PreferredAlignOf);
+   ExprKind == UETT_PreferredAlignOf || ExprKind == UETT_VecStep);
   if (IsUnevaluatedOperand) {
 ExprResult Result = CheckUnevaluatedOperand(E);
 if (Result.isInvalid())
@@ -4035,6 +4035,12 @@
 E = Result.get();
   }
 
+  // The operand for sizeof and alignof is in an unevaluated expression 
context,
+  // so side effects could result in unintended consequences.
+  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
+  E->HasSideEffects(Context, false))
+Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
+
   if (ExprKind == UETT_VecStep)
 return CheckVecStepTraitOperandType(*this, ExprTy, E->getExprLoc(),
 E->getSourceRange());
@@ -4071,12 +4077,6 @@
 return true;
   }
 
-  // The operand for sizeof and alignof is in an unevaluated expression 
context,
-  // so side effects could result in unintended consequences.
-  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
-  E->HasSideEffects(Context, false))
-Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
-
   if (CheckObjCTraitOperandConstraints(*this, ExprTy, E->getExprLoc(),
E->getSourceRange(), ExprKind))
 return true;


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -328,6 +328,7 @@
 // expected-warning@-1 {{declaration does not declare anything}}
   sizeof(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
   // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  // expected-warning@-2 {{expression with side effects has no effect in an unevaluated context}}
   typeid(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
   // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
   // expected-warning@-2 {{expression result unused}}
@@ -335,6 +336,7 @@
 // expected-warning@-1 {{declaration does not declare anything}}
   sizeof(co_yield