Fix llvm::sys::path::remove_dots() to return "." instead of an empty path.
AbandonedPublic

Authored by clayborg on May 15 2018, 9:17 AM.

Details

Summary

LLDB recently started using llvm::sys::path::remove_dots() to normalize paths inside of LLDB. We ran into a few cases where this was falling down for us where paths that resulted in the current working directory (".", "./", "foo/.." and more) would result in an empty file specification since llvm::sys::path::remove_dots() would return an empty path. This fix corrects this issue by having "." returned if the path specified was not empty to begin with and the resulting path would have been empty. Pavel and I believe this is the intention of this function. If this is not correct please comment.

Diff Detail

Repository
rL LLVM
clayborg created this revision.May 15 2018, 9:17 AM

Discussion of the reasoning for this is available in the LLDB patch: https://reviews.llvm.org/D46783

As I said on the lldb patch, I believe this behavior makes more sense for a function like this (i.e., it should never return an invalid path ("") if the input path is valid, even if it means leaving some dots around. (We already keep a leading ../ component for the remove_dot_dot=true case as it is not possible to preserve the path semantics without this.)

unittests/Support/Path.cpp
1166 ↗(On Diff #146851)

Another interesting test here would be to make sure this does return an empty path when the input is also empty.

clayborg added inline comments.May 15 2018, 9:34 AM
unittests/Support/Path.cpp
1166 ↗(On Diff #146851)

I will add one.

clayborg updated this revision to Diff 146913.May 15 2018, 1:56 PM

Added a test that tests for an empty path and making sure remove_dots leaves it empty.

clayborg marked an inline comment as done.May 15 2018, 1:56 PM
ruiu added a subscriber: ruiu.May 15 2018, 2:08 PM

I believe this function was written based on Rob Pike's Clean() function for Go, and the Go's function also returns "." (instead of "") for the current directory. So this change makes sense to me.

https://golang.org/pkg/path/#example_Clean

Maybe you should return "." for "". as well. An empty string as a path component makes sense, as "/foo/bar", "/foo//bar" and "/foo/./bar" are all interpreted as the same path. Go's Clean() returns "." for "" too.

I believe this function was written based on Rob Pike's Clean() function for Go, and the Go's function also returns "." (instead of "") for the current directory. So this change makes sense to me.

https://golang.org/pkg/path/#example_Clean

Maybe you should return "." for "". as well. An empty string as a path component makes sense, as "/foo/bar", "/foo//bar" and "/foo/./bar" are all interpreted as the same path. Go's Clean() returns "." for "" too.

I was surprised by this behavior, so I checked what python does in this case. And indeed, os.path.normpath("") will also return ".". So, it seems there is an established behavior pattern for this input among functions of this type, and it would be best to follow that (if that doesn't cause too much havoc among existing users).

clayborg updated this revision to Diff 147134.May 16 2018, 11:11 AM

Fix empty path to become "." to match python and Go's path utilities.

ruiu accepted this revision.May 16 2018, 11:25 AM

LGTM

This revision is now accepted and ready to land.May 16 2018, 11:25 AM
This revision was automatically updated to reflect the committed changes.
clayborg reopened this revision.May 16 2018, 4:35 PM

Reverted 332508 as it was causing clang test suite problems.

This revision is now accepted and ready to land.May 16 2018, 4:35 PM
clayborg abandoned this revision.May 16 2018, 4:39 PM

So many places that are manually calling remove_dots are expecting an empty path in clang. I am also not sure if any other clients, like lld or other source bases won't be affected so I am going to abandon this patch