[OpenCL] Warn about side effects for unevaluated vec_step arg

[svenvh]

Summary

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

[svenvh]

Add test case for OpenCL.

[Anastasia]

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.

clang/lib/Sema/SemaExpr.cpp

4038

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.

[svenvh]

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.

clang/lib/Sema/SemaExpr.cpp

static bool CheckExtensionTraitOperandType(Sema &S, QualType T,
   }
 
   // Allow sizeof(void)/alignof(void) as an extension, unless in OpenCL where
   // this is an error (OpenCL v1.1 s6.3.k)
   if (T->isVoidType()) {
     unsigned DiagID = S.LangOpts.OpenCL ? diag::err_opencl_sizeof_alignof_type
                                         : diag::ext_sizeof_alignof_void_type;
     S.Diag(Loc, DiagID) << getTraitSpelling(TraitKind) << ArgRange;
     return false;
   }
 
   return true;
 }
 
 static bool CheckObjCTraitOperandConstraints(Sema &S, QualType T,
                                              SourceLocation Loc,
                                              SourceRange ArgRange,

 /// instantiation, etc.
 bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E,
                                             UnaryExprOrTypeTrait ExprKind) {
   QualType ExprTy = E->getType();
   assert(!ExprTy->isReferenceType());
 
   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())
       return true;
     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());
 
   // Explicitly list some types as extensions.
   if (!CheckExtensionTraitOperandType(*this, ExprTy, E->getExprLoc(),
                                       E->getSourceRange(), ExprKind))
     return false;
bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E,
   assert(!ExprTy->isReferenceType());
 
   if (ExprTy->isFunctionType()) {
     Diag(E->getExprLoc(), diag::err_sizeof_alignof_function_type)
         << getTraitSpelling(ExprKind) << E->getSourceRange();
     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(),
                                        E->getSourceRange(), ExprKind))
     return true;
 
   if (ExprKind == UETT_SizeOf) {
     if (DeclRefExpr *DeclRef = dyn_cast<DeclRefExpr>(E->IgnoreParens())) {
       if (ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(DeclRef->getFoundDecl())) {
         QualType OType = PVD->getOriginalType();

clang/test/SemaCXX/coroutines.cpp

 
 namespace std { class type_info; }
 
 void unevaluated() {
   decltype(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
                         // 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}}
   decltype(co_yield 1); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
                         // 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}}
 }
 
 // [expr.await]p2: "An await-expression shall not appear in a default argument."
 // FIXME: A better diagnostic would explicitly state that default arguments are
 // not allowed. A user may not understand that this is "outside a function."

clang/test/SemaOpenCL/vec_step.cl

void foo(int3 arg1, int8 arg2) {
   int res9[vec_step(int4) == 4 ? 1 : -1];
   int res10[vec_step(int8) == 8 ? 1 : -1];
   int res11[vec_step(int16) == 16 ? 1 : -1];
   int res12[vec_step(void) == 1 ? 1 : -1];
 
   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}}
 }