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

Build as a 'bin' crate, linking with rust-lld #14

Merged
merged 2 commits into from Oct 16, 2020
Merged

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Oct 8, 2020

This eliminated the two awkward ld steps from the Makefile, and seems to produce a smaller .efi. It seems to work, based on quick testing in qemu.

Using 'uefi' for target because apparently Rust cares: rust-lang/rust#71881

Some changes to the target specification here are from rustc -Z unstable-options --print target-spec-json --target x86_64-unknown-uefi. We probably want more of the settings from there, but most likely still need our own custom target to change some things.

This eliminated the two awkward `ld` steps from the Makefile, and seems
to produce a smaller `.efi`. It seems to work, based on quick testing in
qemu.

Using 'uefi' for target because apparently Rust cares:
rust-lang/rust#71881

Some changes to the target specification here are from
`rustc -Z unstable-options --print target-spec-json --target x86_64-unknown-uefi`.
We probably want more of the settings from there, but most likely still
need our own custom target to change some things.
@crawfxrd
Copy link
Member

crawfxrd commented Oct 8, 2020

Nice. I couldn't get this working last night when I tried to copy the x86_64-unknown-uefi spec.

the two awkward ld steps

These aren't awkward. They're downright black magic. I'd be glad to see them gone.

We probably want more of the settings from there

stack-probes will be needed for firmware-smmstore.

But we should try to align to the target as close as possible.

The target name should probably be updated too to better match the builtin target, for anything special core or compiler_builtins do. Something like x86_64-unknown-uefi-drv?

@ids1024
Copy link
Member Author

ids1024 commented Oct 8, 2020

They're downright black magic.

Linkers are invariably black magic. But hopefully if we have Rust doing the linking for us, we only have to worry about a smaller subset of the arcana...

But we should try to align to the target as close as possible.

The standard x86_64-unknown-uefi target seems to work as well, after changing pre-link-args. The changes in the PR should be the minimum required changes to the current target file.

Too bad Rust target specifications don't seem to support an "inherits-from" option or anything like that...

x86_64-unknown-uefi uses "panic-strategy": "abort", while it seems we're currently using unwinding panics. Are those unwinding panics actually behaving sensibly?

For ABI details, I suppose we want the same thing as ``x86_64-unknown-uefi`, assuming it's correct. I'm sure issues there can lead to some great bugs...

The target name should probably be updated too to better match the builtin target, for anything special core or compiler_builtins do. Something like x86_64-unknown-uefi-drv?

Sounds good.

@crawfxrd
Copy link
Member

crawfxrd commented Oct 9, 2020

Are those unwinding panics actually behaving sensibly?

We're calling to/from C code (edk2), so I don't think we should be unwinding in the first place. abort seems correct.

Most of the other options that we specify that aren't in x86_64-unknown-uefi seem to be the default, so could be removed.

@ids1024
Copy link
Member Author

ids1024 commented Oct 9, 2020

We're calling to/from C code (edk2), so I don't think we should be unwinding in the first place. abort seems correct.

Apparently Rust has a working group for unwinding across FFI boundaries: https://github.com/rust-lang/project-ffi-unwind

If we're not even testing it, we can probably expect unwinding isn't working quite how it should. (It's also worth noting that abort generates smaller binaries.)

So using x86_64-unknown-uefi, but modified just to change the link flags, seems reasonable. Actually, if desired we could do that without a custom target spec by passing Rustc arguments, but it may be better to use the target spec (so we can easily change other things if needed, etc.).

This is currently identical to the output of
`rustc -Z unstable-options --print target-spec-json --target x86_64-unknown-uefi`
except with changes to `pre-link-args`.
@ids1024
Copy link
Member Author

ids1024 commented Oct 9, 2020

I've pushed an update using a x86_64-unknown-uefi-drv target, that is the same as the standard target except changes to the linker arguments.

ids1024 added a commit to system76/firmware-smmstore that referenced this pull request Oct 12, 2020
Using the same target file as
system76/firmware-setup#14

Builds, though I don't know how to properly test it.
ids1024 added a commit to system76/gop-policy that referenced this pull request Oct 12, 2020
Using the same target file as
system76/firmware-setup#14

Builds, though I don't know how to properly test it.
jackpot51 pushed a commit to system76/firmware-smmstore that referenced this pull request Oct 16, 2020
Using the same target file as
system76/firmware-setup#14

Builds, though I don't know how to properly test it.
jackpot51 pushed a commit to system76/gop-policy that referenced this pull request Oct 16, 2020
Using the same target file as
system76/firmware-setup#14

Builds, though I don't know how to properly test it.
@jackpot51 jackpot51 merged commit c0ecd4e into master Oct 16, 2020
@jackpot51 jackpot51 deleted the crate-type-bin branch October 16, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants