[clang] Fix std::tm etc. mangling on Solaris #106353

rorth · 2024-08-28T09:28:38Z

Recently, Solaris bootstrap got broken because Solaris uses a non-standard mangling of std::tm and a few others. This was fixed with a hack in PR #100724. The Solaris ABI requires mangling std::tm as tm and similarly for std::div_t, std::ldiv_t, and std::lconv, which is what this patch implements. The hack needs to stay in place to allow building with older versions of clang.

Tested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11 (2-stage builds with both clang-19 and gcc-14 as build compiler), and x86_64-pc-linux-gnu.

Recently, Solaris bootstrap got broken because Solaris uses a non-standard mangling of `std::tm` and a few others. This was fixed with a hack in PR Solaris ABI requirements, mangling `std::tm` as `tm` and similarly for `std::div_t`, `std::ldiv_t`, and `std::lconv`. Tested on `amd64-pc-solaris2.11`, `sparcv9-sun-solaris2.11`, and `x86_64-pc-linux-gnu`. Unfortunately, this patch is almost there, but not quite yet: while the new testcase works as expected, the original trigger in `clang/lib/Lex/PPMacroExpansion.cpp` is now mangled into `_ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_RSt8ios_basecPK2tmPKcSA_` (`std::time_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::put(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, tm const*, char const*, char const) const`) instead of the expected `_ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_RSt8ios_basecPK2tmPKcSB_` (`std::time_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::put(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, tm const*, char const*, char const*) const`), i.e. `char const` for the final arg instead of `char const*`. I don't have the slightest idea why, unfortunately.

llvmbot · 2024-08-28T09:29:12Z

@llvm/pr-subscribers-clang

Author: Rainer Orth (rorth)

Changes

Recently, Solaris bootstrap got broken because Solaris uses a non-standard mangling of std::tm and a few others. This was fixed with a hack in PR Solaris ABI requirements, mangling std::tm as tm and similarly for std::div_t, std::ldiv_t, and std::lconv.

Tested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.

Unfortunately, this patch is almost there, but not quite yet: while the new testcase works as expected, the original trigger in clang/lib/Lex/PPMacroExpansion.cpp is now mangled into _ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_RSt8ios_basecPK2tmPKcSA_ (std::time_put<char, std::ostreambuf_iterator<char, std::char_traits<char>> >::put(std::ostreambuf_iterator<char,std::char_traits<char> >,std::ios_base&, char, tm const*, char const*, char const) const) instead of the expected
_ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_RSt8ios_basecPK2tmPKcSB_ (std::time_put<char, std::ostreambuf_iterator<char, std::char_traits<char>> >::put(std::ostreambuf_iterator<char,std::char_traits<char> >,std::ios_base&, char, tm const*, char const*, char const*) const), i.e. char const for the final arg instead of char const*. I don't have the slightest idea why, unfortunately.

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

3 Files Affected:

(modified) clang/lib/AST/ItaniumMangle.cpp (+22)
(modified) clang/lib/Lex/PPMacroExpansion.cpp (+4-2)
(added) clang/test/AST/solaris-tm.cpp (+27)

diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 976670d1efa561..12c926d589b3dc 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -38,6 +38,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/RISCVTargetParser.h"
 #include <optional>
+#include <set>
 
 using namespace clang;
 
@@ -6953,6 +6954,27 @@ bool CXXNameMangler::mangleStandardSubstitution(const NamedDecl *ND) {
     return false;
   }
 
+  if (getASTContext().getTargetInfo().getTriple().isOSSolaris()) {
+    if (const RecordDecl *RD = dyn_cast<RecordDecl>(ND)) {
+      if (!isStdNamespace(Context.getEffectiveDeclContext(RD)))
+        return false;
+
+      // Issue #33114: Need non-standard mangling of std::tm etc. for
+      // Solaris ABI compatibility.
+      static std::set<StringRef> types{"div_t", "ldiv_t", "lconv", "tm"};
+
+      // <substitution> ::= tm # ::std::tm, same for the others
+      if (const IdentifierInfo *II = RD->getIdentifier()) {
+        StringRef type = II->getName();
+        if (types.count(type)) {
+          Out << type.size() << type;
+          return true;
+        }
+      }
+      return false;
+    }
+  }
+
   return false;
 }
 
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index 1d671ab72b0c03..a8991c28f0261f 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1604,10 +1604,12 @@ static bool isTargetVariantEnvironment(const TargetInfo &TI,
   return false;
 }
 
-#if defined(__sun__) && defined(__svr4__)
+#if defined(__sun__) && defined(__svr4__) && defined(__clang__) &&             \
+    __clang__ < 20
 // GCC mangles std::tm as tm for binary compatibility on Solaris (Issue
 // #33114).  We need to match this to allow the std::put_time calls to link
-// (PR #99075).
+// (PR #99075).  clang 20 contains a fix, but the workaround is still needed
+// with older versions.
 asm("_ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_"
     "RSt8ios_basecPKSt2tmPKcSB_ = "
     "_ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_"
diff --git a/clang/test/AST/solaris-tm.cpp b/clang/test/AST/solaris-tm.cpp
new file mode 100644
index 00000000000000..f3d16b7ba1116f
--- /dev/null
+++ b/clang/test/AST/solaris-tm.cpp
@@ -0,0 +1,27 @@
+/// Check that std::tm and a few others are mangled as tm on Solaris only.
+/// Issue #33114.
+///
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple amd64-pc-solaris2.11 | FileCheck --check-prefix=CHECK-SOLARIS %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-unknown-linux-gnu  | FileCheck --check-prefix=CHECK-LINUX %s
+//
+// REQUIRES: x86-registered-target
+//
+// CHECK-SOLARIS: @_Z6tmfunc2tm
+// CHECK-SOLARIS: @_Z7ldtfunc6ldiv_t
+// CHECK-LINUX:   @_Z6tmfuncSt2tm
+// CHECK-LINUX:   @_Z7ldtfuncSt6ldiv_t
+
+namespace std {
+  extern "C" {
+    struct tm {
+      int tm_sec;
+    };
+    struct ldiv_t {
+      long quot;
+    };
+  }
+}
+
+void tmfunc (std::tm tm) {}
+
+void ldtfunc (std::ldiv_t ldt) {}

cor3ntin

Can you also add a changelog entry?
Thanks!

cor3ntin · 2024-08-28T09:39:05Z

clang/lib/AST/ItaniumMangle.cpp

+      // <substitution> ::= tm # ::std::tm, same for the others
+      if (const IdentifierInfo *II = RD->getIdentifier()) {
+        StringRef type = II->getName();
+        if (types.count(type)) {


Suggested change

if (types.count(type)) {

if (llvm::is_contained({"div_t", "ldiv_t", "lconv", "tm"}, type)) {

using a std::set is a bit overkill here

Will do: I'm pretty ignorant of C++ actually...

cor3ntin · 2024-08-28T09:39:58Z

clang/lib/Lex/PPMacroExpansion.cpp

+#if defined(__sun__) && defined(__svr4__) && defined(__clang__) &&             \
+    __clang__ < 20


Does every version of gcc we support handles that?

Certainly: the fix for GCC PR libstdc++-v3/1773 went in 13 years ago. That should be good enough ;-)

rorth · 2024-08-28T09:54:27Z

Can you also add a changelog entry? Thanks!

I've never seen them used in LLVM. Has that changed recently?

cor3ntin · 2024-08-28T13:21:46Z

Can you also add a changelog entry? Thanks!

I've never seen them used in LLVM. Has that changed recently?

Check out clang/docs/ReleaseNotes.rst

rorth · 2024-08-28T18:47:24Z

clang/docs/ReleaseNotes.rst

Ah, that. I was thinking of the GNU style ChangeLog files. Done locally; will push once the remaining mangling error has been fleshed out.

- Use llvm::is_contained.

rorth · 2024-09-27T14:59:54Z

Incorporate review comments. However, I still have a mis-mangling of the code that triggered this patch. I've now been able to create a reduced example:

namespace std {

  extern "C" {
    struct tm {
      int tm_sec;
    };
  }
}

using std::tm;

void
func (std::tm tm, const char *ccp, const char *ccp2)
{
}

On Linux/x86_64, func is mangled as _Z4funcSt2tmPKcS1_ (func(std::tm, char const*, char const*)), as expected. However, when targetting Solaris with this patch, I get instead _Z4func2tmPKcS0_ (func(tm, char const*, char const), i.e. the * has been dropped from the last arg; somehow the substitution is wrong) and I still haven't figured out what I'm doing wrong here.

Any suggestions?

efriedma-quic · 2024-09-27T20:06:25Z

The code assumes standard substitutions are not themselves substitutable. So you probably need to call mangleSubstitution/addSubstitution explicitly in the code. mangleSubstitution checks if there's an existing substitution, addSubstitution appends to the list of substitutions. See https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-compression .

Make sure you have a testcase with a function that takes two std::tm arguments.

rorth · 2024-10-01T11:12:15Z

Ah, that makes sense. I can see this when compiling my testcase for amd64-solaris or x86_64-linux.

However, I'm still having a hard time around here: a simple

          if (!mangleSubstitution(RD))
            addSubstitution(RD);

just leads to infinite recursion (obviously), but I couldn't find how to drop the std:: namespace from RD. That's me being completely ignorant of the clang frontend...

efriedma-quic · 2024-10-01T16:27:45Z

just leads to infinite recursion (obviously),

Try casting the pointer to uintptr_t?

Might be simpler to mess with CXXNameMangler::mangleUnscopedName, though, which I think is where the St prefix is getting added.

rorth · 2024-10-07T08:20:07Z

just leads to infinite recursion (obviously),

Try casting the pointer to uintptr_t?

That just created a total mess, unfortunately, like duplicating args or dropping the * from char const *.

Might be simpler to mess with CXXNameMangler::mangleUnscopedName, though, which I think is where the St prefix is getting added.

This worked seamlessly indeed. Thanks for the suggestion. I'm currently running a last round of testing and will post the working patch once I'm done.

- Extend testcase.

efriedma-quic

LGTM

rorth requested a review from efriedma-quic August 28, 2024 09:28

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 28, 2024

cor3ntin reviewed Aug 28, 2024

View reviewed changes

- Add ReleaseNotes entry.

9cdeb8a

- Use llvm::is_contained.

- Handle in CXXNameMangler::mangleUnscopedName.

b6cc034

- Extend testcase.

rorth changed the title ~~[WIP][clang] Fix std::tm etc. mangling on Solaris~~ [clang] Fix std::tm etc. mangling on Solaris Oct 7, 2024

efriedma-quic approved these changes Oct 7, 2024

View reviewed changes

rorth merged commit b3c1403 into llvm:main Oct 7, 2024
8 of 10 checks passed

	if (types.count(type)) {
	if (llvm::is_contained({"div_t", "ldiv_t", "lconv", "tm"}, type)) {

		#if defined(__sun__) && defined(__svr4__) && defined(__clang__) && \
		__clang__ < 20

[clang] Fix std::tm etc. mangling on Solaris #106353

[clang] Fix std::tm etc. mangling on Solaris #106353

Uh oh!

Conversation

rorth commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 28, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

rorth Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

rorth Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

rorth commented Aug 28, 2024

Uh oh!

cor3ntin commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rorth commented Aug 28, 2024

Uh oh!

rorth commented Sep 27, 2024

Uh oh!

efriedma-quic commented Sep 27, 2024

Uh oh!

rorth commented Oct 1, 2024

Uh oh!

efriedma-quic commented Oct 1, 2024

Uh oh!

rorth commented Oct 7, 2024

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rorth commented Aug 28, 2024 •

edited

Loading

cor3ntin commented Aug 28, 2024 •

edited

Loading