Set dso_local for more items

[Bobo1239]

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

[rust-highfive]

r? @estebank

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

[nagisa]

r? @nagisa

[nagisa]

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.

[nagisa]

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

[Bobo1239]

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?

[nagisa]

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.

[Bobo1239]

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<GlobalValue>(Global)->isDSOLocal();
+}
+
 extern "C" void LLVMRustSetDSOLocal(LLVMValueRef Global, bool is_dso_local) {
   unwrap<GlobalValue>(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

[nagisa]

Can you please squash the commits together?

[Bobo1239]

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

[nagisa]

@bors r+

Thanks!

[bors]

📌 Commit bfecc1ecf13c1a9541d3519a4e19c81776c7787d has been approved by nagisa

[nagisa]

@bors rollup=never

[bors]

⌛ Testing commit bfecc1ecf13c1a9541d3519a4e19c81776c7787d with merge 18bf98e745b9feb479eeafaa54d72476fc50e59a...

[bors]

💔 Test failed - checks-actions

[Bobo1239]

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)

[nagisa]

@bors r+

[bors]

📌 Commit f7ed4a7 has been approved by nagisa

[bors]

⌛ Testing commit f7ed4a7 with merge be8450e...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing be8450e to master...