Page MenuHomePhabricator

FileSpec::Resolve should not turn a filename-only FileSpec into a qualified FileSpec if that file doesn't exist
ClosedPublic

Authored by jasonmolenda on Feb 6 2015, 4:31 PM.

Details

Reviewers
zturner
Summary

FileSpec::Resolve used to use the realpath() POSIX API to fully qualify filenames. If realpath() was passed "ls" as an argument, if there was no "ls" in the current working directory, it would not change the argument.

The switch to llvm::sys::fs::make_absolute() changed this behavior. Now if you pass in "ls", make_absolute() will prepend the current working directory path and return that.

I can see the argument for make_absolute()'s behavior. If I were *creating* a file, so it didn't exist yet, then the make_absolute() behavior would be reasonable. But this isn't how lldb is using this -- we want to find a file that actually exists on the filesystem.

The failure mode for this is that a user does "lldb ls". Previously lldb would search through $PATH and find /bin/ls. Now when lldb creates a FileSpec for "ls", it resolves it into current-working-directory/ls which doesn't exist and it fails.

This is the second bug clearly tied to this change in behavior (see r222498 where I was fixing another one a couple months ago) - and it's a problematic landmine to leave in place given what lldb means when it says "Resolve" for a FileSpec.

Zachary, I'm not very familiar with the SmallVector/SmallString classes, this is probably not the best way to accomplish what I want to do here. What do you think?

Diff Detail

Repository
rL LLVM

Event Timeline

jasonmolenda retitled this revision from to FileSpec::Resolve should not turn a filename-only FileSpec into a qualified FileSpec if that file doesn't exist.
jasonmolenda updated this object.
jasonmolenda edited the test plan for this revision. (Show Details)
jasonmolenda added a reviewer: zturner.
jasonmolenda added a subscriber: Unknown Object (MLST).

Forgot to include lldb-commits on the subscription list.

zturner edited edge metadata.Feb 6 2015, 5:35 PM

Could we add a test that verifies the behavior of this function? Greg created an SBFileSpec test recently in lldb/test/functionalities/paths/TestPaths.py. As long as we don't use a builtin command like ls or something like it should be fine. Instead you could use something like sys.executable, then the os.path module to get out the filename, then verify that after you resolve it, it matches sys.executable.

source/Host/common/FileSpec.cpp
167–173

You can write this like this:

llvm::SmallString<PATH_MAX> original_path(path.begin(), path.end());

172–178

I'm preeeety sure that make_absolute() guarantees the result will be null terminated. But not quite 100%. If you can determine that it's guaranteed to be null terminated, you can replace all of this with path.data(), which returns a char*. If you want to be extra certain though, you can still make this shorter by writing this instead:

path.push_back(0);
path.pop_back();
if (stat(path.data())) // path.data() is now definitely null terminated.

185–186

This can just be path = original_path;

http://lists.cs.uiuc.edu/pipermail/lldb-dev/2014-October/005551.html

Here's a thread that provides some context for why this was changed.

Your failure mode mentions invoking "lldb ls". If FileSpec::Resolve just
ends up returning "ls" unchanged, who actually resolves it? In the thread i
linked above, it seems i was also dealing with the same issue. Perhaps it's
time to add a function to resolve an executable using PATH? We could put it
in llvm::sys::fs and call it llvm::sys::resolve_executable(StringRef Exe,
bool use_path). Seems generally useful outside of lldb. The LLDB could just
always call this function before constructing a FileSpec to a target
executable.

Fwiw though, i agree if it doesn't exist in the current dir, we shouldn't
be prepending the working dir to it. Let me test this CL on Windows Monday
before you commit though, if you don't mind.

Thanks for all the comments, no rush on this. I need to update the patch to integrate your suggested changes and try to write a test case still.

This comment was removed by jasonmolenda.
jasonmolenda edited edge metadata.Feb 16 2015, 6:53 PM
jasonmolenda set the repository for this revision to rL LLVM.

Updated patch to include the fixes Zachary suggested, added a simple test case for this behavior to functionality/paths.

zturner accepted this revision.Feb 17 2015, 5:49 PM
zturner edited edge metadata.
zturner added inline comments.
source/Host/common/FileSpec.cpp
173

nit: s/nul/null/

179–180

After you call path.clear(), the length is 0. So you can change these two lines to path.append(original_path.begin(), original_path.end()). I think path = original_path will also work.

test/functionalities/paths/TestPaths.py
32–35

Admittedly this is pretty unlikely. You might be able to make it a *tad* better by using tempfile.mktemp() to generate the name of a file which definitely doesn't exist in the current directory. then using something from os.path to strip the directory name from it, and then passing that. I don't feel too strongly though.

This revision is now accepted and ready to land.Feb 17 2015, 5:49 PM
jasonmolenda edited edge metadata.

Updated patch to use path.clear(), path.append(original_path.begin(), original_path.end()); as suggested by Zachary.

Kept the "be sure we have a nul terminated string". nul is '\0'. null is (void*)0. We want a nul terminated cstring here.

Left the testsuite as-is, I agree it would be better to get a randomly generated temp name but this is going to be fine.

jasonmolenda closed this revision.Feb 24 2015, 6:37 PM

Sending source/Host/common/FileSpec.cpp
Sending test/functionalities/paths/TestPaths.py
Transmitting file data ..
Committed revision 230451.