Page MenuHomePhabricator

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

Authored by kfunk on Jul 10 2017, 3:46 AM.

Details

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)

Event Timeline

kfunk created this revision.Jul 10 2017, 3:46 AM
JDevlieghere added a project: Restricted Project.Jul 10 2017, 6:42 AM
JDevlieghere added a subscriber: cfe-commits.
kfunk updated this revision to Diff 105928.Jul 10 2017, 3:51 PM

Add test

kfunk edited the summary of this revision. (Show Details)Jul 10 2017, 3:53 PM
kfunk edited the summary of this revision. (Show Details)
kfunk updated this revision to Diff 105929.Jul 10 2017, 3:56 PM

Remove unnecessary sed line in test driver

yawanng edited edge metadata.Jul 10 2017, 4:23 PM

LGTM. Please wait for Alexander approval.

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

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

kfunk edited the summary of this revision. (Show Details)Jul 10 2017, 4:25 PM
kfunk added inline comments.Jul 11 2017, 2:41 AM
clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
296

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

alexfh requested changes to this revision.Jul 11 2017, 6:29 AM

Thank you for the fix! A few comments below.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
291–299

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

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

5–7

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

This revision now requires changes to proceed.Jul 11 2017, 6:29 AM
kfunk updated this revision to Diff 108061.Jul 25 2017, 5:59 AM
kfunk edited edge metadata.

Addressed concerns

kfunk marked 3 inline comments as done.Jul 25 2017, 5:59 AM
alexfh accepted this revision.Jul 25 2017, 7:01 AM

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

This revision is now accepted and ready to land.Jul 25 2017, 7:01 AM
This revision was automatically updated to reflect the committed changes.
kfunk added a comment.Jul 25 2017, 7:30 AM

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!