[clang-tidy] clang-apply-replacements: Don't insert null entry

[kfunk]

Summary

[clang-tidy] clang-apply-replacements: Don't insert null entry

Fix crash when running clang-apply-replacements on YML files which
contain an invalid file path. Make sure we never add a nullptr into the
map. The previous code started adding nullptr to the map after the first
warnings via errs() has been emitted.

Backtrace:

Starting program:
/home/kfunk/devel/build/llvm/bin/clang-apply-replacements /tmp/tmpIqtp7m
[Thread debugging using libthread_db enabled]
Using host libthread_db library
"/lib/x86_64-linux-gnu/libthread_db.so.1".
Described file '.moc/../../../../../../src/qt5.8/qtremoteobjects/src/remoteobjects/qremoteobjectregistrysource_p.h' doesn't exist. Ignoring...
...

Program received signal SIGSEGV, Segmentation fault.
main (argc=<optimized out>, argv=<optimized out>) at /home/kfunk/devel/src/llvm/tools/clang/tools/extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:262
(gdb) p FileAndReplacements.first
$1 = (const clang::FileEntry *) 0x0 
(gdb)

Added tests.

Before patch:

******************** TEST 'Clang Tools :: clang-apply-replacements/invalid-files.cpp' FAILED ********************
Script:
--
mkdir -p /home/kfunk/devel/build/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/invalid-files
clang-apply-replacements /home/kfunk/devel/build/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/invalid-files
ls -1 /home/kfunk/devel/build/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/invalid-files | FileCheck /home/kfunk/devel/src/llvm/tools/clang/tools/extra/test/clang-apply-replacements/invalid-files.cpp --check-prefix=YAML
--
Exit Code: 139 

Command Output (stderr):
--
Described file 'idonotexist.h' doesn't exist. Ignoring...
/home/kfunk/devel/build/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Output/invalid-files.cpp.script: line 4:  9919 Segmentation fault      clang-apply-replacements /home/kfunk/devel/build/llvm/tools/clang/tools/extra/test/clang-apply-   replacements/Output/Inputs/invalid-files

--

After Patch:

PASS: Clang Tools :: clang-apply-replacements/invalid-files.cpp (5 of 6)

[kfunk]

Add test

[kfunk]

Remove unnecessary sed line in test driver

[yawanng]

LGTM. Please wait for Alexander approval.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

296 ↗

(On Diff #105929)

Maybe add some error message here if it's not an existence issue. like "invalid file path" or something? The same below.

[kfunk]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

296 ↗

(On Diff #105929)

Is that really necessary? I think the user won't care.

[alexfh]

Thank you for the fix! A few comments below.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

291–299 ↗

(On Diff #105929)

I'd swap the positive and the negative branches and remove continue:

if (const FileEntry *Entry = ...) {
  GroupedReplacements[Entry].push_back(R);
} else if (...) {
  errs() << ...;
}

Same below.

test/clang-apply-replacements/invalid-files.cpp

1 ↗

(On Diff #105929)

No need for Inputs in the path inside the build directory.

5–7 ↗

(On Diff #105929)

FileCheck is not needed here. Just // RUN: ls %T/invalid-files/invalid-files.yaml should be enough to assert the file existence.

[kfunk]

Addressed concerns

[alexfh]

LG. Do you need someone to commit the patch for you?

[kfunk]

Seems to have worked:

Committing to https://llvm.org/svn/llvm-project/clang-tools-extra/trunk ...
        A       test/clang-apply-replacements/Inputs/invalid-files/invalid-files.yaml
        A       test/clang-apply-replacements/invalid-files.cpp
        M       clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
Committed r308974

I presume this Diff is being auto-closed by Phab?

Anyhow: Thanks for the review!

clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

bool mergeAndDeduplicate(const TUReplacements &TUs,
                          clang::SourceManager &SM) {
 
   // Group all replacements by target file.
   std::set<StringRef> Warned;
   for (const auto &TU : TUs) {
     for (const tooling::Replacement &R : TU.Replacements) {
       // Use the file manager to deduplicate paths. FileEntries are
       // automatically canonicalized.
-      const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath());
-      if (!Entry && Warned.insert(R.getFilePath()).second) {
+      if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
+        GroupedReplacements[Entry].push_back(R);
+      } else if (Warned.insert(R.getFilePath()).second) {
           errs() << "Described file '" << R.getFilePath()
                  << "' doesn't exist. Ignoring...\n";
-        continue;
       }
-      GroupedReplacements[Entry].push_back(R);
     }
   }
 
   // Ask clang to deduplicate and report conflicts.
   return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
 }
 
 bool mergeAndDeduplicate(const TUDiagnostics &TUs,
                          FileToReplacementsMap &GroupedReplacements,
                          clang::SourceManager &SM) {
 
   // Group all replacements by target file.
   std::set<StringRef> Warned;
   for (const auto &TU : TUs) {
     for (const auto &D : TU.Diagnostics) {
       for (const auto &Fix : D.Fix) {
         for (const tooling::Replacement &R : Fix.second) {
           // Use the file manager to deduplicate paths. FileEntries are
           // automatically canonicalized.
-          const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath());
-          if (!Entry && Warned.insert(R.getFilePath()).second) {
+          if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
+            GroupedReplacements[Entry].push_back(R);
+          } else if (Warned.insert(R.getFilePath()).second) {
               errs() << "Described file '" << R.getFilePath()
                      << "' doesn't exist. Ignoring...\n";
-            continue;
           }
-          GroupedReplacements[Entry].push_back(R);
         }
       }
     }
   }
 
   // Ask clang to deduplicate and report conflicts.
   return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
 }

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/invalid-files/invalid-files.yaml

+---
+MainSourceFile:  ''
+Replacements:
+  - FilePath:        idontexist.h
+    Offset:          2669
+    Length:          0
+    ReplacementText: ' override'
+  - FilePath:        idontexist.h
+    Offset:          2669
+    Length:          0
+    ReplacementText: ' override'
+...

clang-tools-extra/trunk/test/clang-apply-replacements/invalid-files.cpp

+// RUN: mkdir -p %T/invalid-files
+// RUN: clang-apply-replacements %T/invalid-files
+//
+// Check that the yaml files are *not* deleted after running clang-apply-replacements without remove-change-desc-files.
+// RUN: ls %T/Inputs/invalid-files/invalid-files.yaml