Page MenuHomePhabricator

Do not explicitly depend on llvm tools during standalone build
AbandonedPublic

Authored by serge-sans-paille on Feb 13 2019, 10:07 AM.

Details

Summary

When building lldb in standalone mode, the llvm-nm, llvm-objdump etc target are not available, so only conditionnaly include them.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
xiaobai added a subscriber: xiaobai.

I think you might need to rebase this patch because D57233 seems to accomplish part of what this patch does, but I don't see those changes here.

labath resigned from this revision.Feb 13 2019, 11:07 PM

Alex and Stefan are the standalone build experts these days, so I'll let them do the reviewing here.

All these targets are exported from the LLVM build-tree since D56606 and from the install-tree since D57383.

I did not revert or continue to work on D57233 yet, because it needs a good concept first. LLDB_BUILT_STANDALONE is not the right condition here. What we really want to know is:

  • Have LLVM tools/utils been built (LLVM_BUILD_TOOLS/UTILS) when we have a build-tree.
  • Have LLVM tools/utils been installed (LLVM_INSTALL_TOOLS/UTILS) when we have an install-tree.
  • What tree do we have? I have seen hacks to determine it, but not sure whether there's a clean way.

@xiaobai && @compnerd Do you know if find_package can tell us anything about it?

@sgraenitz - not really ... the find_package will load the LLVMConfig.cmake that is generated. We only have what we keep in there.

@sgraenitz: I don't think find_package alone will help you here. LLVMConfig doesn't necessarily tell you what's actually been built and/or installed, but it can tell you what's exported. Here's what I would do:

  1. List the tools you need for LLDB_TEST_DEPS from LLVM
  2. For each tool, check to see if it's exported from the LLVM CMake package (this is as simple as checking if it's a target)
  3. If it's not exported, proceed to the next tool
  4. If it is exported, use find_program to check if it was built/installed (LLVMConfig should tell you where it is -- LLVM_TOOLS_BINARY_DIR)
  5. If it is built, add it to LLDB_TEST_DEPS.
  6. If it's not built, don't add it to LLDB_TEST_DEPS.

I think this should work regardless of if you have a build tree or an install tree. I don't see any logic in here that is specific to one or the other, but if you absolutely do need to check, probably the most reliable way is to check for the existince of the LLVM_BUILD_* variables. I think the install tree also has a variable LLVM_INSTALL_PREFIX, so that could be another way to check.

labath added a subscriber: labath.Feb 14 2019, 11:41 AM

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build? I know that they're important for in-tree builds, as that makes sure everything is built when you run "check-lldb". However, that's isn't going to help you with standalone builds. The binaries are already there, or they aren't, and there's nothing we can do to change that. I'd consider just removing all non-lldb targets from the deps list when in standalone mode.

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build? I know that they're important for in-tree builds, as that makes sure everything is built when you run "check-lldb". However, that's isn't going to help you with standalone builds. The binaries are already there, or they aren't, and there's nothing we can do to change that. I'd consider just removing all non-lldb targets from the deps list when in standalone mode.

I think it's useful to be able to fully test lldb if you have all the available tools even if it's built standalone. This is especially relevant for swift-lldb, which more often than not is built standalone because swift is often built standalone. I'm generally not in favor of keeping things around only because somebody downstream uses it, but I can see the usefulness for people who work on upstream LLDB as well. I'm not sure if other people feel the same way though.

I think it's useful to be able to fully test lldb if you have all the available tools even if it's built standalone. This is especially relevant for swift-lldb, which more often than not is built standalone because swift is often built standalone. I'm generally not in favor of keeping things around only because somebody downstream uses it, but I can see the usefulness for people who work on upstream LLDB as well. I'm not sure if other people feel the same way though.

I certainly agree with that, but I don't see how fiddling with LLDB_TEST_DEPS is going to help testing at all. AFAICT, all it does it set's up the dependencies in the cmake graph. That is not going to help you run the tests. If the tools are there, the tests will run fine; if they aren't the tests will fail.

Now if we were talking about detecting the available tools and automatically warning about their absence and/or skipping tests, that would be a different story, but I don't see anyone proposing that...

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build?

There's a lot of ways to build LLDB (in-tree, standalone, monorepo, single-/multi-config generator) in a lot of contexts (OSs, against build/install-tree) and multiple test frameworks with different requirements (python, lit, unittests). We use a load of conditions to handle all this and they complicate the CMake configuration. Some of these build on wrong assumptions (as we see here with LLDB_BUILT_STANDALONE or earlier with LLDB_TEST_C/CXX_COMPILER) that always require long explanations. We never test everything and often use quick-fixes that have the tendency to add new quirks. Flakiness of tests add confusion on top of it.

IMHO using targets directly in both, in-tree vs. standalone builds, would be a good first step in streamlining the configuration process. We could warn/error early on missing dependencies. We could use generator expressions consistently to query paths of external binaries, Clang headers, etc.

Now if we were talking about detecting the available tools and automatically warning about their absence and/or skipping tests, that would be a different story, but I don't see anyone proposing that...

That's part of it, yes.

Ok, I see. Thanks for explaining that. I agree that it would be great if we could reduce the differences between an in-tree and a standalone build to a minimum.

Yap, will try and come up with a proposal soon.

@serge-sans-paille Sorry for diverging the discussion so much. Is it working for you now or are you still eager to get this patch in?

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora, because it wasn't working automagically otherwise. Reading the discussion, I don't think I missed some configuration stuff, or what did I miss when configuring the build of lldb in standalone mode?

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora,

Could you check what's the state of the master branch? It's possible some of the changes we've been talking about didn't make the 8.0 cut. We still have the time to cherry-pick it over, but it's going to run out soon.

sgraenitz removed a subscriber: mgorny.

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora, because it wasn't working automagically otherwise. Reading the discussion, I don't think I missed some configuration stuff, or what did I miss when configuring the build of lldb in standalone mode?

You build against an installed LLVM and you want to run LLDB tests? AFAIK you need to:

  • configure LLVM with LLVM_INSTALL_UTILS=ON
  • configure standalone LLDB with LLVM_EXTERNAL_LIT=/path/to/llvm-build-tree/bin/llvm-lit

However, llc and dsymutil are tools and they should not be affected.
If you don't want to run the test suite, pass LLDB_INCLUDE_TESTS=OFF.

Maybe @mgorny can add some info here?

sgraenitz accepted this revision.Feb 20 2019, 10:28 AM
sgraenitz added a subscriber: hans.

@serge-sans-paille After all, it appears to me that your issue is caused by something else, but if this change fixes it, I am not against taking it for now. It shouldn't do any harm. Please get in touch with @hans if this is really relevant for release_80.

However, please note that your patch base is not up-to-date with master [1] or release_80 [2] branches. The meanwhile changes shouldn't affect the dsymutil or llc targets, but maybe it's a hint towards the actual problem.
Also note that the lines you want to modify are there since May [3] and September [4] respectively, so it's been this way already in the 7.0 release.

(I am off till early March and will only check my inbox rarely. Hope everything's gonna work out well.)

[1] https://github.com/llvm-mirror/lldb/blob/master/lit/CMakeLists.txt
[2] https://github.com/llvm-mirror/lldb/blob/release_80/lit/CMakeLists.txt
[3] https://reviews.llvm.org/rL331447#change-GVDgVmL1F2CA
[4] https://reviews.llvm.org/rL342733#change-GVDgVmL1F2CA

This revision is now accepted and ready to land.Feb 20 2019, 10:28 AM

@sgraenitz : it's possible that r352382 fixes my issue. I'll double check that first!

serge-sans-paille abandoned this revision.Feb 25 2019, 5:17 AM

@sgraenitz It's fixed! I'm dropping this patch then, thanks for investigating o/