This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Symlink the Clang resource directory to the LLDB build directory in standalone builds
ClosedPublic

Authored by teemperor on Sep 30 2020, 8:32 AM.

Details

Summary

When doing a standalone build (i.e., building just LLDB against an existing LLVM/Clang
installation), LLDB is currently unable to find any Clang resource directory that contains
all the builtin headers we need to parse real source code. This causes several tests that
actually parse source code on disk within the expression parser to fail (most notably nearly
all the import-std-module tests).

The reason why LLDB can't find the resource directory is that we search based on the path
of the LLDB shared library path. We assumed that the Clang resource directory is in the
same prefix and has the same relative path to the LLDB shared library (e.g.,
../clang/10.0.0/include). However for a standalone build where the existing Clang can
be anywhere on the disk, so we can't just rely on the hardcoded relative paths to the
LLDB shared library.

It seems we can either solve this by copying the resource directory to the LLDB installation,
symlinking it there or we pass the path to the Clang installation to the code that is trying to find the resource
directory. When building the LLDB framework we currently copy the resource directory
over to the framework folder (this is why the import-std-module are not failing on the
Green Dragon standalone bot).

This patch symlinks the resource directory of Clang into the LLDB build directory. The
reason for that is simply that this is only needed when running LLDB from the build
directory. Once LLDB and Clang/LLVM are installed the already existing logic can find
the Clang resource directory by searching relative to the LLDB shared library.

Diff Detail

Event Timeline

teemperor created this revision.Sep 30 2020, 8:32 AM
teemperor requested review of this revision.Sep 30 2020, 8:32 AM
kastiglione accepted this revision.Sep 30 2020, 8:57 AM

Nice fix. Every time I see issues with the relative resource directory heuristic, I think "someone should improve that".

lldb/cmake/modules/LLDBConfig.cmake
258–259

I realize that this logic previously existed, but should this if/elseif chain check that these paths exists before committing to them? As it stands now, if Clang_DIR has a value that doesn't exist on disk, and meanwhile LLVM_DIR is also assigned, but does exist on disk, then this logic will take Clang_DIR over LLVM_DIR, when it shouldn't.

273

should this remain set, so that it can be checked at runtime anyway? In other words, could it ever not exist at the time of this check, but will exist by the time it's used at runtime?

lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
73–77 ↗(On Diff #295293)

should a standalone build put its resource dir first in the search path?

This revision is now accepted and ready to land.Sep 30 2020, 8:57 AM
JDevlieghere requested changes to this revision.Sep 30 2020, 9:25 AM

I'm not convinced this is the right solution. First of all I don't think embedding a build path in LLDB in acceptable. Swift uses the standalone builds and with this change the linux toolchain will have some random CI path embedded in LLDB, which based on the comment above it wouldn't even need. The existing code implies that this should work once LLDB is installed. If this issue is indeed limited in scope to the build directory, I think we should either create a symlink or copy over the resource directory. An alternative, more general approach might be to make the source directory configurable through a setting so that this could work wherever LLDB lives and then have the test suite populate that setting based on the location of the resource directory in the clang build dir.

This revision now requires changes to proceed.Sep 30 2020, 9:25 AM
teemperor planned changes to this revision.Sep 30 2020, 12:32 PM

Having a symlink to the Clang resource dir inside the LLDB *build* dir (but not the install) seems like a reasonable workaround. I'm not sure how valuable this as a setting is as the value should never actually change after LLDB installation and most likely people will just break their install by accident when touching it. But I think having a fully optional CMake parameter would be a good follow up in case people have some setup where the install prefixes don't match.

I must have missed something, isn't this embedded the installed path of the clang resource dir? The diff description says "i.e., building just LLDB against an existing LLVM/Clang installation". I agree that embedding a build path is bad, but embedding clang's installed path seems legit.

I must have missed something, isn't this embedded the installed path of the clang resource dir? The diff description says "i.e., building just LLDB against an existing LLVM/Clang installation". I agree that embedding a build path is bad, but embedding clang's installed path seems legit.

You can build both against a LLVM/Clang installation (like in /usr or some other prefix) or just a LLVM/Clang build directory that never went through the actual install step to some prefix (but that acts as if it's an installed Clang).

teemperor updated this revision to Diff 295852.Oct 2 2020, 9:34 AM
teemperor retitled this revision from [lldb] Pass the Clang resource directory to LLDB in standalone builds to [lldb] Symlink the Clang resource directory to the LLDB build directory in standalone builds.
teemperor edited the summary of this revision. (Show Details)
  • Replaced with symlinking resource dir to LLDB build directory
JDevlieghere added inline comments.Oct 2 2020, 10:01 AM
lldb/source/API/CMakeLists.txt
210

Can you check that this works in a non-framework Xcode build (or any other multi-config generator). I think the generator expressions are resolved too late.

teemperor updated this revision to Diff 296252.Oct 5 2020, 12:00 PM
  • Fixed Xcode build.

Yeah it seems that the LIBRARY_OUTPUT_DIRECTORY doesn't actually contain the subdirectory, so moved over to TARGET_FILE_DIR which is anyway closer to the way this should work (as we search relative to the liblldb directory).

JDevlieghere accepted this revision.Oct 5 2020, 12:12 PM

Thanks. LGTM!

This revision is now accepted and ready to land.Oct 5 2020, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 12:29 AM
mnovakovic added inline comments.
lldb/source/API/CMakeLists.txt
210

Good catch and this doesn't work in a non-Framework context. I am building standalone LLDB and it fails on the next line: file(MAKE_DIRECTORY "${LLDB_CLANG_RESOURCE_DIR_PARENT}")

That line is not even needed because line 214 builds the directory already, the fix for this could be to just remove like 211 and only build directory in the post_build phase.

mnovakovic added inline comments.Nov 2 2020, 5:19 PM
lldb/source/API/CMakeLists.txt
210

Actually removing the line 211 doesn't solve the issue, the failure is only postponed until later when the line 214 is executed (link stage looks like it) and it fails because ${LLDB_CLANG_RESOURCE_DIR_PARENT} is evaluated to an empty string.

mnovakovic added inline comments.Nov 2 2020, 9:32 PM
lldb/source/API/CMakeLists.txt
210

Looks like I can't change the comment - my bad, looks like the deleting line 211 does the trick

Gentoo uses plain /usr/lib/clang/X.Y.Z which doesn't really match any of the tested paths. Do you think there's a point in adding yet another case there or should I just override it locally?