[clang-rename] Exit gracefully when no input provided

[xgupta]

Summary

clang-rename on a non existing file segfaults

Command to run -
$ clang-rename -offset=0 -new-name=plop asdasd

Error while processing llvm-project/asdasd.
clang-rename: llvm-project/llvm/include/llvm/Support/ErrorOr.h:237:
llvm::ErrorOr<T>::storage_type* llvm::ErrorOr<T>::getStorage()
[with T = const clang::FileEntry*; llvm::ErrorOr<T>::storage_type = const clang::FileEntry*]:
Assertion `!HasError && "Cannot get value when an error exists!"' failed.

[1] 827497 IOT instruction clang-rename -offset=0 -new-name=plop asdasd

This fixes https://github.com/llvm/llvm-project/issues/36471.

[vmiklos]

Looks good to me, but probably you want an approval from @arphaman, @klimek or @kbobyrev before committing.

[kbobyrev]

LGTM

clang/tools/clang-rename/ClangRename.cpp

130

Probably something like "input must be provided" would be more understandable by the user, but this is also fine.

[xgupta]

address comment

[xgupta]

clang/tools/clang-rename/ClangRename.cpp

130

Thanks for the suggestion!

[dyung]

Is there a reason a test was not added with this change?

[xgupta]

In D148439#4272810, @dyung wrote:

Is there a reason a test was not added with this change?

Sorry, I missed that. Adding.

[dyung]

This change is also causing 33 test failures on a build bot https://lab.llvm.org/buildbot/#/builders/139/builds/39267

[xgupta]

In D148439#4272813, @dyung wrote:

This change is also causing 33 test failures on a build bot https://lab.llvm.org/buildbot/#/builders/139/builds/39267

Yes, checking for the reason.

[kbobyrev]

Oh, wait, I'm sorry, I didn't look into it closely :( Yeah, the Input file is not really needed, most of the time the users of clang-rename (not sure there are many with clangd being developed) use CLI flags for that.

It's better to revert this patch. My bad, should've looked a bit closer into this.

[xgupta]

adjust the return and add test case

[xgupta]

In D148439#4272904, @kbobyrev wrote:

Oh, wait, I'm sorry, I didn't look into it closely :( Yeah, the Input file is not really needed, most of the time the users of clang-rename (not sure there are many with clangd being developed) use CLI flags for that.

It's better to revert this patch. My bad, should've looked a bit closer into this.

No issue, I also consider it a straightforward fix and forget to run check-clang.

[kbobyrev]

Yeah, this should be a right approach! Thanks!

clang/test/clang-rename/NonExistFile.cpp

2

Rather than asdasd please use a meaningful name (e.g. non-existing-file).

clang/tools/clang-rename/ClangRename.cpp

233

It is worth including the filename in the error message, otherwise it won't be possible to understand which one is missing (there can be multiple IIRC, right?).

Also, it's better to put this check to the top of main, where the OP if first declared, since this is a sanity check and we want to fail if the inputs are corrupted.

[xgupta]

address comments

[xgupta]

clang/tools/clang-rename/ClangRename.cpp

233

Thanks for the suggestion, I agree, I updated the error message regarding the filename.

But I am not sure running a similar for loop again should be the correct way.
So just sharing the diff before committing, WDYT -

diff --git a/clang/tools/clang-rename/ClangRename.cpp b/clang/tools/clang-rename/ClangRename.cpp
index 24c9d8521..bc777cc01 100644
--- a/clang/tools/clang-rename/ClangRename.cpp
+++ b/clang/tools/clang-rename/ClangRename.cpp
@@ -106,6 +106,17 @@ int main(int argc, const char **argv) {
   }
   tooling::CommonOptionsParser &OP = ExpectedParser.get();
 
+  auto Files = OP.getSourcePathList();
+  tooling::RefactoringTool Tool(OP.getCompilations(), Files);
+    auto &FileMgr = Tool.getFiles();
+      for (const auto &File : Files) {
+      auto Entry = FileMgr.getFile(File);
+      if (!Entry) {
+        errs() << "clang-rename: " << File << " does not exist.\n";
+        return 1;
+      }
+    }
+
   if (!Input.empty()) {
     // Populate QualifiedNames and NewNames from a YAML file.
     ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
@@ -162,8 +173,6 @@ int main(int argc, const char **argv) {
     return 1;
   }
 
-  auto Files = OP.getSourcePathList();
-  tooling::RefactoringTool Tool(OP.getCompilations(), Files);
   tooling::USRFindingAction FindingAction(SymbolOffsets, QualifiedNames, Force);
   Tool.run(tooling::newFrontendActionFactory(&FindingAction).get());
   const std::vector<std::vector<std::string>> &USRList =
@@ -222,7 +231,6 @@ int main(int argc, const char **argv) {
     DiagnosticsEngine Diagnostics(
         IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
         &DiagnosticPrinter, false);
-    auto &FileMgr = Tool.getFiles();
     SourceManager Sources(Diagnostics, FileMgr);
     Rewriter Rewrite(Sources, DefaultLangOptions);

[xgupta]

Ping for reivew.

[aaron.ballman]

LGTM, but please add a release note when landing the changes.

[xgupta]

In D148439#4298606, @aaron.ballman wrote:

LGTM, but please add a release note when landing the changes.

Thanks for the review, added a release note for change.

[kbobyrev]

Thanks Aaron!

Yes, fair enough, that looks good to me, probably no need to move a sanity check to the front of main() in this case.

Also, just as a note: I don't know how many people use clang-rename anymore (especially with clangd being present), I rewrote a lot of what clang-rename has to offer for clangd with better precision and results. I think the tool is still useful, but not as much as before.

[aaron.ballman]

In D148439#4299178, @kbobyrev wrote:

Thanks Aaron!

Yes, fair enough, that looks good to me, probably no need to move a sanity check to the front of main() in this case.

Also, just as a note: I don't know how many people use clang-rename anymore (especially with clangd being present), I rewrote a lot of what clang-rename has to offer for clangd with better precision and results. I think the tool is still useful, but not as much as before.

Should we consider deprecating clang-rename in favor of clangd, or do you think that's premature?

[kbobyrev]

The difference is that clang-rename is a stand-alone binary with CLI whereas clangd isn't typically used as such (it can be, but it's a weird use-case).

I don't know if there are any existing users out there, if there are then I assume there are very few.

It was created as an intern project long ago (I guess 2014/2015?) and was in a semi-functional state, and then when I was an intern in 2016 I improved it significantly and built the integration with Vim and updated one for Emacs. We had some use-cases, but AFAIK there are none anymore at Google, and I believe most users we had were there.

Personally, I don't see any reason to keep clang-rename and support it, I don't think it's useful anymore. But this is Open-Source world so some people might object.

[aaron.ballman]

In D148439#4303202, @kbobyrev wrote:

The difference is that clang-rename is a stand-alone binary with CLI whereas clangd isn't typically used as such (it can be, but it's a weird use-case).

I don't know if there are any existing users out there, if there are then I assume there are very few.

It was created as an intern project long ago (I guess 2014/2015?) and was in a semi-functional state, and then when I was an intern in 2016 I improved it significantly and built the integration with Vim and updated one for Emacs. We had some use-cases, but AFAIK there are none anymore at Google, and I believe most users we had were there.

Personally, I don't see any reason to keep clang-rename and support it, I don't think it's useful anymore. But this is Open-Source world so some people might object.

I think it's worth seeing if we can remove it, as it reduces around maintenance burden, build times, repo size, etc (all very, very slightly though, so I'm fine if folks push back). I'll put together an RFC to see if there is resistance to the idea.

[kbobyrev]

Sounds good, thank you!

clang/docs/ReleaseNotes.rst

- Clang's "static assertion failed" diagnostic now points to the static assertion
   (`#61951 <https://github.com/llvm/llvm-project/issues/61951>`_)
 - ``-Wformat`` now recognizes ``%lb`` for the ``printf``/``scanf`` family of
   functions.
   (`#62247: <https://github.com/llvm/llvm-project/issues/62247>`_).
 
 Bug Fixes in This Version
 -------------------------
 
