rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.23k stars 12.7k forks source link

consider disallowing setting target_family if target_os=none #74247

Open ehuss opened 4 years ago

ehuss commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

On Sat, Jul 11, 2020 at 01:32:12PM -0700, Alex Gaynor wrote:

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

Interesting! Are you actually building std, or just core and alloc? Are there any cfg(unix) or cfg(... = "unix") conditionals that this is actually affecting?

alex commented 4 years ago

Just core and alloc. No way to run std inside the kernel :-)

I can't imagine there's any cfg unix conditionals its impacting, but I guess I never checked. I'll do some quick greps over liballoc and libcore.

On Sat, Jul 11, 2020 at 4:34 PM Josh Triplett notifications@github.com wrote:

On Sat, Jul 11, 2020 at 01:32:12PM -0700, Alex Gaynor wrote:

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

Interesting! Are you actually building std, or just core and alloc? Are there any cfg(unix) or cfg(... = "unix") conditionals that this is actually affecting?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/74247#issuecomment-657125445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBHEIWSYP66GZH7KQLLR3DED5ANCNFSM4OXMGL2A .

-- All that is necessary for evil to succeed is for good people to do nothing.

alex commented 4 years ago

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 commented 4 years ago

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

alex commented 4 years ago

Yeah, I can do that.

nagisa commented 4 years ago

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 commented 4 years ago

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

alex commented 4 years ago

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

geofft commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/rust-lang/rust/pull/64051#issuecomment-706763655 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

ojeda commented 4 years ago

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.

nagisa commented 3 years ago

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 commented 3 months ago

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

Ericson2314 commented 3 months ago

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

ehuss commented 2 months ago

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.