[clang][codegen] Mention the invariant that LLVM demangler should be … #117346
Conversation
…able to handle mangled names generated by clang. https://discourse.llvm.org/t/rfc-clang-diagnostic-for-demangling-failures/82835/8
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Viktoriia Bakalova (VitaNuo) Changes…able to handle mangled names generated by clang. https://discourse.llvm.org/t/rfc-clang-diagnostic-for-demangling-failures/82835/8 Since we're putting the work on the above RFC on hold, let's leave a comment in the source code pointing to prior efforts and the suggestion of further steps. Full diff: https://github.com/llvm/llvm-project/pull/117346.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index b854eeb62a80ce..6eef085ae336eb 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2047,6 +2047,13 @@ StringRef CodeGenModule::getMangledName(GlobalDecl GD) {
GD.getWithKernelReferenceKind(KernelReferenceKind::Kernel),
ND));
+ // This invariant should hold true in the future.
+ // Prior work: https://discourse.llvm.org/t/rfc-clang-diagnostic-for-demangling-failures/82835/8
+ // https://github.com/llvm/llvm-project/issues/111345
+ // assert(llvm::isMangledName(MangledName) &&
+ // llvm::demangle(MangledName) != MangledName &&
+ // "LLVM demangler must demangle clang-generated names");
+
auto Result = Manglings.insert(std::make_pair(MangledName, GD));
return MangledDeclNames[CanonicalGD] = Result.first->first();
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/CodeGen/CodeGenModule.cpp
Outdated
| // Prior work: | ||
| // https://discourse.llvm.org/t/rfc-clang-diagnostic-for-demangling-failures/82835/8 | ||
| // https://github.com/llvm/llvm-project/issues/111345 | ||
| // assert(llvm::isMangledName(MangledName) && |
There was a problem hiding this comment.
isMangledName doesn't seem to exist?
But, also, we'd only want to validate output actually produced by Clang mangleCXXName, and not e.g. a user-written void f() asm("_ZGARBAGE"); void f() {}
Maybe we want:if ((MangledName.startswith("_Z") || MangledName.startswith("?")) & !GD->hasAttr<AsmLabelAttr>())?
There was a problem hiding this comment.
No, it doesn't exist. This was to demonstrate the idea.
Thanks, sure, I can change the text.
…a mangled name is.
|
(please don't merge PRs that haven't been approved, that's against LLVM practices/policies ( https://llvm.org/docs/CodeReview.html#code-review-workflow ), unless they weren't intended for pre-commit review in the first place (if they were only meant for presubmit checks)) |
|
Yes, review comments don't indicate approval after the comments are addressed unless the reviewer explicitly says that. The reviewer may want to take a look at the updated changes, or responses to review comments, or maybe the review wasn't complete for whatever reason. That said, given it's just a comment change, I'm okay with leaving it in-tree until @jyknight follows up on the review. |
5 participants
…able to handle mangled names generated by clang.
https://discourse.llvm.org/t/rfc-clang-diagnostic-for-demangling-failures/82835/8
Since we're putting the work on the above RFC on hold, let's leave a comment in the source code pointing to prior efforts and the suggestion of further steps.