From 023cc968e1295994ed8039da43b0f2f4ea4e9390 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Fri, 19 Nov 2021 19:33:29 -0800 Subject: [PATCH 1/3] Make `LLVMRustGetOrInsertGlobal` always return a `GlobalVariable` `Module::getOrInsertGlobal` returns a `Constant*`, which is a super class of `GlobalVariable`, but if the given type doesn't match an existing declaration, it returns a bitcast of that global instead. This causes UB when we pass that to `LLVMGetVisibility` which unconditionally casts the opaque argument to a `GlobalValue*`. Instead, we can do our own get-or-insert without worrying whether existing types match exactly. It's not relevant when we're just trying to get/set the linkage and visibility, and if types are needed we can bitcast or error nicely from `rustc_codegen_llvm` instead. --- .../rustc_llvm/llvm-wrapper/RustWrapper.cpp | 12 ++++++- src/test/ui/issues/issue-91050.rs | 34 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/issues/issue-91050.rs diff --git a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp index e77d29bed7122..f3d8eb2602a37 100644 --- a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp @@ -124,8 +124,18 @@ extern "C" LLVMValueRef LLVMRustGetOrInsertFunction(LLVMModuleRef M, extern "C" LLVMValueRef LLVMRustGetOrInsertGlobal(LLVMModuleRef M, const char *Name, size_t NameLen, LLVMTypeRef Ty) { + Module *Mod = unwrap(M); StringRef NameRef(Name, NameLen); - return wrap(unwrap(M)->getOrInsertGlobal(NameRef, unwrap(Ty))); + + // We don't use Module::getOrInsertGlobal because that returns a Constant*, + // which may either be the real GlobalVariable*, or a constant bitcast of it + // if our type doesn't match the original declaration. We always want the + // GlobalVariable* so we can access linkage, visibility, etc. + GlobalVariable *GV = Mod->getGlobalVariable(NameRef, true); + if (!GV) + GV = new GlobalVariable(*Mod, unwrap(Ty), false, + GlobalValue::ExternalLinkage, nullptr, NameRef); + return wrap(GV); } extern "C" LLVMValueRef diff --git a/src/test/ui/issues/issue-91050.rs b/src/test/ui/issues/issue-91050.rs new file mode 100644 index 0000000000000..50763d30931b7 --- /dev/null +++ b/src/test/ui/issues/issue-91050.rs @@ -0,0 +1,34 @@ +// build-pass +// compile-flags: --crate-type lib -Ccodegen-units=1 + +// This test declares globals by the same name with different types, which +// caused problems because Module::getOrInsertGlobal would return a Constant* +// bitcast instead of a GlobalVariable* that could access linkage/visibility. +// In alt builds with LLVM assertions this would fail: +// +// rustc: /checkout/src/llvm-project/llvm/include/llvm/Support/Casting.h:269: +// typename cast_retty::ret_type llvm::cast(Y *) [X = llvm::GlobalValue, Y = llvm::Value]: +// Assertion `isa(Val) && "cast() argument of incompatible type!"' failed. +// +// In regular builds, the bad cast was UB, like "Invalid LLVMRustVisibility value!" + +pub mod before { + #[no_mangle] + pub static GLOBAL1: [u8; 1] = [1]; +} + +pub mod inner { + extern "C" { + pub static GLOBAL1: u8; + pub static GLOBAL2: u8; + } + + pub fn call() { + drop(unsafe { (GLOBAL1, GLOBAL2) }); + } +} + +pub mod after { + #[no_mangle] + pub static GLOBAL2: [u8; 1] = [2]; +} From 3b2cfa574699d3d92c1114837e347dd7ee248fcb Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Sat, 20 Nov 2021 16:29:15 -0800 Subject: [PATCH 2/3] Add another test variant of issue-91050 Co-authored-by: Simonas Kazlauskas --- .../{issue-91050.rs => issue-91050-1.rs} | 2 +- src/test/ui/issues/issue-91050-2.rs | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) rename src/test/ui/issues/{issue-91050.rs => issue-91050-1.rs} (92%) create mode 100644 src/test/ui/issues/issue-91050-2.rs diff --git a/src/test/ui/issues/issue-91050.rs b/src/test/ui/issues/issue-91050-1.rs similarity index 92% rename from src/test/ui/issues/issue-91050.rs rename to src/test/ui/issues/issue-91050-1.rs index 50763d30931b7..403a41462ef18 100644 --- a/src/test/ui/issues/issue-91050.rs +++ b/src/test/ui/issues/issue-91050-1.rs @@ -1,5 +1,5 @@ // build-pass -// compile-flags: --crate-type lib -Ccodegen-units=1 +// compile-flags: --crate-type=rlib --emit=llvm-ir -Cno-prepopulate-passes // This test declares globals by the same name with different types, which // caused problems because Module::getOrInsertGlobal would return a Constant* diff --git a/src/test/ui/issues/issue-91050-2.rs b/src/test/ui/issues/issue-91050-2.rs new file mode 100644 index 0000000000000..2ff954d15cabe --- /dev/null +++ b/src/test/ui/issues/issue-91050-2.rs @@ -0,0 +1,24 @@ +// build-pass +// compile-flags: --crate-type=rlib --emit=llvm-ir -Cno-prepopulate-passes + +// This is a variant of issue-91050-1.rs -- see there for an explanation. + +pub mod before { + extern "C" { + pub static GLOBAL1: [u8; 1]; + } + + pub unsafe fn do_something_with_array() -> u8 { + GLOBAL1[0] + } +} + +pub mod inner { + extern "C" { + pub static GLOBAL1: u8; + } + + pub unsafe fn call() -> u8 { + GLOBAL1 + 42 + } +} From 3aa1954b0bfc3f10917d5ff8fa315ac3cae5c45a Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Sat, 20 Nov 2021 17:02:37 -0800 Subject: [PATCH 3/3] Move the issue-91050 tests to appease tidy --- src/test/ui/{issues => statics}/issue-91050-1.rs | 0 src/test/ui/{issues => statics}/issue-91050-2.rs | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename src/test/ui/{issues => statics}/issue-91050-1.rs (100%) rename src/test/ui/{issues => statics}/issue-91050-2.rs (100%) diff --git a/src/test/ui/issues/issue-91050-1.rs b/src/test/ui/statics/issue-91050-1.rs similarity index 100% rename from src/test/ui/issues/issue-91050-1.rs rename to src/test/ui/statics/issue-91050-1.rs diff --git a/src/test/ui/issues/issue-91050-2.rs b/src/test/ui/statics/issue-91050-2.rs similarity index 100% rename from src/test/ui/issues/issue-91050-2.rs rename to src/test/ui/statics/issue-91050-2.rs