+- Fix segfault while running clang-rename on a non existing file.
+  (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``
   driver mode and emit an error which suggests using ``/TC`` or ``/TP``
   ``clang-cl`` options instead.
   (`#59307 <https://github.com/llvm/llvm-project/issues/59307>`_)
 - Fix assert that fails when the expression causing the this pointer to be

clang/test/clang-rename/NonExistFile.cpp

+// RUN: not clang-rename -offset=0 -new-name=bar non-existing-file 2>&1 | FileCheck %s
+// CHECK: clang-rename: non-existing-file does not exist.

clang/tools/clang-rename/ClangRename.cpp

if (!Input.empty()) {
     for (const auto &Info : Infos) {
       if (!Info.QualifiedName.empty())
         QualifiedNames.push_back(Info.QualifiedName);
       else
         SymbolOffsets.push_back(Info.Offset);
       NewNames.push_back(Info.NewName);
     }
   }
 
   // Check the arguments for correctness.
   if (NewNames.empty()) {
     errs() << "clang-rename: -new-name must be specified.\n\n";
     return 1;
   }
 
   if (SymbolOffsets.empty() == QualifiedNames.empty()) {
     errs() << "clang-rename: -offset and -qualified-name can't be present at "
DiagnosticsEngine Diagnostics(
         &DiagnosticPrinter, false);
     auto &FileMgr = Tool.getFiles();
     SourceManager Sources(Diagnostics, FileMgr);
     Rewriter Rewrite(Sources, DefaultLangOptions);
 
     Tool.applyAllReplacements(Rewrite);
     for (const auto &File : Files) {
       auto Entry = FileMgr.getFile(File);
+      if (!Entry) {
+        errs() << "clang-rename: " << File << " does not exist.\n";
+        return 1;
+      }
       const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
       Rewrite.getEditBuffer(ID).write(outs());
     }
   }
 
   return ExitCode;
 }