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.

Sign up for GitHub

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

Jump to bottom

Use RelocModel::Pic for UEFI targets #101413

Merged
merged 1 commit into from
Sep 10, 2022
Merged

Use RelocModel::Pic for UEFI targets #101413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/rustc_target/src/spec/tests/tests_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ impl Target {
if self.position_independent_executables && !triple.ends_with("-linuxkernel") {
assert_eq!(self.relocation_model, RelocModel::Pic);
}
if self.relocation_model == RelocModel::Pic {
// The UEFI targets do not support dynamic linking but still require PIC (#101377).
if self.relocation_model == RelocModel::Pic && self.os != "uefi" {
assert!(self.dynamic_linking || self.position_independent_executables);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This assert should not be removed in general, you can add an exception for UEFI with an explanation instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Should uefi set position_independent_executables and static_position_independent_executables?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should uefi set position_independent_executables and static_position_independent_executables?

Hard to tell by reading the in-code docs what these exactly control. But I think codegen_ssa is the only user and it simply causes PIC output requests to be silently converted to NoPIC output requests. In that case, I think UEFI should set both since PIC is technically supported, though not required nor used by anyone I know.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this assertion is misleading. UEFI does not support dynamic linking, nor are PIC-executables really necessary. However, it still requires a PIC'ish-reloc-model to allow the custom UEFI loaders to relocate the images during load.

I already mentioned this in #100537, it is not clear to me what effect RelocModel::Static has. The LLVM docs are basically non-existant. If it causes .reloc to be stripped from PE32 binaries, then it is not applicable to UEFI.

Copy link
Contributor

Choose a reason for hiding this comment

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

static_position_independent_executables is for when there is no dynamic linker.

Well, yeah, sure, but my question was rather what exactly it controls? Does it make rustc refuse compilation of non-pic? does it enforce static-pic regardless of what the caller chose? Does it mean pic is mandatory, or just supported?

Maybe there is no clear definition like this, and it just controls the one knob we currently have in codegen_ssa. That would be fine with me. In that case, UEFI should probably set this to true.

it is not clear to me what effect RelocModel::Static has.

AFAIK it fixes a specific load location at link time (eg 0x10000, depends on the linker) as opposed to allowing it to be loaded anywhere at runtime as with RelocModel::Pic.

I see. This likely means on PE32 .reloc is stripped and relocation is not supported? In that case, I think the assertion is wrong and needs to go. A relocatable binary should not imply dynamic-linking or position-independen-code. Since reloc and PIC are different things on UEFI (and also on Windows).

Copy link
Member

Choose a reason for hiding this comment

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

Well, yeah, sure, but my question was rather what exactly it controls? Does it make rustc refuse compilation of non-pic? does it enforce static-pic regardless of what the caller chose? Does it mean pic is mandatory, or just supported?

It controls if pic compilation is supported or not. It doesn't make it mandatory. In the linker code for the msvc linker it doesn't distinguish between pic and non-pic executables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested adding this into uefi_msvc_base:

position_independent_executables: true,
static_position_independent_executables: true,

and it doesn't seem to cause any problem, at least in the simple test I did. Do you want me to add that to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Friendly ping -- should I add the above lines to this PR? Or should we stick to the existing changes?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure.

if self.static_position_independent_executables {
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_target/src/spec/uefi_msvc_base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
// code runs in the same environment, no process separation is supported.

use crate::spec::{LinkerFlavor, LldFlavor, PanicStrategy};
use crate::spec::{RelocModel, StackProbeType, TargetOptions};
use crate::spec::{StackProbeType, TargetOptions};

pub fn opts() -> TargetOptions {
let mut base = super::msvc_base::opts();
Expand Down Expand Up @@ -47,7 +47,6 @@ pub fn opts() -> TargetOptions {
stack_probes: StackProbeType::Call,
singlethread: true,
linker: Some("rust-lld".into()),
relocation_model: RelocModel::Static,
..base
}
}
2 changes: 1 addition & 1 deletion src/test/codegen/abi-efiapi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ trait Freeze { }
#[lang="copy"]
trait Copy { }

//x86_64: define dso_local win64cc void @has_efiapi
//x86_64: define win64cc void @has_efiapi
//i686: define void @has_efiapi
//aarch64: define dso_local void @has_efiapi
//arm: define dso_local void @has_efiapi
Expand Down