[Coverage] Hide coverage for regions with incorrect end locations (PR39942)

[vsk]

Summary

... instead of crashing.

The real bug here is due to clang generating coverage for a conditional
operator expanded from a macro. The location of the end of the
conditional expression is not correct. The actual fix for this requires
chasing down an issue in the lexer/preprocessor.

[rnk]

I did some digging and I figured out where things go wrong. The issue is the repeated string-izing of the macro argument, the repetition of #func here. This hacky patch seems to fix the issue:

diff --git a/clang/lib/Lex/MacroArgs.cpp b/clang/lib/Lex/MacroArgs.cpp
index 5aa4679fad4..29fd25a43bb 100644
--- a/clang/lib/Lex/MacroArgs.cpp
+++ b/clang/lib/Lex/MacroArgs.cpp
@@ -318,7 +318,7 @@ const Token &MacroArgs::getStringifiedArgument(unsigned ArgNo,
   if (StringifiedArgs.empty())
     StringifiedArgs.resize(getNumMacroArguments(), {});

-  if (StringifiedArgs[ArgNo].isNot(tok::string_literal))
+  //if (StringifiedArgs[ArgNo].isNot(tok::string_literal))
     StringifiedArgs[ArgNo] = StringifyArgument(getUnexpArgument(ArgNo), PP,
                                                /*Charify=*/false,
                                                ExpansionLocStart,

Basically, the second time we process a string-ized macro argument, we hit the cache, which uses the wrong expansion location for the second expansion. I'm going to try cleaning this up and we'll see if the cache is really needed.

[rnk]

PTAL: https://reviews.llvm.org/D65428

[vsk]

Thanks Reid!

clang/lib/CodeGen/CoverageMappingGen.cpp

while (RegionStack.size() > ParentIndex) {
         MostRecentLocation = EndLoc;
         // If this region happens to span an entire expansion, we need to make
         // sure we don't overlap the parent region with it.
         if (StartLoc == getStartOfFileOrMacro(StartLoc) &&
             EndLoc == getEndOfFileOrMacro(EndLoc))
           MostRecentLocation = getIncludeOrExpansionLoc(EndLoc);
 
         assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
-        assert(SpellingRegion(SM, Region).isInSourceOrder());
+
+        // FIXME: See llvm.org/PR39942. The clang preprocessor may not preserve
+        // the correct end location for conditional expressions. Hide coverage
+        // for such regions instead of crashing/asserting. This should be
+        // turned back into an assert.
+        if (!SpellingRegion(SM, Region).isInSourceOrder())
+          Region.setEndLoc(Region.getBeginLoc());
+
         SourceRegions.push_back(Region);
 
         if (ParentOfDeferredRegion) {
           ParentOfDeferredRegion = false;
 
           // If there's an existing deferred region, keep the old one, because
           // it means there are two consecutive returns (or a similar pattern).
           if (!DeferredRegion.hasValue() &&

clang/test/CoverageMapping/bad-coverage-for-condexpr-in-macro-pr39942.cpp

+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name pr39942.cpp %s | FileCheck %s
+
+class a;
+template <class b> a &operator<<(b &, const char *);
+int c;
+#define d(l) l(__FILE__, __LINE__, c)
+#define COMPACT_GOOGLE_LOG_ERROR d(e)
+#define f(g, condition) 0 ? (void)0 : h() & g
+#define i(j) COMPACT_GOOGLE_LOG_##j.g()
+#define k(j) f(i(j), )
+class e {
+public:
+  e(const char *, int, int);
+  a &g();
+};
+class h {
+public:
+  void operator&(a &);
+};
+#define m(lib, func) \
+#func;               \
+  k(ERROR) << #func; \
+  return 0 // CHECK: File 1, [[@LINE-1]]:4 -> [[@LINE-1]]:4 = (#0 - #1)
+bool n() { m(, ); }