This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix shared library directory computation on windows
ClosedPublic

Authored by labath on Feb 16 2021, 6:22 AM.

Details

Summary

Our code for locating the shared library directory works via dladdr (or
the windows equivalent) to locate the path of an address known to reside
in liblldb. This works great for C++ programs, but there's a catch.

When (lib)lldb is used from python (like in our test suite), this dladdr
call will return a path to the _lldb.so (or such) file in the python
directory. To compensate for this, we have code which attempts to
resolve this symlink, to ensure we get the canonical location. However,
here's the second catch.

On windows, this file is not a symlink (but a copy), so this logic
fails. Since most of our other paths are derived from the liblldb
location, all of these paths will be wrong, when running the test suite.
One effect of this was the failure to find lldb-server in D96202.

To fix this issue, I add some windows-specific code to locate the
liblldb directory. Since it cannot rely on symlinks, it works by
manually walking the directory tree -- essentially doing the opposite of
what we do when computing the python directory.

To avoid python leaking back into the host code, I implement this with
the help of a callback which can be passed to HostInfo::Initialize in
order to assist with the directory location. The callback lives inside
the python plugin.

I also strenghten the existing path test to ensure the returned path is
the right one.

Diff Detail

Event Timeline

labath requested review of this revision.Feb 16 2021, 6:22 AM
labath created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 6:22 AM
JDevlieghere accepted this revision.Feb 16 2021, 10:58 AM

Seems like a reasonable solution. LGTM.

lldb/include/lldb/Host/HostInfoBase.h
44–46

Why a function pointer and no function_ref?

This revision is now accepted and ready to land.Feb 16 2021, 10:58 AM
labath added inline comments.Feb 16 2021, 11:03 AM
lldb/include/lldb/Host/HostInfoBase.h
44–46

function_ref can only be used for objects which do not escape the current function (just like StringRef), while this value is persisted. It could be a std::function, but that seems a bit overkill.

JDevlieghere added inline comments.Feb 16 2021, 11:05 AM
lldb/include/lldb/Host/HostInfoBase.h
44–46

Forgive my ignorance, but how does that apply to SharedLibraryDirectoryHelper which is a static method?

labath added inline comments.Feb 16 2021, 11:35 AM
lldb/include/lldb/Host/HostInfoBase.h
44–46

It doesn't -- persisting a function_ref pointing to a static method would actually work in this case. However, that's the _only_ thing that would work, so there's no benefit to doing it.
And there's the downside that if someone tries to something more fancy through that function_ref, it would blow up. E.g., something like this would be impossible

HostInfo::Initialize([important_variable](FileSpec &this_file) { do_stuff(this_file, important_variable); });

Because by the time that the function_ref gets called, the storage for important_variable (along with the entire lambda object) would be destroyed. It's exactly the same situation as with a StringRef. Imagine code like:

StringRef g_foo;
HostInfo::Initialize(StringRef foo) { g_foo = foo; } 
HostInfo::Stuff() { /* do something with g_foo */ }

This would work just fine if you always pass a string literal for the StringRef, but would blow up as soon as one tries to pass a temporary string. And there are two ways to make it safer:

  • use std::string (like std::function)
  • use const char * (like a raw function pointer, except function pointers are even safer, as dynamically allocating functions is not common)
JDevlieghere added inline comments.Feb 16 2021, 11:37 AM
lldb/include/lldb/Host/HostInfoBase.h
44–46

Okay, sounds like we're on the same page, I misunderstood your comment as "this can't work for the current case" which led to the confusion on my part. I agree that making the interface as safe as possible is desirable.

This revision was landed with ongoing or failed builds.Feb 18 2021, 6:38 AM
This revision was automatically updated to reflect the committed changes.