[PATCH] D113565: Split headers from implementations in MLIR C API Bazel build.

Wed Nov 10 18:12:21 PST 2021

mehdi_amini added inline comments.

================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:774
+    ],
+    alwayslink = True,
 )
----------------
GMNGeoffrey wrote:
> mehdi_amini wrote:
> > phawkins wrote:
> > > mehdi_amini wrote:
> > > > GMNGeoffrey wrote:
> > > > > phawkins wrote:
> > > > > > phawkins wrote:
> > > > > > > GMNGeoffrey wrote:
> > > > > > > > phawkins wrote:
> > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > GMNGeoffrey wrote:
> > > > > > > > > > > 😢 alwayslink is a scourge. Is it really the only way?
> > > > > > > > > > Same concern here: why is this needed?
> > > > > > > > > I removed it from this target, where I don't think it's needed. In general it *is* needed on all the libraries that implement the exported C API, because otherwise Bazel will strip the C API out.
> > > > > > > > Why wasn't it needed before? We're using this internally without issue IIUC. We've worked hard in many places to scour this from the codebase, so I don't love seeing it return
> > > > > > > > 
> > > > > > > > @stellaraccident can you weigh in on these build rules?
> > > > > > > It wasn't needed before, because internally we build monolithic binaries.
> > > > > > > 
> > > > > > > Here I'm doing something different: I'm building one shared library that vends the C API (capi.so) and another shared library (a Python extension) that consumes it. Without `alwayslink`, the C API `.so` file will not contain the C API: it ends up completely empty because there are no *users* of the API present and everything ends up stripped out. I did not put this here idly.
> > > > > > > 
> > > > > > > I note this is exactly how IREE deploys things, although IREE does it with CMake not Bazel.
> > > > > > > 
> > > > > > To add to that, from the Bazel docs, the definition of `alwayslink` is:
> > > > > > "If 1, any binary that depends (directly or indirectly) on this C++ precompiled library will link in all the object files archived in the static library, even if some contain no symbols referenced by the binary. This is useful if your code isn't explicitly called by code in the binary, e.g., if your code registers to receive some callback provided by some service."
> > > > > > 
> > > > > > That's precisely what we want here, for use in a rule like this:
> > > > > > ```
> > > > > > cc_binary(
> > > > > >     name = "mlir_capi.so",
> > > > > >     deps = [
> > > > > >         "@llvm-project//mlir:MLIRBindingsPythonCAPI",
> > > > > >     ],
> > > > > >     linkshared = 1,
> > > > > >     linkopts = ["-Wl,-soname=mlir_capi.so", "-Wl,-rpath='$$ORIGIN'"],
> > > > > > )
> > > > > > ```
> > > > > > 
> > > > > > The only alternative I can think of would be to have two copies of the implementation rules, one with `alwayslink` and one without, or a macro that does that.
> > > > > We had a higher-bandwidth discussion in a side channel and agreed that it was best not to expose alwayslink to unsuspecting users. We'll use a macro to create separate targets for the normal library, the headers-only version, and the version with alwayslink. This limits the weirdness to users doing things that are not very well supported in Bazel (but hopefully will be with `cc_shared_library`)
> > > > > That's precisely what we want here, for use in a rule like this:
> > > > 
> > > > 
> > > > I'm not sure I understand this.
> > > > Your quote is about "any **binary** that depends ...", it isn't clear to me that this applies to shared libraries.
> > > > 
> > > > For example if I test with this simple BUILD file:
> > > > ```
> > > > cc_binary(
> > > >     name = "test.so",
> > > >     deps = [
> > > >         ":test_lib",
> > > >     ],
> > > >     linkshared = 1,
> > > >     linkopts = ["-Wl,-soname=mlir_capi.so", "-Wl,-rpath='$$ORIGIN'"],
> > > > )
> > > > 
> > > > cc_library(
> > > >   name = "test_lib",
> > > >   srcs = [
> > > >     "test.cc"
> > > >   ]
> > > > )
> > > > ```
> > > > 
> > > > The `test.cc` file will be linked into the .so ; the linker invocation will be `-shared -o bazel-out/k8-fastbuild/bin/libtest_lib.so -Wl,-S -fuse-ld=gold -Wl,-no-as-needed -Wl,-z,relro,-z,now -B/usr/bin -pass-exit-codes bazel-out/k8-fastbuild/bin/_objs/test_lib/test.pic.o -lstdc++ -lm`
> > > > And `nm` on the final .so shows the symbol from test.cc present.
> > > > 
> > > > So I still don't know what is the situation you're having that is different here?
> > > > 
> > > This does not match the behavior I am seeing. I ran this:
> > > 
> > > ```
> > > mkdir myproject
> > > cd myproject
> > > touch WORKSPACE
> > > cat > BUILD.bazel <<EOF
> > > cc_binary(
> > >     name = "test.so",
> > >     deps = [
> > >         ":test_lib",
> > >     ],
> > >     linkshared = 1,
> > >     linkopts = ["-Wl,-soname=mlir_capi.so", "-Wl,-rpath='$$ORIGIN'"],
> > > )
> > > 
> > > cc_library(
> > >   name = "test_lib",
> > >   srcs = [
> > >     "test.cc"
> > >   ],
> > >   alwayslink=True,
> > > )
> > > EOF
> > > 
> > > cat > test.cc <<EOF
> > > int veryobvioussymbol(int x) { return x * 2; }
> > > EOF
> > > 
> > > bazel-4.1.0-linux-x86_64  build :test.so
> > > nm bazel-bin/test.so | grep veryobvious
> > > ```
> > > 
> > > I see a non-empty grep output if and only if `alwayslink=True` is set on the `cc_library`.
> > Actually I looked that the linker invocation of the `libtest_lib.so` instead of `libtest.so`... So scratch that Bazel is not pulling in the static archive in linking the shared library here...
> Ok so Mehdi you agree that alwayslink is necessary?
Yeah I agree (I guess until cc_shared_library?)

Seems like this changed in Bazel at some point: https://docs.bazel.build/versions/main/command-line-reference.html#flag--legacy_whole_archive

I really don't know why they thought it'd be a good design :(

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113565/new/

https://reviews.llvm.org/D113565