[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 ↗

(On Diff #102773)

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

test/FixIt/fixit-format-darwin.m

71 ↗

(On Diff #102773)

I think you mean pr instead of radar ;)

[alexander-shaposhnikov]

minor update

[alexander-shaposhnikov]

Ping

[arphaman]

include/clang/Edit/EditedSource.h

50 ↗

(On Diff #102781)

I think a name like UseLoc is more appropriate.

lib/Edit/EditedSource.cpp

80 ↗

(On Diff #102781)

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 ↗

(On Diff #102781)

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.

cfe/trunk/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;
+    SourceLocation ImmediateExpansionLoc;
+    // Location of argument use inside the top-level macro
+    SourceLocation UseLoc;
+
+    bool operator==(const MacroArgUse &Other) const {
+      return std::tie(Identifier, ImmediateExpansionLoc, UseLoc) ==
+             std::tie(Other.Identifier, Other.ImmediateExpansionLoc,
+                      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,

cfe/trunk/lib/Edit/EditedSource.cpp

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;
+  SourceLocation ImmediateExpansionLoc =
+      SourceMgr.getImmediateExpansionRange(DefArgLoc).first;
+  ExpansionLoc = ImmediateExpansionLoc;
+  while (SourceMgr.isMacroBodyExpansion(ExpansionLoc))
+    ExpansionLoc = SourceMgr.getImmediateExpansionRange(ExpansionLoc).first;
   SmallString<20> Buf;
   StringRef ArgName = Lexer::getSpelling(SourceMgr.getSpellingLoc(DefArgLoc),
                                          Buf, SourceMgr, LangOpts);
-  ArgUse = {nullptr, SourceLocation()};
+  ArgUse = MacroArgUse{nullptr, SourceLocation(), SourceLocation()};
   if (!ArgName.empty())
-    ArgUse = {&IdentTable.get(ArgName), SourceMgr.getSpellingLoc(DefArgLoc)};
+    ArgUse = {&IdentTable.get(ArgName), ImmediateExpansionLoc,
+              SourceMgr.getSpellingLoc(DefArgLoc)};
 }
 
 void EditedSource::startingCommit() {}
 
 void EditedSource::finishedCommit() {
   for (auto &ExpArg : CurrCommitMacroArgExps) {
     SourceLocation ExpLoc;
     MacroArgUse ArgUse;
bool EditedSource::canInsertInOffset(SourceLocation OrigLoc, FileOffset Offs) {
   }
 
   if (SourceMgr.isMacroArgExpansion(OrigLoc)) {
     SourceLocation ExpLoc;
     MacroArgUse ArgUse;
     deconstructMacroArgLoc(OrigLoc, ExpLoc, ArgUse);
     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.ImmediateExpansionLoc, ArgUse.UseLoc) !=
+                     std::tie(U.ImmediateExpansionLoc, 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;
+    SourceLocation ExpLoc;
     deconstructMacroArgLoc(OrigLoc, ExpLoc, ArgUse);
-    if (ArgUse.first)
+    if (ArgUse.Identifier)
       CurrCommitMacroArgExps.emplace_back(ExpLoc, ArgUse);
   }
-  
   FileEdit &FA = FileEdits[Offs];
   if (FA.Text.empty()) {
     FA.Text = copyString(text);
     return true;
   }
 
   if (beforePreviousInsertions)
     FA.Text = copyString(Twine(text) + FA.Text);

cfe/trunk/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());
+}