Skip to content
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

[libunwind][AIX] Remove weak declaration "__xlcxx_personality_v0" #112436

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

xingxue-ibm
Copy link
Contributor

@xingxue-ibm xingxue-ibm commented Oct 15, 2024

__xlcxx_personality_v0 is the personality routine in libc++abi for the EH of applications generated by the legacy IBM C++ compiler. Since the EH info generated by the legacy compiler does not provide the location of the personality routine, this routine is hard-coded as the handler for legacy EH in the unwinder. The symbol is resolved dynamically using dlopen() to avoid a hard dependency of libunwind on libc++abi for cases such as non-C++ applications. The weak declaration of __xlcxx_personality_v0 was originally intended to bypass dlopen() if the C++ application generated by the legacy compiler is statically linked with the new LLVM C++ compiler. Unfortunately, this causes problems with runtime linking for Clang-compiled code using the unwinder that does not link with libc++abi.

On the other hand, the C++ runtime libraries shipped for AIX are actually stripped and statically linking is not supported. So, we can fix the problem by removing the __xlcxx_personality_v0 weak declaration. Besides, dlopen() would work as long as the libc++abi shared library is available.

@xingxue-ibm xingxue-ibm self-assigned this Oct 15, 2024
@xingxue-ibm xingxue-ibm requested a review from a team as a code owner October 15, 2024 20:51
@xingxue-ibm xingxue-ibm changed the title [libunwind][AIX] Remove weak definition "__xlcxx_personality_v0". [libunwind][AIX] Remove weak definition "__xlcxx_personality_v0" Oct 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 15, 2024

@llvm/pr-subscribers-libunwind

Author: Xing Xue (xingxue-ibm)

Changes

__xlcxx_personality_v0 is the personality routine in libc++abi for the EH of applications generated by the legacy IBM C++ compiler. Since the EH info generated by the legacy compiler does not provide the location of the personality routine, this routine is directly referenced as the handler for legacy EH in the unwinder. The symbol is resolved dynamically using dlopen() to avoid a hard dependency of libunwind on libc++abi for cases such as non-C++ applications. The weak definition __xlcxx_personality_v0 was originally intended to bypass dlopen() if the C++ application generated by the legacy compiler is statically linked with the new LLVM C++ compiler. Unfortunately this causes problems with runtime linking for Clang compiled code using the unwinder but does not link with libc++abi.

On the other hand, the C++ runtime libraries shipped for AIX are actually stripped and statically linking is not supported. So, we can fix the problem by removing the __xlcxx_personality_v0 weak definition. Besides, dlopen() would work whether it is static or dynamic linking.


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

1 Files Affected:

  • (modified) libunwind/src/UnwindCursor.hpp (+22-30)
diff --git a/libunwind/src/UnwindCursor.hpp b/libunwind/src/UnwindCursor.hpp
index ce6dced535e781..8869be5236b1e6 100644
--- a/libunwind/src/UnwindCursor.hpp
+++ b/libunwind/src/UnwindCursor.hpp
@@ -2033,7 +2033,6 @@ typedef _Unwind_Reason_Code __xlcxx_personality_v0_t(int, _Unwind_Action,
                                                      uint64_t,
                                                      _Unwind_Exception *,
                                                      struct _Unwind_Context *);
-__attribute__((__weak__)) __xlcxx_personality_v0_t __xlcxx_personality_v0;
 }
 
 static __xlcxx_personality_v0_t *xlcPersonalityV0;
@@ -2126,42 +2125,35 @@ bool UnwindCursor<A, R>::getInfoFromTBTable(pint_t pc, R &registers) {
     // function __xlcxx_personality_v0(), which is the personality for the state
     // table and is exported from libc++abi, is directly assigned as the
     // handler here. When a legacy XLC++ frame is encountered, the symbol
-    // is resolved dynamically using dlopen() to avoid hard dependency from
-    // libunwind on libc++abi.
+    // is resolved dynamically using dlopen() to avoid a hard dependency of
+    // libunwind on libc++abi in cases such as non-C++ applications.
 
     // Resolve the function pointer to the state table personality if it has
-    // not already.
+    // not already done.
     if (xlcPersonalityV0 == NULL) {
       xlcPersonalityV0InitLock.lock();
       if (xlcPersonalityV0 == NULL) {
-        // If libc++abi is statically linked in, symbol __xlcxx_personality_v0
-        // has been resolved at the link time.
-        xlcPersonalityV0 = &__xlcxx_personality_v0;
+        // Resolve __xlcxx_personality_v0 using dlopen().
+        const char libcxxabi[] = "libc++abi.a(libc++abi.so.1)";
+        void *libHandle;
+        // The AIX dlopen() sets errno to 0 when it is successful, which
+        // clobbers the value of errno from the user code. This is an AIX
+        // bug because according to POSIX it should not set errno to 0. To
+        // workaround before AIX fixes the bug, errno is saved and restored.
+        int saveErrno = errno;
+        libHandle = dlopen(libcxxabi, RTLD_MEMBER | RTLD_NOW);
+        if (libHandle == NULL) {
+          _LIBUNWIND_TRACE_UNWINDING("dlopen() failed with errno=%d\n",
+                                     errno);
+          assert(0 && "dlopen() failed");
+        }
+        xlcPersonalityV0 = reinterpret_cast<__xlcxx_personality_v0_t *>(
+            dlsym(libHandle, "__xlcxx_personality_v0"));
         if (xlcPersonalityV0 == NULL) {
-          // libc++abi is dynamically linked. Resolve __xlcxx_personality_v0
-          // using dlopen().
-          const char libcxxabi[] = "libc++abi.a(libc++abi.so.1)";
-          void *libHandle;
-          // The AIX dlopen() sets errno to 0 when it is successful, which
-          // clobbers the value of errno from the user code. This is an AIX
-          // bug because according to POSIX it should not set errno to 0. To
-          // workaround before AIX fixes the bug, errno is saved and restored.
-          int saveErrno = errno;
-          libHandle = dlopen(libcxxabi, RTLD_MEMBER | RTLD_NOW);
-          if (libHandle == NULL) {
-            _LIBUNWIND_TRACE_UNWINDING("dlopen() failed with errno=%d\n",
-                                       errno);
-            assert(0 && "dlopen() failed");
-          }
-          xlcPersonalityV0 = reinterpret_cast<__xlcxx_personality_v0_t *>(
-              dlsym(libHandle, "__xlcxx_personality_v0"));
-          if (xlcPersonalityV0 == NULL) {
-            _LIBUNWIND_TRACE_UNWINDING("dlsym() failed with errno=%d\n", errno);
-            assert(0 && "dlsym() failed");
-          }
-          dlclose(libHandle);
-          errno = saveErrno;
+          _LIBUNWIND_TRACE_UNWINDING("dlsym() failed with errno=%d\n", errno);
+          assert(0 && "dlsym() failed");
         }
+        errno = saveErrno;
       }
       xlcPersonalityV0InitLock.unlock();
     }

Copy link

github-actions bot commented Oct 15, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@hubert-reinterpretcast hubert-reinterpretcast left a comment

Choose a reason for hiding this comment

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

I think a small test case could be added with a int main(void) {} C program linked with -brtl and LDR_PRELOAD/LDR_PRELOAD64 set to libunwind.a(libunwind.so.1).

libunwind/src/UnwindCursor.hpp Outdated Show resolved Hide resolved
libunwind/src/UnwindCursor.hpp Outdated Show resolved Hide resolved
libunwind/src/UnwindCursor.hpp Show resolved Hide resolved
@hubert-reinterpretcast hubert-reinterpretcast changed the title [libunwind][AIX] Remove weak definition "__xlcxx_personality_v0" [libunwind][AIX] Remove weak declaration "__xlcxx_personality_v0" Oct 15, 2024
@hubert-reinterpretcast
Copy link
Collaborator

@xingxue-ibm, I edited the PR description text. Please check my edits.

@xingxue-ibm
Copy link
Contributor Author

xingxue-ibm commented Oct 16, 2024

@xingxue-ibm, I edited the PR description text. Please check my edits.

Thanks for the editing, @hubert-reinterpretcast!

- fix comments
- add a LIT test case
- will put driver-by fix to remove dlclose() in a separate PR
@xingxue-ibm
Copy link
Contributor Author

I think a small test case could be added with a int main(void) {} C program linked with -brtl and LDR_PRELOAD/LDR_PRELOAD64 set to libunwind.a(libunwind.so.1).

Added a test case aix_runtime_link.pass.cpp although the test case needs to be in C. The reason for the .cpp suffix is the libunwind test configuration does not recognize .c as a suffix for test cases. Also, %{cxx} is used to compile. However, the test configuration uses -nostdlib++ -L %{lib} -lunwind -ldl -Wl,-bbigtoc as link flags so it does not link with libc++abi and the test case does fail without the change.

Copy link
Collaborator

@hubert-reinterpretcast hubert-reinterpretcast left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@xingxue-ibm
Copy link
Contributor Author

Since this is an AIX-specific issue and the fix has been approved by AIX compiler and runtime expert @hubert-reinterpretcast (thank you), I plan to land it tomorrow if there are no objections.

@xingxue-ibm xingxue-ibm merged commit 2ef24e0 into llvm:main Oct 17, 2024
61 checks passed
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 21, 2024
…vm#112436)

`__xlcxx_personality_v0` is the personality routine in `libc++abi` for
the EH of applications generated by the legacy IBM C++ compiler. Since
the EH info generated by the legacy compiler does not provide the
location of the personality routine, this routine is hard-coded as the
handler for legacy EH in the unwinder. The symbol is resolved
dynamically using `dlopen()` to avoid a hard dependency of `libunwind`
on `libc++abi` for cases such as non-C++ applications. The weak
declaration of `__xlcxx_personality_v0` was originally intended to
bypass `dlopen()` if the C++ application generated by the legacy
compiler is statically linked with the new LLVM C++ compiler.
Unfortunately, this causes problems with runtime linking for
Clang-compiled code using the unwinder that does not link with
`libc++abi`.

On the other hand, the C++ runtime libraries shipped for AIX are
actually stripped and statically linking is not supported. So, we can
fix the problem by removing the `__xlcxx_personality_v0` weak
declaration. Besides, `dlopen()` would work as long as the libc++abi
shared library is available.
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
…vm#112436)

`__xlcxx_personality_v0` is the personality routine in `libc++abi` for
the EH of applications generated by the legacy IBM C++ compiler. Since
the EH info generated by the legacy compiler does not provide the
location of the personality routine, this routine is hard-coded as the
handler for legacy EH in the unwinder. The symbol is resolved
dynamically using `dlopen()` to avoid a hard dependency of `libunwind`
on `libc++abi` for cases such as non-C++ applications. The weak
declaration of `__xlcxx_personality_v0` was originally intended to
bypass `dlopen()` if the C++ application generated by the legacy
compiler is statically linked with the new LLVM C++ compiler.
Unfortunately, this causes problems with runtime linking for
Clang-compiled code using the unwinder that does not link with
`libc++abi`.

On the other hand, the C++ runtime libraries shipped for AIX are
actually stripped and statically linking is not supported. So, we can
fix the problem by removing the `__xlcxx_personality_v0` weak
declaration. Besides, `dlopen()` would work as long as the libc++abi
shared library is available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants