Support using const pointers in asm const operand#138618
Support using const pointers in asm const operand#138618nbdd0121 wants to merge 11 commits intorust-lang:mainfrom
const operand#138618Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
r? codegen |
This comment has been minimized.
This comment has been minimized.
|
r? compiler-errors |
|
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_gcc |
|
☔ The latest upstream changes (presumably #140415) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
|
☔ The latest upstream changes (presumably #141668) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I just have one question, then r=me. This was not an easy delta review. More fool me 😂 |
|
With this PR ThinLTO would no longer be able to rename functions when making them public as the inline asm now directly embeds the symbol names, right? |
Symbol names are handled the same way as today's |
| let alloc = alloc.inner(); | ||
| let typ = self.val_ty(init).get_aligned(alloc.align.bytes()); | ||
|
|
||
| let global = self.declare_global_with_linkage(sym.name, typ, GlobalKind::Exported); |
There was a problem hiding this comment.
Exporting is wrong. That would lead to symbol conflicts if two asm blocks use the same const.
There was a problem hiding this comment.
Hmm, I guess changing the Exported to something else is not going to be sufficient and I need to also do some local dedup in case a asm!() is monomorphized twice due to MIR inlining? This is not an issue as the name hint is only used for global_asm/naked_asm but GCC needs injected symbol names for inline asm too.
I guess I really need to build a working version of cg_gcc and test locally.
There was a problem hiding this comment.
Could we have a test that catches this? My understanding is that Rust does run its test suite on gcc too.
There was a problem hiding this comment.
Yeah, I'll do that. I've already reproduced the issue locally. It looks like it has been much easier to build Rust with cg_gcc now, which isn't the case when I started working on this in 2024.
| &self, | ||
| global_alloc: GlobalAlloc<'tcx>, | ||
| name_hint: Option<Instance<'tcx>>, | ||
| ) -> Result<(Self::Value, Option<Instance<'tcx>>), u64>; |
There was a problem hiding this comment.
This method is not used inside cg_ssa, so it shouldn't be defined there.
There was a problem hiding this comment.
It's correlated to scalar_to_backend and I feel this is a good place to put the doc-comments to be shared. I could move it out. Do you want me to duplicate the documentation in cg_gcc and cg_llvm?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is currently a no-op, but will be useful when const in `global_asm!` can be pointers.
Currently global_asm already have symbol names when using v0 scheme, this makes them obtain symbols with legacy scheme too.
This gives the asm-const code the basic ability to deal wiht pointer and provenances, which lays the ground work for asm_const_ptr. Note that `SymStatic` is not removed, as it supports `#[thread_local]` statics where CTFE does not.
With the previous commit, now we can see there are some code duplication for the handling of `GlobalAlloc` inside backends. Do some clean up to unify them.
CTFE pointers created via type ID, `without_provenance` or pointers to const ZSTs can now be codegenned with all 3 backends. These pointers are generated in the same way as integers.
The backend now fully supports codegen of const pointers, remove the block inside typeck behind a new feature gate. Tests are also added.
|
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. |
|
r? bjorn3 |
|
☔ The latest upstream changes (presumably #155978) made this pull request unmergeable. Please resolve the merge conflicts. |
| "asm! and global_asm! sym operands are not yet supported", | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please restore this newline.
| // current codegen unit | ||
| tcx.symbol_name(instance) | ||
| } | ||
| GlobalAlloc::Static(def_id) => { |
There was a problem hiding this comment.
What is the difference between GlobalAsmOperandRef::Const with a static and GlobalAsmOperandRef::SymStatic?
There was a problem hiding this comment.
#[thread_local] works with sym STATIC today, but it cannot be referenced in CTFE. I would actually be in favour of removing that feature.
There was a problem hiding this comment.
I would tend to agree with removing #[thread_local] support for sym STATIC. On some platforms (cough Windows cough) #[thread_local] accesses go through a function shim to ensure cross-dylib accesses work, so sym STATIC would probably already be broken for those platforms.
| span, | ||
| "asm! and global_asm! sym operands are not yet supported", | ||
| ); | ||
| } |
There was a problem hiding this comment.
This error should apply to all constants that reference a symbol.
View all comments
Implements RFC#3848 with tracking issue #128464
This adds support of const pointers for asm
constin addition to plain integers.The inline
asm!support is implemented usingiconstraint, and theglobal_asm!andnaked_asm!support is implemented by insertingsymbol + offsetand makesymbolcompiler-used. For unnamed consts, it will create additional internal & hidden symbols so that they can be referenced by global_asm.The feature is also implemented for GCC backend but it's untested.