This is an archive of the discontinued LLVM Phabricator instance.

[Bazel] Add support for lld
ClosedPublic

Authored by GMNGeoffrey on Aug 3 2021, 5:22 PM.

Details

Summary

This patch adds a Bazel configuration to build lld. That includes a
BUILD.bazel file to export the libunwind headers for use by lld. Since
the lld target itself requires libxml2 (through WindowsManifest) it's
currently disabled on Buildkite and marked manual, but all the libraries
build.

Diff Detail

Event Timeline

GMNGeoffrey created this revision.Aug 3 2021, 5:22 PM
GMNGeoffrey requested review of this revision.Aug 3 2021, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2021, 5:22 PM
MaskRay added inline comments.Aug 3 2021, 6:46 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
16

Use single quotes for echo arguments to avoid some escapes.

22

If LLD_REPOSITORY is undefined, LLD_REVISION doesn't need to be defined.

70

Is *.h needed in srcs?

147

In CMake, with LLVM_ENABLE_LIBXML2 this dependency is essentially a no-op. Can Bazel allow optional builds?

239

Just MachO

The special name can go to the old port which will go away soon.

304

This comment is repeated multiple times. Is there a Bazel bug or anything we can fix in this file?

GMNGeoffrey marked 2 inline comments as done.
  • Switch to single quotes to reduce escaping
  • Move MachO2 hdrs to srcs
  • Replace big comment with pointer to Bazel issue
GMNGeoffrey added inline comments.Aug 4 2021, 4:31 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
70

hdrs is only for headers that are exported for dependent libraries. Other headers should be declared in srcs:

Headers not meant to be included by a client of this library should be listed in the srcs attribute instead, even if they are included by a published header.

https://docs.bazel.build/versions/main/be/c-cpp.html#cc_library.hdrs

Actually I noticed that the MachO2 headers should move to srcs as well.

147

I don't totally understand the question. What do you mean by optional builds? There's configurability based on select

239

This matches the name in the CMake file (as do all the library names). I'd prefer not to diverge. Having these match makes it much easier to compare the Bazel and CMake rules. I'm happy to update this once the CMake name changes.

304
GMNGeoffrey added inline comments.Aug 4 2021, 5:37 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
239
MaskRay added inline comments.Aug 4 2021, 5:39 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
147

You said "Since the lld target itself requires libxml2 (through WindowsManifest) it's currently disabled on Buildkite and marked manual, but all the libraries build."

I think it'd be good to drop tags = ["manual"]. In CMake, WindowsManifest is built regardless of availability of libxml2.
When libxml2 is disabled, WindowsManifest is a stub.

  • Rename MachO->MachoOld, Macho2->MachO
GMNGeoffrey added inline comments.Aug 4 2021, 5:50 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
147

Hmm @chandlerc added -lxml2 to WindowsManifest linkopts unconditionally, so we have everything that depends on it marked "manual" and "nobuildkite" (because our RBE runners don't have libxml2). I would be totally fine with removing that, but I think Chandler put it there for a reason (there's a comment about mt.exe not being cross-platform). We do have LLVM_ENABLE_LIBXML2 hardcoded to false though (for now, we need to enable more configurability), so honestly I'm a bit confused

https://github.com/llvm/llvm-project/blob/597e407cf23b92/utils/bazel/llvm-project-overlay/llvm/BUILD.bazel#L2385
https://github.com/llvm/llvm-project/blob/597e407cf23b92/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h#L325

MaskRay accepted this revision.Aug 4 2021, 5:54 PM

Thanks!

utils/bazel/llvm-project-overlay/lld/BUILD.bazel
147

This manual/nobuildkite is existing, I think having the tags here is fine and can be fixed along with the other manual/nobuildkite targets.

This revision is now accepted and ready to land.Aug 4 2021, 5:54 PM
MaskRay added inline comments.Aug 4 2021, 5:56 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
364

I think the canonical spelling is macOS

  • Use silly macOS spelling and remove some unnecessary subjunctives
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
364

Fine I'll use that as long as we can agree that that's silly

This revision was automatically updated to reflect the committed changes.