[diagnostics] Avoid crashes while printing macro backtraces

When attempting to map a source into a given level of macro expansion,
this code was ignoring the possibility that the start and end of the
range might take wildly different paths through the tree of macro
expansions. It was assuming that the begin spelling location would
always precede the end spelling location, which is false. A macro can
easily transpose its arguments.

This also fixes a related issue where there are extra macro arguments
between the begin location and the end location. In this situation, we
now highlight the entire macro invocation.

Pair programmed with Richard Smith.

Fixes PR12818.

llvm-svn: 254981

diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp
index 6ef9a89..caf1f0d 100644 (file)
--- a/clang/lib/Frontend/DiagnosticRenderer.cpp
+++ b/clang/lib/Frontend/DiagnosticRenderer.cpp
@@ -308,34 +308,77 @@ void DiagnosticRenderer::emitModuleBuildStack(const SourceManager &SM) {
 
 /// A recursive function to trace all possible backtrace locations
 /// to match the \p CaretLocFileID.
-static SourceLocation retrieveMacroLocation(SourceLocation Loc,
-                                            FileID MacroFileID,
-                                            FileID CaretFileID,
-                                            bool getBeginLoc,
-                                            const SourceManager *SM) {
-  if (MacroFileID == CaretFileID) return Loc;
-  if (!Loc.isMacroID()) return SourceLocation();
+static SourceLocation
+retrieveMacroLocation(SourceLocation Loc, FileID MacroFileID,
+                      FileID CaretFileID,
+                      const SmallVectorImpl<FileID> &CommonArgExpansions,
+                      bool IsBegin, const SourceManager *SM) {
+  assert(SM->getFileID(Loc) == MacroFileID);
+  if (MacroFileID == CaretFileID)
+    return Loc;
+  if (!Loc.isMacroID())
+    return SourceLocation();
 
   SourceLocation MacroLocation, MacroArgLocation;
 
   if (SM->isMacroArgExpansion(Loc)) {
-    MacroLocation = SM->getImmediateSpellingLoc(Loc);
-    MacroArgLocation = getBeginLoc ? SM->getImmediateExpansionRange(Loc).first
-                                   : SM->getImmediateExpansionRange(Loc).second;
+    // Only look at the immediate spelling location of this macro argument if
+    // the other location in the source range is also present in that expansion.
+    if (std::binary_search(CommonArgExpansions.begin(),
+                           CommonArgExpansions.end(), MacroFileID))
+      MacroLocation = SM->getImmediateSpellingLoc(Loc);
+    MacroArgLocation = IsBegin ? SM->getImmediateExpansionRange(Loc).first
+                               : SM->getImmediateExpansionRange(Loc).second;
   } else {
-    MacroLocation = getBeginLoc ? SM->getImmediateExpansionRange(Loc).first
-                                : SM->getImmediateExpansionRange(Loc).second;
+    MacroLocation = IsBegin ? SM->getImmediateExpansionRange(Loc).first
+                            : SM->getImmediateExpansionRange(Loc).second;
     MacroArgLocation = SM->getImmediateSpellingLoc(Loc);
   }
 
-  MacroFileID = SM->getFileID(MacroLocation);
-  MacroLocation = retrieveMacroLocation(MacroLocation, MacroFileID, CaretFileID,
-                                        getBeginLoc, SM);
-  if (MacroLocation.isValid()) return MacroLocation;
+  if (MacroLocation.isValid()) {
+    MacroFileID = SM->getFileID(MacroLocation);
+    MacroLocation =
+        retrieveMacroLocation(MacroLocation, MacroFileID, CaretFileID,
+                              CommonArgExpansions, IsBegin, SM);
+    if (MacroLocation.isValid())
+      return MacroLocation;
+  }
 
   MacroFileID = SM->getFileID(MacroArgLocation);
   return retrieveMacroLocation(MacroArgLocation, MacroFileID, CaretFileID,
-                               getBeginLoc, SM);
+                               CommonArgExpansions, IsBegin, SM);
+}
+
+/// Walk up the chain of macro expansions and collect the FileIDs identifying the
+/// expansions.
+static void getMacroArgExpansionFileIDs(SourceLocation Loc,
+                                        SmallVectorImpl<FileID> &IDs,
+                                        bool IsBegin, const SourceManager *SM) {
+  while (Loc.isMacroID()) {
+    if (SM->isMacroArgExpansion(Loc)) {
+      IDs.push_back(SM->getFileID(Loc));
+      Loc = SM->getImmediateSpellingLoc(Loc);
+    } else {
+      auto ExpRange = SM->getImmediateExpansionRange(Loc);
+      Loc = IsBegin ? ExpRange.first : ExpRange.second;
+    }
+  }
+}
+
+/// Collect the expansions of the begin and end locations and compute the set
+/// intersection. Produces a sorted vector of FileIDs in CommonArgExpansions.
+static void computeCommonMacroArgExpansionFileIDs(
+    SourceLocation Begin, SourceLocation End, const SourceManager *SM,
+    SmallVectorImpl<FileID> &CommonArgExpansions) {
+  SmallVector<FileID, 4> BeginArgExpansions;
+  SmallVector<FileID, 4> EndArgExpansions;
+  getMacroArgExpansionFileIDs(Begin, BeginArgExpansions, /*IsBegin=*/true, SM);
+  getMacroArgExpansionFileIDs(End, EndArgExpansions, /*IsBegin=*/false, SM);
+  std::sort(BeginArgExpansions.begin(), BeginArgExpansions.end());
+  std::sort(EndArgExpansions.begin(), EndArgExpansions.end());
+  std::set_intersection(BeginArgExpansions.begin(), BeginArgExpansions.end(),
+                        EndArgExpansions.begin(), EndArgExpansions.end(),
+                        std::back_inserter(CommonArgExpansions));
 }
 
 // Helper function to fix up source ranges.  It takes in an array of ranges,
@@ -387,10 +430,12 @@ static void mapDiagnosticRanges(
     }
 
     // Do the backtracking.
+    SmallVector<FileID, 4> CommonArgExpansions;
+    computeCommonMacroArgExpansionFileIDs(Begin, End, SM, CommonArgExpansions);
     Begin = retrieveMacroLocation(Begin, BeginFileID, CaretLocFileID,
-                                  true /*getBeginLoc*/, SM);
+                                  CommonArgExpansions, /*IsBegin=*/true, SM);
     End = retrieveMacroLocation(End, BeginFileID, CaretLocFileID,
-                                false /*getBeginLoc*/, SM);
+                                CommonArgExpansions, /*IsBegin=*/false, SM);
     if (Begin.isInvalid() || End.isInvalid()) continue;
 
     // 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 d499400..15eebb6 100644 (file)
