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

Add linkage attributes to extern "C" blocks #429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saethlin
Copy link

@saethlin saethlin commented Oct 4, 2024

This crate is now used in the compiler since rust-lang/rust#126930.
The problem I'm fixing is just like rust-lang/rust#118084; if you try to run

RUSTFLAGS=-Zcross-crate-inline-threshold=always ./x.py build library

After around 30 minutes (-Zcross-crate-inline-threshold=always makes compilation much slower) you get

  = note: rust-lld: error: undefined symbol: blake3_hash_many_avx512
          >>> referenced by ffi_avx512.rs:49 (/home/ben/.cargo/registry/src/index.crates.io-6f17d22bba15001f/blake3-1.5.2/src/ffi_avx512.rs:49)
          >>>               /home/ben/rust-master/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/rustc_main-1766fdd0f895a9f6.rustc_main.5f5b835cb13221b6-cgu.0.rcgu.o:(blake3::compress_subtree_wide::<blake3::join::SerialJoin>)
          >>> referenced by ffi_avx512.rs:49 (/home/ben/.cargo/registry/src/index.crates.io-6f17d22bba15001f/blake3-1.5.2/src/ffi_avx512.rs:49)
          >>>               /home/ben/rust-master/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/rustc_main-1766fdd0f895a9f6.rustc_main.5f5b835cb13221b6-cgu.0.rcgu.o:(blake3::compress_parents_parallel)
... any many more!

I think this is exactly the same problem as the above-linked issue: the blake3 crate does not have any link(name = attributes, so it only links correctly if all calls to the extern "C" functions are monomorphized by blake3 crate. But with -Zcross-crate-inline-threshold=always, nearly all monomorphization is done by the last crate to be compiled, and without these attributes we lose track of these symbols by the time we need them.

I'm not filing an issue because I'm pretty sure nobody else cares about my silly flag continuing to work when bootstrapping the compiler, but I occasionally find legitimate codegen bugs with it, so I'm sending you the patch to keep it working.

I've confirmed in my local dev setup that this PR makes it again possible to build the compiler with -Zcross-crate-inline-threshold=always.

@oconnor663
Copy link
Member

oconnor663 commented Oct 4, 2024

Interesting, yes, let's fix this. CI is showing some build failures in cargo test --features=prefer_intrinsics. As you'll see, we get these symbols from different places in different modes. One confusing point is that SSE and AVX2 are available in stable Rust, so we have pure Rust intrinsics implementations for those, but not for AVX-512, which still links against C code (but different C sources). Can you get that working on your box?

@saethlin
Copy link
Author

saethlin commented Oct 4, 2024

Oh I see what you mean, the avx512 situation adds another dimension to the matrix. I'll need a bit to untangle this.

@saethlin
Copy link
Author

saethlin commented Oct 4, 2024

Oh dear it's the workflow approval dance.

@saethlin
Copy link
Author

saethlin commented Oct 4, 2024

A bit of scope creep later, now I've:

  • gated all of the linkage attributes on their matching cfg that's set by the build script
  • renamed blake3_neon to blake3_neon_ffi so that it matches
  • Added two new cfgs so that the build script can indicate which library is providing avx512 implementations

@saethlin saethlin marked this pull request as ready for review October 4, 2024 23:01
@saethlin
Copy link
Author

saethlin commented Oct 4, 2024

Works on my machine too!

@saethlin
Copy link
Author

👋 Just generating a notification to make sure this PR isn't lost.

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.

2 participants