[libc++abi] Refactor exception type demangling into a separate function

[ldionne]

Summary

As a fly-by fix, also let __cxa_demangle allocate its buffer alone,
since we are not allowed to pass a non-malloc'd buffer to it.

[urnathan]

libcxxabi/src/cxa_default_handlers.cpp

60–61

This isn't code you're changing the meaning of, but this is broken. the buffer passed to __cxa_demangle must come from malloc. If ever a thrown type demangles to more than 1024 chars, people are going to be sad.

[urnathan]

libcxxabi/src/cxa_default_handlers.cpp

32–34

if (auto *res = ...)
  return res;
else
  return str;

? No need to pass in status -- we don't care why it can't demangle.

[ldionne]

Address review comment.

libcxxabi/src/cxa_default_handlers.cpp

60–61

Thank you, that is an excellent observation.

[ldionne]

Address additional review comment.

libcxxabi/src/cxa_default_handlers.cpp

32–34

I updated before seeing this comment, but yes, I agree, I can drop status as well.

[ldionne]

Fix CI with exceptions disabled.

[urnathan]

libcxxabi/src/cxa_default_handlers.cpp

60–61

thanks, but you're now going to leak. you need something like:

if (name != thrown_type->name())
  std::free (name);

somewhere. (I'm going to presume the optimizer can spot that's always false in the LIBCXXABI_NON_DEMANGLING_TERMINATE case).

N.B. our demangler calls std::terminate in the event of memory exhaustion, rather than return null. I'm slowly trying to resolve that problem.

[ldionne]

Never leak!

libcxxabi/src/cxa_default_handlers.cpp

60–61

I know we're leaking, but I was thinking it didn't matter because we were terminating anyways. This is actually what we were doing previously if the demangled name was more than 1024 characters long. __cxa_demangle would realloc and the resulting pointer would be leaked (assuming realloc doesn't try to free if the given buffer wasn't allocated with malloc).

But hey, I can fix this easily with unique_ptr anyways.

[urnathan]

libcxxabi/src/cxa_default_handlers.cpp

60–61

oh, good point. maybe a comment about leak don't care?

[ldionne]

libcxxabi/src/cxa_default_handlers.cpp

60–61

There was a comment before above the demangle() function, but I removed it since I'm not leaking anymore (I use std::unique_ptr). Please LMK if that looks OK to you.

[urnathan]

libcxxabi/src/cxa_default_handlers.cpp

60–61

This is fine. Sorry for not noticing that leaking doesn't matter and causing unnecessary rework. I hope the rework is not bloaty.

[ldionne]

Looks like @urnathan is OK with the patch now (thanks for the review!).

The AIX CI has been failing due to unrelated reasons, so ignoring that.

libcxxabi/src/cxa_default_handlers.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //
 // This file implements the default terminate_handler, unexpected_handler and
 // new_handler.
 //===----------------------------------------------------------------------===//
 
 #include <exception>
+#include <memory>
 #include <stdlib.h>
 #include "abort_message.h"
 #include "cxxabi.h"
 #include "cxa_handlers.h"
 #include "cxa_exception.h"
 #include "private_typeinfo.h"
 #include "include/atomic_support.h" // from libc++
 
 #if !defined(LIBCXXABI_SILENT_TERMINATE)
 
 static constinit const char* cause = "uncaught";
 
+#ifndef _LIBCXXABI_NO_EXCEPTIONS
+// Demangle the given string, or return the string as-is in case of an error.
+static std::unique_ptr<char const, void (*)(char const*)> demangle(char const* str)
+{
+#if !defined(LIBCXXABI_NON_DEMANGLING_TERMINATE)
+    if (const char* result = __cxxabiv1::__cxa_demangle(str, nullptr, nullptr, nullptr))
+        return {result, [](char const* p) { std::free(const_cast<char*>(p)); }};
+#endif
+    return {str, [](char const*) { /* nothing to free */ }};
+}
+#endif
+
 __attribute__((noreturn))
 static void demangling_terminate_handler()
 {
 #ifndef _LIBCXXABI_NO_EXCEPTIONS
     // If there might be an uncaught exception
     using namespace __cxxabiv1;
     __cxa_eh_globals* globals = __cxa_get_globals_fast();
     if (globals)
     {
         __cxa_exception* exception_header = globals->caughtExceptions;
         // If there is an uncaught exception
         if (exception_header)
         {
             _Unwind_Exception* unwind_exception =
                 reinterpret_cast<_Unwind_Exception*>(exception_header + 1) - 1;
             if (__isOurExceptionClass(unwind_exception))
             {
                 void* thrown_object =
                     __getExceptionClass(unwind_exception) == kOurDependentExceptionClass ?
                         ((__cxa_dependent_exception*)exception_header)->primaryException :
                         exception_header + 1;
                 const __shim_type_info* thrown_type =
                     static_cast<const __shim_type_info*>(exception_header->exceptionType);
-#if !defined(LIBCXXABI_NON_DEMANGLING_TERMINATE)
-                // Try to get demangled name of thrown_type
-                int status;
-                char buf[1024];
-                size_t len = sizeof(buf);
-                const char* name = __cxa_demangle(thrown_type->name(), buf, &len, &status);
-                if (status != 0)
-                    name = thrown_type->name();
-#else
-                const char* name = thrown_type->name();
-#endif
+                auto name = demangle(thrown_type->name());
                 // If the uncaught exception can be caught with std::exception&
                 const __shim_type_info* catch_type =
                     static_cast<const __shim_type_info*>(&typeid(std::exception));
                 if (catch_type->can_catch(thrown_type, thrown_object))
                 {
                     // Include the what() message from the exception
                     const std::exception* e = static_cast<const std::exception*>(thrown_object);
                     abort_message("terminating due to %s exception of type %s: %s",
-                                  cause, name, e->what());
+                                  cause, name.get(), e->what());
                 }
                 else
                     // Else just note that we're terminating due to an exception
                     abort_message("terminating due to %s exception of type %s",
-                                   cause, name);
+                                   cause, name.get());
             }
             else
                 // Else we're terminating due to a foreign exception
                 abort_message("terminating due to %s foreign exception", cause);
         }
     }
 #endif
     // Else just note that we're terminating