Set dso_local for more items by Bobo1239 · Pull Request #85276 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation28 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Related to #83592. (cc @nagisa)

Noticed that on x86_64 with relocation-model: static R_X86_64_GOTPCREL relocations were still generated in some cases. (related: Rust-for-Linux/linux#135; Rust-for-Linux needs these fixes to successfully build)

First time doing anything with LLVM so not sure whether this is correct but the following are some of the things I've tried to convince myself.

C equivalent

Example from clang which also sets dso_local in these cases:
clang-12 -fno-PIC -S -emit-llvm test.c

extern int A;

int* a() { return &A; }

int B;

int* b() { return &B; }

; ModuleID = 'test.c'
source_filename = "test.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@A = external dso_local global i32, align 4
@B = dso_local global i32 0, align 4

; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32* @a() #0 {
  ret i32* @A
}

; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32* @b() #0 {
  ret i32* @B
}

attributes #0 = { noinline nounwind optnone uwtable "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" "unsafe-fp-math"="false" "use-soft-float"="false" }

!llvm.module.flags = !{!0}
!llvm.ident = !{!1}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{!"clang version 12.0.0 (https://github.com/llvm/llvm-project/ b978a93635b584db380274d7c8963c73989944a1)"}

clang-12 -fno-PIC -c test.c
objdump test.o -r:

test.o:     file format elf64-x86-64

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE
0000000000000006 R_X86_64_64       A
0000000000000016 R_X86_64_64       B


RELOCATION RECORDS FOR [.eh_frame]:
OFFSET           TYPE              VALUE
0000000000000020 R_X86_64_PC32     .text
0000000000000040 R_X86_64_PC32     .text+0x0000000000000010

Comparison to pre-LLVM 12 output

rustc --emit=obj,llvm-ir --target=x86_64-unknown-none-linuxkernel --crate-type rlib test.rs

#![feature(no_core, lang_items)] #![no_core]

#[lang="sized"] trait Sized {}

#[lang="sync"] trait Sync {}

#[lang = "drop_in_place"] pub unsafe fn drop_in_place<T: ?Sized>(_: *mut T) {}

impl Sync for i32 {}

pub static STATIC: i32 = 32;

extern { pub static EXT_STATIC: i32; }

pub fn a() -> &'static i32 { &STATIC } pub fn b() -> &'static i32 { unsafe {&EXT_STATIC} }

objdump test.o -r
nightly-2021-02-20 (rustc target is x86_64-linux-kernel):

RELOCATION RECORDS FOR [.text._ZN4test1a17h1024ba65f3424175E]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_32S      _ZN4test6STATIC17h3adc41a83746c9ffE


RELOCATION RECORDS FOR [.text._ZN4test1b17h86a6a80c1190ac8dE]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_32S      EXT_STATIC

nightly-2021-05-10:

RELOCATION RECORDS FOR [.text._ZN4test1a17he846f03bf37b2d20E]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_GOTPCREL  _ZN4test6STATIC17h5a059515bf3d4968E-0x0000000000000004


RELOCATION RECORDS FOR [.text._ZN4test1b17h7e0f7f80fbd91125E]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_GOTPCREL  EXT_STATIC-0x0000000000000004

This PR:

RELOCATION RECORDS FOR [.text._ZN4test1a17he846f03bf37b2d20E]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_32S      _ZN4test6STATIC17h5a059515bf3d4968E


RELOCATION RECORDS FOR [.text._ZN4test1b17h7e0f7f80fbd91125E]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_32S      EXT_STATIC

r? @estebank

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

@@ -353,6 +361,7 @@ impl StaticMethods for CodegenCx<'ll, 'tcx> {

llvm::LLVMRustSetLinkage(new_g, linkage);
llvm::LLVMRustSetVisibility(new_g, visibility);
llvm::LLVMRustSetDSOLocal(new_g, dso_local);

Choose a reason for hiding this comment

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

The "other piece of code" is here. Since we're defining the static here, I think you can, and probably should, just unconditionally set dso_local based on should_assume_dso_local here.

I think.

Choose a reason for hiding this comment

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

Do things still work if this is moved outside of this conditional (e.g. to near set_global_alignment call below)?

Choose a reason for hiding this comment

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

Testsuite passes locally at least and rust-for-linux builds. But I have to query linkage/visibility again or pull these lines before the if block making it a little bit harder to follow. Should I go forward with the change or were you just wondering what if?

Choose a reason for hiding this comment

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

I think we should go ahead with it. I'm quite worried that there may be places in the codegen where a static is declared with the right type, but misses setting dso_local, in which case this branch would not be hit.

On another note is I think the code should look a more like:

if self.should_assume_dso_local(...) { 
     llvm::LLVMRustSetDSOLocal(g, true);
}

There's almost never a reason to unset the flag and the part of the backend that has declared the static may have a better idea as to whether the static can be a dso_local based on factors that should_assume_dso_local can't consider.

The fact that it is necessary to get the linkage/visibility back from LLVM I think is indicative of some larger refactor being necessary here so that the linkage/visibility derived from the Rust source is available more directly here. But that's out of scope for this PR. I thin it is perfectly fine to do whatever necessary to obtain the information you need to pass into should_assume_dso_global here.

Choose a reason for hiding this comment

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

Good thinking. And indeed if I apply the following patch on top of my latest commit we have a few tests which trigger the assertion. Should be out of scope for this PR though since that will need a deeper investigation.

Patch

diff --git a/compiler/rustc_codegen_llvm/src/consts.rs b/compiler/rustc_codegen_llvm/src/consts.rs index 245842df1b0..2748fbe0190 100644 --- a/compiler/rustc_codegen_llvm/src/consts.rs +++ b/compiler/rustc_codegen_llvm/src/consts.rs @@ -361,6 +361,12 @@ fn codegen_static(&self, def_id: DefId, is_mutable: bool) { llvm::LLVMRustSetLinkage(new_g, linkage); llvm::LLVMRustSetVisibility(new_g, visibility);

           let linkage = base::linkage_from_llvm(linkage);

           let visibility = base::visibility_from_llvm(visibility);

           if self.should_assume_dso_local(linkage, visibility) {

               llvm::LLVMRustSetDSOLocal(g, true);

```
           }
```

            // To avoid breaking any invariants, we leave around the old
            // global for the moment; we'll replace all references to it
            // with the new global later. (See base::codegen_backend.)

@@ -372,9 +378,10 @@ fn codegen_static(&self, def_id: DefId, is_mutable: bool) {

         let linkage = base::linkage_from_llvm(llvm::LLVMRustGetLinkage(g));
         let visibility = base::visibility_from_llvm(llvm::LLVMRustGetVisibility(g));

       if self.should_assume_dso_local(linkage, visibility) {

           llvm::LLVMRustSetDSOLocal(g, true);

```
       }
```

```
       assert_eq!(
```
```
           llvm::LLVMRustIsDSOLocal(g),
```

           self.should_assume_dso_local(linkage, visibility)

       );

       // As an optimization, all shared statics which do not have interior
       // mutability are placed into read-only memory.

diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 7fb978d61dc..2b515983c6e 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1014,6 +1014,7 @@ pub fn LLVMConstExtractValue( pub fn LLVMSetSection(Global: &Value, Section: *const c_char); pub fn LLVMRustGetVisibility(Global: &Value) -> Visibility; pub fn LLVMRustSetVisibility(Global: &Value, Viz: Visibility);

pub fn LLVMRustIsDSOLocal(Global: &Value) -> bool; pub fn LLVMRustSetDSOLocal(Global: &Value, is_dso_local: bool); pub fn LLVMGetAlignment(Global: &Value) -> c_uint; pub fn LLVMSetAlignment(Global: &Value, Bytes: c_uint); diff --git a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp index 2e135fbe2bd..0fd19d55903 100644 --- a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp @@ -1648,6 +1648,10 @@ extern "C" void LLVMRustSetVisibility(LLVMValueRef V, LLVMSetVisibility(V, fromRust(RustVisibility)); }

+extern "C" bool LLVMRustIsDSOLocal(LLVMValueRef Global) {

return unwrap(Global)->isDSOLocal(); +}
extern "C" void LLVMRustSetDSOLocal(LLVMValueRef Global, bool is_dso_local) { unwrap(Global)->setDSOLocal(is_dso_local); }

failures:
    [ui] ui/issues/issue-33992.rs
    [ui] ui/linkage-attr/linkage-detect-extern-generated-name-collision.rs
    [ui] ui/linkage-attr/linkage-detect-local-generated-name-collision.rs
    [ui] ui/type-alias-impl-trait/bound_reduction.rs#full_tait
    [ui] ui/type-alias-impl-trait/bound_reduction.rs#min_tait

Can you please squash the commits together?

Done. Left the typo fix for the existing test in a separate commit unless you want me to squash it as well.

📌 Commit bfecc1ecf13c1a9541d3519a4e19c81776c7787d has been approved by nagisa

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

May 17, 2021

⌛ Testing commit bfecc1ecf13c1a9541d3519a4e19c81776c7787d with merge 18bf98e745b9feb479eeafaa54d72476fc50e59a...

This comment has been minimized.

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

May 18, 2021

The test case wasn't actually checked for x64 due to a small difference in the name.

Needed to account for unspecified register and a different mov variant when building without optimizations. Applied the following changes across the two commits:

diff --git a/src/test/assembly/static-relocation-model.rs b/src/test/assembly/static-relocation-model.rs index bd456817199..ce2b3b1cfa4 100644 --- a/src/test/assembly/static-relocation-model.rs +++ b/src/test/assembly/static-relocation-model.rs @@ -34,7 +34,7 @@ impl Sync for u8 {} }

// CHECK-LABEL: banana: -// x64: movb chaenomeles(%{{[a-z0-9]+}}), %{{[a-z]+}} +// x64: movb chaenomeles{{((%[a-z0-9]+))?}}, %{{[a-z0-9]+}} // A64: adrp [[REG:[a-z0-9]+]], chaenomeles // A64-NEXT: ldrb {{[a-z0-9]+}}, {{[}}[[REG]], :lo12:chaenomeles] #[no_mangle] @@ -45,7 +45,7 @@ pub fn banana() -> u8 { }

// CHECK-LABEL: peach: -// x64: movb banana(%{{[a-z0-9]+}}), %{{[a-z]+}} +// x64: movb banana{{((%[a-z0-9]+))?}}, %{{[a-z0-9]+}} // A64: adrp [[REG2:[a-z0-9]+]], banana // A64-NEXT: ldrb {{[a-z0-9]+}}, {{[}}[[REG2]], :lo12:banana] #[no_mangle] @@ -56,8 +56,8 @@ pub fn peach() -> u8 { }

// CHECK-LABEL: mango: -// x64: movq EXOCHORDA(%{{[a-z0-9]+}}), %{{[a-z]+}} -// x64-NEXT: movb (%rax), %al +// x64: movq EXOCHORDA{{((%[a-z0-9]+))?}}, %[[REG:[a-z0-9]+]] +// x64-NEXT: movb (%[[REG]]), %{{[a-z0-9]+}} // A64: adrp [[REG2:[a-z0-9]+]], EXOCHORDA // A64-NEXT: ldr {{[a-z0-9]+}}, {{[}}[[REG2]], :lo12:EXOCHORDA] #[no_mangle] @@ -68,7 +68,7 @@ pub fn mango() -> u8 { }

// CHECK-LABEL: orange: -// x64: movl $PIERIS, %{{[a-z]+}} +// x64: mov{{l|absq}} $PIERIS, %{{[a-z0-9]+}} // A64: adrp [[REG2:[a-z0-9]+]], PIERIS // A64-NEXT: add {{[a-z0-9]+}}, [[REG2]], :lo12:PIERIS #[no_mangle]

Tested locally with the CI docker containers so should hopefully succeed now. (only x86_64-gnu and x86_64-gnu-nopt though)

📌 Commit f7ed4a7 has been approved by nagisa

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

May 18, 2021

Bobo1239 added a commit to Bobo1239/linux that referenced this pull request

May 22, 2021