FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty.

[clayborg]

Summary

After switching to LLVM normalization, if we init FileSpec with "." we would end up with m_directory being NULL and m_filename being "".

This patch fixes this by allowing the path to be normalized and if it normalized to nothing, set it to m_filename.

[clayborg]

This patch is needed for some PathMappingList fixes I have coming up.

[labath]

Preserving the dot if it is the only path component is perfectly reasonable -- "" is not a valid path and we shouldn't convert a valid path into an invalid one. However, I think this should be done on the llvm side, in the remote_dots function and not as a workaround on top of it.

[clayborg]

I agree, but we should still guard against it and test in in LLDB to ensure this doesn't regress. I'll be happy to make a LLVM patch. Many people have branches that link against older LLVMs and the check is cheap.

[labath]

Adding a test for that in lldb is fine. If we're relying on some behavior we want to make sure it doesn't change without us noticing. However, I don't think we should make any special allowments for people using lldb with older versions of llvm. It sends the wrong message. Making this work in this case is easy enough, but this is just the tip of the iceberg. We have made many changes in the past where we add/modify/extend an llvm interface to make it work for our needs, and then almost immediately start using it in lldb (the last example of that is the switch to llvm path handling functions in FileSpec, if anyone tries use that with a older version of llvm, he'll get a bunch of errors). We're a single project with a single release schedule (I know a lot don't follow the official release schedule, but then they should at least cut from the same trunk revision; all the people that I know about do just that) and we don't support mixing revisions. If anyone tries to do things differently, he's on his own. If he's lucky, he'll get a build error. If not, he gets to track down subtle behavior differences.

[clayborg]

So the function in llvm is called llvm::sys::path::remove_dots(...) and it is removing the dots. Not sure it is correct to be changing a function that says "remove_dots" to not remove dots and actually return something with a . in it... Seems like this should be taken care of in LLDB. Thoughts?

[clayborg]

In D46783#1097549, @labath wrote:

Preserving the dot if it is the only path component is perfectly reasonable -- "" is not a valid path and we shouldn't convert a valid path into an invalid one. However, I think this should be done on the llvm side, in the remote_dots function and not as a workaround on top of it.

so the issue isn't just keeping a single ".", but "./" and "foo/.." and many others that result in the "." as being the result. And I don't think changing a function that is supposed to remove dots to not actually remove them is a good idea.

[labath]

In D46783#1099561, @clayborg wrote:

So the function in llvm is called llvm::sys::path::remove_dots(...) and it is removing the dots. Not sure it is correct to be changing a function that says "remove_dots" to not remove dots and actually return something with a . in it... Seems like this should be taken care of in LLDB. Thoughts?

That's kinda true, but I think this is a corner case that the author simply didn't think of when writing that function. Even though it is not mentioned anywhere, I think that a part of the contract of that function is that it returns a *valid* path, which is *equivalent* to the input path. "" is neither valid nor equivalent to ".", so I think this should trump the desire to remove all dots. I don't think any user would want the current behavior.

In fact when you look at the function description, it speaks about removing "./" components. "." does not have a "./" component.

So yes, I still believe the place for this is in the remove_dots function. Feel free to add me to the llvm patch, and I'll help you argue the case. If it turns out that present behavior is actually desired/depended on by somebody, then we can do it on our side, but I expect this is just a bug/corner case that nobody cared for until now.

[labath]

(Also, the function already does not remove leading .. components, so there is precedent already for not removing stuff when it causes the path to become not equivalent.)

[clayborg]

So I made a fix to LLVM and there are tests that are testing for the empty string:

[ RUN      ] Support.RemoveDots
/Users/gclayton/Documents/src/llvm/clean/llvm/unittests/Support/Path.cpp:1149: Failure
      Expected: ""
To be equal to: remove_dots(".\\\\\\\\\\", false, path::Style::windows)
      Which is: "."
/Users/gclayton/Documents/src/llvm/clean/llvm/unittests/Support/Path.cpp:1166: Failure
      Expected: ""
To be equal to: remove_dots("./////", false, path::Style::posix)
      Which is: "."

Are we not concerned with this?

[labath]

If those are the only tests that fail, then I'd still go for it, as I still firmly believe this is the correct behavior for such a function. Such a low level test does not have to mean that someone really cares about this, it could be more of a case of documenting existing behavior (the svn history is not really clear on this). If there are other llvm/clang tests failing as a result of changing that, then we can reconsider.

[clayborg]

Submitted the LLVM patch: https://reviews.llvm.org/D46887

[labath]

Thank you.

[clayborg]

Fixed LLDB test botes with:

Sending unittests/Utility/FileSpecTest.cpp
Transmitting file data .done
Committing transaction...
Committed revision 332511.

[clayborg]

Reviving this patch so I can get the changes into LLDB. Clang is expecting an empty path in many locations and I don't feel comfortable changing remove_dots in clang after trying it. So I would like to use this patch to fix things in LLDB.

[labath]

It would still be interesting to align remove_dots with the "standard practice", but it looks like that would end up being a big project. I think we can live with tweak on our side in this case. If someone ever comes around to changing remote_dots behavior, at least it we will be ready.

Thank you for trying though.

[labath]

This has broken the unit tests. Looks like a bad merge that did not take
into account the refactoring in r332088.

[clayborg]

Updated to what was committed.

[clayborg]

I did run unit tests and they all passed here?

[labath]

Sorry, I was wrong about the cause, but the bots are red nonetheless. My bet is it's the last {"", "."}, test, which is not working because of an early return in SetFile. TBH, I am not sure we would want that to work anyway, as we too treat an empty filespec specially in a lot of places.

[labath]

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/23447

[clayborg]

Fixed with:

svn commit unittests/Utility/FileSpecTest.cpp
Sending unittests/Utility/FileSpecTest.cpp
Transmitting file data .done
Committing transaction...
Committed revision 332633.

[clayborg]

Fixed with
svn commit unittests/Utility/FileSpecTest.cpp
Sending unittests/Utility/FileSpecTest.cpp
Transmitting file data .done
Committing transaction...
Committed revision 332633.

lldb/trunk/source/Utility/FileSpec.cpp

void FileSpec::SetFile(llvm::StringRef pathname, bool resolve, Style style) {
   // Normalize the path by removing ".", ".." and other redundant components.
   if (needsNormalization(resolved))
     llvm::sys::path::remove_dots(resolved, true, m_style);
 
   // Normalize back slashes to forward slashes
   if (m_style == Style::windows)
     std::replace(resolved.begin(), resolved.end(), '\\', '/');
 
+  if (resolved.empty()) {
+    // If we have no path after normalization set the path to the current
+    // directory. This matches what python does and also a few other path
+    // utilities. 
+    m_filename.SetString(".");
+    return;
+  }
+
   m_filename.SetString(llvm::sys::path::filename(resolved, m_style));
   llvm::StringRef dir = llvm::sys::path::parent_path(resolved, m_style);
   if (!dir.empty())
     m_directory.SetString(dir);
 }
 
 void FileSpec::SetFile(llvm::StringRef path, bool resolve,
                        const llvm::Triple &Triple) {

lldb/trunk/unittests/Utility/FileSpecTest.cpp

std::pair<const char *, const char *> posix_tests[] = {
       {"/foo//bar/./baz", "/foo/bar/baz"},
       {"/./foo", "/foo"},
       {"/", "/"},
       {"//", "/"},
       {"//net", "//net"},
       {"/..", "/"},
       {"/.", "/"},
       {"..", ".."},
-      {".", ""},
+      {".", "."},
+      {"", "."},
       {"../..", "../.."},
-      {"foo/..", ""},
+      {"foo/..", "."},
       {"foo/../bar", "bar"},
       {"../foo/..", ".."},
       {"./foo", "foo"},
       {"././foo", "foo"},
       {"../foo", "../foo"},
       {"../../foo", "../../foo"},
   };
   for (auto test : posix_tests) {
std::pair<const char *, const char *> windows_tests[] = {
       {R"(\\)", R"(\)"},
       {R"(\\net)", R"(\\net)"},
       {R"(c:\..)", R"(c:\)"},
       {R"(c:\.)", R"(c:\)"},
       // TODO: fix llvm::sys::path::remove_dots() to return "\" below.
       {R"(\..)", R"(\..)"},
       //      {R"(c:..)", R"(c:..)"},
       {R"(..)", R"(..)"},
-      {R"(.)", R"()"},
+      {R"(.)", R"(.)"},
       // TODO: fix llvm::sys::path::remove_dots() to return "c:\" below.
       {R"(c:..\..)", R"(c:\..\..)"},
       {R"(..\..)", R"(..\..)"},
-      {R"(foo\..)", R"()"},
+      {R"(foo\..)", R"(.)"},
       {R"(foo\..\bar)", R"(bar)"},
       {R"(..\foo\..)", R"(..)"},
       {R"(.\foo)", R"(foo)"},
       {R"(.\.\foo)", R"(foo)"},
       {R"(..\foo)", R"(..\foo)"},
       {R"(..\..\foo)", R"(..\..\foo)"},
+      {"", "."},
   };
   for (auto test : windows_tests) {
     EXPECT_EQ(test.second,
               FileSpec(test.first, false, FileSpec::Style::windows).GetPath())
         << "Original path: " << test.first;
   }
 }