Direct access external data#158409
Conversation
|
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.
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?
There was a problem hiding this comment.
They're being compiled into an executable, and our kernel builds core on demand as part of its build with all the same flags as the rest of the kernel. Under codegen-units>1 ThinLTO is always used, but the output isn't necessarily a PIE object, and this causes the kernel build to break due to the presence of GOT indirection (we have a rule preventing this in our linker script), even when we're not trying to use LTO if we don't setcodegen-units=1. This is all pretty surprising to us, as we didn't expect to have issues w/ LTO builds since they've been working forever in C++. FWIW, I'm pretty sure we tried setting all the various relocation-model flags on the core build w/o success, but we can try again.
I also think the test I added should just work w/ direct-access-external-data when compiling and linking against an rlib.
There was a problem hiding this comment.
Well, I'll be damned. That did solve the issue. So this does at least unblock me. Thanks for the suggestion. I had wrongly assumed that setting the platform defaults to position independent in the target spec would have amounted to the same thing, especially coupled with the fact that the Rust parts are only slotting into a minor part of the kernel build that's still mostly driven by clang.
I do still think the example in the test should be something that can work out of the box for platforms that default to PIE, though. That said, I'm guessing there may be a better way to spell that, such that we don't need about half of this patch. If you have thoughts about that I'm all ears.
In the meantime, unless there's an objection I'll pare this PR down to just adding the module flag and the minimal set of tests.
There was a problem hiding this comment.
I would expect setting it in the target spec to also work, as long as it's not explicitly overwritten on the command line. sess.relocation_model() looks like a pretty simple fallback to the target: self.opts.cg.relocation_model.unwrap_or(self.target.relocation_model).
In the meantime, unless there's an objection I'll pare this PR down to just adding the module flag and the minimal set of tests.
Sounds good to me.
|
r? @nikic |
This comment has been minimized.
This comment has been minimized.
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
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
7f4494e to
1b5adcf
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
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.