Fix PR20069: bad loop pragma arguments crash FE

[meheff]

Summary

This patch fixes a crash when handling malformed arguments to loop pragmas such as: "#pragma clang loop vectorize(()". Essentially any argument which is not an identifier or constant resulted in a crash. This patch also changes a couple of the error messages which weren't quite correct. New behavior with this patch vs old behavior:

#pragma clang loop vectorize(1)
OLD: error: missing keyword; expected 'enable' or 'disable'
NEW: error: invalid argument; expected 'enable' or 'disable'

#pragma clang loop vectorize()
OLD: error: expected ')'
NEW: error: missing argument to loop pragma 'vectorize'

#pragma clang loop vectorize_width(bad)
OLD: error: missing value; expected a positive integer value
NEW: error: invalid argument; expected a positive integer value

#pragma clang loop vectorize(bad)
OLD: invalid keyword 'bad'; expected 'enable' or 'disable'
NEW: error: invalid argument; expected 'enable' or 'disable'

In the last example above, the old behavior isn't incorrect. I made the change as it simplified the code a bit and made the message consistent regardless of what the argument is (rather than just printing the argument if it is an identifier). Happy to change it back if anyone thinks it's a step backwards.

include/clang/Basic/DiagnosticParseKinds.td

def err_omp_unexpected_clause : Error<
   "unexpected OpenMP clause '%0' in directive '#pragma omp %1'">;
 def err_omp_more_one_clause : Error<
   "directive '#pragma omp %0' cannot contain more than one '%1' clause">;
 
 // Pragma loop support.
 def err_pragma_loop_invalid_option : Error<
   "%select{invalid|missing}0 option%select{ %1|}0; expected vectorize, "
   "vectorize_width, interleave, interleave_count, unroll, or unroll_count">;
+def err_pragma_loop_missing_argument : Error<
+  "missing argument to loop pragma %0">;
 } // end of Parse Issue category.
 
 let CategoryName = "Modules Issue" in {
 def err_module_expected_ident : Error<
   "expected a module name after module import">;
 def err_module_expected_semi : Error<
   "expected ';' after module name">;
 }
 
 } // end of Parser diagnostics

include/clang/Basic/DiagnosticSemaKinds.td

def err_pragma_push_visibility_mismatch : Error<
   "#pragma visibility push with no matching #pragma visibility pop">;
 def note_surrounding_namespace_ends_here : Note<
   "surrounding namespace with visibility attribute ends here">;
 def err_pragma_pop_visibility_mismatch : Error<
   "#pragma visibility pop with no matching #pragma visibility push">;
 def note_surrounding_namespace_starts_here : Note<
   "surrounding namespace with visibility attribute starts here">;
 def err_pragma_loop_invalid_value : Error<
-  "%select{invalid|missing}0 value%select{ %1|}0; expected a positive integer value">;
+  "invalid argument; expected a positive integer value">;
 def err_pragma_loop_invalid_keyword : Error<
-  "%select{invalid|missing}0 keyword%select{ %1|}0; expected 'enable' or 'disable'">;
+  "invalid argument; expected 'enable' or 'disable'">;
 def err_pragma_loop_compatibility : Error<
   "%select{incompatible|duplicate}0 directives '%1(%2)' and '%3(%4)'">;
 def err_pragma_loop_precedes_nonloop : Error<
   "expected a for, while, or do-while loop to follow the '#pragma clang loop' "
   "directive">;
 
 /// Objective-C parser diagnostics
 def err_duplicate_class_def : Error<

lib/Parse/ParsePragma.cpp

while (Tok.is(tok::identifier)) {
     if (Tok.isNot(tok::l_paren)) {
       PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;
       return;
     }
 
     // FIXME: All tokens between '(' and ')' should be stored and parsed as a
     // constant expression.
     PP.Lex(Tok);
-    Token Value;
-    if (Tok.is(tok::identifier) || Tok.is(tok::numeric_constant))
-      Value = Tok;
+    if (Tok.is(tok::r_paren)) {
+      // Nothing between the parentheses.
+      PP.Diag(Tok.getLocation(), diag::err_pragma_loop_missing_argument)
+          << OptionInfo;
+      return;
+    }
+    Token Value = Tok;
 
     // Read ')'
     PP.Lex(Tok);
     if (Tok.isNot(tok::r_paren)) {
       PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren;
       return;
     }
 

lib/Sema/SemaStmtAttr.cpp

LoopHintAttr::OptionType Option =
           .Case("unroll", LoopHintAttr::Unroll)
           .Case("unroll_count", LoopHintAttr::UnrollCount)
           .Default(LoopHintAttr::Vectorize);
 
   int ValueInt;
   if (Option == LoopHintAttr::Vectorize || Option == LoopHintAttr::Interleave ||
       Option == LoopHintAttr::Unroll) {
     if (!ValueInfo) {
-      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword)
-          << /*MissingKeyword=*/true << "";
+      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword);
       return nullptr;
     }
-
     if (ValueInfo->isStr("disable"))
       ValueInt = 0;
     else if (ValueInfo->isStr("enable"))
       ValueInt = 1;
     else {
-      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword)
-          << /*MissingKeyword=*/false << ValueInfo;
+      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword);
       return nullptr;
     }
   } else if (Option == LoopHintAttr::VectorizeWidth ||
              Option == LoopHintAttr::InterleaveCount ||
              Option == LoopHintAttr::UnrollCount) {
     // FIXME: We should support template parameters for the loop hint value.
     // See bug report #19610.
     llvm::APSInt ValueAPS;
-    if (!ValueExpr || !ValueExpr->isIntegerConstantExpr(ValueAPS, S.Context)) {
-      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value)
-          << /*MissingValue=*/true << "";
-      return nullptr;
-    }
-
-    if ((ValueInt = ValueAPS.getSExtValue()) < 1) {
-      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value)
-          << /*MissingValue=*/false << ValueInt;
+    if (!ValueExpr || !ValueExpr->isIntegerConstantExpr(ValueAPS, S.Context) ||
+        (ValueInt = ValueAPS.getSExtValue()) < 1) {
+      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value);
       return nullptr;
     }
   } else
     llvm_unreachable("Unknown loop hint option");
 
   return LoopHintAttr::CreateImplicit(S.Context, Option, ValueInt,
                                       A.getRange());
 }

test/Parser/pragma-loop.cpp

 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize(enable
 /* expected-error {{expected ')'}} */ #pragma clang loop interleave(enable
 /* expected-error {{expected ')'}} */ #pragma clang loop unroll(enable
 
 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize_width(4
 /* expected-error {{expected ')'}} */ #pragma clang loop interleave_count(4
 /* expected-error {{expected ')'}} */ #pragma clang loop unroll_count(4
 
+/* expected-error {{missing argument to loop pragma 'vectorize'}} */ #pragma clang loop vectorize()
+/* expected-error {{missing argument to loop pragma 'interleave_count'}} */ #pragma clang loop interleave_count()
+/* expected-error {{missing argument to loop pragma 'unroll'}} */ #pragma clang loop unroll()
+
 /* expected-error {{missing option}} */ #pragma clang loop
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword(enable)
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop vectorize(enable) badkeyword(4)
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize(enable) ,
 
   while (i-4 < Length) {
     List[i] = i;
   }
 
-/* expected-error {{invalid value 0; expected a positive integer value}} */ #pragma clang loop vectorize_width(0)
-/* expected-error {{invalid value 0; expected a positive integer value}} */ #pragma clang loop interleave_count(0)
-/* expected-error {{invalid value 0; expected a positive integer value}} */ #pragma clang loop unroll_count(0)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop vectorize_width(0)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop interleave_count(0)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop unroll_count(0)
   while (i-5 < Length) {
     List[i] = i;
   }
 
-/* expected-error {{invalid value -1294967296; expected a positive integer value}} */ #pragma clang loop vectorize_width(3000000000)
-/* expected-error {{invalid value -1294967296; expected a positive integer value}} */ #pragma clang loop interleave_count(3000000000)
-/* expected-error {{invalid value -1294967296; expected a positive integer value}} */ #pragma clang loop unroll_count(3000000000)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop vectorize_width(3000000000)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop interleave_count(3000000000)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop unroll_count(3000000000)
   while (i-6 < Length) {
     List[i] = i;
   }
 
-/* expected-error {{missing value; expected a positive integer value}} */ #pragma clang loop vectorize_width(badvalue)
-/* expected-error {{missing value; expected a positive integer value}} */ #pragma clang loop interleave_count(badvalue)
-/* expected-error {{missing value; expected a positive integer value}} */ #pragma clang loop unroll_count(badvalue)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop vectorize_width(badvalue)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop interleave_count(badvalue)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop unroll_count(badvalue)
   while (i-6 < Length) {
     List[i] = i;
   }
 
-/* expected-error {{invalid keyword 'badidentifier'; expected 'enable' or 'disable'}} */ #pragma clang loop vectorize(badidentifier)
-/* expected-error {{invalid keyword 'badidentifier'; expected 'enable' or 'disable'}} */ #pragma clang loop interleave(badidentifier)
-/* expected-error {{invalid keyword 'badidentifier'; expected 'enable' or 'disable'}} */ #pragma clang loop unroll(badidentifier)
+/* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop vectorize(badidentifier)
+/* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop interleave(badidentifier)
+/* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop unroll(badidentifier)
   while (i-7 < Length) {
     List[i] = i;
   }
 
+// PR20069 - Loop pragma arguments that are not identifiers or numeric
+// constants crash FE.
+/* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop vectorize(()
+/* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop interleave(*)
+/* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop unroll(=)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop vectorize_width(^)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop interleave_count(/)
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop unroll_count(==)
+  while (i-8 < Length) {
+    List[i] = i;
+  }
+
 #pragma clang loop vectorize(enable)
 /* expected-error {{expected a for, while, or do-while loop to follow the '#pragma clang loop' directive}} */ int j = Length;
   List[0] = List[1];
 
   while (j-1 < Length) {
     List[j] = j;
   }