[clang] Fix format specifiers fixits

[alexander-shaposhnikov]

Summary

This diff fixes printf "fixits" in the case when there is a wrapping macro and
the format string needs multiple replacements.
In the presence of a macro there was an extra logic in EditedSource.cpp
to handle multiple uses of the same macro argument
(see the old comment inside EditedSource::canInsertInOffset)
which was mistriggerred when the argument was used only once
but required multiple adjustments), as a result the "fixit" was breaking down the format string
by dropping the second format specifier, i.e.
Log1("test 4: %s %s", getNSInteger(), getNSInteger())
was getting replaced with
Log1("test 4: %ld ", (long)getNSInteger(), (long)getNSInteger())
(if one removed the macro and used printf directly it would work fine).
In this diff we track the location where the macro argument is used and
(as it was before) the modifications originating from all the locations except the first one are rejected,
but multiple changes are allowed.

Test plan: make check-all

[mehdi_amini]

Interestingly I got the opposite issue recently: calling through a macro with a single format specifier was *adding* new specific in the fixit in some conditions.

I'm not the best person to review this change though, let me add a few folks.

[alexander-shaposhnikov]

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

[arphaman]

Thanks for working on this!

LGTM with a couple of fixes:

test/FixIt/fixit-format-darwin.m

3 ↗

(On Diff #101663)

You can just pipe the result of grep into FileCheck without using another temporary file.

54 ↗

(On Diff #101663)

Typo: Artificial.

[alexander-shaposhnikov]

Address comments

[mehdi_amini]

In D33976#775918, @alexshap wrote:

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

I'll CC you on the bug when I get time to reduce the test case and file the bug.

[mehdi_amini]

In D33976#776486, @mehdi_amini wrote:

In D33976#775918, @alexshap wrote:

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

I'll CC you on the bug when I get time to reduce the test case and file the bug.

I finally got to reduce it: https://bugs.llvm.org/show_bug.cgi?id=33447

cfe/trunk/include/clang/Edit/EditedSource.h

struct FileEdit {
     unsigned RemoveLen;
 
     FileEdit() : RemoveLen(0) {}
   };
 
   typedef std::map<FileOffset, FileEdit> FileEditsTy;
   FileEditsTy FileEdits;
 
-  llvm::DenseMap<unsigned, llvm::TinyPtrVector<IdentifierInfo*>>
+  // Location of argument use inside the macro body 
+  typedef std::pair<IdentifierInfo*, SourceLocation> MacroArgUse;
+  llvm::DenseMap<unsigned, SmallVector<MacroArgUse, 2>>
     ExpansionToArgMap;
-  SmallVector<std::pair<SourceLocation, IdentifierInfo*>, 2>
+  SmallVector<std::pair<SourceLocation, MacroArgUse>, 2>
     CurrCommitMacroArgExps;
 
   IdentifierTable IdentTable;
   llvm::BumpPtrAllocator StrAlloc;
 
 public:
   EditedSource(const SourceManager &SM, const LangOptions &LangOpts,
                const PPConditionalDirectiveRecord *PPRec = nullptr)
bool commitInsertFromRange(SourceLocation OrigLoc, FileOffset Offs,
                              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,
-                              IdentifierInfo *&II);
+                              MacroArgUse &ArgUse);
 
   void startingCommit();
   void finishedCommit();
 };
 
 }
 
 } // end namespace clang
 
 #endif

cfe/trunk/lib/Edit/EditedSource.cpp

 using namespace edit;
 
 void EditsReceiver::remove(CharSourceRange range) {
   replace(range, StringRef());
 }
 
 void EditedSource::deconstructMacroArgLoc(SourceLocation Loc,
                                           SourceLocation &ExpansionLoc,
-                                          IdentifierInfo *&II) {
+                                          MacroArgUse &ArgUse) {
   assert(SourceMgr.isMacroArgExpansion(Loc));
   SourceLocation DefArgLoc = SourceMgr.getImmediateExpansionRange(Loc).first;
   ExpansionLoc = SourceMgr.getImmediateExpansionRange(DefArgLoc).first;
   SmallString<20> Buf;
   StringRef ArgName = Lexer::getSpelling(SourceMgr.getSpellingLoc(DefArgLoc),
                                          Buf, SourceMgr, LangOpts);
-  II = nullptr;
-  if (!ArgName.empty()) {
-    II = &IdentTable.get(ArgName);
-  }
+  ArgUse = {nullptr, SourceLocation()};
+  if (!ArgName.empty())
+    ArgUse = {&IdentTable.get(ArgName), SourceMgr.getSpellingLoc(DefArgLoc)};
 }
 
 void EditedSource::startingCommit() {}
 
 void EditedSource::finishedCommit() {
   for (auto &ExpArg : CurrCommitMacroArgExps) {
     SourceLocation ExpLoc;
-    IdentifierInfo *II;
-    std::tie(ExpLoc, II) = ExpArg;
-    auto &ArgNames = ExpansionToArgMap[ExpLoc.getRawEncoding()];
-    if (std::find(ArgNames.begin(), ArgNames.end(), II) == ArgNames.end()) {
-      ArgNames.push_back(II);
-    }
+    MacroArgUse ArgUse;
+    std::tie(ExpLoc, ArgUse) = ExpArg;
+    auto &ArgUses = ExpansionToArgMap[ExpLoc.getRawEncoding()];
+    if (std::find(ArgUses.begin(), ArgUses.end(), ArgUse) == ArgUses.end())
+      ArgUses.push_back(ArgUse);
   }
   CurrCommitMacroArgExps.clear();
 }
 
 StringRef EditedSource::copyString(const Twine &twine) {
   SmallString<128> Data;
   return copyString(twine.toStringRef(Data));
 }
 
 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)) {
-    IdentifierInfo *II;
     SourceLocation ExpLoc;
-    deconstructMacroArgLoc(OrigLoc, ExpLoc, II);
+    MacroArgUse ArgUse;
+    deconstructMacroArgLoc(OrigLoc, ExpLoc, ArgUse);
     auto I = ExpansionToArgMap.find(ExpLoc.getRawEncoding());
     if (I != ExpansionToArgMap.end() &&
-        std::find(I->second.begin(), I->second.end(), II) != I->second.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()) {
       // 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
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)) {
-    IdentifierInfo *II;
     SourceLocation ExpLoc;
-    deconstructMacroArgLoc(OrigLoc, ExpLoc, II);
-    if (II)
-      CurrCommitMacroArgExps.emplace_back(ExpLoc, II);
+    MacroArgUse ArgUse;
+    deconstructMacroArgLoc(OrigLoc, ExpLoc, ArgUse);
+    if (ArgUse.first)
+      CurrCommitMacroArgExps.emplace_back(ExpLoc, ArgUse);
   }
   
   FileEdit &FA = FileEdits[Offs];
   if (FA.Text.empty()) {
     FA.Text = copyString(text);
     return true;
   }
 

cfe/trunk/test/FixIt/fixit-format-darwin.m

+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fsyntax-only -fblocks -Wformat -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+/* This is a test of code modifications created by darwin format fix-its hints 
+   that are provided as part of warning */
+
+int printf(const char * restrict, ...);
+
+#if __LP64__
+typedef long NSInteger;
+typedef unsigned long NSUInteger;
+#else
+typedef int NSInteger;
+typedef unsigned int NSUInteger;
+#endif
+NSInteger getNSInteger();
+NSUInteger getNSUInteger();
+
+#define Log1(...) \
+do { \
+  printf(__VA_ARGS__); \
+} while (0)
+
+#define Log2(...) \
+do { \
+  printf(__VA_ARGS__); \
+  printf(__VA_ARGS__); \
+} while (0) \
+
+#define Log3(X, Y, Z) \
+do { \
+  printf(X, Y); \
+  printf(X, Z); \
+} while (0) \
+
+void test() {
+  printf("test 1: %s", getNSInteger()); 
+  // CHECK: printf("test 1: %ld", (long)getNSInteger());
+  printf("test 2: %s %s", getNSInteger(), getNSInteger());
+  // CHECK: printf("test 2: %ld %ld", (long)getNSInteger(), (long)getNSInteger());
+  
+  Log1("test 3: %s", getNSInteger());
+  // CHECK: Log1("test 3: %ld", (long)getNSInteger());
+  Log1("test 4: %s %s", getNSInteger(), getNSInteger());
+  // CHECK: Log1("test 4: %ld %ld", (long)getNSInteger(), (long)getNSInteger());
+  
+  Log2("test 5: %s", getNSInteger());
+  // CHECK: Log2("test 5: %ld", (long)getNSInteger()); 
+  Log2("test 6: %s %s", getNSInteger(), getNSInteger());
+  // CHECK: Log2("test 6: %ld %ld", (long)getNSInteger(), (long)getNSInteger());
+  
+  // 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());
+}