This is an archive of the discontinued LLVM Phabricator instance.

Differential D107414

[Bazel] Add support for lld
ClosedPublic

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

Details

Reviewers
chandlerc
MaskRay
Commits
rG693a95a69416: [Bazel] Add support for lld
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
Herald added subscribers: mstorsjo, dschuff. · View Herald TranscriptAug 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
Herald added subscribers: llvm-commits, aheejin. · View Herald Transcript
Harbormaster completed remote builds in B117775: Diff 363912.Aug 3 2021, 5:26 PM
MaskRay added inline comments.Aug 3 2021, 6:46 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
17

Use single quotes for echo arguments to avoid some escapes.

23

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

71

Is *.h needed in srcs?

148

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

240

Just MachO

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

305

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

GMNGeoffrey updated this revision to Diff 364246.Aug 4 2021, 2:19 PM
GMNGeoffrey marked 2 inline comments as done.
  • Switch to single quotes to reduce escaping
  • Move MachO2 hdrs to srcs
Harbormaster completed remote builds in B118002: Diff 364246.Aug 4 2021, 2:22 PM
GMNGeoffrey updated this revision to Diff 364286.Aug 4 2021, 4:30 PM
  • 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
71

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.

148

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

240

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.

305

Filed https://github.com/bazelbuild/bazel/issues/13803 and linked to it instead

Harbormaster completed remote builds in B118035: Diff 364286.Aug 4 2021, 4:32 PM
GMNGeoffrey added inline comments.Aug 4 2021, 5:37 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
240

https://reviews.llvm.org/D107516 touche :-P

MaskRay added inline comments.Aug 4 2021, 5:39 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
148

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.

GMNGeoffrey updated this revision to Diff 364303.Aug 4 2021, 5:44 PM
  • Rename MachO->MachoOld, Macho2->MachO
Harbormaster completed remote builds in B118045: Diff 364303.Aug 4 2021, 5:45 PM
GMNGeoffrey added inline comments.Aug 4 2021, 5:50 PM
utils/bazel/llvm-project-overlay/lld/BUILD.bazel
148

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
148

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

GMNGeoffrey updated this revision to Diff 364305.Aug 4 2021, 5:58 PM
  • 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

Harbormaster completed remote builds in B118048: Diff 364305.Aug 4 2021, 5:59 PM
Closed by commit rG693a95a69416: [Bazel] Add support for lld (authored by GMNGeoffrey). · Explain WhyAug 4 2021, 6:00 PM
This revision was automatically updated to reflect the committed changes.
GMNGeoffrey added a commit: rG693a95a69416: [Bazel] Add support for lld.

Revision Contents

PathSize
utils/
bazel/
llvm-project-overlay/
libunwind/
20 lines
lld/
383 lines

Diff 364306

utils/bazel/llvm-project-overlay/libunwind/BUILD.bazel

  • This file was added.
# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
# See https://llvm.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
package(
default_visibility = ["//visibility:public"],
licenses = ["notice"],
)
# The ld64 linker and lld-macho use the libunwind headers only for the constant
# definitions, in order to parse and convert DWARF to the compact encoding.
cc_library(
name = "unwind_headers_only",
hdrs = [
"include/__libunwind_config.h",
"include/libunwind.h",
"include/mach-o/compact_unwind_encoding.h",
],
strip_include_prefix = "include",
)

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

  • This file was added.
# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
# See https://llvm.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
load("//llvm:tblgen.bzl", "gentbl")
package(
default_visibility = ["//visibility:public"],
licenses = ["notice"],
)
# TODO: Actually compute version info
genrule(
name = "config_version_gen",
outs = ["include/lld/Common/Version.inc"],
cmd = "echo '#define LLD_VERSION_STRING \"git\"' > $@",
)
MaskRayUnsubmitted
Done
ReplyInline Actions

Use single quotes for echo arguments to avoid some escapes.

