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

lto cannot be used for proc-macro crate type without -Zdylib-lto #3143

Closed
marvin-hansen opened this issue Dec 30, 2024 · 22 comments · Fixed by #3147
Closed

lto cannot be used for proc-macro crate type without -Zdylib-lto #3143

marvin-hansen opened this issue Dec 30, 2024 · 22 comments · Fixed by #3147

Comments

@marvin-hansen
Copy link
Contributor

The last release, 0.56, introduced a new setting for LTO (PR #3104).

However, this conflicts with the official Bzlmod LTO example and throws an error
that embed-bitcode does contain symbols for LTO. That is consistently the case on my CI.

Okay, when I enable the new LTO setting in my .bazelrc via

build --@rules_rust//rust/settings:lto=thin

bazel build throws then an error, both locally on MacOS and on CI / Linux:

error: lto cannot be used for proc-macrocrate type without-Zdylib-lto``

Indeed, I have a bunch of proc macros in my repo.

Currently, the previous LTO config throws an error and the new LTO setting also throws an error.

How do you advice to setup LTO?
Where do I set the Zdylib-lto flag?

havasd added a commit to havasd/rules_rust that referenced this issue Dec 31, 2024
Proc-macro crates can not be linked with lto and we should not emit bitcode either.
This fixes bazelbuild#3143
@havasd
Copy link
Contributor

havasd commented Dec 31, 2024

I have fix for it in #3147

@marvin-hansen
Copy link
Contributor Author

Thank you, appreciate your effort to get this solved.

havasd added a commit to havasd/rules_rust that referenced this issue Dec 31, 2024
Proc-macro crates can not be linked with lto and we should not emit bitcode either.
This fixes bazelbuild#3143
havasd added a commit to havasd/rules_rust that referenced this issue Dec 31, 2024
Proc-macro crates can not be linked with lto and we should not emit bitcode either.
This fixes bazelbuild#3143
github-merge-queue bot pushed a commit that referenced this issue Jan 2, 2025
…#3147)

I would like to make lto setting public in toolchain so that when we are
providing custom toolchains it would be easier to make configurations
for LTO with `select` statements.

The logic which determines the flags based on cargo outputs.

Proc-macro crates can not be linked with lto and we should not emit
bitcode either. This fixes #3143

---------

Co-authored-by: Daniel Wagner-Hall <[email protected]>
@marvin-hansen
Copy link
Contributor Author

@havasd Okay, now as the fix is merged, how am I supposed to use this one?

@havasd
Copy link
Contributor

havasd commented Jan 3, 2025

you can use, git_override to pick up the latest version if you use MODULE.bazel. Ideally it should work out of the box. If you use workspace you need to pick download the main branch in http_archive with https://github.com/bazelbuild/rules_rust/archive/refs/heads/main.zip.

bazel_dep(name = "rules_rust", version = "0.56.0")
git_override(
  module_name = "rules_rust",
  commit = "568bb7b70f32d52e6626c313fc1d3dc18a0757af",
  remote = "https://github.com/bazelbuild/rules_rust.git",
)

@marvin-hansen
Copy link
Contributor Author

marvin-hansen commented Jan 3, 2025 via email

@havasd
Copy link
Contributor

havasd commented Jan 3, 2025

I see, I misunderstood your question.

With the original implementation you can do this --@rules_rust//rust/settings/lto=thin
this can be put to .bazelrc or passed to CLI. Also if you specify --@rules_rust//rust/settings/lto=manual your original settings should work without any change.

With my latest changes ideally you could make it work by creating your own toolchain and there specifying the lto flag.

load("//rust/private:lto.bzl", "rust_lto_flag")

rust_lto_flag(
    name = "lto_thin",
    build_setting_default = "thin",
    visibility = ["//visibility:public"],
)

rust_lto_flag(
    name = "lto_off",
    build_setting_default = "unspecified",
    visibility = ["//visibility:public"],
)

load("@rules_rust//rust:toolchain.bzl", "rust_toolchain")

rust_toolchain(
  ....
  lto = select({
          "//:release": ":lto_thin",
          "//conditions:default": ":lto_off",
  }),
  ...
)

Now how you prepare a custom toolchain that is a different story.

@marvin-hansen
Copy link
Contributor Author

Hey, would it possible to update the official comp_opt example with the new toolchain approach?

https://github.com/bazelbuild/rules_rust/tree/main/examples/compile_opt

I am asking b/c right now, the official LTO/compiler optimization example is pinned to v0.46 of rules_rust and I am pretty certain once the new release comes out with LTO config changed again, people will ask how to set this up just to figure out that the official code example is basically pinned to an old version of the rules.

https://github.com/bazelbuild/rules_rust/blob/main/examples/compile_opt/MODULE.bazel

github-merge-queue bot pushed a commit that referenced this issue Jan 3, 2025
Fixes comment mentioned in
[3143#issuecomment-2568947858](#3143 (comment))

---------

Co-authored-by: UebelAndre <[email protected]>
@ParkMyCar
Copy link
Contributor

Sorry @marvin-hansen for the breakage here, and thanks @havasd for jumping on a fix!

I'm quite surprised you hit this Marvin, because generally proc-macros are built in the exec configuration since they're used by the host running the compilation, and LTO is already skipped in this scenario. The example you linked doesn't have any proc-macros, any chance you could share an example of how you hit this? I want to make sure there aren't other cases I missed when implementing these LTO changes.

@havasd
Copy link
Contributor

havasd commented Jan 5, 2025

@ParkMyCar this happens when you have proc-macros in your own project. We had this issue as well.

Another issue is when you have a proc-macro and you build its dependencies with linker-plugin-lto in this case you need to have compatible linker installed on your host what rustc uses (llvm 19+ on latest rust version).

What cargo is doing in this case (it is called build dependency:

  • it creates a duplicate action for a proc-macro and its transitive dependencies
  • it builds proc-macros with non-lto and opt-level=0
  • it builds the transitive dependencies with non-lto and opt-level=0 as well

Whereas you have overlapping dependencies this means that you will compile those crates 2 times.

@ParkMyCar
Copy link
Contributor

Everything you listed totally makes sense! What I'm confused by is when building a proc-macro it should always get built in the exec configuration, and as a result its dependencies too. When something is built in the exec config we skip all of the LTO logic and only emit object code as well as skip specifying the -Clto= flag, meaning linker-plugin-lto should never get used. So it seems like LTO shouldn't be getting applied for proc-macros or its dependencies?

Also FWIW the project I'm using Bazel with also has proc-macros, and we're using clang-18 with Rust 1.83, so it's at least working in that case. Maybe I have something configured differently though!

@havasd
Copy link
Contributor

havasd commented Jan 6, 2025

I will have to check the exec configuration because I am not familiar with that part. Afaik proc_macro_deps is used only to mark proc-macros as dependencies but when you have normal dependencies you have to put them to deps section or we are using them wrongly.

Maybe 18+ is working as well. We were using ubuntu 22.04 with default clang 14 and that has problems with linking.

@marvin-hansen
Copy link
Contributor Author

marvin-hansen commented Jan 6, 2025 via email

@marvin-hansen
Copy link
Contributor Author

@havasd

I found a workaround on my CI with the old clang version by setting the LTO flag slightly differently using flto:

        rustc_flags = select({
            "//:release": [
                #  Somehow the standard Rust lto flag doesn't work anymore beginning with rules rust 0.56.0
                "-Clink-arg=-flto",
                "-Clink-arg=-s",
                "-Ccodegen-units=1",
                "-Cpanic=abort",
                "-Copt-level=3",
                "-Cstrip=symbols",
                # "-Ctarget-cpu=native", # Only use this when the build CPU is the same as the target CPU
            ],
            "//conditions:default": [
                "-Copt-level=0",
            ],
        }),

At the root level, the BUILD file then defiones the release flag:

###############################################################################
# RUST RELEASE MODE OPTIMIZATION
###############################################################################
config_setting(
    name = "release",
    values = {
        "compilation_mode": "opt",
    },
)

I am not 100% certain if that is the right path, but it works reliably on both, my Macbook and CI.

@havasd
Copy link
Contributor

havasd commented Jan 6, 2025

If you have custom toolchain you can do:

extra_rustc_flags_for_crate_types = select({
    # Enable LTO for libs and bins only, not for proc-macros:
    # proc-macros do not benefit from LTO, and cannot be dynamically linked with LTO.
    # Enable LTO in release mode only, to not make fastbuild slower.
    # TODO add lto-plugin for native libs
    # https://doc.rust-lang.org/rustc/linker-plugin-lto.html
    # https://doc.rust-lang.org/rustc/command-line-arguments.html#--crate-type-a-list-of-types-of-crates-for-the-compiler-to-emit
    ":release_mode": {
        "lib": ["-Clto=thin"],
        "rlib": ["-Clto=thin"],
        "bin": ["-Clto=thin"],
    },
    "//conditions:default": {},
})

This is what we were using instead of the linker-plugin flag. It is probably less efficient but the LTO results are the same.

@marvin-hansen
Copy link
Contributor Author

marvin-hansen commented Jan 6, 2025 via email

@havasd
Copy link
Contributor

havasd commented Jan 6, 2025

We have something like this for macos and linux (we were using this before the new LTO settings):

BUILD.bazel:

rust_toolchain(
  name = "rust_linux_toolchain_impl",
  allocator_library = "@rules_rust//ffi/cc/allocator_library:allocator_library",
  binary_ext = "",
  cargo = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:cargo",
  cargo_clippy = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:cargo_clippy_bin",
  clippy_driver = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:clippy_driver_bin",
  default_edition = "2021",
  dylib_ext = ".so",
  exec_triple = "x86_64-unknown-linux-gnu",
  global_allocator_library = "@rules_rust//ffi/cc/global_allocator_library:global_allocator_library",
  llvm_cov = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:llvm_cov_bin",
  llvm_profdata = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:llvm_profdata_bin",
  rust_doc = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:rustdoc",
  rust_std = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:rust_std-x86_64-unknown-linux-gnu",
  rustc = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:rustc",
  rustc_lib = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:rustc_lib",
  rustfmt = "@@rules_rust~~rust~rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools//:rustfmt_bin",
  staticlib_ext = ".a",
  stdlib_linkflags = [],
  target_triple = "x86_64-unknown-linux-gnu",
  extra_exec_rustc_flags = [],
  extra_rustc_flags = select({
    ":release_mode": [
      # https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units
      "-Ccodegen-units=1",
    ],
    "//conditions:default": [
      # https://doc.rust-lang.org/rustc/codegen-options/index.html#embed-bitcode
      "-Cembed-bitcode=no",
    ],
  }),
  extra_rustc_flags_for_crate_types = select({
    # Enable LTO for libs and bins only, not for proc-macros:
    # proc-macros do not benefit from LTO, and cannot be dynamically linked with LTO.
    # Enable LTO in release mode only, to not make fastbuild slower.
    # TODO add lto-plugin for native libs
    # https://doc.rust-lang.org/rustc/linker-plugin-lto.html
    # https://doc.rust-lang.org/rustc/command-line-arguments.html#--crate-type-a-list-of-types-of-crates-for-the-compiler-to-emit
    ":release_mode": {
        "lib": ["-Clto=thin"],
        "rlib": ["-Clto=thin"],
        "bin": ["-Clto=thin"],
    },
    "//conditions:default": {
        # Even in fastbuild mode, optimize proc-macros:
        # it makes the overall build faster, as they run in the compiler for each decorator.
        "proc-macro": ["-Copt-level=3"],
    },
  }),
  visibility = ["//visibility:public"],
)

toolchain(
  name = "rust_linux_toolchain",
  exec_compatible_with = [
    "@platforms//cpu:x86_64",
    "@platforms//os:linux",
  ],
  target_compatible_with = [
    "@platforms//cpu:x86_64",
    "@platforms//os:linux",
  ],
  toolchain = ":rust_linux_toolchain_impl",
  toolchain_type = "@rules_rust//rust:toolchain",
)

MODULE.bazel

rust = use_extension("@rules_rust//rust:extensions.bzl", "rust")
rust.toolchain(
...
)
use_repo(rust, "rust_toolchains")

# Do not register the default rust toolchains, use our own
#register_toolchains("@rust_toolchains//:all")
register_toolchains(
  "//:rust_linux_toolchain",
  "@rust_toolchains//:rust_darwin_aarch64__aarch64-apple-darwin__stable",
)

@ParkMyCar
Copy link
Contributor

I will have to check the exec configuration because I am not familiar with that part.

The exec config is used by Bazel to build a tool that is used for the execution of the current build. Cargo Build scripts and proc-macros get built in the exec config because they're executed on the host currently running a build step.

Maybe 18+ is working as well. We were using ubuntu 22.04 with default clang 14 and that has problems with linking.

Ahh that could totally be it! I think the current minimum version of LLVM supported by rustc is 17?

@ParkMyCar
Copy link
Contributor

Not to make things more complicated, but just as a heads up I was experimenting with cross language LTO this weekend and added an experimental feature to set linker-plugin-lto when building binaries, #3162.

@havasd
Copy link
Contributor

havasd commented Jan 6, 2025

I will have to check the exec configuration because I am not familiar with that part.

The exec config is used by Bazel to build a tool that is used for the execution of the current build. Cargo Build scripts and proc-macros get built in the exec config because they're executed on the host currently running a build step.

Thanks for the pointers. I think now I understand the problem. The linking and the compile flags works perfectly when they are built as part of a target where the proc_macro is in the proc_macro_deps.

However, if you build this target standalone like you run bazel build :example_proc_macro -c opt <lto flags> then we will build it normally. This happens if you have ... as a target as well.
Most probably the main culprit is that the rust_proc_macro is not set to "exec" configuration by default.

Same problem arises with our tests of proc-macros as well as they implicit dependencies.

@havasd
Copy link
Contributor

havasd commented Jan 6, 2025

Okay, so I think I know what is the problem:

  • when you build the proc-macro library itself there it is not dependent therefore we don't propagate the cfg = "exec", which is fine because we have a fix in place already
  • however the dependencies of the proc-macro are using this transition which doesn't do anything(?). Changing this to "exec" fixed the compilation of the proc-macro .so
  • unit tests: it is not good because it compiles the proc-macro as crate-type "bin" and we want to enable LTO on it but the dependencies doesn't have LTO
  • doc tests: and integration tests are seems to be okay

@ParkMyCar
Copy link
Contributor

Ohhhh great finds @havasd! That totally makes sense if you build a rust_proc_macro directly then it won't get built in the exec config. But also that transition you found I think is the answer. When a target is a dependency of a proc-macro the transition will set the _is_proc_macro_dep to true. In lto.bzl we can do this same check and if so, don't do any LTO.

@ParkMyCar
Copy link
Contributor

@havasd this is what I was thinking #3185. I will need to look into this more though because as-is the unit test isn't passing. Transitions are new to me though so maybe I'm missing something obvious

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 a pull request may close this issue.

3 participants