Use RelocModel::Pic for UEFI targets #101413

nicholasbishop · 2022-09-04T17:13:44Z

In #100537, the relocation model for UEFI targets was changed from PIC (the default value) to
static. There was some dicussion of this change here: #100537 (comment)

It turns out that this can cause compilation to fail as described in #101377, so switch back to PIC.

Fixes #101377

rust-highfive · 2022-09-04T17:13:48Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2022-09-04T17:13:48Z

⚠️ Warning ⚠️

These commits modify compiler targets. (See the Target Tier Policy.)

nicholasbishop · 2022-09-04T17:21:05Z

CCing some folks from #100537: @petrochenkov @dvdhrm @oli-obk

petrochenkov · 2022-09-04T17:49:58Z

GlobalValue with DLLImport Storage is dso_local!

I wonder why is there a DLLImport if dynamic linking is not supported on this target.
(I'm ok with partially reverting #100537 if this needs to be fixed fast.)

compiler/rustc_target/src/spec/uefi_msvc_base.rs

petrochenkov · 2022-09-04T18:38:07Z

compiler/rustc_target/src/spec/tests/tests_impl.rs

@@ -146,9 +146,6 @@ impl Target {
        if self.position_independent_executables && !triple.ends_with("-linuxkernel") {
            assert_eq!(self.relocation_model, RelocModel::Pic);
        }
-        if self.relocation_model == RelocModel::Pic {
-            assert!(self.dynamic_linking || self.position_independent_executables);
-        }


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

Should uefi set position_independent_executables and static_position_independent_executables?

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.

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.

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).

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.

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?

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

dvdhrm · 2022-09-05T09:56:36Z

Looks good to me! As mentioned on the linked bug-report, I think we need to investigate why we end up with DLLImport, though. This should not happen as EFI loaders will ignore .idata sections.

compiler/rustc_target/src/spec/tests/tests_impl.rs

petrochenkov · 2022-09-09T18:41:17Z

r? @petrochenkov
r=me after small cleanup (#101413 (comment)).
@rustbot author

In rust-lang#100537, the relocation model for UEFI targets was changed from PIC (the default value) to static. There was some dicussion of this change here: rust-lang#100537 (comment) It turns out that this can cause compilation to fail as described in rust-lang#101377, so switch back to PIC. Fixes rust-lang#101377

nicholasbishop · 2022-09-10T00:48:59Z

@rustbot review

petrochenkov · 2022-09-10T04:29:19Z

@bors r+ rollup

bors · 2022-09-10T04:29:21Z

📌 Commit 54d9ba8 has been approved by petrochenkov

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#101413 (Use RelocModel::Pic for UEFI targets) - rust-lang#101595 (Fix ICE report flags display.) - rust-lang#101616 (Adapt test for msan message change) - rust-lang#101624 (rustdoc: remove unused CSS `#search { position: relative }`) - rust-lang#101633 (Rustdoc-Json: Correcty handle intra-doc-links to items without HTML page) - rust-lang#101634 (Rustdoc-Json Tests: Use ``@is`` and ``@ismany`` more often.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

nicholasbishop · 2022-09-10T15:20:01Z

FYI I've filed a followup ticket here: #101656

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 4, 2022

rust-highfive assigned estebank Sep 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2022

This comment has been minimized.

Sign in to view

nicholasbishop force-pushed the bishop-remove-uefi-static-reloc branch from 6470745 to bfac442 Compare September 4, 2022 17:36

petrochenkov reviewed Sep 4, 2022

View reviewed changes

compiler/rustc_target/src/spec/uefi_msvc_base.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

nicholasbishop force-pushed the bishop-remove-uefi-static-reloc branch from bfac442 to 86c0cc7 Compare September 4, 2022 18:26

petrochenkov reviewed Sep 4, 2022

View reviewed changes

nicholasbishop force-pushed the bishop-remove-uefi-static-reloc branch from 86c0cc7 to c52e25d Compare September 4, 2022 18:49

petrochenkov reviewed Sep 9, 2022

View reviewed changes

compiler/rustc_target/src/spec/tests/tests_impl.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2022

rust-highfive assigned petrochenkov and unassigned estebank Sep 9, 2022

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 9, 2022

nicholasbishop force-pushed the bishop-remove-uefi-static-reloc branch from c52e25d to 54d9ba8 Compare September 9, 2022 19:26

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2022

matthiaskrgr mentioned this pull request Sep 10, 2022

Rollup of 6 pull requests #101639

Merged

bors merged commit 857a43d into rust-lang:master Sep 10, 2022

rustbot added this to the 1.65.0 milestone Sep 10, 2022

phil-opp mentioned this pull request Sep 10, 2022

v0.11: Redesign config and build system, split into sub-crates, and provide disk image builder rust-osdev/bootloader#232

Merged

33 tasks

nicholasbishop deleted the bishop-remove-uefi-static-reloc branch September 10, 2022 15:16

petrochenkov mentioned this pull request Nov 21, 2022

codegen-llvm: never combine DSOLocal and DllImport #104679

Merged

workingjubilee added the O-UEFI UEFI label Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use RelocModel::Pic for UEFI targets #101413

Use RelocModel::Pic for UEFI targets #101413

nicholasbishop commented Sep 4, 2022

rust-highfive commented Sep 4, 2022

rust-highfive commented Sep 4, 2022

nicholasbishop commented Sep 4, 2022

This comment has been minimized.

petrochenkov commented Sep 4, 2022

This comment has been minimized.

petrochenkov Sep 4, 2022

nicholasbishop Sep 4, 2022

bjorn3 Sep 5, 2022

dvdhrm Sep 5, 2022

dvdhrm Sep 5, 2022

dvdhrm Sep 5, 2022

bjorn3 Sep 5, 2022

nicholasbishop Sep 5, 2022

nicholasbishop Sep 9, 2022

bjorn3 Sep 9, 2022

dvdhrm commented Sep 5, 2022

petrochenkov commented Sep 9, 2022

nicholasbishop commented Sep 10, 2022

petrochenkov commented Sep 10, 2022

bors commented Sep 10, 2022

nicholasbishop commented Sep 10, 2022

Use RelocModel::Pic for UEFI targets #101413

Use RelocModel::Pic for UEFI targets #101413

Conversation

nicholasbishop commented Sep 4, 2022

rust-highfive commented Sep 4, 2022

rust-highfive commented Sep 4, 2022

nicholasbishop commented Sep 4, 2022

This comment has been minimized.

petrochenkov commented Sep 4, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdhrm commented Sep 5, 2022

petrochenkov commented Sep 9, 2022

nicholasbishop commented Sep 10, 2022

petrochenkov commented Sep 10, 2022

bors commented Sep 10, 2022

nicholasbishop commented Sep 10, 2022