More hacking on mapDiagnosticRanges to make it handle more cases.

This still isn't quite right, but it fixes a crash.

I factored out findCommonParent because we need it on the result of 
getImmediateExpansionRange: for a function macro, the beginning
and end of an expansion range can come out of different
macros/macro arguments, which means the resulting range is a complete
mess to handle consistently.

I also made some changes to how findCommonParent works; it works somewhat
better in some cases, and somewhat worse in others, but I think overall
it's a better balance.  I'm coming to the conclusion that mapDiagnosticRanges
isn't using the right algorithm, though: chasing the caret is fundamentally
more complicated than any algorithm which only considers one FileID for the
caret can handle because each SourceLocation doesn't really have a single parent.
We need to follow the same path of choosing expansion locations and spelling
locations which the caret used to come up with the correct range
in the general case.

Fixes <rdar://problem/12847524>.

llvm-svn: 170049

2 files changed, 95 insertions, 26 deletions

diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp
index 89d3867aadd..aeab506b410 100644
--- a/clang/lib/Frontend/DiagnosticRenderer.cpp
+++ b/clang/lib/Frontend/DiagnosticRenderer.cpp
@@ -306,6 +306,56 @@ void DiagnosticRenderer::emitModuleBuildStack(const SourceManager &SM) {
   }
 }
 
+// Find the common parent for the beginning and end of the range.
+static void findCommonParent(SourceLocation &Begin, SourceLocation &End,
+                             const SourceManager *SM) {
+  if (Begin.isInvalid() || End.isInvalid()) {
+    Begin = End = SourceLocation();
+    return;
+  }
+
+  FileID BeginFileID = SM->getFileID(Begin);
+  FileID EndFileID = SM->getFileID(End);
+
+  // First, crawl the expansion chain for the beginning of the range.
+  llvm::SmallDenseMap<FileID, SourceLocation> BeginLocsMap;
+  BeginLocsMap[BeginFileID] = Begin;
+  while (Begin.isMacroID() && BeginFileID != EndFileID) {
+    if (SM->isMacroArgExpansion(Begin)) {
+      Begin = SM->getImmediateSpellingLoc(Begin);
+      if (Begin.isMacroID())
+        continue;
+      Begin = SourceLocation();
+      BeginFileID = FileID();
+      break;
+    }
+    Begin = SM->getImmediateExpansionRange(Begin).first;
+    BeginFileID = SM->getFileID(Begin);
+    BeginLocsMap[BeginFileID] = Begin;
+  }
+
+  // Then, crawl the expansion chain for the end of the range.
+  if (BeginFileID != EndFileID) {
+    while (End.isMacroID() && !BeginLocsMap.count(EndFileID)) {
+      if (SM->isMacroArgExpansion(End)) {
+        End = SM->getImmediateSpellingLoc(End);
+        if (End.isMacroID())
+          continue;
+        End = SourceLocation();
+        EndFileID = FileID();
+        break;
+      }
+      End = SM->getImmediateExpansionRange(End).second;
+      EndFileID = SM->getFileID(End);
+    }
+    if (End.isMacroID()) {
+      Begin = BeginLocsMap[EndFileID];
+      BeginFileID = EndFileID;
+    }
+  }
+  assert(Begin.isValid() == End.isValid());
+}
+
 // Helper function to fix up source ranges.  It takes in an array of ranges,
 // and outputs an array of ranges where we want to draw the range highlighting
 // around the location specified by CaretLoc.
