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

Compile test_num_f128 conditionally on reliable_f128_math config #132696

Merged

Conversation

raoulstrackx
Copy link
Contributor

With #132434 merged, our internal SGX CI started failing with:

05:27:34   = note: rust-lld: error: undefined symbol: fmodl
05:27:34           >>> referenced by arith.rs:617 (core/src/ops/arith.rs:617)
05:27:34           >>>               /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-5d5f11eb008c9091.std.d8141acc61ab7ac8-cgu.10.rcgu.o:(std::num::test_num::h7dd9449f6c01fde8)
05:27:34           >>> did you mean: fmodf
05:27:34           >>> defined in: /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/libcompiler_builtins-0376f439a2ebf305.rlib(compiler_builtins-0376f439a2ebf305.compiler_builtins.c22db39d25d6f802-cgu.148.rcgu.o)

This originated from the test_num_f128 test not having the required conditional compilation. This PR fixes that issue.

cc: @jethrogb, @workingjubilee

@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 6, 2024
@@ -2,7 +2,9 @@
#![cfg(reliable_f128)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work. I disregarded that as I wanted to have a minimal PR. Should I remove this, move the conditional compilation to the mod tests; statement (but that's even easier for developers to miss), or use a mod statement within this tests module with a proper single conditional compilation statement?

Copy link
Contributor

@tgross35 tgross35 Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That config is set here

rust/library/std/build.rs

Lines 120 to 143 in 4d215e2

let has_reliable_f128 = match (target_arch.as_str(), target_os.as_str()) {
// We can always enable these in Miri as that is not affected by codegen bugs.
_ if is_miri => true,
// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
("arm64ec", _) => false,
// Selection bug <https://github.com/llvm/llvm-project/issues/96432>
("mips64" | "mips64r6", _) => false,
// Selection bug <https://github.com/llvm/llvm-project/issues/95471>
("nvptx64", _) => false,
// ABI bugs <https://github.com/rust-lang/rust/issues/125109> et al. (full
// list at <https://github.com/rust-lang/rust/issues/116909>)
("powerpc" | "powerpc64", _) => false,
// ABI unsupported <https://github.com/llvm/llvm-project/issues/41838>
("sparc", _) => false,
// Stack alignment bug <https://github.com/llvm/llvm-project/issues/77401>. NB: tests may
// not fail if our compiler-builtins is linked.
("x86", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86_64", "windows") if target_env == "gnu" && target_abi != "llvm" => false,
// There are no known problems on other platforms, so the only requirement is that symbols
// are available. `compiler-builtins` provides all symbols required for core `f128`
// support, so this should work for everything else.
_ => true,
};
and at this point should really only cover platforms where LLVM crashes or ABI problems lead to buggy results. reliable_f128_math is the gate that should cover anything that relies on libm symbols. I think we just didn't catch this because fmodl is available on many platforms even when the rest of the math symbols might not be.

So moving only calls to functions that rely on f128 libm to reliable_f128_math should be the needed correction (suggested by @beetrees below), everything else should continue to work on SGX and other platforms as long as it isn't in the linked list.

@jethrogb
Copy link
Contributor

jethrogb commented Nov 6, 2024

LGTM

@rust-log-analyzer

This comment has been minimized.

@beetrees
Copy link
Contributor

beetrees commented Nov 6, 2024

cc @tgross35

@tgross35
Copy link
Contributor

tgross35 commented Nov 6, 2024

r? @tgross35

@rustbot rustbot assigned tgross35 and unassigned workingjubilee Nov 6, 2024
@tgross35 tgross35 added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Nov 6, 2024
@tgross35
Copy link
Contributor

tgross35 commented Nov 7, 2024

Could you apply beetrees suggestion and ./x fmt to fix the tidy bug? Should be good after that (and a squash)

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2024
@workingjubilee
Copy link
Member

Sorry about that folks, we've been trying to make the builtins calls less of a problem but it's slow going.

@raoulstrackx raoulstrackx force-pushed the raoul/rte-235-fix_fmodl_missing_symbol_issue branch from 62c0c99 to 66c2e38 Compare November 7, 2024 08:23
@rust-log-analyzer

This comment has been minimized.

@raoulstrackx raoulstrackx force-pushed the raoul/rte-235-fix_fmodl_missing_symbol_issue branch 2 times, most recently from 1c21304 to c614469 Compare November 7, 2024 09:38
@rust-log-analyzer

This comment has been minimized.

@raoulstrackx raoulstrackx force-pushed the raoul/rte-235-fix_fmodl_missing_symbol_issue branch from c614469 to 0720880 Compare November 7, 2024 10:33
@tgross35
Copy link
Contributor

tgross35 commented Nov 7, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 7, 2024

📌 Commit 0720880 has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2024
…issing_symbol_issue, r=tgross35

Compile `test_num_f128` conditionally on `reliable_f128_math` config

With rust-lang#132434 merged, our internal SGX CI started failing with:
```
05:27:34   = note: rust-lld: error: undefined symbol: fmodl
05:27:34           >>> referenced by arith.rs:617 (core/src/ops/arith.rs:617)
05:27:34           >>>               /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-5d5f11eb008c9091.std.d8141acc61ab7ac8-cgu.10.rcgu.o:(std::num::test_num::h7dd9449f6c01fde8)
05:27:34           >>> did you mean: fmodf
05:27:34           >>> defined in: /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/libcompiler_builtins-0376f439a2ebf305.rlib(compiler_builtins-0376f439a2ebf305.compiler_builtins.c22db39d25d6f802-cgu.148.rcgu.o)
```
This originated from the `test_num_f128` test not having the required conditional compilation. This PR fixes that issue.

cc: `@jethrogb,` `@workingjubilee`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2024
…issing_symbol_issue, r=tgross35

Compile `test_num_f128` conditionally on `reliable_f128_math` config

With rust-lang#132434 merged, our internal SGX CI started failing with:
```
05:27:34   = note: rust-lld: error: undefined symbol: fmodl
05:27:34           >>> referenced by arith.rs:617 (core/src/ops/arith.rs:617)
05:27:34           >>>               /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-5d5f11eb008c9091.std.d8141acc61ab7ac8-cgu.10.rcgu.o:(std::num::test_num::h7dd9449f6c01fde8)
05:27:34           >>> did you mean: fmodf
05:27:34           >>> defined in: /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/libcompiler_builtins-0376f439a2ebf305.rlib(compiler_builtins-0376f439a2ebf305.compiler_builtins.c22db39d25d6f802-cgu.148.rcgu.o)
```
This originated from the `test_num_f128` test not having the required conditional compilation. This PR fixes that issue.

cc: ``@jethrogb,`` ``@workingjubilee``
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 7, 2024
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 8, 2024
…issing_symbol_issue, r=tgross35

Compile `test_num_f128` conditionally on `reliable_f128_math` config

With rust-lang#132434 merged, our internal SGX CI started failing with:
```
05:27:34   = note: rust-lld: error: undefined symbol: fmodl
05:27:34           >>> referenced by arith.rs:617 (core/src/ops/arith.rs:617)
05:27:34           >>>               /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-5d5f11eb008c9091.std.d8141acc61ab7ac8-cgu.10.rcgu.o:(std::num::test_num::h7dd9449f6c01fde8)
05:27:34           >>> did you mean: fmodf
05:27:34           >>> defined in: /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/libcompiler_builtins-0376f439a2ebf305.rlib(compiler_builtins-0376f439a2ebf305.compiler_builtins.c22db39d25d6f802-cgu.148.rcgu.o)
```
This originated from the `test_num_f128` test not having the required conditional compilation. This PR fixes that issue.

cc: ```@jethrogb,``` ```@workingjubilee```
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132639 (core: move intrinsics.rs into intrinsics folder)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132737 (bootstrap: Print better message if lock pid isn't available)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)
 - rust-lang#132741 (Update mips64 data layout to match LLVM 20 change)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c9009ae into rust-lang:master Nov 8, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
Rollup merge of rust-lang#132696 - fortanix:raoul/rte-235-fix_fmodl_missing_symbol_issue, r=tgross35

Compile `test_num_f128` conditionally on `reliable_f128_math` config

With rust-lang#132434 merged, our internal SGX CI started failing with:
```
05:27:34   = note: rust-lld: error: undefined symbol: fmodl
05:27:34           >>> referenced by arith.rs:617 (core/src/ops/arith.rs:617)
05:27:34           >>>               /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-5d5f11eb008c9091.std.d8141acc61ab7ac8-cgu.10.rcgu.o:(std::num::test_num::h7dd9449f6c01fde8)
05:27:34           >>> did you mean: fmodf
05:27:34           >>> defined in: /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/libcompiler_builtins-0376f439a2ebf305.rlib(compiler_builtins-0376f439a2ebf305.compiler_builtins.c22db39d25d6f802-cgu.148.rcgu.o)
```
This originated from the `test_num_f128` test not having the required conditional compilation. This PR fixes that issue.

cc: ````@jethrogb,```` ````@workingjubilee````
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…issing_symbol_issue, r=tgross35

Compile `test_num_f128` conditionally on `reliable_f128_math` config

With rust-lang#132434 merged, our internal SGX CI started failing with:
```
05:27:34   = note: rust-lld: error: undefined symbol: fmodl
05:27:34           >>> referenced by arith.rs:617 (core/src/ops/arith.rs:617)
05:27:34           >>>               /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-5d5f11eb008c9091.std.d8141acc61ab7ac8-cgu.10.rcgu.o:(std::num::test_num::h7dd9449f6c01fde8)
05:27:34           >>> did you mean: fmodf
05:27:34           >>> defined in: /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/libcompiler_builtins-0376f439a2ebf305.rlib(compiler_builtins-0376f439a2ebf305.compiler_builtins.c22db39d25d6f802-cgu.148.rcgu.o)
```
This originated from the `test_num_f128` test not having the required conditional compilation. This PR fixes that issue.

cc: ````@jethrogb,```` ````@workingjubilee````
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132639 (core: move intrinsics.rs into intrinsics folder)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132737 (bootstrap: Print better message if lock pid isn't available)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)
 - rust-lang#132741 (Update mips64 data layout to match LLVM 20 change)

r? `@ghost`
`@rustbot` modify labels: rollup
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-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants