Loop pragma parsing. NFC.

[SjoerdMeijer]

Summary

Yesterday I was a bit distracted by loop pragma parsing (D64471), but still am a bit today too (this patch).

[SjoerdMeijer]

clang/lib/Parse/ParsePragma.cpp

1015

I guess this does behave slightly different, i.e. the assert in a release build.

Before, in a release build, "unroll" was returned for an unexpected pragma, but now we return the empty string "".

[Meinersbur]

clang/lib/Parse/ParsePragma.cpp

1009–1017

getName() returns StringRef. No need to use a std::string yet.

1011–1014

This unconditionally creates (at least) 5 std::string objects.

1041–1046

[style] We don't use const for local variables. Could also use auto here since the type is already explicit on the RHS.

[suggestion] IsLoopHint would indicate the meaning of the boolean variable.

[SjoerdMeijer]

thanks for taking a look and the suggestions!

[SjoerdMeijer]

This comment was removed by SjoerdMeijer.

[Meinersbur]

LGTM

[Meinersbur]

lib/Parse/ParsePragma.cpp

1011 ↗

(On Diff #209299)

[serious] I know I already accepted the patch, but I just noticed something:
"clang loop " + Str.str() will allocate a temporary std::string, Str will potentially point to it, then the temporary string will be released. Str will then point to released memory and returned by this function, i.e. a use-after-free.

[SjoerdMeijer]

lib/Parse/ParsePragma.cpp

1011 ↗

(On Diff #209299)

Oopsy, thanks for spotting this! Will fix this!

[SjoerdMeijer]

Fixed the string problems.

[SjoerdMeijer]

this is not important at all, but might be nice clean up, so friendly ping :-)

[Meinersbur]

clang/lib/Parse/ParsePragma.cpp

1010

[serious] Use-after-free here again. This line will do the following:

StringRef ClangLoopStr;
{
std::string tmp = "clang loop " + Str.str()
ClangLoopStr = tmp;
// tmp.~string() 
}
// Any use of ClangLoopStr will use memory released by tmp.~string()

Let me suggest a solution:

std::string ClangLoopStr = (Twine("clang loop ") + Str).str();
std::string Result = llvm::StringSwitch<StringRef>(Str)
               .Case("loop", ClangLoopStr)
               .Case("unroll_and_jam", Str)
               .Case("unroll", Str)
               .Default("");
return Result; // NRVO, ClangLoopStr will be released here, but if it was chosen by the StringSwitch, Result will hold a copy, so ClangLoopStr is not referenced anymore.

Note that this will alloc one more std::string in the non-ClangLoopStr cases than before the patch, but I don't think it's important.

[SjoerdMeijer]

Sorry, should have done my std::string, StringRef, and Twine homework a lot better!

Thanks for your help and suggestions, will fix this.

[SjoerdMeijer]

thanks for the suggestions; comments addressed.

[Meinersbur]

clang/lib/Parse/ParsePragma.cpp

1010

[nit] I am surprised it works without llvm:: qualifier for llvm::Twine. Maybe add it for consistency?

1016

[nit] One could just return llvm::StringSwitch... without a Result variable. I used it in the suggestion so I could comment on what happens when returning. Personally, there is nothing that I could care less about, so use what you prefer.

[SjoerdMeijer]

Thanks, and will fix your suggestions before committing.

clang/lib/Parse/ParsePragma.cpp

 struct PragmaLoopHintInfo {
   Token PragmaName;
   Token Option;
   ArrayRef<Token> Toks;
 };
 } // end anonymous namespace
 
 static std::string PragmaLoopHintString(Token PragmaName, Token Option) {
-  std::string PragmaString;
-  if (PragmaName.getIdentifierInfo()->getName() == "loop") {
-    PragmaString = "clang loop ";
-    PragmaString += Option.getIdentifierInfo()->getName();
-  } else if (PragmaName.getIdentifierInfo()->getName() == "unroll_and_jam") {
-    PragmaString = "unroll_and_jam";
-  } else {
-    assert(PragmaName.getIdentifierInfo()->getName() == "unroll" &&
-           "Unexpected pragma name");
-    PragmaString = "unroll";
-  }
-  return PragmaString;
+  StringRef Str = PragmaName.getIdentifierInfo()->getName();
+  StringRef ClangLoopStr = "clang loop " + Str.str();
+  Str = llvm::StringSwitch<StringRef>(Str)
+               .Case("loop", ClangLoopStr)
+               .Case("unroll_and_jam", Str)
+               .Case("unroll", Str)
+               .Default("");
+  assert(Str.size() && "Unexpected pragma name");
+  return std::string(Str);
 }
 
 bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
   assert(Tok.is(tok::annot_pragma_loop_hint));
   PragmaLoopHintInfo *Info =
       static_cast<PragmaLoopHintInfo *>(Tok.getAnnotationValue());
 
   IdentifierInfo *PragmaNameInfo = Info->PragmaName.getIdentifierInfo();
   Hint.PragmaNameLoc = IdentifierLoc::create(
       Actions.Context, Info->PragmaName.getLocation(), PragmaNameInfo);
 
   // It is possible that the loop hint has no option identifier, such as
   // #pragma unroll(4).
   IdentifierInfo *OptionInfo = Info->Option.is(tok::identifier)
                                    ? Info->Option.getIdentifierInfo()
                                    : nullptr;
   Hint.OptionLoc = IdentifierLoc::create(
       Actions.Context, Info->Option.getLocation(), OptionInfo);
 
   llvm::ArrayRef<Token> Toks = Info->Toks;
 
   // Return a valid hint if pragma unroll or nounroll were specified
   // without an argument.
-  bool PragmaUnroll = PragmaNameInfo->getName() == "unroll";
-  bool PragmaNoUnroll = PragmaNameInfo->getName() == "nounroll";
-  bool PragmaUnrollAndJam = PragmaNameInfo->getName() == "unroll_and_jam";
-  bool PragmaNoUnrollAndJam = PragmaNameInfo->getName() == "nounroll_and_jam";
-  if (Toks.empty() && (PragmaUnroll || PragmaNoUnroll || PragmaUnrollAndJam ||
-                       PragmaNoUnrollAndJam)) {
+  auto IsLoopHint = llvm::StringSwitch<bool>(PragmaNameInfo->getName())
+                        .Cases("unroll", "nounroll", "unroll_and_jam",
+                               "nounroll_and_jam", true)
+                        .Default(false);
+
+  if (Toks.empty() && IsLoopHint) {
     ConsumeAnnotationToken();
     Hint.Range = Info->PragmaName.getLocation();
     return true;
   }
 
   // The constant expression is always followed by an eof token, which increases
   // the TokSize by 1.
   assert(!Toks.empty() &&