[preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[akyrtzi]

Summary

This is useful for being able to parse the preprocessor directive blocks even the header that defined the macro that they check for hasn't been included.

[voskresensky.vladimir]

Hello Argyrios,
This is a good addition to simplify reuse of preprocessor in IDEs. Thanks for doing this.

From our experience of integrating clang PP into NetBeans, the following change gives more flexibility:

• introduce method in PPCallbacks and consult it what is the preferred value for undefined symbol

It will be similar to approach used for unresolved includes and FileNotFound method.
It also gives dynamic behavior for clients: client might still prefer to keep skipping some blocks like unix, mac, win, solaris, LP64, cplusplus, ...

Thanks,
Vladimir.

[akyrtzi]

Hey Vladimir, what you are proposing is orthogonal to this patch. You are proposing for "the client to provide the value for an undefined identifier", and the patch is about the client not knowing what the value should be so it fallbacks to parsing all tokens to get the max amount of info. Note that both of the techniques can be combined well, if the client provides the value, the preprocessor will take it into account, otherwise if it is stays unresolved it will fallback to lexing all tokens.
But what you are proposing is not a replacement for what the patch is doing.

[benlangmuir]

It would be nice if the doc comment for the single file parse mode flag was updated to include this new functionality.

lib/Lex/PPDirectives.cpp

2709

Nitpick: this is misaligned. Same with many other calls to pushConditionalLevel in this patch.

2774

Why is wasSkipping false here?

[akyrtzi]

lib/Lex/PPDirectives.cpp

2774

For #if it sets wasskip to true to indicate later for HandleElseDirective that the block should not be skipped.
But here (inside HandleElseDirective) setting wasskip doesn't really affect anything, the HandleEndifDirective doesn't check it. I chose to set it to false as indication that the #else block does get parsed.

[benlangmuir]

lib/Lex/PPDirectives.cpp

2774

Thanks for explaining. That makes sense to me.

[akyrtzi]

Enhanced doc-comment for the preprocessor option, and fixed indentation on a couple of calls.

[benlangmuir]

Thanks, this looks good to me. I'd appreciate if @klimek could take a quick look though.

include/clang/Lex/PreprocessorOptions.h

102

nitpick: should use the full words "maximum" and probably also "information" here

[akyrtzi]

Improving doc comment.

[klimek]

Generally this patch lg from my side - how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang.
When I saw your first patch my reaction was "wow, this works with this little change - cool".
With this patch it's "well, that doesn't seem to add too much complexity, so looks good".
I don't want to realize 5 patches down the line that we're sprinkling all of clang with single file conditional code without some discussion about the strategy with Richard.

include/clang/Lex/Preprocessor.h

1838–1841

A short comment on the struct what the semantics are would be helpful.

[voskresensky.vladimir]

In D34263#782391, @akyrtzi wrote:

Hey Vladimir, what you are proposing is orthogonal to this patch. You are proposing for "the client to provide the value for an undefined identifier", and the patch is about the client not knowing what the value should be so it fallbacks to parsing all tokens to get the max amount of info. Note that both of the techniques can be combined well, if the client provides the value, the preprocessor will take it into account, otherwise if it is stays unresolved it will fallback to lexing all tokens.
But what you are proposing is not a replacement for what the patch is doing.

I'm not sure :-)

What I find problematic in this patch is PPOpts->SingleFileParseMode checks.
Let's suppose we implement what I mentioned above => how is it going to co-exist nicely? I think code will be less understandable with both: check of flag and call to PPCallbacks.
What I propose is to move the logic from the PPOpts single flag into PPCallbacks. And from check of flag to query of PPCallbacks.
Of course when you create PPCallbacks you can consult global SingleFileParseMode to create default implementation to answer "any symbol is defined", so you get your use-case easily handled, but it also gives others more flexibility.

Thanks,
Vladimir.

[akyrtzi]

In D34263#783694, @klimek wrote:

how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang.

There is no other patch for using this preprocessor option. Any related improvements down the road will be about general improvements for error recovery, for example things like this:

• Introduce an UnresolvedTypename type instead of changing unresolved types to int.
• For @interace A : B, don't completely drop B from the super-class list of A if it is unresolved.

These kind of improvements are not conditional.

[akyrtzi]

In D34263#783770, @voskresensky.vladimir wrote:

What I find problematic in this patch is PPOpts->SingleFileParseMode checks.
Let's suppose we implement what I mentioned above => how is it going to co-exist nicely? I think code will be less understandable with both: check of flag and call to PPCallbacks.
What I propose is to move the logic from the PPOpts single flag into PPCallbacks. And from check of flag to query of PPCallbacks.
Of course when you create PPCallbacks you can consult global SingleFileParseMode to create default implementation to answer "any symbol is defined", so you get your use-case easily handled, but it also gives others more flexibility.

There is a bit of misunderstanding on what this patch is doing, the patch is not about answering "any symbol is defined". See my cfe-dev email (http://lists.llvm.org/pipermail/cfe-dev/2017-June/054254.html) that provides some more context.
The patch is doing these 2 things:

• Keep track of whether a preprocessor directive condition used a symbol that was not resolved at all
• If above is true then make the preprocessor parse all directive blocks, not arbitrary choose one of them.

Here's an example to clarify the difference:

Say that you have this code:

#if TARGET_IOS
@interface A @end
#else
@interface B @end
#endif

1. Without any changes this will happen:
   • TARGET_IOS is unresolved so it will be treated as 0
   • #if block will be skipped, #else will be parsed
   • AST will contain @interface B

2. With adding a PPCallback that answers "any symbol is defined" (what you are proposing):
   • TARGET_IOS is undefined but the PPCallback sets it to 1
   • #if block will be parsed, #else will be skipped
   • AST will contain @interface A

3. With this patch and enabling SingleFileParseMode
   • TARGET_IOS is unresolved
   • Both #if and #else blocks will be parsed
   • AST will contain both @interface A and @interface B

As you can see 2. and 3. above are not overlapping so they can "co-exist nicely" like this:

• TARGET_IOS is undefined, ask the PPCallback if it has a value for it or not
  • If the callback provides a value then parse as 1. or 2. depending on the value
  • If the callback cannot provide a specific value then parse as 3.

[akyrtzi]

Provide doc-comments for struct DirectiveEvalResult.

[akyrtzi]

Update doc-comment for Preprocessor::EvaluateDirectiveExpression

[klimek]

In D34263#784168, @akyrtzi wrote:

In D34263#783694, @klimek wrote:

how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang.

There is no other patch for using this preprocessor option. Any related improvements down the road will be about general improvements for error recovery, for example things like this:

• Introduce an UnresolvedTypename type instead of changing unresolved types to int.
• For @interace A : B, don't completely drop B from the super-class list of A if it is unresolved.

These kind of improvements are not conditional.

That's great! I was always hoping we'd get some diag improvements out of these :D

[klimek]

oh, and lg from my side

[voskresensky.vladimir]

Here's an example to clarify the difference:

Thanks for the example. You are right, I missed this difference in patch.

include/clang/Lex/Preprocessor.h

   /// \brief A fast PTH version of SkipExcludedConditionalBlock.
   void PTHSkipExcludedConditionalBlock();
 
+  /// Information about the result for evaluating an expression for a
+  /// preprocessor directive.
+  struct DirectiveEvalResult {
+    /// Whether the expression was evaluated a true or not.
+    bool Conditional;
+    /// True if the expression contained identifiers that were undefined.
+    bool IncludedUndefinedIds;
+  };
+
   /// \brief Evaluate an integer constant expression that may occur after a
   /// \#if or \#elif directive and return it as a bool.
   ///
   /// If the expression is equivalent to "!defined(X)" return X in IfNDefMacro.
-  bool EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro);
+  DirectiveEvalResult EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro);
 
   /// \brief Install the standard preprocessor pragmas:
   /// \#pragma GCC poison/system_header/dependency and \#pragma once.

include/clang/Lex/PreprocessorOptions.h

   std::string TokenCache;
 
   /// When enabled, preprocessor is in a mode for parsing a single file only.
+  ///
+  /// Disables #includes of other files and if there are unresolved identifiers
+  /// in preprocessor directive conditions it causes all blocks to be parsed so
+  /// that the client can get the maximum amount of information from the parser.
   bool SingleFileParseMode = false;
 
   /// When enabled, the preprocessor will construct editor placeholder tokens.

lib/Lex/PPDirectives.cpp

           assert(CurPPLexer->LexingRawMode && "We have to be skipping here!");
           CurPPLexer->LexingRawMode = false;
           IdentifierInfo *IfNDefMacro = nullptr;
-          const bool CondValue = EvaluateDirectiveExpression(IfNDefMacro);
+          const bool CondValue = EvaluateDirectiveExpression(IfNDefMacro).Conditional;
           CurPPLexer->LexingRawMode = true;
           if (Callbacks) {
             const SourceLocation CondEnd = CurPPLexer->getSourceLocation();

     // Evaluate the condition of the #elif.
     IdentifierInfo *IfNDefMacro = nullptr;
     CurPTHLexer->ParsingPreprocessorDirective = true;
-    bool ShouldEnter = EvaluateDirectiveExpression(IfNDefMacro);
+    bool ShouldEnter = EvaluateDirectiveExpression(IfNDefMacro).Conditional;
     CurPTHLexer->ParsingPreprocessorDirective = false;
 
     // If this condition is true, enter it!

   }
 
   // Should we include the stuff contained by this directive?
-  if (!MI == isIfndef) {
+  if (PPOpts->SingleFileParseMode && !MI) {
+    // In 'single-file-parse mode' undefined identifiers trigger parsing of all
+    // the directive blocks.
+    CurPPLexer->pushConditionalLevel(DirectiveTok.getLocation(),
+                                     /*wasskip*/true, /*foundnonskip*/false,
+                                     /*foundelse*/false);
+  } else if (!MI == isIfndef) {
     // Yes, remember that we are inside a conditional, then lex the next token.
     CurPPLexer->pushConditionalLevel(DirectiveTok.getLocation(),
                                      /*wasskip*/false, /*foundnonskip*/true,

   // Parse and evaluate the conditional expression.
   IdentifierInfo *IfNDefMacro = nullptr;
   const SourceLocation ConditionalBegin = CurPPLexer->getSourceLocation();
-  const bool ConditionalTrue = EvaluateDirectiveExpression(IfNDefMacro);
+  const DirectiveEvalResult DER = EvaluateDirectiveExpression(IfNDefMacro);
+  const bool ConditionalTrue = DER.Conditional;
   const SourceLocation ConditionalEnd = CurPPLexer->getSourceLocation();
 
   // If this condition is equivalent to #ifndef X, and if this is the first

                   (ConditionalTrue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False));
 
   // Should we include the stuff contained by this directive?
-  if (ConditionalTrue) {
+  if (PPOpts->SingleFileParseMode && DER.IncludedUndefinedIds) {
+    // In 'single-file-parse mode' undefined identifiers trigger parsing of all
+    // the directive blocks.
+    CurPPLexer->pushConditionalLevel(IfToken.getLocation(), /*wasskip*/true,
+                                     /*foundnonskip*/false, /*foundelse*/false);
+  } else if (ConditionalTrue) {
     // Yes, remember that we are inside a conditional, then lex the next token.
     CurPPLexer->pushConditionalLevel(IfToken.getLocation(), /*wasskip*/false,
                                    /*foundnonskip*/true, /*foundelse*/false);

   if (Callbacks)
     Callbacks->Else(Result.getLocation(), CI.IfLoc);
 
+  if (PPOpts->SingleFileParseMode && CI.WasSkipping) {
+    // In 'single-file-parse mode' undefined identifiers trigger parsing of all
+    // the directive blocks.
+    CurPPLexer->pushConditionalLevel(CI.IfLoc, /*wasskip*/false,
+                                     /*foundnonskip*/true, /*foundelse*/true);
+    return;
+  }
+
   // Finally, skip the rest of the contents of this block.
   SkipExcludedConditionalBlock(CI.IfLoc, /*Foundnonskip*/true,
                                /*FoundElse*/true, Result.getLocation());

                     SourceRange(ConditionalBegin, ConditionalEnd),
                     PPCallbacks::CVK_NotEvaluated, CI.IfLoc);
 
+  if (PPOpts->SingleFileParseMode && CI.WasSkipping) {
+    // In 'single-file-parse mode' undefined identifiers trigger parsing of all
+    // the directive blocks.
+    CurPPLexer->pushConditionalLevel(ElifToken.getLocation(), /*wasskip*/true,
+                                     /*foundnonskip*/false, /*foundelse*/false);
+    return;
+  }
+
   // Finally, skip the rest of the contents of this block.
   SkipExcludedConditionalBlock(CI.IfLoc, /*Foundnonskip*/true,
                                /*FoundElse*/CI.FoundElse,

lib/Lex/PPExpressions.cpp

 
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
                                      Token &PeekTok, bool ValueLive,
+                                     bool &IncludedUndefinedIds,
                                      Preprocessor &PP);
 
 /// DefinedTracker - This struct is used while parsing expressions to keep track

   /// TheMacro - When the state is DefinedMacro or NotDefinedMacro, this
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
+  bool IncludedUndefinedIds = false;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.

   MacroDefinition Macro = PP.getMacroDefinition(II);
   Result.Val = !!Macro;
   Result.Val.setIsUnsigned(false); // Result is signed intmax_t.
+  DT.IncludedUndefinedIds = !Macro;
 
   // If there is a macro, mark it used.
   if (Result.Val != 0 && ValueLive)

     Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
     Result.setIdentifier(II);
     Result.setRange(PeekTok.getLocation());
+    DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
+                               II->getTokenID() != tok::kw_false);
     PP.LexNonComment(PeekTok);
     return false;
   }

       // Just use DT unmodified as our result.
     } else {
       // Otherwise, we have something like (x+y), and we consumed '(x'.
-      if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive, PP))
+      if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive,
+                                   DT.IncludedUndefinedIds, PP))
         return true;
 
       if (PeekTok.isNot(tok::r_paren)) {

 /// evaluation, such as division by zero warnings.
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
                                      Token &PeekTok, bool ValueLive,
+                                     bool &IncludedUndefinedIds,
                                      Preprocessor &PP) {
   unsigned PeekPrec = getPrecedence(PeekTok.getKind());
   // If this token isn't valid, report the error.

     // Parse the RHS of the operator.
     DefinedTracker DT;
     if (EvaluateValue(RHS, PeekTok, DT, RHSIsLive, PP)) return true;
+    IncludedUndefinedIds = DT.IncludedUndefinedIds;
 
     // Remember the precedence of this operator and get the precedence of the
     // operator immediately to the right of the RHS.

       RHSPrec = ThisPrec+1;
 
     if (PeekPrec >= RHSPrec) {
-      if (EvaluateDirectiveSubExpr(RHS, RHSPrec, PeekTok, RHSIsLive, PP))
+      if (EvaluateDirectiveSubExpr(RHS, RHSPrec, PeekTok, RHSIsLive,
+                                   IncludedUndefinedIds, PP))
         return true;
       PeekPrec = getPrecedence(PeekTok.getKind());
     }

       // Parse anything after the : with the same precedence as ?.  We allow
       // things of equal precedence because ?: is right associative.
       if (EvaluateDirectiveSubExpr(AfterColonVal, ThisPrec,
-                                   PeekTok, AfterColonLive, PP))
+                                   PeekTok, AfterColonLive,
+                                   IncludedUndefinedIds, PP))
         return true;
 
       // Now that we have the condition, the LHS and the RHS of the :, evaluate.

 /// EvaluateDirectiveExpression - Evaluate an integer constant expression that
 /// may occur after a #if or #elif directive.  If the expression is equivalent
 /// to "!defined(X)" return X in IfNDefMacro.
-bool Preprocessor::EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro) {
+Preprocessor::DirectiveEvalResult
+Preprocessor::EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro) {
   SaveAndRestore<bool> PPDir(ParsingIfOrElifDirective, true);
   // Save the current state of 'DisableMacroExpansion' and reset it to false. If
   // 'DisableMacroExpansion' is true, then we must be in a macro argument list

     
     // Restore 'DisableMacroExpansion'.
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-    return false;
+    return {false, DT.IncludedUndefinedIds};
   }
 
   // If we are at the end of the expression after just parsing a value, there

 
     // Restore 'DisableMacroExpansion'.
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-    return ResVal.Val != 0;
+    return {ResVal.Val != 0, DT.IncludedUndefinedIds};
   }
 
   // Otherwise, we must have a binary operator (e.g. "#if 1 < 2"), so parse the
   // operator and the stuff after it.
   if (EvaluateDirectiveSubExpr(ResVal, getPrecedence(tok::question),
-                               Tok, true, *this)) {
+                               Tok, true, DT.IncludedUndefinedIds, *this)) {
     // Parse error, skip the rest of the macro line.
     if (Tok.isNot(tok::eod))
       DiscardUntilEndOfDirective();
     
     // Restore 'DisableMacroExpansion'.
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-    return false;
+    return {false, DT.IncludedUndefinedIds};
   }
 
   // If we aren't at the tok::eod token, something bad happened, like an extra

 
   // Restore 'DisableMacroExpansion'.
   DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-  return ResVal.Val != 0;
+  return {ResVal.Val != 0, DT.IncludedUndefinedIds};
 }

test/Index/singe-file-parse.m

 // CHECK: [[@LINE+1]]:8: ObjCInstanceMethodDecl=some_meth
 -(void)some_meth;
 @end
+
+#if 1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test1
+@interface Test1 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test2 @end
+#endif
+
+#if 0
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test3 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test4
+@interface Test4 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test5
+@interface Test5 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test6
+@interface Test6 @end
+#endif
+
+#define SOMETHING_DEFINED 1
+#if SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test7
+@interface Test7 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test8 @end
+#endif
+
+#if defined(SOMETHING_NOT_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test9
+@interface Test9 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test10
+@interface Test10 @end
+#endif
+
+#if defined(SOMETHING_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test11
+@interface Test11 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test12 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test13
+@interface Test13 @end
+#elif SOMETHING_NOT_DEFINED2
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test14
+@interface Test14 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test15
+@interface Test15 @end
+#endif
+
+#ifdef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test19
+@interface Test19 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test20
+@interface Test20 @end
+#endif
+
+#ifdef SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test21
+@interface Test21 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test22 @end
+#endif
+
+#ifndef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test23
+@interface Test23 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test24
+@interface Test24 @end
+#endif
+
+#ifndef SOMETHING_DEFINED
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test25 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test26
+@interface Test26 @end
+#endif
+
+#if 1 < SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test27
+@interface Test27 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test28
+@interface Test28 @end
+#endif