[NFC][Clang] Fix static code analyzer concern about null value dereference

[Manna]

Summary

CurLexer is dereferenced and should not be null in clang::​Preprocessor::​SkipExcludedConditionalBlock(clang::​SourceLocation, clang::​SourceLocation, bool, bool, clang::​SourceLocation)

This patch adds an assert for NULL value check of pointer CurLexer.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

2009–2010 ↗

(On Diff #551552)

Current.Previous can't be null here because AutoFound is true.

[tahonermann]

clang/lib/Format/TokenAnnotator.cpp

2009–2010 ↗

(On Diff #551552)

Could you please elaborate on why you believe it is safe to move the check of Current.Previous inside the body of the if statement? Doing so will short circuit the remaining else if cases such that Current.setType() will not be called at all. It isn't obvious to me that those cases should not be considered if the previous token was not one of kw_operator or identifier. This looks like it has potential to change behavior.

The change that was originally proposed is clearly safe.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

2009–2010 ↗

(On Diff #551552)

Could you please elaborate on why you believe it is safe to move the check of Current.Previous inside the body of the if statement? Doing so will short circuit the remaining else if cases such that Current.setType() will not be called at all. It isn't obvious to me that those cases should not be considered if the previous token was not one of kw_operator or identifier. This looks like it has potential to change behavior.

Ahh, you are right.

The change that was originally proposed is clearly safe.

My point that Previous can't be null still stands. So we should either make no changes here or add an assertion just before the if statement at line 1991 above.

[Manna]

This comment was removed by Manna.

[Manna]

clang/lib/Format/TokenAnnotator.cpp

2009–2010 ↗

(On Diff #551552)

Thank you @owenpan and @tahonermann for reviews and feedbacks, I have removed my previous change since Current.Previous can't be null here.

[tahonermann]

clang/lib/Lex/PPDirectives.cpp

494–495

Perhaps it would make sense to assert CurLexer here as well.

556

I don't think this change is sufficient. If CurLexer is null, then the else branch will be entered and an unconditional dereference occurs there as well. It looks like more analysis is needed, but perhaps the correct fix is to add a non-null assertion somewhere above.

[Manna]

clang/lib/Lex/PPDirectives.cpp

556

Thank you @tahonermann for review and feedback. Yes, my analysis was wrong.

perhaps the correct fix is to add a non-null assertion somewhere above.

It sounds reasonable to me. I have added an assert above.

[tahonermann]

clang/lib/Lex/PPDirectives.cpp

494–496

I would prefer that this assert be split up so that, when/if a failure occurs, we'll be able to tell which predicate failed.

[Manna]

Thanks @tahonermann for review and feedback. I have added an assert separately to tell which predicate failed if a failure occurs in future.

[tahonermann]

clang/lib/Lex/PPDirectives.cpp

494–495

Would you mind splitting out all of the predicates? The updates to the messages in the suggested edit are intended to match the style present in other similar assertions.

[Manna]

This patch splits all predicates and updates assert messages.

[Manna]

clang/lib/Lex/PPDirectives.cpp

494–495

Thanks @tahonermann for the suggestion. Done

[tahonermann]

Thanks, Soumi, looks good to me!

[Manna]

In D158293#4629232, @tahonermann wrote:

Thanks, Soumi, looks good to me!

Thank you @tahonermann for reviews!

clang/lib/Lex/PPDirectives.cpp

void Preprocessor::SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
   // SkipExcludedConditionalBlock() recursively, SkippingRangeStateTy should
   // change to do a second lookup in endLexPass function instead of reusing the
   // lookup pointer.
   assert(!SkippingExcludedConditionalBlock &&
          "calling SkipExcludedConditionalBlock recursively");
   llvm::SaveAndRestore SARSkipping(SkippingExcludedConditionalBlock, true);
 
   ++NumSkipped;
   assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
+  assert(CurLexer && "Invalid lexer value");
 
   if (PreambleConditionalStack.reachedEOFWhileSkipping())
     PreambleConditionalStack.clearSkipInfo();
   else
     CurPPLexer->pushConditionalLevel(IfTokenLoc, /*isSkipping*/ false,
                                      FoundNonSkipPortion, FoundElse);
 
   // Enter raw mode to disable identifier lookup (and thus macro expansion),
   // disabling warnings, etc.
void Preprocessor::SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
   } SkippingRangeState(*this);
 
   while (true) {
     if (CurLexer->isDependencyDirectivesLexer()) {
       CurLexer->LexDependencyDirectiveTokenWhileSkipping(Tok);
     } else {
       SkippingRangeState.beginLexPass();
       while (true) {
         CurLexer->Lex(Tok);
 
         if (Tok.is(tok::code_completion)) {
           setCodeCompletionReached();
           if (CodeComplete)
             CodeComplete->CodeCompleteInConditionalExclusion();
           continue;
         }