MaskRay: Use single quotes for echo arguments to avoid some escapes.
genrule(
name = "vcs_version_gen",
outs = ["Common/VCSVersion.inc"],
cmd = "echo '#undef LLD_REVISION' >> $@\n" +
"echo '#undef LLD_REPOSITORY' >> $@\n",
MaskRayUnsubmitted
Done
ReplyInline Actions

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

MaskRay: If LLD_REPOSITORY is undefined, `LLD_REVISION` doesn't need to be defined.
)
# See https://github.com/bazelbuild/bazel/issues/13803
cc_library(
name = "vcs_version",
hdrs = ["Common/VCSVersion.inc"],
strip_include_prefix = "Common",
)
cc_library(
name = "Common",
srcs = [":config_version_gen"] + glob(["Common/*.cpp"]),
hdrs = glob(["include/lld/Common/*.h"]),
includes = ["include"],
deps = [
":vcs_version",
"//llvm:CodeGen",
"//llvm:Core",
"//llvm:Option",
"//llvm:Support",
"//llvm:Target",
],
)
gentbl(
name = "elf_options_inc_gen",
# See https://github.com/bazelbuild/bazel/issues/13803
strip_include_prefix = "ELF",
tbl_outs = [(
"-gen-opt-parser-defs",
"ELF/Options.inc",
)],
tblgen = "//llvm:llvm-tblgen",
td_file = "ELF/Options.td",
td_srcs = [
"//llvm:include/llvm/Option/OptParser.td",
],
)
cc_library(
name = "ELF",
srcs = glob([
"ELF/*.cpp",
"ELF/*.h",
"ELF/Arch/*.cpp",
]),
includes = ["ELF"],
textual_hdrs = [
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Is *.h needed in srcs?

MaskRay: Is `*.h` needed in `srcs`?
GMNGeoffreyAuthorUnsubmitted
Done
ReplyInline Actions

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.

GMNGeoffrey: hdrs is only for headers that are exported for dependent libraries. Other headers should be…
"ELF/Arch/PPCInsns.def",
],
deps = [
":Common",
":elf_options_inc_gen",
"//llvm:AllTargetsAsmParsers",
"//llvm:AllTargetsCodeGens",
"//llvm:AllTargetsDisassemblers",
"//llvm:Analysis",
"//llvm:BinaryFormat",
"//llvm:BitReader",
"//llvm:BitWriter",
"//llvm:CodeGen",
"//llvm:Core",
"//llvm:DebugInfoDWARF",
"//llvm:Demangle",
"//llvm:IPO",
"//llvm:LTO",
"//llvm:Linker",
"//llvm:MC",
"//llvm:Object",
"//llvm:Option",
"//llvm:Passes",
"//llvm:Support",
"//llvm:TransformUtils",
],
)
gentbl(
name = "coff_options_inc_gen",
# See https://github.com/bazelbuild/bazel/issues/13803
strip_include_prefix = "COFF",
tbl_outs = [(
"-gen-opt-parser-defs",
"COFF/Options.inc",
)],
tblgen = "//llvm:llvm-tblgen",
td_file = "COFF/Options.td",
td_srcs = [
"//llvm:include/llvm/Option/OptParser.td",
],
)
cc_library(
name = "COFF",
srcs = glob([
"COFF/*.cpp",
"COFF/*.h",
]),
includes = ["COFF"],
deps = [
":Common",
":coff_options_inc_gen",
"//llvm:AllTargetsAsmParsers",
"//llvm:AllTargetsCodeGens",
"//llvm:AllTargetsDisassemblers",
"//llvm:Analysis",
"//llvm:BitReader",
"//llvm:BitWriter",
"//llvm:Core",
"//llvm:DebugInfoPDB",
"//llvm:IPO",
"//llvm:LTO",
"//llvm:LibDriver",
"//llvm:Linker",
"//llvm:MC",
"//llvm:Object",
"//llvm:Option",
"//llvm:Support",
"//llvm:Symbolize",
"//llvm:TransformUtils",
"//llvm:WindowsManifest",
],
)
gentbl(
name = "mingw_options_inc_gen",
MaskRayUnsubmitted
Not Done
ReplyInline Actions

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

MaskRay: In CMake, with `LLVM_ENABLE_LIBXML2` this dependency is essentially a no-op. Can Bazel allow…
GMNGeoffreyAuthorUnsubmitted
Not Done
ReplyInline Actions

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

GMNGeoffrey: I don't totally understand the question. What do you mean by optional builds? There's…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

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.

MaskRay: You said "Since the lld target itself requires libxml2 (through WindowsManifest) it's currently…
GMNGeoffreyAuthorUnsubmitted
Done
ReplyInline Actions

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

GMNGeoffrey: Hmm @chandlerc added `-lxml2` to WindowsManifest linkopts unconditionally, so we have…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

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

MaskRay: This manual/nobuildkite is existing, I think having the tags here is fine and can be fixed…
tbl_outs = [(
"-gen-opt-parser-defs",
"MinGW/Options.inc",
)],
tblgen = "//llvm:llvm-tblgen",
td_file = "MinGW/Options.td",
td_srcs = [
"//llvm:include/llvm/Option/OptParser.td",
],
)
cc_library(
name = "MinGW",
srcs = glob([
"MinGW/*.cpp",
"MinGW/*.h",
]),
includes = ["MinGW"],
deps = [
":Common",
":mingw_options_inc_gen",
"//llvm:Option",
"//llvm:Support",
],
)
cc_library(
name = "Core",
srcs = glob([
"lib/Core/*.cpp",
"lib/Core/*.h",
]),
hdrs = glob(["include/lld/Core/*.h"]),
deps = [
":Common",
],
)
cc_library(
name = "YAML",
srcs = glob([
"lib/ReaderWriter/YAML/*.cpp",
"lib/ReaderWriter/YAML/*.h",
]),
hdrs = ["include/lld/ReaderWriter/YamlContext.h"],
deps = [
":Common",
":Core",
"//llvm:Support",
],
)
cc_library(
name = "MachOOld",
srcs = glob([
"lib/ReaderWriter/MachO/*.cpp",
"lib/ReaderWriter/MachO/*.h",
]),
hdrs = ["include/lld/ReaderWriter/MachOLinkingContext.h"],
deps = [
":Common",
":Core",
":YAML",
"//llvm:DebugInfoDWARF",
"//llvm:Object",
"//llvm:Support",
],
)
gentbl(
name = "macho_options_inc_gen",
# See https://github.com/bazelbuild/bazel/issues/13803
strip_include_prefix = "MachO",
tbl_outs = [(
"-gen-opt-parser-defs",
"MachO/Options.inc",
)],
tblgen = "//llvm:llvm-tblgen",
td_file = "MachO/Options.td",
td_srcs = [
"//llvm:include/llvm/Option/OptParser.td",
],
)
cc_library(
name = "MachO",
srcs = glob([
"MachO/**/*.cpp",
"MachO/**/*.h",
]),
includes = ["MachO"],
deps = [
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Just MachO

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

MaskRay: Just `MachO` The special name can go to the old port which will go away soon.
GMNGeoffreyAuthorUnsubmitted
Done
ReplyInline Actions

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.

GMNGeoffrey: This matches the name in the CMake file (as do all the library names). I'd prefer not to…
GMNGeoffreyAuthorUnsubmitted
Done
ReplyInline Actions

https://reviews.llvm.org/D107516 touche :-P

GMNGeoffrey: https://reviews.llvm.org/D107516 touche :-P
":Common",
":macho_options_inc_gen",
"//libunwind:unwind_headers_only",
"//llvm:LTO",
],
)
cc_library(
name = "ReaderWriter",
srcs = glob([
"lib/ReaderWriter/*.cpp",
"lib/ReaderWriter/*.h",
]),
deps = [
":Common",
":Core",
":YAML",
"//llvm:Object",
"//llvm:Support",
],
)
gentbl(
name = "darwin_driver_options_inc_gen",
# See https://github.com/bazelbuild/bazel/issues/13803
strip_include_prefix = "lib/Driver",
tbl_outs = [(
"-gen-opt-parser-defs",
"lib/Driver/DarwinLdOptions.inc",
)],
tblgen = "//llvm:llvm-tblgen",
td_file = "lib/Driver/DarwinLdOptions.td",
td_srcs = [
"//llvm:include/llvm/Option/OptParser.td",
],
)
cc_library(
name = "Driver",
srcs = glob([
"lib/Driver/*.cpp",
"lib/Driver/*.h",
]),
includes = ["lib/Driver"],
deps = [
":Common",
":Core",
":MachOOld",
":ReaderWriter",
":YAML",
":darwin_driver_options_inc_gen",
"//llvm:Object",
"//llvm:Option",
"//llvm:Support",
],
)
gentbl(
name = "wasm_options_inc_gen",
# See https://github.com/bazelbuild/bazel/issues/13803
strip_include_prefix = "wasm",
tbl_outs = [(
"-gen-opt-parser-defs",
"wasm/Options.inc",
)],
MaskRayUnsubmitted
Not Done
ReplyInline Actions

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

MaskRay: This comment is repeated multiple times. Is there a Bazel bug or anything we can fix in this…
GMNGeoffreyAuthorUnsubmitted
Done
ReplyInline Actions

Filed https://github.com/bazelbuild/bazel/issues/13803 and linked to it instead

GMNGeoffrey: Filed https://github.com/bazelbuild/bazel/issues/13803 and linked to it instead
tblgen = "//llvm:llvm-tblgen",
td_file = "wasm/Options.td",
td_srcs = [
"//llvm:include/llvm/Option/OptParser.td",
],
)
cc_library(
name = "Wasm",
srcs = glob([
"wasm/*.cpp",
"wasm/*.h",
]),
includes = ["wasm"],
deps = [
":Common",
":wasm_options_inc_gen",
"//llvm:AllTargetsAsmParsers",
"//llvm:AllTargetsCodeGens",
"//llvm:BinaryFormat",
"//llvm:Core",
"//llvm:LTO",
"//llvm:Object",
"//llvm:Option",
"//llvm:Support",
],
)
cc_binary(
name = "lld",
srcs = glob([
"tools/lld/*.cpp",
"tools/lld/*.h",
]),
tags = [
"manual", # External dependency (libxml through WindowsManifest)
"nobuildkite", # TODO(gcmn): Remote executors don't have libxml
],
deps = [
":COFF",
":Common",
":Core",
":Driver",
":ELF",
":MachOOld",
":MachO",
":MinGW",
":ReaderWriter",
":Wasm",
":YAML",
"//llvm:Support",
],
)
# These are the required names for lld running under different environs.
#
# Unix/Linux require that the binary be named "ld.lld".
# macOS require that the binary be named "ld64.lld".
# Windows require that the binary be named "lld-link".
MaskRayUnsubmitted
Not Done
ReplyInline Actions

I think the canonical spelling is macOS

MaskRay: I think the canonical spelling is macOS
GMNGeoffreyAuthorUnsubmitted
Done
ReplyInline Actions

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

GMNGeoffrey: Fine I'll use that as long as we can agree that that's silly
# WebAssembly builds require that the binary be named "wasm-ld".
genrule(
name = "gen_lld",
srcs = [":lld"],
outs = [
"ld.lld",
"ld64.lld.darwinnew",
"ld64.lld",
"lld-link",
"wasm-ld",
],
cmd =
"target=$$(basename $<); for n in ld.lld ld64.lld.darwinnew ld64.lld lld-link wasm-ld; do ln -sf $$target $(@D)/$$n; done",
output_to_bindir = 1,
tags = [
"manual", # External dependency (libxml through WindowsManifest)
"nobuildkite", # TODO(gcmn): Remote executors don't have libxml
],
)