[clang] Fix format specifiers fixits for nested macros

[alexander-shaposhnikov]

Summary

ExpansionLoc was previously calculated incorrectly in the case of
nested macros expansions. In this diff we build the stack of expansions where the last one
is the actual expansion (in the source code) which should be used for grouping together
the edits. The definition of MacroArgUse is adjusted accordingly: now it's essentially the stack of expansions plus
the location of argument use inside the top-most macro.
This diff fixes https://bugs.llvm.org/show_bug.cgi?id=33447 .

Test plan: make check-all

[mehdi_amini]

Thanks for fixing! (I'm still not the best qualified to review this)

lib/Edit/EditedSource.cpp

78–82

Nit: llvm::find_if allows to skip begin/end as llvm::find_if(I->second, [] ...

test/FixIt/fixit-format-darwin.m

71

I think you mean pr instead of radar ;)

[alexander-shaposhnikov]

minor update

[alexander-shaposhnikov]

Ping

[arphaman]

include/clang/Edit/EditedSource.h

50

I think a name like UseLoc is more appropriate.

lib/Edit/EditedSource.cpp

80

Do you need to compare the entire expansion stack, or can you get away with just the comparison of the front of the stack?

[alexander-shaposhnikov]

Address code review comments

[alexander-shaposhnikov]

lib/Edit/EditedSource.cpp

80

yeah, you are right, we can compare only the fronts because the entire stack can be recovered from that (via calling SourceMgr.getImmediateExpansionRange - like what's going on inside EditedSource::deconstructMacroArgLoc)

[alexander-shaposhnikov]

probably we don't need to keep the entire expansion stack in MacroArgUse (i was doing that because it was easier to understand what's going on).
I will update this diff soon.

[alexander-shaposhnikov]

update, rerun the tests

[arphaman]

LGTM.

include/clang/Edit/EditedSource.h

 
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Edit/FileOffset.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/Support/Allocator.h"
 #include <map>
+#include <tuple>
 
 namespace clang {
   class LangOptions;
   class PPConditionalDirectiveRecord;
 
 namespace edit {
   class Commit;
   class EditsReceiver;
 
 class EditedSource {
   const SourceManager &SourceMgr;
   const LangOptions &LangOpts;
   const PPConditionalDirectiveRecord *PPRec;
 
   struct FileEdit {
     StringRef Text;
     unsigned RemoveLen;
 
     FileEdit() : RemoveLen(0) {}
   };
 
   typedef std::map<FileOffset, FileEdit> FileEditsTy;
   FileEditsTy FileEdits;
 
-  // Location of argument use inside the macro body 
-  typedef std::pair<IdentifierInfo*, SourceLocation> MacroArgUse;
-  llvm::DenseMap<unsigned, SmallVector<MacroArgUse, 2>>
-    ExpansionToArgMap;
+  struct MacroArgUse {
+    IdentifierInfo *Identifier;
+    // Stack of macro expansions at given point of source code
+    SmallVector<SourceLocation, 1> ExpansionStack;
+    // Location of argument use inside the top-level macro
+    SourceLocation UseLoc;
+
+    bool operator==(const MacroArgUse &Other) const {
+      return std::tie(Identifier, ExpansionStack, UseLoc) ==
+             std::tie(Other.Identifier, Other.ExpansionStack, Other.UseLoc);
+    }
+  };
+
+  llvm::DenseMap<unsigned, SmallVector<MacroArgUse, 2>> ExpansionToArgMap;
   SmallVector<std::pair<SourceLocation, MacroArgUse>, 2>
     CurrCommitMacroArgExps;
 
   IdentifierTable IdentTable;
   llvm::BumpPtrAllocator StrAlloc;
 
 public:
   EditedSource(const SourceManager &SM, const LangOptions &LangOpts,
bool commitInsertFromRange(SourceLocation OrigLoc, FileOffset Offs,
                              FileOffset InsertFromRangeOffs, unsigned Len,
                              bool beforePreviousInsertions);
   void commitRemove(SourceLocation OrigLoc, FileOffset BeginOffs, unsigned Len);
 
   StringRef getSourceText(FileOffset BeginOffs, FileOffset EndOffs,
                           bool &Invalid);
   FileEditsTy::iterator getActionForOffset(FileOffset Offs);
   void deconstructMacroArgLoc(SourceLocation Loc,
-                              SourceLocation &ExpansionLoc,
                               MacroArgUse &ArgUse);
 
   void startingCommit();
   void finishedCommit();
 };
 
 }
 
 } // end namespace clang
 
 #endif

lib/Edit/EditedSource.cpp

 using namespace clang;
 using namespace edit;
 
 void EditsReceiver::remove(CharSourceRange range) {
   replace(range, StringRef());
 }
 
 void EditedSource::deconstructMacroArgLoc(SourceLocation Loc,
-                                          SourceLocation &ExpansionLoc,
                                           MacroArgUse &ArgUse) {
   assert(SourceMgr.isMacroArgExpansion(Loc));
   SourceLocation DefArgLoc = SourceMgr.getImmediateExpansionRange(Loc).first;
-  ExpansionLoc = SourceMgr.getImmediateExpansionRange(DefArgLoc).first;
+  SmallVector<SourceLocation, 1> ExpansionStack;
+  ExpansionStack.push_back(
+      SourceMgr.getImmediateExpansionRange(DefArgLoc).first);
+  while (SourceMgr.isMacroBodyExpansion(ExpansionStack.back()))
+    ExpansionStack.push_back(
+        SourceMgr.getImmediateExpansionRange(ExpansionStack.back()).first);
   SmallString<20> Buf;
   StringRef ArgName = Lexer::getSpelling(SourceMgr.getSpellingLoc(DefArgLoc),
                                          Buf, SourceMgr, LangOpts);
-  ArgUse = {nullptr, SourceLocation()};
+  ArgUse = MacroArgUse{nullptr, {}, SourceLocation()};
   if (!ArgName.empty())
-    ArgUse = {&IdentTable.get(ArgName), SourceMgr.getSpellingLoc(DefArgLoc)};
+    ArgUse = {&IdentTable.get(ArgName), std::move(ExpansionStack),
+              SourceMgr.getSpellingLoc(DefArgLoc)};
 }
 
 void EditedSource::startingCommit() {}
 
 void EditedSource::finishedCommit() {
   for (auto &ExpArg : CurrCommitMacroArgExps) {
     SourceLocation ExpLoc;
     MacroArgUse ArgUse;

 bool EditedSource::canInsertInOffset(SourceLocation OrigLoc, FileOffset Offs) {
   FileEditsTy::iterator FA = getActionForOffset(Offs);
   if (FA != FileEdits.end()) {
     if (FA->first != Offs)
       return false; // position has been removed.
   }
 
   if (SourceMgr.isMacroArgExpansion(OrigLoc)) {
-    SourceLocation ExpLoc;
     MacroArgUse ArgUse;
-    deconstructMacroArgLoc(OrigLoc, ExpLoc, ArgUse);
+    deconstructMacroArgLoc(OrigLoc, ArgUse);
+    assert(!ArgUse.ExpansionStack.empty());
+    SourceLocation ExpLoc = ArgUse.ExpansionStack.back();
     auto I = ExpansionToArgMap.find(ExpLoc.getRawEncoding());
     if (I != ExpansionToArgMap.end() &&
-        std::find_if(
-            I->second.begin(), I->second.end(), [&](const MacroArgUse &U) {
-              return ArgUse.first == U.first && ArgUse.second != U.second;
-            }) != I->second.end()) {
+        find_if(I->second, [&](const MacroArgUse &U) {
+          return ArgUse.Identifier == U.Identifier &&
+                 std::tie(ArgUse.ExpansionStack.front(), ArgUse.UseLoc) !=
+                     std::tie(U.ExpansionStack.front(), U.UseLoc);
       // Trying to write in a macro argument input that has already been
       // written by a previous commit for another expansion of the same macro
       // argument name. For example:
       //
       // \code
       //   #define MAC(x) ((x)+(x))
       //   MAC(a)
       // \endcode
       //
       // A commit modified the macro argument 'a' due to the first '(x)'
       // expansion inside the macro definition, and a subsequent commit tried
       // to modify 'a' again for the second '(x)' expansion. The edits of the
       // second commit will be rejected.
       return false;
     }
   }
-
   return true;
 }
 
 bool EditedSource::commitInsert(SourceLocation OrigLoc,
                                 FileOffset Offs, StringRef text,
                                 bool beforePreviousInsertions) {
   if (!canInsertInOffset(OrigLoc, Offs))
     return false;
   if (text.empty())
     return true;
 
   if (SourceMgr.isMacroArgExpansion(OrigLoc)) {
-    SourceLocation ExpLoc;
     MacroArgUse ArgUse;
-    deconstructMacroArgLoc(OrigLoc, ExpLoc, ArgUse);
-    if (ArgUse.first)
-      CurrCommitMacroArgExps.emplace_back(ExpLoc, ArgUse);
+    deconstructMacroArgLoc(OrigLoc, ArgUse);
+    if (ArgUse.Identifier) {
+      assert(!ArgUse.ExpansionStack.empty());
+      CurrCommitMacroArgExps.emplace_back(ArgUse.ExpansionStack.back(), ArgUse);
+    }
   }
-  
   FileEdit &FA = FileEdits[Offs];
   if (FA.Text.empty()) {
     FA.Text = copyString(text);
     return true;
   }
 
   if (beforePreviousInsertions)
     FA.Text = copyString(Twine(text) + FA.Text);

test/FixIt/fixit-format-darwin.m

void test() {
   
   // Artificial test to check that X (in Log3(X, Y, Z))
   // is modified only according to the diagnostics
   // for the first printf and the modification caused 
   // by the second printf is dropped.
   Log3("test 7: %s", getNSInteger(), getNSUInteger());
   // CHECK: Log3("test 7: %ld", (long)getNSInteger(), (unsigned long)getNSUInteger());
 }
+
+#define Outer1(...) \
+do { \
+  printf(__VA_ARGS__); \
+} while (0)
+
+#define Outer2(...) \
+do { \
+  Outer1(__VA_ARGS__); Outer1(__VA_ARGS__); \
+} while (0)
+
+void bug33447() {
+  Outer2("test 8: %s", getNSInteger());  
+  // CHECK: Outer2("test 8: %ld", (long)getNSInteger());
+  Outer2("test 9: %s %s", getNSInteger(), getNSInteger());
+  // CHECK: Outer2("test 9: %ld %ld", (long)getNSInteger(), (long)getNSInteger());
+}