[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

Remove arg.

[clayborg]

source/Utility/FileSpec.cpp

821

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

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.

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 (*this == FileSpec(".", false))
+    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);
 }
 //------------------------------------------------------------------
 /// 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
 ///     file, \b false otherwise.
 //------------------------------------------------------------------