Adds an arbitrary pseudonym to clang"s windows mangler...

[memory-thrasher]

trafficstars

…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.

https://github.com/llvm/llvm-project/issues/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.

[github-actions[bot]]

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 @ followed by their GitHub username.

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.

[llvmbot]

@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.

https://github.com/llvm/llvm-project/issues/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.

---

Full diff: https://github.com/llvm/llvm-project/pull/97792.diff

2 Files Affected:

• (modified) clang/lib/AST/MicrosoftMangle.cpp (+43-14)
• (added) clang/test/CodeGen/ms_mangler_templatearg_opte.cpp (+42)

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
+};
+

[memory-thrasher]

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]

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.

[memory-thrasher]

@bolshakov-a @eefriedman @zygoloid hoping yall are the right reviewers to tag?

[MaxEW707]

@memory-thrasher

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 -msvc triple is supposed to adhere to msvc mangling including all its bugs. I'll try to get to this PR sometime this weekend under the assumption we will use the current custom ad-hoc mangling that isn't MSVC compatible in the interim.

Funnily enough we were just discussing this here, https://github.com/llvm/llvm-project/issues/94650#issuecomment-2209626547, and I was about to write up a discourse on such a discussion tomorrow morning.

[memory-thrasher]

Godbolt for reference: https://godbolt.org/z/b9v8KhPET

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.

likely with a new triple

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:

• msvc for all non-templates, itanium for all templates. Also causes any explicit extern template instances to be ignored to prevent the already remote possibility that a specialization or instance could be provided by a library (which would of course be mangled msvc-style).
• an __attribute__ or other intrinsic to specify (in a header file) that an alternate mangler is to be used for the single connected symbol. As much as I hate the idea of compiler-specific code features, sometimes they are the right answer.

I prefer the former.

[memory-thrasher]

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.

[rnk]

I think this NTTP mangling code needs some significant updates, refactoring, and an increase in test coverage. I see cd93532dfc455255cb2fa553090d14aaa52b106b landed last May, probably when I was offline for a while.

[memory-thrasher]

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]

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.

[memory-thrasher]

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.

[bolshakov-a]

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]

Alright. Pinging the other reviewers. @rnk @tahonermann @MaxEW707 @zmodem

[memory-thrasher]

I finally got the tests to behave.

[github-actions[bot]]

@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 by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

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. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!