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

treewide: support i386-pc-elf target, make errors prettier #50562

Closed
wants to merge 1 commit into from

Conversation

oxij
Copy link
Member

@oxij oxij commented Nov 18, 2018

This target used by coreboot and its payloads.

/cc @Ericson2314

This target used by coreboot and its payloads.
@Ericson2314
Copy link
Member

Do you actually want no libc or do you want newlib? @matthewbauer where did you newlib stuff go?

@matthewbauer
Copy link
Member

This is probably okay! I think you should add an entry to examples.nix to provide an example config for this. Something like "pc-embedded".

I think the ARM toolchains always come with newlib - but it might not be common with this toolchain? Better to get as close as we can to what is found in the wild. There's really no standardization for these things. But maybe we can make some attempts at standardizing things.

To be honest, I was hoping we could avoid this one just because it uses i386 which could complicate some logic (search for isi686 in Nixpkgs). Maybe having this will improve the situation though.

I don't really understand the difference between this and i686-none-elf. Perhaps this one will support some '90s CPUs while the i686 doesn't? I would prefer to leave pc off here as that's not really a vendor. It is also kind of confusing whether "elf" refers to the ABI or the OS (in i686-none-elf: cpu=i686, vendor=unknown, os=none, abi=elf).

@Ericson2314
Copy link
Member

Oh yes, let's get rid of pc and just use unknown. pc was a stupid convenience with no benefit, just added in the 1990s because somebody thought unknown looked too scary.

# NOTE: normal stuff should use i686, this type is purely for
# "i386-pc-elf" target (e.g. coreboot and its payloads)
isi386 = { cpu = cpuTypes.i386; };

isi686 = { cpu = cpuTypes.i686; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think we should have isx86_32 and isx86_64, with isi686 as an alias for isx86_32, perhaps to be deprecated. This matches what we do for arm. It also avoids a situation where we falsely say i386 isn't i686, which ignores subtyping / compatibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good! If anything truly works on just i686 then just ask users to do stdenv.hostPlatform.parsed.cpu.name == "i686".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM does http://llvm.org/doxygen/Triple_8cpp_source.html has i386 through i986, fwiw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should limit it to i386, i486, i586, and i686. The newer ones never really caught on.

@matthewbauer
Copy link
Member

Another thing with libc = null, it should only build gcc once. Is that the case currently @Ericson2314

@oxij
Copy link
Member Author

oxij commented Nov 18, 2018

@Ericson2314

Do you actually want no libc or do you want newlib?

coreboot needs no libc, it's a BIOS.

Oh yes, let's get rid of pc and just use unknown.

The problem is that AFAIK you have to set the host platform to i386-pc-elf for coreboot to cross-compile. Sure, it can be parsed in nix as whatever, but the argument to the configure script must have the pc bit.

So I think we should have isx86_32 and isx86_64.

SGTM. Should I add those in this PR?

@matthewbauer

I think you should add an entry to examples.nix to provide an example config for this

I have the whole of coreboot as an example in the patch queue somewhere but it depends on some more changes to gcc I can't upstream yet (not cleaned up). And I have no other examples of this.

Perhaps this one will support some '90s CPUs while the i686 doesn't?

AFAIK i386 in this instance is a 133tspeak for "real mode". I.e. the most basic set of opcodes that all x86 CPUs should be able to run just after the reset.

@Ericson2314
Copy link
Member

coreboot needs no libc, it's a BIOS.

There exists many pure functions in libc, which newlib provides.

Is that the case currently @Ericson2314

It didn't look like it. TBH this is part of the reason why I hop we can get away with the newlib case. it should be basically free at runtime when not used (since static linking).

The problem is that AFAIK you have to set the host platform to i386-pc-elf for coreboot to cross-compile.

We can lie to coreboot or patch it.

SGTM. Should I add those in this PR?

Please do, and also remove isi386 and make isi686 = isx86_32. Maybe even add i486, i586, etc, so people get the point that the granularity is over the top.

AFAIK i386 in this instance is a 133tspeak for "real mode". I.e. the most basic set of opcodes that all x86 CPUs should be able to run just after the reset.

I doubt its anything that specific. I bios would get a trip though the whole 1980s in different modes. I think this is just some arbitrary historical choice on coreboot's part.

@matthewbauer
Copy link
Member

matthewbauer commented Nov 19, 2018

Yeah sounds fine to just follow coreboot in this respect for compatibility, though. I would want to verify this only uses 1 GCC stage. Is that correct?

@Ericson2314
Copy link
Member

@matthewbauer I disagree; I still think coreboot is asking with ridiculous specificity.

@@ -82,6 +82,7 @@ rec {
aarch64 = { bits = 64; significantByte = littleEndian; family = "arm"; version = "8"; };
aarch64_be = { bits = 64; significantByte = bigEndian; family = "arm"; version = "8"; };

i386 = { bits = 32; significantByte = littleEndian; family = "x86"; }; # see inspect.nix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use version like arm.

@@ -174,14 +176,17 @@ rec {
types.kernel = enum (attrValues kernels);

kernels = with execFormats; with kernelFamilies; setTypes types.openKernel {
# kernel-less targets
elf = { execFormat = elf; families = { inherit none; }; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elf isn't a kernel - it's an abi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And barely an ABI, since that normally refers to the machine code itself not the container.

@Ericson2314
Copy link
Member

Maybe we should do this in steps so there's less to think about. We can get coreboot working in parallel.

@oxij
Copy link
Member Author

oxij commented Nov 21, 2018 via email

@oxij
Copy link
Member Author

oxij commented Nov 21, 2018 via email

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/i586-cross-compiling-ambiguous-i686-use/1482/6

@goertzenator
Copy link
Contributor

Here a quick idea of what isx86_32 implementation looks like.

I'm still trying to get all this working, but it seemed salient to share it now. I'm also working on a RFC for isx86_32.

@Ericson2314
Copy link
Member

Anything left here that isn't on master?

@matthewbauer
Copy link
Member

I think getting rid of libcChooser would be nice... but otherwise I think this isn't needed anymore.

@oxij
Copy link
Member Author

oxij commented Feb 19, 2019

Yes, only libcChooser thing is left here. Let's close this, I'll make a separate PR for that later.

@oxij oxij closed this Feb 19, 2019
@Ericson2314
Copy link
Member

@oxij Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants