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

Improve default target options for x86_64-unknown-linux-none #134765

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Noratrieb
Copy link
Member

Without a standard library, we cannot unwind, so it should be panic=abort by default.

Additionally, it does not have std because while it is Linux, it cannot use libc, which std uses today for Linux.

Thirdly, relocation-model=pie is a very funny default because it forces users to write their own dynamic loader, which is fun, but probably not the most appropriate default for this target. If someone wants to write a dynamic loader, they should, but they should explicitly ask for PIE.

fixes #134763

cc @morr0ne, as you added the target, and @Noratrieb, who reviewed that PR (:D).

Without a standard library, we cannot unwind, so it should be
panic=abort by default.

Additionally, it does not have std because while it is
Linux, it cannot use libc, which std uses today for Linux.
Writing our own dynamic loader is fun, but shouldn't be the default
expectation.
@rustbot
Copy link
Collaborator

rustbot commented Dec 25, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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
Copy link
Collaborator

rustbot commented Dec 25, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 25, 2024
@Noratrieb Noratrieb changed the title Linux none cant unwind silly Improve default target options for x86_64-unknown-linux-none Dec 25, 2024
jieyouxu

This comment was marked as outdated.

@morr0ne
Copy link
Contributor

morr0ne commented Dec 25, 2024

The panic abort looks good to me. For context, it was actually more of an oversight since I already use this cargo config in most/all of my test projects:

[unstable]
build-std = ["core", "compiler_builtins", "alloc"]
build-std-features = ["compiler-builtins-mem", "panic_immediate_abort"]

[build]
rustflags = ["-Cpanic=abort"]
target = "x86_64-unknown-linux-none"

About std support - we might add it in the future, but that's still up for discussion. If we do end up getting std support for the target, changing the default panicking strategy should be fine. I don't think rust targets make any guarantees about that.

For the relocation model, I'm running some tests right now. To be honest, I need to check if there was a specific reason for defaulting to PIE or if I just overlooked it when writing the target. I remember having some interesting discussions about this with the maintainer of rustix and origin, which might have factored into the decision - I'll look back at those conversations.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#21 exporting to docker image format
#21 sending tarball 26.3s done
#21 DONE 40.3s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.94s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
.

failures:

---- spec::tests::x86_64_unknown_linux_none stdout ----
thread 'spec::tests::x86_64_unknown_linux_none' panicked at compiler/rustc_target/src/spec/mod.rs:3211:53:
called `Result::unwrap()` on an `Err` value: "targets that support dynamic linking must use the `pic` relocation model"


failures:
    spec::tests::x86_64_unknown_linux_none

@workingjubilee
Copy link
Member

About std support - we might add it in the future, but that's still up for discussion. If we do end up getting std support for the target, changing the default panicking strategy should be fine. I don't think rust targets make any guarantees about that.

In general targets are not beholden to such, no.

@morr0ne
Copy link
Contributor

morr0ne commented Dec 25, 2024

After digging deeper, I have some reservations about switching to a static relocation model. One of the primary goals with this target is enabling developers to write both a libc and dynamic linker in rust. This means we need to support building shared objects that are themselves dynamically linked. From what I understand, supporting dynamic linking requires the target to use a pic model by default. This is actually why the ci is failing with:

"targets that support dynamic linking must use the `pic` relocation model"

It seems we need to keep the current default. While this target is somewhat unique, I suspect this requirements here exists for good technical reasons.

As an alternative solution, what if we added a note in the target documentation recommending the static relocation model for most use cases, along with an example cargo config showing how to set this up?

@workingjubilee
Copy link
Member

I believe people can still override the default, no?

@morr0ne
Copy link
Contributor

morr0ne commented Dec 25, 2024

I believe people can still override the default, no?

They can ovveride the relocation model, which is what you are "supposed" to do right now. But afaik the target is what decides if dynamic relocation is supported or not.

@jieyouxu jieyouxu dismissed their stale review December 25, 2024 18:40

Outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

statics are null on x86_64-unknown-linux-none
7 participants