@@ -329,40 +379,38 @@ static void mapDiagnosticRanges(
     SourceLocation Begin = I->getBegin(), End = I->getEnd();
     bool IsTokenRange = I->isTokenRange();
 
-    FileID BeginFileID = SM->getFileID(Begin);
-    FileID EndFileID = SM->getFileID(End);
-
-    // Find the common parent for the beginning and end of the range.
-
-    // First, crawl the expansion chain for the beginning of the range.
-    llvm::SmallDenseMap<FileID, SourceLocation> BeginLocsMap;
-    while (Begin.isMacroID() && BeginFileID != EndFileID) {
-      BeginLocsMap[BeginFileID] = Begin;
-      Begin = SM->getImmediateExpansionRange(Begin).first;
-      BeginFileID = SM->getFileID(Begin);
-    }
+    // Compute the common parent; we can't highlight a range where
+    // the begin and end have different FileIDs.
+    findCommonParent(Begin, End, SM);
 
-    // Then, crawl the expansion chain for the end of the range.
-    if (BeginFileID != EndFileID) {
-      while (End.isMacroID() && !BeginLocsMap.count(EndFileID)) {
-        End = SM->getImmediateExpansionRange(End).second;
-        EndFileID = SM->getFileID(End);
-      }
-      if (End.isMacroID()) {
-        Begin = BeginLocsMap[EndFileID];
-        BeginFileID = EndFileID;
-      }
-    }
+    FileID BeginFileID = SM->getFileID(Begin);
 
     while (Begin.isMacroID() && BeginFileID != CaretLocFileID) {
       if (SM->isMacroArgExpansion(Begin)) {
+        // We have a macro argument; take the spelling loc, which is
+        // a step closer to where the argument was written.
         Begin = SM->getImmediateSpellingLoc(Begin);
         End = SM->getImmediateSpellingLoc(End);
+        BeginFileID = SM->getFileID(Begin);
+        assert(BeginFileID == SM->getFileID(End));
       } else {
+        // Take the next expansion in the expansion chain.
         Begin = SM->getImmediateExpansionRange(Begin).first;
         End = SM->getImmediateExpansionRange(End).second;
+
+        // Compute the common parent again; the beginning and end might
+        // come out of different macro expansions.
+        findCommonParent(Begin, End, SM);
+        BeginFileID = SM->getFileID(Begin);
       }
-      BeginFileID = SM->getFileID(Begin);
+    }
+
+    // If this is the expansion of a macro argument, point the range at the
+    // use of the argument in the definition of the macro, not the expansion.
+    if (SM->isMacroArgExpansion(Begin)) {
+      assert(SM->isMacroArgExpansion(End));
+      Begin = End = SM->getImmediateExpansionRange(Begin).first;
+      IsTokenRange = true;
     }
 
     // Return the spelling location of the beginning and end of the range.
diff --git a/clang/test/Misc/caret-diags-macros.c b/clang/test/Misc/caret-diags-macros.c
index ce62425e780..8fcfeb070cd 100644
--- a/clang/test/Misc/caret-diags-macros.c
+++ b/clang/test/Misc/caret-diags-macros.c
@@ -181,10 +181,10 @@ void foo_aa()
 }
 // CHECK:         {{.*}}:180:21: warning: expression result unused
 // CHECK-NEXT:      iequals(__LINE__, BARC(4,3,2,6,8), 8);
-// CHECK-NEXT: {{^                    \^~~~~~~~~~~~~~~}}
+// CHECK-NEXT: {{^                    \^          ~}}
 // CHECK-NEXT:    {{.*}}:179:51: note: expanded from macro 'BARC'
 // CHECK-NEXT:    #define /* */ BARC(c, /* */b, a, ...) (a+b+/* */c + __VA_ARGS__ +0)
-// CHECK-NEXT: {{^                                       ~~~~~~~~~~ \^}}
+// CHECK-NEXT: {{^                                                  \^}}
 
 #define APPEND2(NUM, SUFF) -1 != NUM ## SUFF
 #define APPEND(NUM, SUFF) APPEND2(NUM, SUFF)
@@ -205,3 +205,24 @@ void foo_aa()
 // CHECK-NEXT:    {{.*}}:189:31: note: expanded from macro 'APPEND2'
 // CHECK-NEXT:    #define APPEND2(NUM, SUFF) -1 != NUM ## SUFF
 // CHECK-NEXT: {{^                           ~~ \^  ~~~~~~~~~~~}}
+
+
+unsigned long strlen_test(const char *s);
+#define __darwin_obsz(object) __builtin_object_size (object, 1)
+#define sprintf2(str, ...) \
+  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
+#define Cstrlen(a)  strlen_test(a)
+#define Csprintf    sprintf2
+void f(char* pMsgBuf, char* pKeepBuf) {
+Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", Cstrlen(pKeepBuf));
+}
+// CHECK:         {{.*}}:217:62: warning: format specifies type 'int' but the argument has type 'unsigned long'
+// CHECK-NEXT:    Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", Cstrlen(pKeepBuf));
+// CHECK-NEXT: {{^                                                    ~~~      \^~~~~~~~~~~~~~~~~}}
+// CHECK-NEXT: {{^                                                    %1ld}}
+// CHECK-NEXT:    {{.*}}:214:21: note: expanded from macro 'Cstrlen'
+// CHECK-NEXT:    #define Cstrlen(a)  strlen_test(a)
+// CHECK-NEXT: {{^                    \^}}
+// CHECK-NEXT:    {{.*}}:213:56: note: expanded from macro 'sprintf2'
+// CHECK-NEXT:      __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
+// CHECK-NEXT: {{^                                                       \^}}