Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bazel] Add support for building lldb #87589

Merged
merged 8 commits into from
Apr 4, 2024

Conversation

keith
Copy link
Member

@keith keith commented Apr 4, 2024

This adds build configuration for building LLDB on macOS and Linux. It uses a default subset of features that should work out of the box with macOS + Ubuntu. It is notably missing python support right now, although some of the scaffolding is there, because of the complexity of linking a python dylib, especially if you plan to distribute the resulting liblldb.so.

Most of this build file is pretty simple, one of the unfortunate patterns I had to use was to split the header and sources cc_library targets to break circular dependencies.

This adds build configuration for building LLDB on macOS and Linux. It
uses a default subset of features that should work out of the box with
macOS + Ubuntu. It is notably missing python support right now, although
some of the scaffolding is there, because of the complexity of linking a
python dylib, especially if you plan to distribute the resulting
liblldb.so.

Most of this build file is pretty simple, one of the unfortunate
patterns I had to use was to split the header and sources cc_library
targets to break circular dependencies.
@keith keith requested a review from aaronmondal April 4, 2024 03:04
@keith keith marked this pull request as ready for review April 4, 2024 03:04
@keith keith requested a review from rupprecht as a code owner April 4, 2024 03:04
@llvmbot llvmbot added the bazel "Peripheral" support tier build system: utils/bazel label Apr 4, 2024
Copy link
Contributor

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow this looks pretty good!

The only concern I have is the -l* in the linkopts. I remember going to fairly great lengths to remove those for other third-party deps like zlib because they made the build susceptible to cache poisoning and had reproducibility issues. The deps used there are fairly complex, so I'm not sure how viable it is to add all of them as third-party deps. However, the bazel central registry seems to have at least some of them. Maybe we could borrow some build files from there.

utils/bazel/llvm-project-overlay/lldb/BUILD.bazel Outdated Show resolved Hide resolved
utils/bazel/llvm-project-overlay/lldb/BUILD.bazel Outdated Show resolved Hide resolved
@keith keith requested a review from aaronmondal April 4, 2024 16:05
@keith keith requested a review from aaronmondal April 4, 2024 17:19
@keith keith merged commit 4b077ed into llvm:main Apr 4, 2024
4 checks passed
@keith keith deleted the ks/bazel-add-support-for-building-lldb branch April 4, 2024 22:10
@keith
Copy link
Member Author

keith commented Apr 4, 2024

thanks for the review!

@chsigg
Copy link
Contributor

chsigg commented Apr 5, 2024

Hi Keith, the bazel buildkite is not happy with this change.

ERROR: Target @llvm-project//lldb:DebugServerCommon is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    @llvm-project//lldb:DebugServerCommon (4f455a)
    @llvm-project//lldb:DebugServerMacOSX (189b94)   <-- target platform (@local_config_platform//:host) didn't satisfy constraint @platforms//:incompatible

Is this something you can fix forward?

@hokein
Copy link
Collaborator

hokein commented Apr 5, 2024

IIUC, the debugserver binary and its dependencies (e.g. DebugServerCommon) are only available on MacOS. If we build these targets explicitly with bazel on Linux, bazel will give the incompatible and cannot be built, but was explicitly requested. error message.

The bazel buildkite setup is like:

  1. run bazelisk query //... + @llvm-project//... command to list all available targets;
  2. run bazelisk test on each target which is list in step1;

Step1 lists some incompatible targets like DebugServerCommon which causes the issue. I guess the fix is to not emit these incompatible targets in step1 for non-MacOS platforms.

@keith
Copy link
Member Author

keith commented Apr 5, 2024

Would it be safe to change that CI configuration to only do a bazel test @llvm-project//...? Otherwise I think we'd need to manually filter out incompatible targets, since query //... doesn't do that for us

@chsigg
Copy link
Contributor

chsigg commented Apr 8, 2024

I don't know the reason behind bazel query + bazel test. There probably was one at some point, but I can't find where it came from. It always seemed a bit surprising, exactly because it will trip up on incompatible targets and also ignore things like manual tags.

@rupprecht
Copy link
Collaborator

I landed bee3377 to make buildkite green.

As for why we do query+test instead of just test: I think this comes from https://github.com/openxla/iree/blob/bb7e536ecd31ea24007aa7202f3a0c41b897e05c/build_tools/bazel/build_test_all.sh

# We use `bazel query //...` piped to `bazel test` rather than the simpler
# `bazel test //...` because the latter excludes targets tagged "manual". The
# "manual" tag allows targets to be excluded from human wildcard builds, but we
# want them built by CI unless they are excluded with tags.

While that may be true for iree, I don't think it's (currently) true for LLVM. i.e., I don't know if we have any tags = ["manual"] stuff that we explicitly want to build in CI.


Separate from the buildkite issue: we have some internal rules for building LLDB, and they look a bit different than this, but that's partly because it requires a bunch of other internal stuff. I'll see if there's anything we have that we can contribute.

@keith
Copy link
Member Author

keith commented Apr 8, 2024

If the goal is to include manual targets, we can instead do:

bazel query "attr(tags, 'manual', //...)"

and test those with test //... + those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants