[PATCH] D25006: Make FilePath of Replacement an absolute file path when possible.

[Haojian Wu via cfe-commits]

hokein created this revision.
hokein added reviewers: klimek, djasper, ioeric.
hokein added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

The FilePath of the Replacement constructed from a SourceManager can be
an absolute file path or a file path relative to the build directory (It
depends on the compliation commands).

If the FilePath is a relative path, it may caused crash issues
when applying the Replacement if the running directory of the binary is
not the same with the build directory. Have verified the crashes in
several clang-tools, e.g. clang-rename, clang-move, clang-change-name,
        clang-tidy (fixed in D17335).

By making the FilePath an absolute path, it resolves the crashes.

https://reviews.llvm.org/D25006

Files:
  lib/Tooling/Core/Replacement.cpp
  unittests/Tooling/RefactoringTest.cpp

Index: unittests/Tooling/RefactoringTest.cpp
===================================================================
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -466,7 +466,7 @@
 void expectReplacementAt(const Replacement &Replace,
                          StringRef File, unsigned Offset, unsigned Length) {
   ASSERT_TRUE(Replace.isApplicable());
-  EXPECT_EQ(File, Replace.getFilePath());
+  EXPECT_TRUE(Replace.getFilePath().endswith(File));
   EXPECT_EQ(Offset, Replace.getOffset());
   EXPECT_EQ(Length, Replace.getLength());
 }
Index: lib/Tooling/Core/Replacement.cpp
===================================================================
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tooling {
@@ -103,8 +104,22 @@
                                         StringRef ReplacementText) {
   const std::pair<FileID, unsigned> DecomposedLocation =
       Sources.getDecomposedLoc(Start);
-  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
-  this->FilePath = Entry ? Entry->getName() : InvalidLocation;
+  this->FilePath = InvalidLocation;
+  if (const FileEntry *Entry =
+          Sources.getFileEntryForID(DecomposedLocation.first)) {
+    auto Dir = Sources.getFileManager()
+                   .getVirtualFileSystem()
+                   ->getCurrentWorkingDirectory();
+    if (Dir) {
+      llvm::SmallString<128> AbsolutePath(Entry->getName());
+      std::error_code EC =
+          llvm::sys::fs::make_absolute((Dir.get()), AbsolutePath);
+      if (!EC) {
+        llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
+        this->FilePath = AbsolutePath.str();
+      }
+    }
+  }
   this->ReplacementRange = Range(DecomposedLocation.second, Length);
   this->ReplacementText = ReplacementText;
 }



Index: unittests/Tooling/RefactoringTest.cpp
===================================================================
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -466,7 +466,7 @@
 void expectReplacementAt(const Replacement &Replace,
                          StringRef File, unsigned Offset, unsigned Length) {
   ASSERT_TRUE(Replace.isApplicable());
-  EXPECT_EQ(File, Replace.getFilePath());
+  EXPECT_TRUE(Replace.getFilePath().endswith(File));
   EXPECT_EQ(Offset, Replace.getOffset());
   EXPECT_EQ(Length, Replace.getLength());
 }
Index: lib/Tooling/Core/Replacement.cpp
===================================================================
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tooling {
@@ -103,8 +104,22 @@
                                         StringRef ReplacementText) {
   const std::pair<FileID, unsigned> DecomposedLocation =
       Sources.getDecomposedLoc(Start);
-  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
-  this->FilePath = Entry ? Entry->getName() : InvalidLocation;
+  this->FilePath = InvalidLocation;
+  if (const FileEntry *Entry =
+          Sources.getFileEntryForID(DecomposedLocation.first)) {
+    auto Dir = Sources.getFileManager()
+                   .getVirtualFileSystem()
+                   ->getCurrentWorkingDirectory();
+    if (Dir) {
+      llvm::SmallString<128> AbsolutePath(Entry->getName());
+      std::error_code EC =
+          llvm::sys::fs::make_absolute((Dir.get()), AbsolutePath);
+      if (!EC) {
+        llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
+        this->FilePath = AbsolutePath.str();
+      }
+    }
+  }
   this->ReplacementRange = Range(DecomposedLocation.second, Length);
   this->ReplacementText = ReplacementText;
 }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits