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

De-stabilize target spec JSON #71009

Open
Mark-Simulacrum opened this issue Apr 10, 2020 · 21 comments
Open

De-stabilize target spec JSON #71009

Mark-Simulacrum opened this issue Apr 10, 2020 · 21 comments
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. A-target-specs Area: Compile-target specifications T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

In a recent Zulip discussion, it was noted that we currently consider JSON target specifications unstable, in the sense that options are relatively freely added and removed.

Currently, the compiler will load target specifications from three places.

If a triple is passed without trailing ".json":

  • a built-in list, specified as Rust structs
  • $RUST_TARGET_PATH, split like PATH on the system we're running on, or, if not set, the current directory.
    • we then attempt to load $triple.json from the directories listed

If a triple is passed with .json, then we directly attempt to load that file path (relative or absolute).

This is the history as best as I can tell:

I cannot currently find any explicit discussion around stability, but presumably that discussion did happen at some time, happy to receive links to it. I would like to propose that we de-stabilize the RUST_TARGET_PATH and file-path loading, making only built-in targets usable. To my knowledge, this essentially is already the status quo: you need to build core/std to actually use a target, and that's not possible on beta/stable without intentional stability holes like RUSTC_BOOTSTRAP.

@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 10, 2020
@Mark-Simulacrum
Copy link
Member Author

@rustbot prioritize

Let's try this out!

@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2020

Error: The feature prioritize is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please let @rust-lang/release know if you're having trouble with this bot.

@jonas-schievink jonas-schievink added A-stability Area: `#[stable]`, `#[unstable]` etc. A-target-specs Area: Compile-target specifications labels Apr 10, 2020
@therealprof
Copy link
Contributor

There seems to be some concern (raised on #rust-embedded:matrix.org) that this might negatively impact some tier 3 ports unless there're some guarantees along the lines of rust-lang/rfcs#2803 to provide a miniscule bar of entry as internal target specification.

@Mark-Simulacrum
Copy link
Member Author

Can you clarify how those tier 3 targets are currently working on stable?

@jonas-schievink
Copy link
Contributor

They don't work on stable, there was just some confusion about what this means for eg. Rust forks using custom LLVM backends. I believe we've resolved all of that though.

@therealprof
Copy link
Contributor

Can you clarify how those tier 3 targets are currently working on stable?

I've no idea whether and how/whether they could work on stable. All I know is that there are some target specifications around which might potentially work with a stable compiler (at least if they were included) and some people raised that concern that this would limit the ability to achieve support in the future.

@Mark-Simulacrum
Copy link
Member Author

Ah, okay. I would love to know what those use cases are.

I think I missed it in my first post - was thinking about it though - but I meant to also say that if we do move ahead with this I would want to do so very slowly; we'd land some warning on stable telling people that if they are using these today, to comment here, and let that sit for several cycles before actually removing the ability from stable users. I hope that can help alleviate your concern that there might be unknown use cases that people are using today.

@phil-opp
Copy link
Contributor

phil-opp commented Apr 11, 2020

I think de-stabilizing target JSON files is a good idea, given that they are effectively unstable already and only sparsely documented. However, I'm a bit worried about the user-experience for people that want to continue working with custom target specifications on nightly. I think we should provide some sort of file-based opt-in (e.g. a feature flag or a .cargo/config configuration key) instead of requiring a -Z command line parameter.

This is especially important because cargo also contains special support for custom targets (see e.g. rust-lang/cargo#5228 and rust-lang/cargo#6778), so we might need to make that part of the cargo command line API unstable too. The problem is that there is currently no equivalent to RUSTFLAGS for cargo, so unstable command line features are only usable with an explicit -Z command line argument. For example, the -Zbuild-std functionality requires you to pass that flag to every invocation of cargo build or cargo run, otherwise the compilation fails. This would make the use of custom targets very cumbersome. Edit: I created a proposal for a build.cargoflags configuration key at rust-lang/cargo#8127, which would alleviate my concerns. Edit2: There is now a way to configure unstable flags through config files: rust-lang/cargo#8393.

@petrochenkov
Copy link
Contributor

cc #71769 (a PR that breaks custom target specifications that specify CRT objects)

@cardoe
Copy link
Contributor

cardoe commented Aug 2, 2020

Looks like this has some interaction with #38338.

@cardoe
Copy link
Contributor

cardoe commented Aug 2, 2020

So @Mark-Simulacrum, if I'm working on targets for compiling code as bare metal Xen targets and I have those as JSON files is the suggestion that I have to build a custom rustc to get that support if the JSON files go away or get them added into the compiler?

@cardoe
Copy link
Contributor

cardoe commented Aug 2, 2020

Also interacts with #32819.

@Mark-Simulacrum
Copy link
Member Author

No, the suggestion is that you use a nightly compiler when using such targets (or, yes, get them added to the compiler, as tier 3 to start).

@nagisa
Copy link
Member

nagisa commented Jan 20, 2021

#80838 is an example of an unstable change to JSON targets.

@ojeda
Copy link
Contributor

ojeda commented Jan 20, 2021

We are using target spec files in Rust for Linux. I don't think it is reasonable to expect all users to use only pre-defined targets. It also creates a cyclic dependency between rustc and other projects that should be avoided if possible (see related discussions at #74247 and #77916).

Having said that, a certain degree of instability can be accommodated. That is to say, I think kernels, embedded & freestanding projects can handle a few changes here and there in the beginning. Nevertheless, I'm sure some degree of stability, even if it is not a perfect 100%, would be appreciated.

What about providing compiler flags instead? This would also allow rustc to declare stability on a per-flag basis, which seems very helpful to tackle this problem step by step. It would be more in line with how other compilers provide this functionality and easier to integrate with other build systems, too. And probably a good opportunity to decouple the options from the LLVM backend ones (where possible).

you need to build core/std to actually use a target, and that's not possible on beta/stable without intentional stability holes like RUSTC_BOOTSTRAP.

I'd assume most projects that build core etc. would want to support stable compilers if they could. For Linux (if we are successful) we definitely need to (eventually), so even if we customize core ourselves, the closest upstream core is to a state where it can be compiled with a stable compiler, the better (more code reuse, easier to base new projects on top of it, etc.). See also the related https://internals.rust-lang.org/t/dogfooding-z-build-std-in-rustbuild/13775.

@ColinFinck
Copy link
Contributor

It should also be noted that many target JSON options have accidentally been renamed in Rust 1.49.0, the current stable release.
This happened in dc004d4 (thanks @bjorn3 for pinpointing to this commit!)

This was fixed 4 days later in dd682cb. Unfortunately, the fix didn't make it into Rust 1.49.0.

As a result, I'm currently required to apply ugly workarounds just for Rust 1.49.0 when building a cross-compiler: ColinFinck/meta-rust@c746ab6

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2024

I haven't seen it noted, and I'm not sure where to mention this, but one thing I've experienced is that the target spec files break periodically. I'm a little concerned about if we ever do "stabilize" target specs for real, that they cannot be reliably stable over time, particularly due to changes in LLVM.

The issue we've hit the most often is changes in the data-layout. Occasionally when LLVM is updated, we get errors like:

data-layout for target `target-14846500547859978937`, `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`, differs from LLVM target's `x86_64-unknown-none-gnu` default layout, `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

which forces us to update the spec file. I don't know if it is possible to better deal with it, or to not be as tied to the backend.

@phil-opp
Copy link
Contributor

Maybe it would make sense to remove the data-layout field from the target JSON or at least make it optional? Since #120062, the Rust compiler also requires custom targets to exactly match the data layout of the chosen LLVM target. So there is only one valid value, which the compiler could just fill in.

@bjorn3
Copy link
Member

bjorn3 commented Feb 15, 2024

GCC and Cranelift are backends for rustc too. Cranelift doesn't have any knowledge about type alignments and I don't know if libgccjit allows querying it. In any case the data layout needs to be independent of the backend for the abi to match between backends, which is very important for the Cranelift backend as it uses an LLVM compiled standard library when you use it from the rustc-codegen-cranelift-preview component.

@ChrisDenton
Copy link
Member

But isn't the issue here that it has to match LLVM exactly, so it's not truly backend independent? It's de-facto specified by LLVM, no?

@bjorn3
Copy link
Member

bjorn3 commented Feb 15, 2024

Yes, but Cranelift doesn't care about the alignments. It just has to match what LLVM does for ABI compatibility, which putting the data layout in the target spec solves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. A-target-specs Area: Compile-target specifications T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests