-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[llvm] Allow Rust personality name to contain arbitrary prefix #166095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-ir Author: nora (Noratrieb) ChangesLLVM needs to figure out the type of EH personality for various reasons. To do this, it currently matches against a hardcoded list of names. In Rust, we would like to mangle our personality function to better support linking multiple Rust standard libraries via staticlib. We have currently mangled all symbols except the personality, which remains unmangled because of this LLVM hardcoding. Instead, this now does a suffix match of the personality name, which will work with the mangling scheme used for these internal symbols (e.g. Full diff: https://github.com/llvm/llvm-project/pull/166095.diff 1 Files Affected:
diff --git a/llvm/lib/IR/EHPersonalities.cpp b/llvm/lib/IR/EHPersonalities.cpp
index 9297a82e7d2b0..12ae4748e1f4a 100644
--- a/llvm/lib/IR/EHPersonalities.cpp
+++ b/llvm/lib/IR/EHPersonalities.cpp
@@ -47,7 +47,8 @@ EHPersonality llvm::classifyEHPersonality(const Value *Pers) {
.Case("__C_specific_handler", EHPersonality::MSVC_TableSEH)
.Case("__CxxFrameHandler3", EHPersonality::MSVC_CXX)
.Case("ProcessCLRException", EHPersonality::CoreCLR)
- .Case("rust_eh_personality", EHPersonality::Rust)
+ // Rust mangles its personality function, so we can't test exact equality.
+ .EndsWith("rust_eh_personality", EHPersonality::Rust)
.Case("__gxx_wasm_personality_v0", EHPersonality::Wasm_CXX)
.Case("__xlcxx_personality_v1", EHPersonality::XL_CXX)
.Case("__zos_cxx_personality_v2", EHPersonality::ZOS_CXX)
@@ -77,7 +78,8 @@ StringRef llvm::getEHPersonalityName(EHPersonality Pers) {
case EHPersonality::CoreCLR:
return "ProcessCLRException";
case EHPersonality::Rust:
- return "rust_eh_personality";
+ llvm_unreachable(
+ "Cannot get personality name of Rust personality, since it is mangled");
case EHPersonality::Wasm_CXX:
return "__gxx_wasm_personality_v0";
case EHPersonality::XL_CXX:
|
|
|
010f0e0 to
f6ac569
Compare
LLVM needs to figure out the type of EH personality for various reasons. To do this, it currently matches against a hardcoded list of names. In Rust, we would like to mangle our personality function to better support linking multiple Rust standard libraries via staticlib. We have currently mangled all symbols except the personality, which remains unmangled because of this LLVM hardcoding. Instead, this now does a suffix match of the personality name, which will work with the mangling scheme used for these internal symbols (e.g. `_RNvCseCSg29WUqSe_7___rustc12___rust_alloc`).
f6ac569 to
b0f36bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ask for a test, but a unit test here feels trivial, and there is very little observable behavior hooked up to the EH personality classification routine that distinguishes between unknown EH personalities and the Rust personality. The only hit I found was in InstCombine:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp#L4672
| return "ProcessCLRException"; | ||
| case EHPersonality::Rust: | ||
| return "rust_eh_personality"; | ||
| llvm_unreachable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worried me enough to audit the callers of this function:
../llvm/include/llvm/IR/EHPersonalities.h:LLVM_ABI StringRef getEHPersonalityName(EHPersonality Pers);
../llvm/lib/IR/EHPersonalities.cpp:StringRef llvm::getEHPersonalityName(EHPersonality Pers) {
../llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp: StringRef PersName = getEHPersonalityName(EHPersonality::Wasm_CXX);
../llvm/lib/Transforms/Utils/EscapeEnumerator.cpp: return M->getOrInsertFunction(getEHPersonalityName(Pers),
The most interesting case was the EscapeEnumerator, which needs to compute the name of the default EH personality so it can insert new cleanup landingpads. Presumably the Rust EH personality will never be the platform default EH personality, so there's no correctness issue.
If we wanted to be super-safe we'd make the return value optional or return an empty string, but this seems OK to me as written.
LLVM needs to figure out the type of EH personality for various reasons. To do this, it currently matches against a hardcoded list of names. In Rust, we would like to mangle our personality function to better support linking multiple Rust standard libraries via staticlib. We have currently mangled all symbols except the personality, which remains unmangled because of this LLVM hardcoding.
Instead, this now does a suffix match of the personality name, which will work with the mangling scheme used for these internal symbols (e.g.
_RNvCseCSg29WUqSe_7___rustc12___rust_alloc).Companion Rust PR: rust-lang/rust#148413