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

consider disallowing setting target_family if target_os=none #74247

Open
ehuss opened this issue Jul 11, 2020 · 22 comments
Open

consider disallowing setting target_family if target_os=none #74247

ehuss opened this issue Jul 11, 2020 · 22 comments
Labels
A-codegen Area: Code generation O-bare-metal Target: Rust without an operating system

Comments

@ehuss
Copy link
Contributor

ehuss commented Jul 11, 2020

Should setting target_family require also setting target_os?

Currently, the x86_64-linux-kernel target sets target_family to "unix", but the os is "none". This is the only target which sets the family without the os. This can make it a little confusing/awkward when writing cfg expressions to properly cover targets such as these (sometimes the check for os="none" needs to appear before the family).

In practice today this isn't an issue because libcore, liballoc, and compiler_builtins do not ever check the family. AFAIK, the family doesn't have any other effects other than the cfg settings.

This issue came up during #74033 where I was adjusting some cfg expressions for building std for Cargo. I do not know if Cargo's build-std will ever support x86_64-linux-kernel, but I'd like to try to cover as many targets as possible and avoid special cases if they are not necessary.

@eddyb had some suggestions which I'll copy here:

Does the Linux kernel even have an UNIX API inside of itself? That doesn't sound right but maybe I just haven't seen it before.

(To be clear, I'm thinking that family="unix" is as valid for inside an OS kernel as os="thatosname" - if os="none" is used then surely there should also not be a family? Maybe we should enforce that the family is derived from the OS name and therefore means "OS family")

cc @joshtriplett who I think might know a little about the x86_64-linux-kernel target, and @alex who introduced the target. Perhaps they can shed some light why "unix" was chosen.

@joshtriplett
Copy link
Member

@ehuss Given that the target will not be able to use the vast majority of std, I don't know if it makes sense to use unix, as opposed to some kind of embedded no-os target.

@alex, @geofft, any thoughts? Is there a good reason to use unix here?

@alex
Copy link
Member

alex commented Jul 11, 2020

Hmmm, I don't remember why unix was chosen, it's entirely possible it was a bad cargo-cult.

The target currently does work with build-std in cargo, so ideally that wouldn't regress :-)

In short, simply setting target_family=none seems most obviously correct to me.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 11, 2020 via email

@alex
Copy link
Member

alex commented Jul 11, 2020 via email

@alex
Copy link
Member

alex commented Jul 11, 2020

Yeah, quick grep shows there's no unix references in libcore and liballoc. So I think changing the family to none here is the right move.

@joshtriplett
Copy link
Member

@alex Could you send a PR to that effect, CCing @ehuss and myself, and we can review and approve it?

@alex
Copy link
Member

alex commented Jul 11, 2020

Yeah, I can do that.

@nagisa
Copy link
Member

nagisa commented Jul 11, 2020

Side note, I find the target string itself malformed, if we don't end up setting target_os=linux for this target. To me the target string was always arch-vendor-os-env, and in my head this really parses as arch=x86_64, os=linux, env=kernel rather than arch=x86_64, os=none, env=linux-kernel, which is what this target is.


More on topic, I don’t think we add targets often enough, and IMO targets are all often very unique once you go past the most common ones, to warrant restrictions like these.

@eddyb
Copy link
Member

eddyb commented Jul 12, 2020

@alex was #74257 meant to point to this issue? It points to #72314 instead which I don't see how it's related.

@alex
Copy link
Member

alex commented Jul 12, 2020

@eddyb yes, it was. No idea how that happened.

Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 14, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 14, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 14, 2020
don't mark linux kernel module targets as a unix environment

refs rust-lang#74247

r?@joshtriplett

cc: @ehuss
@geofft
Copy link
Contributor

geofft commented Aug 16, 2020

Re @nagisa, I could be convinced that the target should really be named x86_64-none-linuxkernel or x86_64-unknown-none-linuxkernel or something, yeah.

For what it's worth, the actual Clang build of the kernel uses regular x86_64-linux-gnu as the triple, but as far as I can tell, that's because Clang target triples encode a lot less information than Rust target specs do - things like -mcmodel=kernel -mno-sse -mno-red-zone are all passed via CFLAGS. So I think we're moderately on our own here for naming. (Also platform ifdefs are generally implemented by the platform headers and not by the C compiler, IIRC, so we're definitely on our own for what #[cfg(target_family = "unix")] means.)

Yeah, quick grep shows there's no unix references in libcore and liballoc.

Is it worth enforcing this, with a lint or something preventing libcore and liballoc from having cfg conditions on target_os?

@ehuss
Copy link
Contributor Author

ehuss commented Aug 16, 2020

Is it worth enforcing this, with a lint or something preventing libcore and liballoc from having cfg conditions on target_os?

I believe this is already checked in https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/pal.rs

@Ericson2314
Copy link
Contributor

Ericson2314 commented Oct 11, 2020

I could be convinced that the target should really be named x86_64-none-linuxkernel or x86_64-unknown-none-linuxkernel or something, yeah.

Please! I originally wrote #64051 (comment) in quite a panic, until I was pointed to this issue. As wrote there:

The triples/configs/whatever are a mess, but so far mentioning some OS has meant "I am running on that OS" i.e. "I have a interface[1] for communicating with that kernel and I am one of its processes". But Kernel code doesn't talk to the kernel via syscalls, and thus violates that assumption.

Now maybe Rust has some extra metadata, like this target_family vs target_os distinction, but other tools (some of which I've helped maintain on this front, like https://github.com/NixOS/nixpkgs/blob/master/lib/systems/parse.nix or https://savannah.gnu.org/projects/config/) also parse the the triple, and will run into the exact some sort of issues. Adding a bunch of special cases will be costly, and an ongoing headache. I'm quite desperate to avoid that.

A last thing is even the "linuxkernel" part is unnecessary, x86_64-none or x86_64-unknown-none would also be just fine, because the kernel is "just" a bare metal program that calls itself, and not one that relies on special interfaces beyond hardware interfaces. (It could be argued that only Linux purpose-built to run on some hypervisor and make "hypercalls" needs a non-none OS component.)

@petrochenkov
Copy link
Contributor

If someone fixes the discussed issues for the Linux kernel target, it would be great to fix them for compiler\rustc_target\src\spec\x86_64_unknown_hermit_kernel.rs as well.

@DianaNites
Copy link

DianaNites commented Oct 11, 2020

A last thing is even the "linuxkernel" part is unnecessary, x86_64-none or x86_64-unknown-none would also be just fine,

I don't think this is actually true, as the linux kernel uses -mpreferred-stack-boundary=3, for a 8 byte stack alignment on x86_64 instead of 16, so it probably makes sense for it to be it's own target.

Also now noticing that the targets data_layout says the stack is 128 bit/16 byte aligned?

@Ericson2314
Copy link
Contributor

Ericson2314 commented Oct 11, 2020

@DianaNites I didn't mean to say that Linux doesn't need a custom target --- just about every freestanding program does, and you rightly point out something that Linux needs that is particular to it

Rather, I was trying to say that "linux" shouldn't appear as part of the contents of that target, as Linux is the program being compile, the target describes interfaces / constraints external to that program, and Linux is not external to itself. (C.f. the "don't know your own name" principle).

The awkward thing is all the built-in targets that Rustc supports have names that look like LLVM targets triples, which doesn't leave anywhere to put the "linux" part except for a the end, so I'll just do -linuxkernel and -hermitkernel for now. (I suppose if I am quibbling with that, I should also be quibbling with Rustc even needing to have this built-in target at all, rather than there just being a json file in the linux sources, which is the most modular and avoids the naming problem.)

@Ericson2314
Copy link
Contributor

I put up #77916 for the linuxkernel route, and also likewise changed the target for the Hermet kernel as @petrochenkov suggested.

@ojeda
Copy link
Contributor

ojeda commented Oct 14, 2020

I should also be quibbling with Rustc even needing to have this built-in target at all, rather than there just being a json file in the linux sources, which is the most modular and avoids the naming problem.

That is what I originally thought, but somehow Rustc got the Linux target. Each OS/kernel/platform should define whatever they need. A compiler should only need to expose the options needed to be flexible enough to work for them.

@camelid camelid added the A-codegen Area: Code generation label Oct 20, 2020
Ericson2314 added a commit to QuiltOS/rust that referenced this issue Mar 1, 2021
Whether for Rust's own `target_os`, LLVM's triples, or GNU config's, the
OS-related have fields have been for code running *on* that OS, not code
that is *part* of the OS.

The difference is huge, as syscall interfaces are nothing like
freestanding interfaces. Kernels are (hypervisors and other more exotic
situations aside) freestanding programs that use the interfaces provided
by the hardware. It's *those* interfaces, the ones external to the
program being built and its software dependencies, that are the content
of the target.

For the Linux Kernel in particular, `target_env: "gnu"` is removed for
the same reason: that `-gnu` refers to glibc or GNU/linux, neither of
which applies to the kernel itself.

Relates to rust-lang#74247

Thanks @ojeda for catching some things.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 2, 2021
…, r=joshtriplett

Change built-in kernel targets to be os = none throughout

Whether for Rust's own `target_os`, LLVM's triples, or GNU config's, the
OS-related have fields have been for code running *on* that OS, not code
hat is *part* of the OS.

The difference is huge, as syscall interfaces are nothing like
freestanding interfaces. Kernels are (hypervisors and other more exotic
situations aside) freestanding programs that use the interfaces provided
by the hardware. It's *those* interfaces, the ones external to the
program being built and its software dependencies, that are the content
of the target.

For the Linux Kernel in particular, `target_env: "gnu"` is removed for
the same reason: that `-gnu` refers to glibc or GNU/linux, neither of
which applies to the kernel itself.

Relates to rust-lang#74247
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 3, 2021
…, r=joshtriplett

Change built-in kernel targets to be os = none throughout

Whether for Rust's own `target_os`, LLVM's triples, or GNU config's, the
OS-related have fields have been for code running *on* that OS, not code
hat is *part* of the OS.

The difference is huge, as syscall interfaces are nothing like
freestanding interfaces. Kernels are (hypervisors and other more exotic
situations aside) freestanding programs that use the interfaces provided
by the hardware. It's *those* interfaces, the ones external to the
program being built and its software dependencies, that are the content
of the target.

For the Linux Kernel in particular, `target_env: "gnu"` is removed for
the same reason: that `-gnu` refers to glibc or GNU/linux, neither of
which applies to the kernel itself.

Relates to rust-lang#74247
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 6, 2021
…, r=joshtriplett

Change built-in kernel targets to be os = none throughout

Whether for Rust's own `target_os`, LLVM's triples, or GNU config's, the
OS-related have fields have been for code running *on* that OS, not code
hat is *part* of the OS.

The difference is huge, as syscall interfaces are nothing like
freestanding interfaces. Kernels are (hypervisors and other more exotic
situations aside) freestanding programs that use the interfaces provided
by the hardware. It's *those* interfaces, the ones external to the
program being built and its software dependencies, that are the content
of the target.

For the Linux Kernel in particular, `target_env: "gnu"` is removed for
the same reason: that `-gnu` refers to glibc or GNU/linux, neither of
which applies to the kernel itself.

Relates to rust-lang#74247
JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 7, 2021
…, r=joshtriplett

Change built-in kernel targets to be os = none throughout

Whether for Rust's own `target_os`, LLVM's triples, or GNU config's, the
OS-related have fields have been for code running *on* that OS, not code
hat is *part* of the OS.

The difference is huge, as syscall interfaces are nothing like
freestanding interfaces. Kernels are (hypervisors and other more exotic
situations aside) freestanding programs that use the interfaces provided
by the hardware. It's *those* interfaces, the ones external to the
program being built and its software dependencies, that are the content
of the target.

For the Linux Kernel in particular, `target_env: "gnu"` is removed for
the same reason: that `-gnu` refers to glibc or GNU/linux, neither of
which applies to the kernel itself.

Relates to rust-lang#74247
@nagisa
Copy link
Member

nagisa commented May 5, 2021

The definition of target_family was generalized to expand past unix vs windows, so the answer to the question posed in the issue title is now "we shouldn't do that". But I can still value in disallowing setting of unix or windows values for target_family if target_os is none. So leaving this open.

@workingjubilee workingjubilee added the O-bare-metal Target: Rust without an operating system label Aug 17, 2022
@workingjubilee
Copy link
Member

We no longer have the linuxkernel targets so I think this is done?

@Ericson2314
Copy link
Contributor

What about custom target JSON? It could be an error if one tries to set the family but not the OS in there?

@ehuss
Copy link
Contributor Author

ehuss commented Aug 9, 2024

I'm going to reopen, since I think it would be a good idea to have a check for this for the future, or for things like JSON targets as mentioned.

@ehuss ehuss reopened this Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation O-bare-metal Target: Rust without an operating system
Projects
None yet
Development

No branches or pull requests