Skip to content

Conversation

@camio
Copy link

@camio camio commented Nov 22, 2022

The bazel call that utilized a local LLVM installation failed with
linker errors because the link order of LLVM libraries wasn't in
dependency order. The fix is to utilize LLD via the -fuse-ld flag. LLD,
unlike traditional Unix-like linkers, allows for reverse dependencies
(aka backrefs. See the --warn-backrefs option). Note that the -fuse-id
flag was being used for the Bazel-provided LLVM installation configuration.

Several other changes were made:

  • The build system was broken into two sections: install dependencies and
    build crubit. This was done since the former usually need be done only
    once.
  • Modified the installation instructions to install bazelisk as the
    system installation of bazel was incorrect (the package name is actually
    bazel-bootstrap) and, even if it were correct, would install a version
    that is too old.
  • Clarified the two build options and the reasons why one would prefer
    one to the other.
  • Modified the local LLVM option to build and use lld. This reduces
    variance of tooling used to build this project.
  • Added $LLVM_INSTALL_PATH/bin to the PATH so this compiler is selected
    instead of any system installations.

The `bazel` call that utilized a local LLVM installation failed with
linker errors because the link order of LLVM libraries wasn't in
dependency order. The fix is to utilize LLD via the `-fuse-ld` flag. LLD,
unlike traditional Unix-like linkers, allows for reverse dependencies
(aka backrefs. See the --warn-backrefs option). Note that the `-fuse-id`
flag was being used for the Bazel-provided LLVM installation configuration.

Several other changes were made:
* The build system was broken into two sections: install dependencies and
  build crubit. This was done since the former usually need be done only
  once.
* Modified the installation instructions to install bazelisk as the
  system installation of bazel was incorrect (the package name is actually
  bazel-bootstrap) and, even if it were correct, would install a version
  that is too old.
* Clarified the two build options and the reasons why one would prefer
  one to the other.
* Modified the local LLVM option to build and use `lld`. This reduces
  variance of tooling used to build this project.
* Added `$LLVM_INSTALL_PATH/bin` to the `PATH` so this compiler is selected
  instead of any system installations.
copybara-service bot pushed a commit that referenced this pull request Sep 20, 2024
Well, there's only one right now, but it's structured so that we can do more later.

This is the second time I'm implementing this, and the second implementation is much cleaner: to track the error messages, etc., I just use a fine grained enum. That's it! Doing something with string values is a bad idea. :(

Deployment plan for this CL (and any other CLs that gate to experimental):

- [x] wait until a version of crubit that accepts feature flags is released to the stable toolchain.
- [x] change the bzl to implicitly pass in `:experimental` to all targets -- done in 1fd4aec
- [ ] it's now safe to submit gating CLs, including this one.
- [ ] once we're done gating features, apply `:experimental` _manually_ to all targets that need it, and submit a bzl change that reverts #2. (Which targets to apply `:experimental` to, can be determined by the breakage list from the CL reverting #2.)

PiperOrigin-RevId: 676632613
Change-Id: I693be7e380bfefef3db898a9f02512c678148fd9
copybara-service bot pushed a commit that referenced this pull request Jun 26, 2025
This is a hacky work around for an unfortunately difficult problem: if they are pub(crate), then Crubit won't know, when looking at a _different_ library, that it needs to write fallback types for it. Instead it just fails. See the newly improved test coverage, ish. (That turned out messier than I hoped.)

After this CL, comprehensive fallbacks for pointers ~works, modulo much smaller gaps, presumably. I'd move my focus onto supporting fallbacks by value.

---

The reasons to make them pub(crate) are:

1. Originally, forward declarations were broken. See b/318690257.

2. visibility restriction -- until forward declarations are generally available, it would be nice to restrict their visibility, so that changing their APIs only breaks O(1) targets.

I fixed the bug in (1) exactly so that it would stop being a reason against this CL. As for #2, going for a cost/benefit calculation, we can just expose them publicly and the blast radius will still not be gigantic. We want to release them as part of `:supported` anyway, so this is just being a little bit aggressive.

The refactor necessary to make these remain `pub(crate)` would have been quite large (it requires every caller to `rs_type_kind` specifying which target they are currently in), and it would only be necessary just to support this one corner case. I think the tradeoff here is worthwhile, let's just do it.

PiperOrigin-RevId: 776235188
Change-Id: I9325bfecb5de0226c9fc5e5e52975864447a4fda
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant