Skip to content

Commit

Permalink
Rollup merge of rust-lang#104679 - dvdhrm:rw/dso, r=petrochenkov
Browse files Browse the repository at this point in the history
codegen-llvm: never combine DSOLocal and DllImport

Prevent DllImport from being attached to DSOLocal definitions in the LLVM IR. The combination makes no sense, since definitions local to the compilation unit will never be imported from external objects.

Additionally, LLVM will refuse the IR if it encounters the combination (introduced in [1]):

```
  if (GV.hasDLLImportStorageClass())
    Assert(!GV.isDSOLocal(),
           "GlobalValue with DLLImport Storage is dso_local!", &GV);
```

Right now, codegen-llvm will only apply DllImport to constants and rely on call-stubs for functions. Hence, we simply extend the codegen of constants to skip DllImport for any local definitions.

This was discovered when switching the EFI targets to the static relocation model [2]. With this fixed, we can start another attempt at this.

[1] https://smlnj-gitlab.cs.uchicago.edu/manticore/llvm/commit/509132b368efed10bbdad825403f45e9cf1d6e38
[2] rust-lang#101656
  • Loading branch information
Manishearth authored Nov 23, 2022
2 parents 340fe81 + 46e0b02 commit 9c91297
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 6 deletions.
16 changes: 10 additions & 6 deletions compiler/rustc_codegen_llvm/src/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,18 @@ impl<'ll> CodegenCx<'ll, '_> {
llvm::set_thread_local_mode(g, self.tls_model);
}

let dso_local = unsafe { self.should_assume_dso_local(g, true) };
if dso_local {
unsafe {
llvm::LLVMRustSetDSOLocal(g, true);
}
}

if !def_id.is_local() {
let needs_dll_storage_attr = self.use_dll_storage_attrs && !self.tcx.is_foreign_item(def_id) &&
// Local definitions can never be imported, so we must not apply
// the DLLImport annotation.
!dso_local &&
// ThinLTO can't handle this workaround in all cases, so we don't
// emit the attrs. Instead we make them unnecessary by disallowing
// dynamic linking when linker plugin based LTO is enabled.
Expand Down Expand Up @@ -340,12 +350,6 @@ impl<'ll> CodegenCx<'ll, '_> {
}
}

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

self.instances.borrow_mut().insert(instance, g);
g
}
Expand Down
11 changes: 11 additions & 0 deletions src/test/codegen/auxiliary/extern_decl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Auxiliary crate that exports a function and static. Both always
// evaluate to `71`. We force mutability on the static to prevent
// it from being inlined as constant.

#![crate_type = "lib"]

#[no_mangle]
pub fn extern_fn() -> u8 { unsafe { extern_static } }

#[no_mangle]
pub static mut extern_static: u8 = 71;
25 changes: 25 additions & 0 deletions src/test/codegen/static-relocation-model.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Verify linkage of external symbols in the static relocation model.
//
// compile-flags: -O -C relocation-model=static
// aux-build: extern_decl.rs

#![crate_type = "rlib"]

extern crate extern_decl;

// The `extern_decl` definitions are imported from a statically linked rust
// crate, thus they are expected to be marked `dso_local` without `dllimport`.
//
// The `access_extern()` symbol is from this compilation unit, thus we expect
// it to be marked `dso_local` as well, given the static relocation model.
//
// CHECK: @extern_static = external dso_local local_unnamed_addr global i8
// CHECK: define dso_local i8 @access_extern() {{.*}}
// CHECK: declare dso_local i8 @extern_fn() {{.*}}

#[no_mangle]
pub fn access_extern() -> u8 {
unsafe {
extern_decl::extern_fn() + extern_decl::extern_static
}
}

0 comments on commit 9c91297

Please sign in to comment.