Skip to content

Conversation

lu-zero
Copy link
Contributor

@lu-zero lu-zero commented Jul 22, 2023

This way is possible to write inline assembly code aware of it.

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 22, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2023

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

Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

(disclaimer: not a rust-lang reviewer)

Looks good, although this will probably need to be gated under a feature gate [1] [2] [3] with a UI test.

@bjorn3
Copy link
Member

bjorn3 commented Jul 22, 2023

I just thought of a possible issue. There is no guarantee in which crate a function is codegened, but #[cfg] always applies to the current crate. As such if the crate from which the asm!() originated and the one in which it was codegened have a different -Crelocation-model value you would get inline asm compiled with the wrong relocation model. This is even more of a problem with LTO where everything is effectively codegened in the executable, cdylib or staticlib crate.

@lu-zero
Copy link
Contributor Author

lu-zero commented Jul 22, 2023

The information is mainly useful to correctly consume symbols made available via sym, not sure if somebody has other use for it.

@bjorn3
Copy link
Member

bjorn3 commented Jul 22, 2023

If you have

//- lib.rs compileflags:-Crelocation-model=static -Clto=yes --edition 2021
static FOO: u8 = 42;
pub unsafe fn foo() {
    #[cfg(relocation_model = "static")]
    core::arch::asm!("mov dword ptr [{}], 43", sym FOO);

    #[cfg(relocation_model = "pic")]
    core::arch::asm!("mov dword ptr [rip + {}], 43", sym FOO);
}

//- bar.rs compileflags:-Crelocation-model=pic -Clto=yes --extern lib=liblib.rlib --edition 2021
fn main() {
    unsafe { lib::foo(); }
}

Then core::arch::asm!("mov dword ptr [{}], 43", sym FOO); will be compiled with the PIC relocation model, while core::arch::asm!("mov dword ptr [rip + {}], 43", sym FOO); is necessary to get it working.

@bjorn3
Copy link
Member

bjorn3 commented Jul 22, 2023

In most cases -Crelocation-model for all crates will match, but not always. And likely not for the standard library.

@lu-zero
Copy link
Contributor Author

lu-zero commented Jul 22, 2023

clang and gcc with any bit of inline asm with "m" constraint have to deal with it already, what do they do?

@bjorn3
Copy link
Member

bjorn3 commented Jul 22, 2023

For LTO I think just accept that it happens and for inline and generic functions in C and C++ you need to include the header file, which will evaluate the #ifdef depending on the current compiler invocation and for regular functions it is guaranteed that any codegen will happen locally for non-LTO builds. For rustc all #[cfg] however are fixed at the point of producing the crate metadata that stands in for header files in C and C++.

@lu-zero
Copy link
Contributor Author

lu-zero commented Jul 22, 2023

the equivalent C code is

// lib.c - cc -c lib.c -fno-lto -flto -o lib.o
unsigned char FOO = 42;

void foo() {
#ifdef __PIC__
...
#else 
...
#endif
}
// main.c - cc -c main.c -flto -fPIC -o main.o
void foo();

int main() {
    foo();
    return 0;
}
# cc -flto main.o lib.o -o out
$ cc -flto -fno-pic -c lib.c -o lib.o
$ cc -flto -fPIC -c main.c -o main.o
$ cc -flto main.o lib.o -o out
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib.o: relocation R_X86_64_32S against symbol `FOO' can not be used when making a PIE object; recompile with -fPIE
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: failed to set dynamic section sizes: bad value
collect2: error: ld returned 1 exit status

The linker simply bails. (here the test https://gist.github.com/lu-zero/d4188ed4837c112d6584a5b4de67bca6)

@Amanieu
Copy link
Member

Amanieu commented Jul 23, 2023

Generally you would never have a library that is "less PIC" than the binary using it. So this case should never happen in practice if you using a build system like Cargo.

@b-naber
Copy link
Contributor

b-naber commented Jul 30, 2023

r? compiler

@rustbot rustbot assigned fee1-dead and unassigned b-naber Jul 30, 2023
@fee1-dead
Copy link
Member

r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned fee1-dead Jul 30, 2023
Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

This will need a test.

@bjorn3
Copy link
Member

bjorn3 commented Aug 17, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 17, 2023

📌 Commit 045bb9e0ae5e8ea323be05a75c31c547aa08641b has been approved by bjorn3

It is now in the queue for this repository.

@bors 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 Aug 17, 2023
@bors
Copy link
Collaborator

bors commented Aug 17, 2023

⌛ Testing commit 045bb9e0ae5e8ea323be05a75c31c547aa08641b with merge 01e78a6c25d3590b98677fd03ae1a13333875b18...

@bjorn3
Copy link
Member

bjorn3 commented Aug 18, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 18, 2023

📌 Commit 443d1e3819cf34786f91a1156d9818a9bbf59244 has been approved by bjorn3

It is now in the queue for this repository.

@bors 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 Aug 18, 2023
@bors
Copy link
Collaborator

bors commented Aug 18, 2023

⌛ Testing commit 443d1e3819cf34786f91a1156d9818a9bbf59244 with merge cf492d9ce554381760e95851e23874a8d846db6f...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Aug 18, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 18, 2023
@lu-zero lu-zero force-pushed the relocation-model-in-cfg branch from 443d1e3 to 28ebdf6 Compare August 18, 2023 16:57
@rustbot rustbot added the A-meta Area: Issues & PRs about the rust-lang/rust repository itself label Aug 18, 2023
@bjorn3
Copy link
Member

bjorn3 commented Aug 18, 2023

Looks like you did something wrong when rebasing. There are a couple of unrelated commits.

@rust-log-analyzer

This comment has been minimized.

@lu-zero lu-zero force-pushed the relocation-model-in-cfg branch from 28ebdf6 to c7dedf7 Compare August 18, 2023 17:22
@lu-zero
Copy link
Contributor Author

lu-zero commented Aug 18, 2023

Let see if manually rebasing fixes it.

@lu-zero
Copy link
Contributor Author

lu-zero commented Aug 18, 2023

Hopefully no other targets are pic-impossible.

@rust-log-analyzer

This comment has been minimized.

This way is possible to write inline assembly code aware of it.
@lu-zero lu-zero force-pushed the relocation-model-in-cfg branch from c7dedf7 to c0394c8 Compare August 18, 2023 17:57
@bjorn3
Copy link
Member

bjorn3 commented Aug 19, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 19, 2023

📌 Commit c0394c8 has been approved by bjorn3

It is now in the queue for this repository.

@bors 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 Aug 19, 2023
@bors
Copy link
Collaborator

bors commented Aug 20, 2023

⌛ Testing commit c0394c8 with merge 82c5732...

@bors
Copy link
Collaborator

bors commented Aug 20, 2023

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 82c5732 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 20, 2023
@bors bors merged commit 82c5732 into rust-lang:master Aug 20, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 20, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (82c5732): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.5% [2.5%, 4.6%] 2
Improvements ✅
(primary)
-2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)
-1.9% [-3.1%, -1.0%] 4
All ❌✅ (primary) -2.4% [-2.4%, -2.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 634.076s -> 635.189s (0.18%)
Artifact size: 346.97 MiB -> 347.03 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.