Adds a pseudonym to clang"s windows mangler... #97792
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 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 If you have received no comments on your PR for a week, you can request a review 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. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (memory-thrasher) Changes…to handle template argument values that are pointers one-past-the-end of a non-array symbol. Also improves error messages in other template argument scenarios where clang bails. I don't think I hooked up the unit test right. I'm not sure one is really needed for what boils down to a tweaked if statement. Please advise. Full diff: https://github.com/llvm/llvm-project/pull/97792.diff 2 Files Affected:
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index fac14ce1dce8c..f348a3a2dcce7 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -1922,9 +1922,12 @@ void MicrosoftCXXNameMangler::mangleTemplateArgValue(QualType T,
if (WithScalarType)
mangleType(T, SourceRange(), QMM_Escape);
- // We don't know how to mangle past-the-end pointers yet.
- if (V.isLValueOnePastTheEnd())
- break;
+ // msvc does not support this, so no mangling will ever be "right", so this
+ // was picked arbitrarily.
+ if (V.isLValueOnePastTheEnd()) {
+ Out << "@IN"; // for "invalid pointer" since it always is invalid.
+ return;
+ }
APValue::LValueBase Base = V.getLValueBase();
if (!V.hasLValuePath() || V.getLValuePath().empty()) {
@@ -1938,12 +1941,23 @@ void MicrosoftCXXNameMangler::mangleTemplateArgValue(QualType T,
mangleNumber(V.getLValueOffset().getQuantity());
} else if (!V.hasLValuePath()) {
// FIXME: This can only happen as an extension. Invent a mangling.
- break;
+ DiagnosticsEngine &Diags = Context.getDiags();
+ unsigned DiagID =
+ Diags.getCustomDiagID(DiagnosticsEngine::Error,
+ "cannot mangle this template argument yet "
+ "(non-null base with null lvalue path)");
+ Diags.Report(DiagID);
+ return;
} else if (auto *VD = Base.dyn_cast<const ValueDecl*>()) {
Out << "E";
mangle(VD);
} else {
- break;
+ DiagnosticsEngine &Diags = Context.getDiags();
+ unsigned DiagID = Diags.getCustomDiagID(
+ DiagnosticsEngine::Error,
+ "cannot mangle this template argument yet (empty lvalue path)");
+ Diags.Report(DiagID);
+ return;
}
} else {
if (TAK == TplArgKind::ClassNTTP && T->isPointerType())
@@ -1988,8 +2002,14 @@ void MicrosoftCXXNameMangler::mangleTemplateArgValue(QualType T,
Out << *I;
auto *VD = Base.dyn_cast<const ValueDecl*>();
- if (!VD)
- break;
+ if (!VD) {
+ DiagnosticsEngine &Diags = Context.getDiags();
+ unsigned DiagID = Diags.getCustomDiagID(
+ DiagnosticsEngine::Error,
+ "cannot mangle this template argument yet (null value decl)");
+ Diags.Report(DiagID);
+ return;
+ }
Out << (TAK == TplArgKind::ClassNTTP ? 'E' : '1');
mangle(VD);
@@ -2104,15 +2124,24 @@ void MicrosoftCXXNameMangler::mangleTemplateArgValue(QualType T,
return;
}
- case APValue::AddrLabelDiff:
- case APValue::FixedPoint:
- break;
+ case APValue::AddrLabelDiff: {
+ DiagnosticsEngine &Diags = Context.getDiags();
+ unsigned DiagID = Diags.getCustomDiagID(
+ DiagnosticsEngine::Error, "cannot mangle this template argument yet "
+ "(value type: address label diff)");
+ Diags.Report(DiagID);
+ return;
}
- DiagnosticsEngine &Diags = Context.getDiags();
- unsigned DiagID = Diags.getCustomDiagID(
- DiagnosticsEngine::Error, "cannot mangle this template argument yet");
- Diags.Report(DiagID);
+ case APValue::FixedPoint: {
+ DiagnosticsEngine &Diags = Context.getDiags();
+ unsigned DiagID = Diags.getCustomDiagID(
+ DiagnosticsEngine::Error,
+ "cannot mangle this template argument yet (value type: fixed point)");
+ Diags.Report(DiagID);
+ return;
+ }
+ }
}
void MicrosoftCXXNameMangler::mangleObjCProtocol(const ObjCProtocolDecl *PD) {
diff --git a/clang/test/CodeGen/ms_mangler_templatearg_opte.cpp b/clang/test/CodeGen/ms_mangler_templatearg_opte.cpp
new file mode 100644
index 0000000000000..852a20cfb5712
--- /dev/null
+++ b/clang/test/CodeGen/ms_mangler_templatearg_opte.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc19.33.0 -emit-llvm -std=c++20 -x c++ < %s | FileCheck --check-prefix=WIN64 %s
+
+/*
+
+this works (linux host):
+clang++ -std=c++20 -c ms_mangler_templatearg_opte.cpp -o /dev/null
+
+this dost not:
+/usr/local/bin/clang-17 -cc1 -triple x86_64-pc-windows-msvc19.33.0 -emit-obj -std=c++20 -o ms_mangler_templatearg_opte.obj -x c++ ms_mangler_templatearg_opte.cpp
+
+The first pass is fine, it's the final pass of cesum where L.data = (&ints)+1 that clang bawks at. Obviously this address can't be dereferenced, but the `if constexpr` sees to that. The unused template param should not break the mangler.
+
+*/
+
+
+typedef long long unsigned size_t;
+
+template<class T> struct llist {
+ const T* data;
+ size_t len;
+ constexpr llist(const T* data, size_t len) : data(data), len(len) {};
+ constexpr inline bool empty() const { return len == 0; };
+ constexpr llist<T> next() const { return { data+1, len-1 }; };
+ constexpr const T& peek() const { return data[0]; };
+};
+
+//recurse to iterate over the list, without the need for a terminal overload or duplicated handling of the terminal case
+template<llist<int> L> int cesum() {
+ if constexpr(L.empty()) {
+ return 0;
+ } else {
+ return L.peek() + cesum<L.next()>();
+ }
+};
+
+//constexpr int ints[] = { 1, 2, 7, 8, 9, -17, -10 }; //Note: this does NOT break the unpatched mangler
+constexpr int ints = 7;
+
+int main() {
+ return cesum<llist<int>(&ints, 1)>();//taking address of non-array
+};
+
|
|
Oh looks like that new unit test is getting run. The test execution gave me the command to run them. Working on it now. |
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
2 times, most recently
from
533d157
to
b5a18e0
Compare
|
now the name of the symbol that is being passed is included in the mangle so different value's opte pointers do not collide. Fallback to empty. |
|
@bolshakov-a @eefriedman @zygoloid |
|
Godbolt for reference: https://godbolt.org/z/b9v8KhPET I don't follow that MSVC 1920+ does not support this mangling properly. It appears that it does. I haven't dug too deep into the C++20 MSVC mangling for NTTP yet but it does appear MSVC does support this properly in its mangling scheme. I am not opposed to doing our own ad-hoc mangling for now to just get this working but in general Funnily enough we were just discussing this here, #94650 (comment), and I was about to write up a discourse on such a discussion tomorrow morning. |
Huh. Must be my vstools install on this windows laptop is out of date or broken in some other way. I could copy the scheme in that use case into my PR for at least a non-zero chance that it would match the msvc mangler in all other instances triggering that if. I must say that the thought of digging through the msvc binary to get a comprehensive handling does not feel me with excitement.
I love the idea of another triple though. I would of course be using itanium if interfacing with system resources was not impossible. I'm thinking of two possible interpretations of a hybrid triple:
I prefer the former. |
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
from
b5a18e0
to
9f909ff
Compare
|
PR updated to output mangled function name matching the one from godbolt for this specific use case. I suggest someone better at this stuff than me should eventually attempt to implement a more comprehensive mimicry. |
|
I think this NTTP mangling code needs some significant updates, refactoring, and an increase in test coverage. I see cd93532 landed last May, probably when I was offline for a while. |
Oh for sure. There are 5 other cases in template argument values alone that are still broken, I just made their error messages slightly clearer, at least as to which case is happening. I fixed one specific case I was hitting for my own sake, and am trying to share it but I have little interest in trying to fix the others (for free). |
memory-thrasher
changed the title
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
from
9f909ff
to
d47a70e
Compare
|
an unrelated test (Modules/prune.m) is now failing on my end. I'm doing a clean build because I suspect there's something stale in a cache somewhere. |
|
ya it's something with my local build env. same test is now failing when built from the commit before mine. Looks like they passed on the build server though so this should be ready. I fixed the things you mentioned @bolshakov-a . Let me know if you have a different phrasing for those diagnositcs in mind or if you want any other changes made. |
|
LGTM, but I don't have the commit access, hence cannot merge your PR. Someone closely related to the community should take a look. |
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
from
d47a70e
to
d722361
Compare
|
Alright. Pinging the other reviewers. |
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
from
d722361
to
b3de931
Compare
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
2 times, most recently
from
f0b53c5
to
81095a3
Compare
…alues that are pointers one-past-the-end of a non-array symbol. Also improves error messages in other template argument scenarios where clang bails.
memory-thrasher
force-pushed
the
users/memory-thrasher/msmangler-opte
branch
from
81095a3
to
5e5b80e
Compare
|
I finally got the tests to behave. |
|
@memory-thrasher Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Summary: …to handle template argument values that are pointers one-past-the-end of a non-array symbol. Also improves error messages in other template argument scenarios where clang bails. #97756 I don't think I hooked up the unit test right. I'm not sure one is really needed for what boils down to a tweaked if statement. Please advise. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250712
…to handle template argument values that are pointers one-past-the-end of a non-array symbol. Also improves error messages in other template argument scenarios where clang bails.
#97756
I don't think I hooked up the unit test right. I'm not sure one is really needed for what boils down to a tweaked if statement. Please advise.