Page MenuHomePhabricator

Resolve symlinks when looking for the LLDB shlib
AcceptedPublic

Authored by spyffe on Sep 18 2015, 2:23 PM.

Details

Reviewers
zturner
clayborg
Summary

When running the testsuite, the LLDB shlib is a symlink to its actual location, so finding shlib-relative resources can be problematic. This patch fixes that by resolving symlinks.

Diff Detail

Event Timeline

spyffe updated this revision to Diff 35135.Sep 18 2015, 2:23 PM
spyffe retitled this revision from to Resolve symlinks when looking for the LLDB shlib.
spyffe updated this object.
spyffe added a reviewer: clayborg.
spyffe added a subscriber: lldb-commits.
clayborg requested changes to this revision.Sep 18 2015, 2:26 PM
clayborg edited edge metadata.
clayborg added inline comments.
include/lldb/Host/FileSpec.h
516

rename to ResolveSymbolicLink?

source/Host/common/FileSpec.cpp
818

rename to ResolveSymbolicLink() and return a copy of this object if it isn't a symbolic link

source/Host/common/HostInfoBase.cpp
310–312

No need to check if we change the behavior of ResolveSymbolicLink, this code will just be:

lldb_file_spec = lldb_file_spec.ResolveSymbolicLink();
This revision now requires changes to proceed.Sep 18 2015, 2:26 PM
spyffe updated this revision to Diff 35138.Sep 18 2015, 2:36 PM
spyffe edited edge metadata.

Updated to reflect Greg's comments.

clayborg accepted this revision.Sep 18 2015, 2:37 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 18 2015, 2:37 PM

Can you move this to FileSystem.h? I don't think we should be adding more
things that hit the file system to FileSpec. That's exactly the reason
FileSystem.h exists, because many of the operations will be implemented
differently across platforms, so we should be using the Host layer.

Furthermore, FileSpec can refer to a remote path, so you can't even
guarantee that the OS you're on is the same OS as that which the path
refers to. So another reason why putting this in FileSpec doesn't make
sense in my opinion.

spyffe added a subscriber: spyffe.Sep 18 2015, 3:08 PM

Sure. On it.

Sean

Thanks! Feel free to leave the method in Host/windows/FileSystem.cpp
empty, i'll fill it out.

spyffe updated this revision to Diff 35143.Sep 18 2015, 3:15 PM
spyffe added a reviewer: zturner.
spyffe removed a subscriber: zturner.

At zturner's suggestion, moved the function to FileSystem and renamed it to be more consistent with what other FileSystem functions are called.

zturner edited edge metadata.Sep 18 2015, 3:19 PM

If you change the name back to ResolveSymbolicLink or GetSymbolicLinkTarget, then this looks fine.

include/lldb/Host/FileSystem.h
43

I'd rather not call it realpath, because even though realpath is the way it will be implemented on non-Windows platforms, realpath has some subtleties in its semantics. So anyone reading this function name will expect that it has realpath semantics, even though some of the semantics are only well-defined in the context of a posix-like file system. I actually like your original name ResolveSymbolicLink better, because the behavior is narrow enough that it's easy to implement everywhere, and we don't have to worry about these little edge cases.

spyffe updated this revision to Diff 35144.Sep 18 2015, 3:23 PM
spyffe edited edge metadata.

Restored the old name based on zturner's suggestion that Realpath() is too specific and has semantics that Windows wouldn't honor.

zturner accepted this revision.Sep 18 2015, 3:23 PM
zturner edited edge metadata.

Thanks!