--- a/clang/test/Misc/caret-diags-macros.c
+++ b/clang/test/Misc/caret-diags-macros.c
@@ -220,3 +220,29 @@ Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", strlen_test(pKeepBu
 // CHECK-NEXT:    {{.*}}:206:56: note: expanded from macro 'sprintf2'
 // CHECK-NEXT:      __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
 // CHECK-NEXT: {{^                                                       \^~~~~~~~~~~}}
+
+#define SWAP_AND_APPLY(arg, macro) macro arg
+#define APPLY(macro, arg) macro arg
+#define DECLARE_HELPER() __builtin_printf("%d\n", mylong);
+void use_evil_macros(long mylong) {
+  SWAP_AND_APPLY((), DECLARE_HELPER)
+  APPLY(DECLARE_HELPER, ())
+}
+// CHECK:      {{.*}}:228:22: warning: format specifies type 'int' but the argument has type 'long'
+// CHECK-NEXT:   SWAP_AND_APPLY((), DECLARE_HELPER)
+// CHECK-NEXT:   ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
+// CHECK-NEXT: {{.*}}:224:36: note: expanded from macro 'SWAP_AND_APPLY'
+// CHECK-NEXT: #define SWAP_AND_APPLY(arg, macro) macro arg
+// CHECK-NEXT:                                    ^~~~~~~~~
+// CHECK-NEXT: {{.*}}:226:51: note: expanded from macro 'DECLARE_HELPER'
+// CHECK-NEXT: #define DECLARE_HELPER() __builtin_printf("%d\n", mylong);
+// CHECK-NEXT:                                            ~~     ^~~~~~
+// CHECK-NEXT: {{.*}}:229:9: warning: format specifies type 'int' but the argument has type 'long'
+// CHECK-NEXT:   APPLY(DECLARE_HELPER, ())
+// CHECK-NEXT:   ~~~~~~^~~~~~~~~~~~~~~~~~~
+// CHECK-NEXT: {{.*}}:225:27: note: expanded from macro 'APPLY'
+// CHECK-NEXT: #define APPLY(macro, arg) macro arg
+// CHECK-NEXT:                           ^~~~~~~~~
+// CHECK-NEXT: {{.*}}:226:51: note: expanded from macro 'DECLARE_HELPER'
+// CHECK-NEXT: #define DECLARE_HELPER() __builtin_printf("%d\n", mylong);
+// CHECK-NEXT:                                            ~~     ^~~~~~