Direct access external data#158409
Conversation
When compiling with ThinLTO, the missing module flag will cause the compiler to drop the `dso_local` from external data, leading to GOT slots being used. This breaks Rust usage in the Fuchsia kernel, and for embedded code that cannot use GOT indirection whenever ThinLTO is at play due to incorrect setting of the relocation model in the LLVM module when they are merged. Handling the relocation model will be done separately, as this module flag is a pre-requisite of for that change to affect direct-access-external-data. This patch ensures we emit the right module flag for the option. This matches clang's behavior. See https://github.com/llvm/llvm-project/blob/dbab3f71775f6edd11546385aeddb9d0b053a2b5/clang/lib/CodeGen/CodeGenModule.cpp#L1664
|
These commits modify compiler targets. |
rustbot
added
A-LLVM
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Note: I've included two commits, since I'm trying to solve a single problem, but its fine if these should be split up somehow. |
|
@nikic do you have thoughts here on how we can/should solve this on the llvm side (or if i've done something silly in terms of rustc)? |
This comment has been minimized.
This comment has been minimized.
Without setting the PIC/PIELevel consistently, several things in the LTO pipelines stop working, direct-access-external-data specifically, but this can also lead to incorrect TLS models being used or the wrong relocation type being emitted for them in the backend. Ideally, we'd unify and simplify things on the LLVM side to make this very hard for frontends to get wrong. This PR works around that by setting the module's PICLevel and PIELevel consistent with the way these are set in context.rs in lto.rs.
ilovepi
force-pushed
the
direct_access_external_data
branch
from
b46d076 to
7f4494e
Compare
| extern "C" void LLVMRustSetModulePICLevel(LLVMModuleRef M) { | ||
| unwrap(M)->setPICLevel(PICLevel::Level::BigPIC); | ||
| Module *Mod = unwrap(M); | ||
| if (!Mod->getModuleFlag("PIC Level")) { | ||
| Mod->setPICLevel(PICLevel::Level::BigPIC); | ||
| } | ||
| } | ||
|
|
||
| extern "C" void LLVMRustSetModulePIELevel(LLVMModuleRef M) { | ||
| unwrap(M)->setPIELevel(PIELevel::Level::Large); | ||
| Module *Mod = unwrap(M); | ||
| if (!Mod->getModuleFlag("PIE Level")) { | ||
| Mod->setPIELevel(PIELevel::Level::Large); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not totally convinced this is 100% the right approach. Alternatively, we could set the merge strategy and do it that way, but I figured its probably better to only replace if it doesn't exist. Happy to amend the strategy here.
| let llmod = module.module_llvm.llmod(); | ||
|
|
||
| let reloc_model = cgcx.relocation_model; | ||
| llvm::set_module_pic_and_pie_levels(llmod, reloc_model, &cgcx.crate_types); |
There was a problem hiding this comment.
I'm confused by why this is necessary. Aren't we just loading a previously serialized module here? Shouldn't the PIC/PIE flags have been set at the time the module was created?
There was a problem hiding this comment.
Yeah, I was surprised too. I believe the main contributor is that core & std libs end up w/o the PIE levels set at all, since they aren't being used in an executable. I can't recall if we have other examples or not, but its my vague recollection that we triggered this issue w/o use of core at all when codegen-units>1.
For our kernel specifically, this becomes an issue as we'd really like to use code from core. However, that would be a hard sell if we cannot prevent GOT indirection when accessing globals that would normally be safe to directly access (and have been accessed that way from C++ code for a very long time).
Do you think there's a better way to address this? Maybe somewhere in the lto.rs code?
There was a problem hiding this comment.
So the problem is that libcore/libstd are compiled as PIC but you want them to be PIE? And you're solving this by adding the PIE metadata during LTO? I don't think that's the right way to fix this.
Wouldn't the correct solution here to use build-std and enable PIE for libstd?
|
r? @nikic |
|
☔ The latest upstream changes (presumably #158593) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
5 participants
Today, when using -Zdirect-access-external-data we've seen an issue when compiling code with ThinLTO or when codegen-units > 1 (due to the use of ThinLTO), that causes GOT accesses to occur in cases where they shouldn't. This is due to two reasons. First, unlike clang, rustc does not emit the direct-access-external-data module flag in the LLVM IR. When compiling with ThinLTO, the missing module flag will cause the compiler to drop the
dso_localfrom external data, leading to GOT slots being used. This breaks Rust usage in the Fuchsia kernel, and for embedded code that cannot use GOT indirection. The Clang example can be found here: https://github.com/llvm/llvm-project/blob/dbab3f71775f6edd11546385aeddb9d0b053a2b5/clang/lib/CodeGen/CodeGenModule.cpp#L1664The second issue is that without setting the PIC/PIELevel consistently, several things in the LTO pipelines stop working, including direct-access-external-data, incorrect TLS models being selected, or the wrong relocation type being emitted for them in the backend. In the case of direct-access-external-data, if the relocation model is not set correctly, the indirection will always happen because that is the only thing possible (e.g. you're compiling PIC for a shared lib and not PIE). Ideally, we'd unify and simplify things on the LLVM side to make this very hard for frontends to get wrong, or at least supply more consistent APIs for them to consume. This PR works around that by emitting the module flag, and setting the module's PICLevel and PIELevel consistent with the way these are set in context.rs in lto.rs.