Revert "Guard unused llvm-libunwind symbols to avoid duplicates on Android"#128883
Conversation
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
There was a problem hiding this comment.
Pull request overview
This PR reverts #128667, which had guarded unused llvm-libunwind symbols with a new _LIBUNWIND_NATIVEAOT define to avoid duplicate symbol conflicts on Android (NDK r29). The revert restores the prior approach of using _LIBUNWIND_DISABLE_ZERO_COST_APIS=1 to disable the _Unwind_XXX style APIs.
Changes:
- Revert the
_LIBUNWIND_NATIVEAOTguards in the vendored llvm-libunwind sources (libunwind.cpp,Unwind-EHABI.cpp) and drop the corresponding patch entry fromllvm-libunwind-version.txt. - Restore
-D_LIBUNWIND_DISABLE_ZERO_COST_APIS=1(in place of-D_LIBUNWIND_NATIVEAOT=1) insrc/coreclr/nativeaot/Runtime/CMakeLists.txt. - Remove the
UnwindHelpers::FindUnwindSectionswrapper and calllibunwind::LocalAddressSpace::sThisAddressSpace.findUnwindSectionsdirectly fromUnixNativeCodeManager.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/external/llvm-libunwind/src/Unwind-EHABI.cpp | Remove _LIBUNWIND_NATIVEAOT guard around EHABI C++ exception dispatch functions. |
| src/native/external/llvm-libunwind/src/libunwind.cpp | Remove _LIBUNWIND_NATIVEAOT guard around the public unw_* API block. |
| src/native/external/llvm-libunwind-version.txt | Drop the reverted commit reference from the applied-patches list. |
| src/coreclr/nativeaot/Runtime/unix/UnwindHelpers.h | Remove now-unused FindUnwindSections declaration. |
| src/coreclr/nativeaot/Runtime/unix/UnwindHelpers.cpp | Remove FindUnwindSections wrapper implementation. |
| src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp | Restore direct call into LocalAddressSpace::sThisAddressSpace.findUnwindSections. |
| src/coreclr/nativeaot/Runtime/CMakeLists.txt | Replace _LIBUNWIND_NATIVEAOT define with _LIBUNWIND_DISABLE_ZERO_COST_APIS. |
|
Note This review was generated by Copilot. 🤖 Copilot Code Review — PR #128883Holistic AssessmentMotivation: Justified. PR #128667 introduced Approach: Clean revert restoring the previous Summary: ✅ LGTM. This is a straightforward, complete revert of a commit that caused a regression. All 7 changed files correspond exactly to the inverse of PR #128667. The original Android NDK r29 symbol conflict issue will need to be re-addressed in a follow-up with a different approach that doesn't break ARM unwinding. Detailed Findings✅ Correctness — Revert is complete and accurateAll changes in PR #128667 are fully reverted:
No partial revert or leftover artifacts detected. ✅ Risk Assessment — Low riskReverting to a known-good state that was stable before #128667. The only risk is re-exposing the Android NDK r29 symbol conflict, which is a pre-existing issue that needs a different fix approach.
|
Resolves #128866.
Reverts #128667