Skip to content

[libc++abi][AIX] Use different function pointer types for destructors with 1 or 2 args #89624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 6, 2024

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