Target specification cleanup #706

ojeda · 2022-03-13T21:12:18Z

PR #694 introduced target.json generation on the fly, intentionally keeping the generated files exactly as the ones we previously had in the repository.

However, those target.json files we had were just examples. Now we need to start configuring rustc/LLVM properly depending on what the kernel configuration is, which requires first cleaning up the script as much as possible. In particular, this involves:

Factoring out things that apply to all architectures to reduce complexity.
Removing unused keys (e.g. if they are not needed or if they are already the default).
Fixing any wrong value.
Moving as much as possible into each architecture via command-line options (-C and -Z), so that we reduce our dependency on the script (eventually removing it).

This PR leaves x86 "clean", i.e. with only the 4 required keys in the script and everything else in arch/x86/Makefile as command-line flags.

The other architectures have been fairly cleaned up as well, though I have avoided moving the options to their Makefiles yet (with the exception of RISC-V for a trivial detail), mainly in order to avoid introducing lots of changes upstream before Rust is merged. And, nevertheless, it would be best if arch maintainers take a look at how they exactly want to set it up anyway.

While not strictly part of the cleanup, I added retpoline-external-thunk as well as -Ctarget-cpu/-Ztune-cpu as needed for x86 and -f{data,function}-sections for all. This is intended to have x86 as complete as possible.

During this I found a couple of small issues (in LLVM and rustc), which I will be filling soon. I will also update the live lists with the few new unstable flags we are depending on. I will also start requesting/adding support upstream for some flags we will need.

There are a lot of small commits in this PR in order to justify each change separately. Please see details in the commit messages.

This partially addresses Russell King's feedback.

@mpe In case you want to consider start moving flags to the arch Makefile.
@bjorn3 @nbdd0121 Since you may know some rustc details.

This option was added in `rustc` in order to disable GDB scripts for some no-`std` targets, see rust-lang/rust#44993. Therefore, even though the default in the compiler for targets is `true`, we leave it as `false`. If, in the future, it gets enabled, then we should take into account the `CONFIG_GDB_SCRIPTS` option. Signed-off-by: Miguel Ojeda <[email protected]>

Some months ago we disabled stack probes support due to a bug in LLVM, see commit 53bc4d4 ("rust: disable stack probes"). Until either the bug is solved or we decide to use other kind of probes, keep this disabled for all architectures. Signed-off-by: Miguel Ojeda <[email protected]>

With `rustc`, we can use the `-Cforce-frame-pointers` option to force them to be emitted (so it should work like `always`). However, when the option is set to `n`, what happens depends on the target, thus we cannot control the option completely from the command-line. Therefore, to have as much control as possible from the command-line, we keep the default as `"may-omit"`, which allows us to force it when needed. Also includes a small comment in the `Makefile`. Signed-off-by: Miguel Ojeda <[email protected]>

Since we are not linking with `rustc`, we should not need these. Signed-off-by: Miguel Ojeda <[email protected]>

Signed-off-by: Miguel Ojeda <[email protected]>

As far as I can see in the Rust repository, this is not used for anything we care about. It defaults to `unknown`, too, which should be fine. Signed-off-by: Miguel Ojeda <[email protected]>

We are already using the stable flag, which overrides the target. Signed-off-by: Miguel Ojeda <[email protected]>

We are not linking with `rustc`, so we should not need this. And, if we do, we should use the CLI stable flag anyway. Signed-off-by: Miguel Ojeda <[email protected]>

It defaults to `none`, which is the one we want. Note that some code in `rustc_middle/src/ty/layout.rs` (for `fn` ABI) checks it for `linux`, but not for ARM. Thus nothing should change. Signed-off-by: Miguel Ojeda <[email protected]>

Note that little endian is the default. Signed-off-by: Miguel Ojeda <[email protected]>

We are not linking with `rustc`, so we should not need this. In any case, the default is `true`. Signed-off-by: Miguel Ojeda <[email protected]>

bjorn3 · 2022-03-13T23:54:52Z

scripts/generate_rust_target.rs

@@ -154,7 +154,6 @@ fn main() {
        );
        ts.push("dynamic-linking", true);
        ts.push("crt-static-respected", true);
-        ts.push("executables", true);


Maybe set this to false? This indicates if executables are supported or not. For these targets executables are not supported.

It defaults to false, so it should be fine.

bjorn3 · 2022-03-13T23:56:29Z

arch/x86/Makefile

@@ -138,6 +138,7 @@ else

        KBUILD_CFLAGS += -mno-red-zone
        KBUILD_CFLAGS += -mcmodel=kernel
+        KBUILD_RUSTFLAGS += -Cno-redzone=y


Does this also apply to other targets? Or is x86 the only for which the abi has redzones?

No, this will get only applied to x86. There are other architectures with redzones, e.g. within the kernel powerpc64. I did not touch non-x86 Makefiles (please see PR message), though we/they will have to.

bjorn3 · 2022-03-13T23:58:49Z

scripts/generate_rust_target.rs

@@ -227,7 +227,6 @@ fn main() {
    ts.push("emit-debug-gdb-scripts", false);
    ts.push("frame-pointer", "may-omit");
    ts.push("function-sections", false);
-    ts.push("position-independent-executables", true);


While this isn't necessary here I believe the commut message is incorrect. I think it can also affect the relocation model that is used when using the bin crate type.

I may be missing something, but I only saw/see it used in compiler/rustc_codegen_ssa/src/back/link.rs's link_output_kind().

Just checked it and you're right.

Thanks for double-checking!

bjorn3 · 2022-03-14T00:02:31Z

Makefile

@@ -556,7 +556,7 @@ KBUILD_RUSTFLAGS := $(rust_common_flags) \
 		    -Cpanic=abort -Cembed-bitcode=n -Clto=n \
 		    -Cforce-unwind-tables=n -Ccodegen-units=1 \
 		    -Csymbol-mangling-version=v0 \
-		    -Crelocation-model=static \
+		    -Crelocation-model=static -Zfunction-sections=n \


Is there a kernel config that enables it? If not maybe add one? It really helps with reducing code size for rust.

The next commit after this one enables it under LD_DEAD_CODE_DATA_ELIMINATION for built-in code, though that kernel config is considered experimental.

bjorn3 · 2022-03-14T00:03:12Z

scripts/generate_rust_target.rs

@@ -152,7 +152,6 @@ fn main() {
            "data-layout",
            "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64",
        );
-        ts.push("dynamic-linking", true);


Should probably be set to false instead.

This is the same case as executables, i.e. it defaults to false.

I should probably mention this in their commit messages, since I did that in other cases.

We are not producing executables, so we do not need this. It seems only used to set a different default target for iOS anyway. The default is `false`. Signed-off-by: Miguel Ojeda <[email protected]>

We do not need `rpath`, thus remove from target and command-line, where we rely on the default (`n`). Signed-off-by: Miguel Ojeda <[email protected]>

It is not needed. The default is the empty vector, which is fine. Signed-off-by: Miguel Ojeda <[email protected]>

It does not look like there is anything in the `rustc` compiler or `core`/`alloc` that requires this being `gnu`. Therefore, just remove it and use its default (empty string: `""`). Signed-off-by: Miguel Ojeda <[email protected]>

Signed-off-by: Miguel Ojeda <[email protected]>

Architectures may still override it. Signed-off-by: Miguel Ojeda <[email protected]>

It is only used for linking to adjust the output kind, not for compilation. The default is `false`. Signed-off-by: Miguel Ojeda <[email protected]>

Architectures may use `-Zrelro-level` to set the right value. Signed-off-by: Miguel Ojeda <[email protected]>

Architectures can use `-Zplt` to set the right value. Signed-off-by: Miguel Ojeda <[email protected]>

The default value is `"32"`. Signed-off-by: Miguel Ojeda <[email protected]>

The default value is that of `target-pointer-width`, thus remove the cases where it is the same. Signed-off-by: Miguel Ojeda <[email protected]>

This allows us to be consistent. The triples are not normalized according to LLVM (e.g. vendor does not appear in the second position). Signed-off-by: Miguel Ojeda <[email protected]>

By the way, it was renamed recently to `has-thread-local`. Signed-off-by: Miguel Ojeda <[email protected]>

Architectures may use `-Ctarget-cpu=` to set the right value. The default is `generic`, but LLVM does not support it for RISC-V, thus we need to override it in the `Makefile`. Signed-off-by: Miguel Ojeda <[email protected]>

Signed-off-by: Miguel Ojeda <[email protected]>

Note that `function-sections`'s default is `true`, thus we need to specify it. Signed-off-by: Miguel Ojeda <[email protected]>

Signed-off-by: Miguel Ojeda <[email protected]>

We do not use `dylib`s, `cdylib`s nor cross-compiled proc macros. Signed-off-by: Miguel Ojeda <[email protected]>

We do not deal with `libc`. Signed-off-by: Miguel Ojeda <[email protected]>

The kernel is compiled with `-mgeneral-regs-only`, thus these should not be enabled. Signed-off-by: Miguel Ojeda <[email protected]>

Signed-off-by: Miguel Ojeda <[email protected]>

ojeda · 2022-03-14T18:30:41Z

@bjorn3 Thanks a lot for going through the commits.

I am pushing the changes to the commit messages and a trivial style change.

ojeda added 11 commits March 13, 2022 19:44

rust: target: remove pre-link-args

2da9793

Since we are not linking with `rustc`, we should not need these. Signed-off-by: Miguel Ojeda <[email protected]>

scripts: generate_rust_target: Array is not needed anymore

a3b8a45

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove vendor

8cb00ab

As far as I can see in the Rust repository, this is not used for anything we care about. It defaults to `unknown`, too, which should be fine. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove panic-strategy

d9d5744

We are already using the stable flag, which overrides the target. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove linker-flavor

dfd3d66

We are not linking with `rustc`, so we should not need this. And, if we do, we should use the CLI stable flag anyway. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove os

c8c9b5b

It defaults to `none`, which is the one we want. Note that some code in `rustc_middle/src/ty/layout.rs` (for `fn` ABI) checks it for `linux`, but not for ARM. Thus nothing should change. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: move target-endian to top-level

3b429a1

Note that little endian is the default. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove linker-is-gnu

94ec7f2

We are not linking with `rustc`, so we should not need this. In any case, the default is `true`. Signed-off-by: Miguel Ojeda <[email protected]>

bjorn3 reviewed Mar 13, 2022

View reviewed changes

bjorn3 reviewed Mar 14, 2022

View reviewed changes

bjorn3 approved these changes Mar 14, 2022

View reviewed changes

ojeda added 13 commits March 14, 2022 19:21

rust: target: remove executables

bfd91c4

We are not producing executables, so we do not need this. It seems only used to set a different default target for iOS anyway. The default is `false`. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove has-rpath and rpath=n

7e34060

We do not need `rpath`, thus remove from target and command-line, where we rely on the default (`n`). Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove target-family

2a75bb2

It is not needed. The default is the empty vector, which is fine. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove env

16b2218

It does not look like there is anything in the `rustc` compiler or `core`/`alloc` that requires this being `gnu`. Therefore, just remove it and use its default (empty string: `""`). Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: move x86 redzone option to command-line

5ed0684

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: move x86 code model option to command-line

af47d53

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: move x86 features to command-line

2d2e1fd

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: enable retpoline-external-thunk for x86

acc574f

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: move relocation-model to command-line flag

883b79d

Architectures may still override it. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove position-independent-executables

7923d84

It is only used for linking to adjust the output kind, not for compilation. The default is `false`. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove relro-level

4675d06

Architectures may use `-Zrelro-level` to set the right value. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove needs-plt

f028dee

Architectures can use `-Zplt` to set the right value. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove target-c-int-width

9b8a212

The default value is `"32"`. Signed-off-by: Miguel Ojeda <[email protected]>

ojeda added 12 commits March 14, 2022 19:21

rust: target: remove max-atomic-width where possible

bd32a10

The default value is that of `target-pointer-width`, thus remove the cases where it is the same. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: use same target triple as Makefile.clang

02e1ab1

This allows us to be consistent. The triples are not normalized according to LLVM (e.g. vendor does not appear in the second position). Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove has-elf-tls

07809b0

By the way, it was renamed recently to `has-thread-local`. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove cpu

732b3c3

Architectures may use `-Ctarget-cpu=` to set the right value. The default is `generic`, but LLVM does not support it for RISC-V, thus we need to override it in the `Makefile`. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: x86: use -Ctarget-cpu and -Ztune-cpu

89234c4

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: make output slightly more pretty

d05b11f

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: move function-sections to command-line

abd4171

Note that `function-sections`'s default is `true`, thus we need to specify it. Signed-off-by: Miguel Ojeda <[email protected]>

Makefile: add -Zfunction-sections when supported

423fdca

Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove dynamic-linking

ea19cab

We do not use `dylib`s, `cdylib`s nor cross-compiled proc macros. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: remove crt-static-respected

a2942c4

We do not deal with `libc`. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: arm64: disable neon and fp-armv8

d127d77

The kernel is compiled with `-mgeneral-regs-only`, thus these should not be enabled. Signed-off-by: Miguel Ojeda <[email protected]>

rust: target: improve a few comments

257d449

Signed-off-by: Miguel Ojeda <[email protected]>

ojeda force-pushed the ts-clean branch from 71d0efa to 257d449 Compare March 14, 2022 18:30

ojeda merged commit 99862d1 into Rust-for-Linux:rust Mar 14, 2022

ojeda deleted the ts-clean branch March 14, 2022 18:56

sulix mentioned this pull request Sep 12, 2022

Fix building under UML #881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target specification cleanup #706

Target specification cleanup #706

ojeda commented Mar 13, 2022 •

edited

Loading

bjorn3 Mar 13, 2022

ojeda Mar 14, 2022

bjorn3 Mar 13, 2022

ojeda Mar 14, 2022 •

edited

Loading

bjorn3 Mar 13, 2022

ojeda Mar 14, 2022 •

edited

Loading

bjorn3 Mar 14, 2022

ojeda Mar 14, 2022

bjorn3 Mar 14, 2022

ojeda Mar 14, 2022

bjorn3 Mar 14, 2022

ojeda Mar 14, 2022 •

edited

Loading

ojeda commented Mar 14, 2022

Target specification cleanup #706

Target specification cleanup #706

Conversation

ojeda commented Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

ojeda commented Mar 14, 2022

ojeda commented Mar 13, 2022 •

edited

Loading

ojeda Mar 14, 2022 •

edited

Loading

ojeda Mar 14, 2022 •

edited

Loading

ojeda Mar 14, 2022 •

edited

Loading