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

Compiler builtins missing f16/f128 symbols #128358

Open
tgross35 opened this issue Jul 29, 2024 · 8 comments
Open

Compiler builtins missing f16/f128 symbols #128358

tgross35 opened this issue Jul 29, 2024 · 8 comments
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@tgross35
Copy link
Contributor

Reported by @TimNN here:

I think something in this PR might be broken with regard to whether the f16/f128 builtins are being generated.

I have downloaded https://ci-artifacts.rust-lang.org/rustc-builds/80d8270d8488957f62fbf0df7a19dfe596be92ac/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz

And ran

llvm-nm ./rust-std-nightly-x86_64-unknown-linux-gnu/rust-std-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-7586cb84705120bb.rlib | grep __extend
./rust-std-nightly-x86_64-unknown-linux-gnu/rust-std-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-7586cb84705120bb.rlib:lib.rmeta: no symbols
0000000000000000 T _ZN17compiler_builtins5float6extend13__extendsfdf217hc5379a65d797e83bE
0000000000000000 W __extendsfdf2

I would have expected to see the f16 & f128 symbols there.

Running a rustc build in verbose mode, I see:

/Users/logic/Projects/rustc/llvm-head/build/bootstrap/debug/rustc /Users/logic/Projects/rustc/llvm-head/build/bootstrap/debug/rustc --crate-name compiler_builtins --edition=2021 /Users/logic/.cargo/registry/src/index.crates.io-6f17d22bba15001f/compiler_builtins-0.1.114/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=145 --crate-type lib --emit=dep-info,metadata,link --cfg 'feature="compiler-builtins"' --cfg 'feature="core"' --cfg 'feature="default"' --cfg 'feature="no-f16-f128"' --cfg 'feature="rustc-dep-of-std"' --target aarch64-apple-darwin <truncated>

Note the --cfg 'feature="no-f16-f128"' in there. This is on aarch64 which means that, AFAICT, no-f16-f128 should not trigger.

Commenting out

[target.'cfg(not(any(target_arch = "aarch64", target_arch = "x86", target_arch = "x86_64")))'.dependencies]
compiler_builtins = { version = "0.1.114", features = ["no-f16-f128"] }

Does produce a libcompiler_builtins.rlib which has the f16/f128 symbols.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 29, 2024
@tgross35 tgross35 added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 29, 2024
@tgross35
Copy link
Contributor Author

tgross35 added a commit to tgross35/rust that referenced this issue Jul 29, 2024
tgross35 added a commit to tgross35/rust that referenced this issue Jul 29, 2024
Pinning the resolver to v1 was done in 5abff37 ("Explicit set
workspace.resolver ...") in order to suppress warnings. Since there is
no specific reason not to use the new resolver and since it fixes
issues, change to `resolver = "2"`.

Fixes: rust-lang#128358
tgross35 added a commit to tgross35/rust that referenced this issue Jul 29, 2024
Pinning the resolver to v1 was done in 5abff37 ("Explicit set
workspace.resolver ...") in order to suppress warnings. Since there is
no specific reason not to use the new resolver and since it fixes
issues, change to `resolver = "2"`.

Fixes: rust-lang#128358
@TimNN
Copy link
Contributor

TimNN commented Jul 29, 2024

I ran:

<env omitted> "/Users/logic/Projects/rustc/llvm-head/build/aarch64-apple-darwin/stage0/bin/cargo" "tree" -e features "--target" "aarch64-apple-darwin" "-Zbinary-dep-depinfo" "-v" "-v" "-v" "--features" " panic-unwind backtrace" "--manifest-path" "/Users/logic/Projects/rustc/llvm-head/library/sysroot/Cargo.toml" "-p" "alloc"
alloc v0.0.0 (/Users/logic/Projects/rustc/llvm-head/library/alloc)
├── compiler_builtins feature "default"
│   ├── compiler_builtins v0.1.114
│   │   └── rustc-std-workspace-core feature "default"
│   │       └── rustc-std-workspace-core v1.99.0 (/Users/logic/Projects/rustc/llvm-head/library/rustc-std-workspace-core)
│   │           └── core feature "default"
│   │               └── core v0.0.0 (/Users/logic/Projects/rustc/llvm-head/library/core)
│   └── compiler_builtins feature "compiler-builtins"
│       └── compiler_builtins v0.1.114 (*)
├── compiler_builtins feature "rustc-dep-of-std"
│   ├── compiler_builtins v0.1.114 (*)
│   ├── compiler_builtins feature "compiler-builtins" (*)
│   └── compiler_builtins feature "core"
│       └── compiler_builtins v0.1.114 (*)
└── core feature "default" (*)
[dev-dependencies]
├── rand feature "alloc"
│   ├── rand v0.8.5
│   │   └── rand_core feature "default"
│   │       └── rand_core v0.6.4
│   └── rand_core feature "alloc"
│       └── rand_core v0.6.4
└── rand_xorshift feature "default"
    └── rand_xorshift v0.3.0
        └── rand_core feature "default" (*)

So that doesn't show the feature being activated.

I also tried to set resolver = 2, which I think correctly made the no-f16-f128 feature not trigger, but this fails to compile stage 0 compiler_builtins, because f16.is_nan() doesn't exist.

@tgross35
Copy link
Contributor Author

I also tried to set resolve = 2, which I think correctly made the no-f16-f128 feature not trigger, but this fails to compile stage 0 compiler_builtins, because f16.is_nan() doesn't exist.

Oh you're kidding, that's a third PR for me blocked on #128083 then :(. Guess if #128359 CI fails (likely based on your comment) I will just set no-f16-f128 on all platforms until the beta bump happens.

@TimNN
Copy link
Contributor

TimNN commented Jul 29, 2024

If I override the rustc that compiles stage 0 and set resolver = 2, then for x build --stage 1 library/std I get a libcompiler_builtins.rlib that contains the f16/f128 symbols.

@tgross35 tgross35 added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jul 29, 2024
@tgross35
Copy link
Contributor Author

Thanks for confirming, guess we are waiting on #128359 and then #128083.

@VorpalBlade
Copy link

Possibly related (so this is a regression that breaks code that compiles on stable): #128401

@TimNN
Copy link
Contributor

TimNN commented Jul 31, 2024

Could we, as a quick fix on Resolver 1, move the

[target.'cfg(not(any(target_arch = "aarch64", target_arch = "x86", target_arch = "x86_64")))'.dependencies]
compiler_builtins = { version = "0.1.114", features = ["no-f16-f128"] }

bit into bootstrap?

@tgross35
Copy link
Contributor Author

I was honestly thinking that maybe we should move that logic into compiler_builtins itself - always build unless the platform is known to have a LLVM crash or if no-f16-f128 is enabled. Then nothing in rust-lang/rust should have to worry about the feature flag unless it needs to be disabled.

I'll take a look at doing this in the morning.

tgross35 added a commit to tgross35/rust that referenced this issue Aug 2, 2024
Pinning the resolver to v1 was done in 5abff37 ("Explicit set
workspace.resolver ...") in order to suppress warnings. Since there is
no specific reason not to use the new resolver and since it fixes
issues, change to `resolver = "2"`.

Fixes: rust-lang#128358
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 2, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants