[Lexer] Report more precise skipped regions (PR34166)

[vsk]

Summary

This patch teaches the preprocessor to report more precise source ranges for code that is skipped due to conditional directives.

The new behavior includes the '#' from the opening directive and the full text of the line containing the closing directive in the skipped area. This matches up clang's behavior (we don't IRGen the code between the closing "endif" and the end of a line).

This also affects the code coverage implementation. See llvm.org/PR34166 (this also happens to be rdar://problem/23224058).

[efriedma]

I'm not sure this produces the right locations in general. Consider the following slightly evil testcase:

#\
if 0
#elif \
11\

int a;
#endif

[vsk]

Thanks for the review. I've updated the patch so that we do better with "#\" directives.

[efriedma]

lib/Lex/PPDirectives.cpp

571

This doesn't really handle backslash-escaped newlines correctly. (Not likely to matter, I guess, but better to get it right while we're messing with it.)

Could we use CurPPLexer->getSourceLocation() or something like that, instead of trying to scan the line ourselves?

[vsk]

• Address Eli's comment.

lib/Lex/PPDirectives.cpp

571

Thanks again for the catch! CurPPLexer->getSourceLocation() does the job. I couldn't find an efficient solution which places the end of the skipped range at the end of the line containing the #endif. With getSourceLocation(), we always set the end location to the first column of the line after the #endif. I think this is OK (at least, it shouldn't affect coverage rendering).

[efriedma]

I'd like to see someone more familiar with indexing comment on the effect there; otherwise LGTM.

[arphaman]

lib/Lex/PPDirectives.cpp

585

You'd have to update the pp-tests in clang-tools-extra too

test/Index/skipped-ranges.c

23–25

Some editor clients might want to grey out the skipped PP ranges. The new end location is not ideal as we wouldn't want to grey out the trailing comments after the #endif.

[vsk]

test/Index/skipped-ranges.c

23–25

It seems like the end location we want should be different for the coverage client and the indexer. I'll modify the callback so we pass along both end locations.

[vsk]

• Add an 'EndifLoc' parameter to the SourceRangeSkipped callback so that indexing clients can preserve their existing behavior.
• I'll submit a follow-up patch which updates the pp-trace tests.

[arphaman]

Thanks, LGTM

lib/Lex/PPDirectives.cpp

void Preprocessor::SkipExcludedConditionalBlock(SourceLocation IfTokenLoc,
   }
 
   // Finally, if we are out of the conditional (saw an #endif or ran off the end
   // of the file, just stop skipping and return to lexing whatever came after
   // the #if block.
   CurPPLexer->LexingRawMode = false;
 
   if (Callbacks) {
+    const auto &SM = getSourceManager();
     SourceLocation BeginLoc = ElseLoc.isValid() ? ElseLoc : IfTokenLoc;
-    Callbacks->SourceRangeSkipped(SourceRange(BeginLoc, Tok.getLocation()));
+
+    // The begin loc tells us the start location of the directive, but we want
+    // to include the hash ('#') at the start of the line.
+    SourceLocation HashBeginLoc =
+        BeginLoc.getLocWithOffset(-SM.getSpellingColumnNumber(BeginLoc) + 1);
+
+    // We also want to skip the entire line containing the closing directive.
+    const char *TokStart = SM.getCharacterData(Tok.getLocation());
+    const char *EndOfLine = TokStart;
+    while (true) {
+      char C = *EndOfLine;
+      // We'll warn about reaching the end of file later.
+      if (C == '\0' || C == '\r' || C == '\n')
+        break;
+      ++EndOfLine;
+    }
+    SourceLocation DirectiveEndLoc =
+        Tok.getLocation().getLocWithOffset(EndOfLine - TokStart);
+
+    Callbacks->SourceRangeSkipped(SourceRange(HashBeginLoc, DirectiveEndLoc));
   }
 }
 
 void Preprocessor::PTHSkipExcludedConditionalBlock() {
   while (true) {
     assert(CurPTHLexer);
     assert(CurPTHLexer->LexingRawMode == false);
 
     // Skip to the next '#else', '#elif', or #endif.
     if (CurPTHLexer->SkipBlock()) {

test/CoverageMapping/preprocessor.c

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name preprocessor.c %s | FileCheck %s
 
                  // CHECK: func
 void func() {    // CHECK: File 0, [[@LINE]]:13 -> [[@LINE+5]]:2 = #0
   int i = 0;
-#ifdef MACRO     // CHECK-NEXT: Skipped,File 0, [[@LINE]]:2 -> [[@LINE+2]]:2 = 0
+#ifdef MACRO     // CHECK-NEXT: Skipped,File 0, [[@LINE]]:1 -> [[@LINE+2]]:7 = 0
   int x = i;
 #endif
 }
 
 #if 0
   int g = 0;
 
   void bar() { }
 #endif
 
                  // CHECK: main
 int main() {     // CHECK-NEXT: File 0, [[@LINE]]:12 -> {{[0-9]+}}:2 = #0
   int i = 0;
-#if 0            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:2 -> [[@LINE+4]]:2 = 0
+#  if 0            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:1 -> [[@LINE+4]]:29 = 0
   if(i == 0) {
     i = 1;
   }
-#endif
+#  endif // Mark me skipped!
 
 #if 1
                  // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+1]]:12 = #0
   if(i == 0) {   // CHECK-NEXT: File 0, [[@LINE]]:14 -> [[@LINE+2]]:4 = #1
     i = 1;
   }
-#else            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:2 -> [[@LINE+5]]:2 = 0
+#else            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:1 -> [[@LINE+5]]:7 = 0
   if(i == 1) {
     i = 0;
   }
 }
 #endif
   return 0;
 }

test/Index/skipped-ranges.c

 #ifndef uncool
 
 int probably_hot = 1;
 
 #endif // uncool
 #endif // cool
 
 // RUN: env CINDEXTEST_SHOW_SKIPPED_RANGES=1 c-index-test -test-annotate-tokens=%s:1:1:16:1 %s | FileCheck %s
-// CHECK: Skipping: [5:2 - 6:7]
-// CHECK: Skipping: [8:2 - 12:7]
-// CHECK: Skipping: [14:2 - 20:7]
+// CHECK: Skipping: [5:1 - 6:22]
+// CHECK: Skipping: [8:1 - 12:27]
+// CHECK: Skipping: [14:1 - 20:15]