[NFC][CLANG] Fix dereference issue before null check found by Coverity static analyzer tool

[Manna]

Summary

Reported by Coverity static analyzer tool:

Inside "ParsePragma.cpp" file, in <unnamed>::​PragmaRISCVHandler::​HandlePragma(clang::​Preprocessor &, clang::​PragmaIntroducer, clang::​Token &): All paths that lead to this null pointer comparison already dereference the pointer earlier

PP.Lex(Tok);
II = Tok.getIdentifierInfo();
//deref_ptr_in_call: Dereferencing pointer II. 
StringRef IntrinsicClass = II->getName();
    	
//Dereference before null check (REVERSE_INULL)
//check_after_deref: Null-checking II suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
  if (!II || !(II->isStr("vector") || II->isStr("sifive_vector"))) {
    PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
        << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true
        << "'vector' or 'sifive_vector'";
    return;
}

This patch moves the line StringRef IntrinsicClass = II->getName() after null checking II

[erichkeane]

clang/lib/Parse/ParsePragma.cpp

4042–4044

I think you still need line 4043 here, right? The one where II is set to the Token?

[Manna]

I have updated patch.

[erichkeane]

clang/lib/Parse/ParsePragma.cpp

4058

I'm having trouble figuring out why this isn't a breaking change. PP.Lex(Tok) moves the current 'token' to be the next one (that is, it has side effects!), so now this ends up looking in a different place for the intrin name?

This will result in IntrinsicClass becoming the value of the token 2 after the fact, which doesn't seem right to me. I suspect the correct answer here is to just move IntrinsicClass ~4050.

[Manna]

clang/lib/Parse/ParsePragma.cpp

4042–4044

oops! Fixed now. Thanks @erichkeane for catching!

4058

I am wondering whether do we need IntrinsicClass`. Can we do something like:

if (II->isStr("vector"))
    Actions.DeclareRISCVVBuiltins = true;
  else if (II->isStr("sifive_vector")
    Actions.DeclareRISCVVectorBuiltins = true;

[erichkeane]

clang/lib/Parse/ParsePragma.cpp

4058

Probably not necessary, as long as II doesn't after ~4044.

[Manna]

I have removed IntrinsicClass and updated patch.

[erichkeane]

clang/lib/Parse/ParsePragma.cpp

4051

So I'd say leave this where it was. Once line 4043 happens, II is stable, we just need to make sure it doesn't get changed (which I see it doesnt). So I'd probably just put this back where it was.

[Manna]

clang/lib/Parse/ParsePragma.cpp

4051

Thank you @erichkeane!

clang/lib/Parse/ParsePragma.cpp

void PragmaRISCVHandler::HandlePragma(Preprocessor &PP,
   IdentifierInfo *II = Tok.getIdentifierInfo();
 
   if (!II || !II->isStr("intrinsic")) {
     PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
         << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'intrinsic'";
     return;
   }
 
   PP.Lex(Tok);
   II = Tok.getIdentifierInfo();
-  StringRef IntrinsicClass = II->getName();
   if (!II || !(II->isStr("vector") || II->isStr("sifive_vector"))) {
     PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
         << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true
         << "'vector' or 'sifive_vector'";
     return;
   }
 
   PP.Lex(Tok);
   if (Tok.isNot(tok::eod)) {
     PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
         << "clang riscv intrinsic";
     return;
   }
 
-  if (IntrinsicClass == "vector")
+  if (II->isStr("vector"))
     Actions.DeclareRISCVVBuiltins = true;
-  else if (IntrinsicClass == "sifive_vector")
+  else if (II->isStr("sifive_vector"))
     Actions.DeclareRISCVVectorBuiltins = true;
 }