[FileSpec] Re-implmenet removeLastPathComponent

[JDevlieghere]

Summary

When reading DBGSourcePathRemapping from a dSYM, we remove the last two
path components to make the source lookup more general. However, when
dealing with a relative path that has less than 2 components, we ended
up with an invalid (empty) FileSpec.

This patch changes the behavior of removeLastPathComponent to remove the
last path component, if possible. It does this by checking whether a
parent path exists, and if so using that as the new path. We rely
entirely on LLVM's path implementation to do the heavy lifting.

We now also return a boolean which indicates whether the operator was
successful or not.

[jasonmolenda]

This looks good. My original change in r311622 to add this "remove the last two file path components" should have explicitly said that this is only done for DBGVersion == 2 to make it clearer for people reading the code.

At this point the parsing code behaves like:

DBGVersion 1 (or missing DBGVersion) - ignore the DBGSourcePathRemapping dictionary
DBGVersion 2 - Use DBGSourcePathRemapping, but chop off two components from both source and destination (assumption: the last two components are PROJECT-NAME/PROJECT-VERSION-NUMBER)
DBGVersion 3 - Use DBGSourcePathRemapping without any modifications

These interpretations of the DBGSourcePathRemapping / DBGVersion have been a little ad-hoc, as we've discovered issues in deployed plists that needed to be accommodated by lldb.

[clayborg]

Remove the argument since we don't need it. Just assume we keep ".".

include/lldb/Utility/FileSpec.h

535 ↗

(On Diff #148976)

Why add this? If the path is just "." to begin with, there is nothing to do. I would vote to not add this argument since we don't need it. Add it back if we ever do.

source/Host/macosx/Symbols.cpp

412–415 ↗

(On Diff #148976)

revert

source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

263–266 ↗

(On Diff #148976)

revert

source/Utility/FileSpec.cpp

788 ↗

(On Diff #148976)

Remove arg.

[clayborg]

source/Utility/FileSpec.cpp

821 ↗

(On Diff #148976)

revert whitespace change.

[JDevlieghere]

include/lldb/Utility/FileSpec.h

535 ↗

(On Diff #148976)

The reason I opted for a flag rather than changing the behavior is that other (llvm & foundation) implementation of similar functions behave the same as the current implementation. However I think an argument can be made for this as well, so I'm happy to change it :-)

[JDevlieghere]

• Address Greg's feedback

[clayborg]

LGTM

[labath]

I was also thinking whether this behavior needs to be conditional. If nothing depends on this, then I'm all for changing the condition. However, my question is whether "." is the only path we should treat this way. I'm thinking it would be more consistent to give the root directory the same treatment too (so, "/".RemoveLastComponent() == "/", "//net".RemoveLastComponent()=="//net", etc). I guess you're unlikely to encounter an absolute path with less than two components in the path mappings, but it sounds like this is the behavior you would want there anyway.

Also, a test for the new behavior of the FileSpec function would be in order, as there are some interesting corner cases which I am not sure you get right (e.g, what is the value of "foo".RemoveLastComponent()?)

source/Utility/FileSpec.cpp

796 ↗

(On Diff #148986)

This won't change the result of this particular comparison, but it's best to get in the habit of specifying the path syntax when constructing FileSpecs. "native" is not always the right choice.

[JDevlieghere]

• Replace custom logic with LLVM's path logic.
• Add tests.

[labath]

unittests/Utility/FileSpecTest.cpp

342 ↗

(On Diff #149083)

Is this the behavior you want here? I was thinking we could fold this all the way to "." (arguably "." is a parent of "foo", though I can see how that may be thought to be too magical)

[JDevlieghere]

unittests/Utility/FileSpecTest.cpp

342 ↗

(On Diff #149083)

I like this approach it doesn't consider . to be a special case and therefore things are consistent and straightforward. My worry is that if we add special cases, we risk ending up with missing edge cases (like the previous implementation).

[labath]

unittests/Utility/FileSpecTest.cpp

342 ↗

(On Diff #149083)

Ok, if that works for your use case (as far as path remappings go, "." is a more generic mapping than "foo"), then that's fine by me. I like how you were able to concisely describe the new semantics of this function.

lldb/trunk/include/lldb/Utility/FileSpec.h

public:
   FileSpec CopyByRemovingLastPathComponent() const;
 
   void PrependPathComponent(llvm::StringRef component);
   void PrependPathComponent(const FileSpec &new_path);
 
   void AppendPathComponent(llvm::StringRef component);
   void AppendPathComponent(const FileSpec &new_path);
 
-  void RemoveLastPathComponent();
+  //------------------------------------------------------------------
+  /// Removes the last path component by replacing the current path with its
+  /// parent. When the current path has no parent, this is a no-op.
+  ///
+  /// @return
+  ///     A boolean value indicating whether the path was updated.
+  //------------------------------------------------------------------
+  bool RemoveLastPathComponent();
 
   ConstString GetLastPathComponent() const;
 
   enum EnumerateDirectoryResult {
     eEnumerateDirectoryResultNext,  // Enumerate next entry in the current
                                     // directory
     eEnumerateDirectoryResultEnter, // Recurse into the current entry if it is a
                                     // directory or symlink, or next if not

lldb/trunk/source/Utility/FileSpec.cpp

void FileSpec::AppendPathComponent(llvm::StringRef component) {
 
   SetFile(result, false, m_style);
 }
 
 void FileSpec::AppendPathComponent(const FileSpec &new_path) {
   return AppendPathComponent(new_path.GetPath(false));
 }
 
-void FileSpec::RemoveLastPathComponent() {
-  // CLEANUP: Use StringRef for string handling.
-
-  const bool resolve = false;
-  if (m_filename.IsEmpty() && m_directory.IsEmpty()) {
-    SetFile("", resolve);
-    return;
-  }
-  if (m_directory.IsEmpty()) {
-    SetFile("", resolve);
-    return;
-  }
-  if (m_filename.IsEmpty()) {
-    const char *dir_cstr = m_directory.GetCString();
-    const char *last_slash_ptr = ::strrchr(dir_cstr, '/');
-
-    // check for obvious cases before doing the full thing
-    if (!last_slash_ptr) {
-      SetFile("", resolve);
-      return;
-    }
-    if (last_slash_ptr == dir_cstr) {
-      SetFile("/", resolve);
-      return;
-    }
-    size_t last_slash_pos = last_slash_ptr - dir_cstr + 1;
-    ConstString new_path(dir_cstr, last_slash_pos);
-    SetFile(new_path.GetCString(), resolve);
-  } else
-    SetFile(m_directory.GetCString(), resolve);
+bool FileSpec::RemoveLastPathComponent() {
+  llvm::SmallString<64> current_path;
+  GetPath(current_path, false);
+  if (llvm::sys::path::has_parent_path(current_path, m_style)) {
+    SetFile(llvm::sys::path::parent_path(current_path, m_style), false,
+            m_style);
+    return true;
+  return false;
 }
 //------------------------------------------------------------------
 /// Returns true if the filespec represents an implementation source
 /// file (files with a ".c", ".cpp", ".m", ".mm" (many more)
 /// extension).
 ///
 /// @return
 ///     \b true if the filespec represents an implementation source

lldb/trunk/unittests/Utility/FileSpecTest.cpp

llvm::StringRef is_relative[] = {
     "./foo/bar.c"
   };
   for (const auto &path: is_relative) {
     FileSpec spec(path, false, FileSpec::Style::posix);
     EXPECT_TRUE(spec.IsRelative());
   }
 }
 
+TEST(FileSpecTest, RemoveLastPathComponent) {
+  FileSpec fs_posix("/foo/bar/baz", false, FileSpec::Style::posix);
+  EXPECT_STREQ("/foo/bar/baz", fs_posix.GetCString());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/foo", fs_posix.GetCString());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/", fs_posix.GetCString());
+  EXPECT_FALSE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/", fs_posix.GetCString());
+
+  FileSpec fs_posix_relative("./foo/bar/baz", false, FileSpec::Style::posix);
+  EXPECT_STREQ("foo/bar/baz", fs_posix_relative.GetCString());
+  EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo/bar", fs_posix_relative.GetCString());
+  EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
+  FileSpec fs_posix_relative2("./", false, FileSpec::Style::posix);
+  EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+  EXPECT_FALSE(fs_posix_relative2.RemoveLastPathComponent());
+  EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+
+  FileSpec fs_windows("C:\\foo\\bar\\baz", false, FileSpec::Style::windows);
+  EXPECT_STREQ("C:\\foo\\bar\\baz", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:\\foo\\bar", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:\\foo", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:\\", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:", fs_windows.GetCString());
+  EXPECT_FALSE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:", fs_windows.GetCString());
+}