Skip to content

Conversation

xingxue-ibm
Copy link
Contributor

The destructors generated by the legacy IBM xlclang++ compiler can take 1 or 2 arguments and the differences were handled by type cast where it is needed. Clang now treats the cast here as an error after 999d4f8 landed with -Xextra -Werror. The issue had been worked around by using #pragma GCC diagnostic push/pop. This patch defines 2 separate destructor types for 1 argument and 2 arguments respectively so cast is not needed.

@xingxue-ibm xingxue-ibm added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Apr 22, 2024
@xingxue-ibm xingxue-ibm requested a review from daltenty April 22, 2024 16:35
@xingxue-ibm xingxue-ibm self-assigned this Apr 22, 2024
@xingxue-ibm xingxue-ibm requested a review from a team as a code owner April 22, 2024 16:35
@llvmbot
Copy link
Member

llvmbot commented Apr 22, 2024

@llvm/pr-subscribers-libcxxabi

Author: Xing Xue (xingxue-ibm)

Changes

The destructors generated by the legacy IBM xlclang++ compiler can take 1 or 2 arguments and the differences were handled by type cast where it is needed. Clang now treats the cast here as an error after 999d4f8 landed with -Xextra -Werror. The issue had been worked around by using #pragma GCC diagnostic push/pop. This patch defines 2 separate destructor types for 1 argument and 2 arguments respectively so cast is not needed.


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

1 Files Affected:

  • (modified) libcxxabi/src/aix_state_tab_eh.inc (+7-12)
diff --git a/libcxxabi/src/aix_state_tab_eh.inc b/libcxxabi/src/aix_state_tab_eh.inc
index 9f46001b020906..7a0d03fd59dbea 100644
--- a/libcxxabi/src/aix_state_tab_eh.inc
+++ b/libcxxabi/src/aix_state_tab_eh.inc
@@ -102,8 +102,6 @@ static bool state_tab_dbg() {
 
 namespace __state_table_eh {
 
-using destruct_f = void (*)(void*);
-
 // Definition of flags for the state table entry field 'action flag'.
 enum FSMEntryCount : intptr_t { beginCatch = -1, endCatch = -2, deleteObject = -3, cleanupLabel = -4, terminate = -5 };
 
@@ -145,8 +143,10 @@ struct FSMEntry {
     intptr_t nextStatePtr;
   };
   union {
-    // Address of the destructor function.
-    void (*destructor)(void*, size_t);
+    // Address of the destructor function with 1 argument.
+    void (*destructor)(void*);
+    // Address of the destructor function with 2 arguments.
+    void (*destructor2)(void*, size_t);
     // The address of the catch block or cleanup code.
     void* landingPad;
   };
@@ -191,17 +191,12 @@ static void invoke_destructor(FSMEntry* fsmEntry, void* addr) {
   try {
     if (fsmEntry->elementCount == 1) {
       _LIBCXXABI_TRACE_STATETAB0("calling scalar destructor\n");
-      (*fsmEntry->destructor)(addr, dtorArgument);
+      (*fsmEntry->destructor2)(addr, dtorArgument);
       _LIBCXXABI_TRACE_STATETAB0("returned from scalar destructor\n");
     } else {
       _LIBCXXABI_TRACE_STATETAB0("calling vector destructor\n");
-      // TODO: in the legacy ABI, destructors had a second argument. We don't expect to encounter
-      // destructors of this type in the itanium-based ABI, so this should be safe, but this could use some cleanup.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wcast-function-type"
       __cxa_vec_cleanup(addr, reinterpret_cast<size_t>(fsmEntry->elementCount), fsmEntry->elemSize,
-                        reinterpret_cast<destruct_f>(fsmEntry->destructor));
-#pragma GCC diagnostic pop
+                        fsmEntry->destructor);
       _LIBCXXABI_TRACE_STATETAB0("returned from vector destructor\n");
     }
   } catch (...) {
@@ -218,7 +213,7 @@ static void invoke_delete(FSMEntry* fsmEntry, void* addr) {
   try {
     _LIBCXXABI_TRACE_STATETAB0("..calling delete()\n");
     // 'destructor' holds a function pointer to delete().
-    (*fsmEntry->destructor)(objectAddress, fsmEntry->elemSize);
+    (*fsmEntry->destructor2)(objectAddress, fsmEntry->elemSize);
     _LIBCXXABI_TRACE_STATETAB0("..returned from delete()\n");
   } catch (...) {
     _LIBCXXABI_TRACE_STATETAB0("Uncaught exception in delete(), terminating\n");

Copy link
Member

@daltenty daltenty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit about naming

- rename destructor2 as xlCDestructor
@xingxue-ibm xingxue-ibm merged commit 4721490 into llvm:main May 6, 2024
@xingxue-ibm xingxue-ibm deleted the dtor-func-types branch May 24, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants