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][libcxx][libcxxabi][compiler-rt-builtins] Fix Exception Handling build for wasm #79667

Closed
wants to merge 15 commits into from

Conversation

trcrsired
Copy link

The wasm unwind build appears to be dysfunctional, likely because the author has only supplied a customized LLVM build on request, rather than a fully functional patch.

This patch fixes the build

@trcrsired trcrsired requested a review from a team as a code owner January 26, 2024 23:47
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Jan 26, 2024

@llvm/pr-subscribers-libcxx
@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libunwind

Author: cqwrteur (trcrsired)

Changes

The wasm unwind build appears to be dysfunctional, likely because the author has only supplied a customized LLVM build on request, rather than a fully functional patch.

This patch fixes the build


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

4 Files Affected:

  • (modified) libunwind/include/__libunwind_config.h (+1)
  • (modified) libunwind/src/CMakeLists.txt (+30-23)
  • (modified) libunwind/src/Unwind-wasm.c (+8-8)
  • (modified) libunwind/src/config.h (+1-1)
diff --git a/libunwind/include/__libunwind_config.h b/libunwind/include/__libunwind_config.h
index 8db336b2d727ce7..75e00a75b7e04bf 100644
--- a/libunwind/include/__libunwind_config.h
+++ b/libunwind/include/__libunwind_config.h
@@ -180,6 +180,7 @@
 #endif
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER                                      \
   _LIBUNWIND_HIGHEST_DWARF_REGISTER_LOONGARCH
+# elif defined(__wasm__)
 # else
 #  error "Unsupported architecture."
 # endif
diff --git a/libunwind/src/CMakeLists.txt b/libunwind/src/CMakeLists.txt
index 9c6f5d908b09454..cce76eea007f434 100644
--- a/libunwind/src/CMakeLists.txt
+++ b/libunwind/src/CMakeLists.txt
@@ -1,31 +1,38 @@
 # Get sources
 
-set(LIBUNWIND_CXX_SOURCES
-    libunwind.cpp
-    Unwind-EHABI.cpp
-    Unwind-seh.cpp
-    )
+if(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "wasm32" OR
+  ${CMAKE_SYSTEM_PROCESSOR} STREQUAL "wasm64")
+  set(LIBUNWIND_C_SOURCES
+      Unwind-wasm.c
+      )
+else()
+  set(LIBUNWIND_CXX_SOURCES
+      libunwind.cpp
+      Unwind-EHABI.cpp
+      Unwind-seh.cpp
+      )
+
+  if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
+    list(APPEND LIBUNWIND_CXX_SOURCES
+      Unwind_AIXExtras.cpp
+      )
+  endif()
 
-if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
-  list(APPEND LIBUNWIND_CXX_SOURCES
-    Unwind_AIXExtras.cpp
-    )
-endif()
+  set(LIBUNWIND_C_SOURCES
+      UnwindLevel1.c
+      UnwindLevel1-gcc-ext.c
+      Unwind-sjlj.c
+      )
 
-set(LIBUNWIND_C_SOURCES
-    UnwindLevel1.c
-    UnwindLevel1-gcc-ext.c
-    Unwind-sjlj.c
-    Unwind-wasm.c
-    )
-set_source_files_properties(${LIBUNWIND_C_SOURCES}
-                            PROPERTIES
-                              COMPILE_FLAGS "-std=c99")
+  set(LIBUNWIND_ASM_SOURCES
+      UnwindRegistersRestore.S
+      UnwindRegistersSave.S
+      )
 
-set(LIBUNWIND_ASM_SOURCES
-    UnwindRegistersRestore.S
-    UnwindRegistersSave.S
-    )
+  set_source_files_properties(${LIBUNWIND_C_SOURCES}
+                              PROPERTIES
+                                COMPILE_FLAGS "-std=c99")
+endif()
 
 set(LIBUNWIND_HEADERS
     AddressSpace.hpp
diff --git a/libunwind/src/Unwind-wasm.c b/libunwind/src/Unwind-wasm.c
index f7f39d38b59c181..87be87e9fd92a86 100644
--- a/libunwind/src/Unwind-wasm.c
+++ b/libunwind/src/Unwind-wasm.c
@@ -10,14 +10,11 @@
 //
 //===----------------------------------------------------------------------===//
 
+#if __STDC_VERSION__ < 202311L
 #include <stdbool.h>
-
+#endif
 #include "config.h"
-
-#ifdef __USING_WASM_EXCEPTIONS__
-
 #include "unwind.h"
-#include <threads.h>
 
 _Unwind_Reason_Code __gxx_personality_wasm0(int version, _Unwind_Action actions,
                                             uint64_t exceptionClass,
@@ -35,7 +32,12 @@ struct _Unwind_LandingPadContext {
 
 // Communication channel between compiler-generated user code and personality
 // function
-thread_local struct _Unwind_LandingPadContext __wasm_lpad_context;
+#if __STDC_VERSION__ >= 202311L
+thread_local
+#else
+_Thread_local
+#endif
+    struct _Unwind_LandingPadContext __wasm_lpad_context;
 
 /// Calls to this function is in landing pads in compiler-generated user code.
 /// In other EH schemes, stack unwinding is done by libunwind library, which
@@ -119,5 +121,3 @@ _LIBUNWIND_EXPORT uintptr_t
 _Unwind_GetRegionStart(struct _Unwind_Context *context) {
   return 0;
 }
-
-#endif // defined(__USING_WASM_EXCEPTIONS__)
diff --git a/libunwind/src/config.h b/libunwind/src/config.h
index deb5a4d4d73d467..4cc32392aa72aff 100644
--- a/libunwind/src/config.h
+++ b/libunwind/src/config.h
@@ -66,7 +66,7 @@
   #define _LIBUNWIND_EXPORT
   #define _LIBUNWIND_HIDDEN
 #else
-  #if !defined(__ELF__) && !defined(__MACH__) && !defined(_AIX)
+  #if !defined(__ELF__) && !defined(__MACH__) && !defined(_AIX) && !defined(__wasm__) 
     #define _LIBUNWIND_EXPORT __declspec(dllexport)
     #define _LIBUNWIND_HIDDEN
   #else

Copy link

github-actions bot commented Jan 26, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 32ffc9fdc2cd422c88c926b862adb3de726e3888 2b526821d4e70f3ea83f95d25f804d9fbcf82612 --extensions c,inc,cpp,h -- compiler-rt/lib/builtins/fp_compare_impl.inc libcxx/include/__exception/exception_ptr.h libcxxabi/include/cxxabi.h libcxxabi/src/cxa_exception.cpp libcxxabi/src/cxa_exception.h libunwind/include/libunwind.h libunwind/src/Unwind-wasm.c libunwind/src/assembly.h libunwind/src/cet_unwind.h libunwind/src/config.h libunwind/src/libunwind.cpp libunwind/src/libunwind_ext.h
View the diff from clang-format here.
diff --git a/libcxxabi/include/cxxabi.h b/libcxxabi/include/cxxabi.h
index cf0cc40ff4..64137957cb 100644
--- a/libcxxabi/include/cxxabi.h
+++ b/libcxxabi/include/cxxabi.h
@@ -49,8 +49,9 @@ extern "C" {
 extern _LIBCXXABI_FUNC_VIS void* __cxa_allocate_exception(size_t thrown_size) _LIBCXXABI_NOEXCEPT;
 extern _LIBCXXABI_FUNC_VIS void __cxa_free_exception(void* thrown_exception) _LIBCXXABI_NOEXCEPT;
 // This function is an LLVM extension, which mirrors the same extension in libsupc++ and libcxxrt
-extern _LIBCXXABI_FUNC_VIS __cxa_exception* __cxa_init_primary_exception(void* object, std::type_info* tinfo,
-                                                                         __libcxxabi_exception_destructor_func) _LIBCXXABI_NOEXCEPT;
+extern _LIBCXXABI_FUNC_VIS __cxa_exception*
+__cxa_init_primary_exception(void* object, std::type_info* tinfo,
+                             __libcxxabi_exception_destructor_func) _LIBCXXABI_NOEXCEPT;
 
 // 2.4.3 Throwing the Exception Object
 extern _LIBCXXABI_FUNC_VIS _LIBCXXABI_NORETURN void __cxa_throw(void* thrown_exception, std::type_info* tinfo,

@trcrsired
Copy link
Author

i do not know whether old code should be formatted or not, because the LLVM dev said do not format old code

@trcrsired
Copy link
Author

Fix partial formatting

@trcrsired trcrsired requested review from a team as code owners January 28, 2024 05:04
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc++abi libc++abi C++ Runtime Library. Not libc++. labels Jan 28, 2024
@trcrsired trcrsired changed the title [libunwind] Fix build for wasm [libunwind][libcxx][libcxxabi] Fix build for wasm Jan 28, 2024
@trcrsired trcrsired changed the title [libunwind][libcxx][libcxxabi] Fix build for wasm [libunwind][libcxx][libcxxabi] Fix Exception Handling build for wasm Jan 28, 2024
libcxx/include/__exception/exception_ptr.h Outdated Show resolved Hide resolved
libunwind/include/__libunwind_config.h Outdated Show resolved Hide resolved
libunwind/include/libunwind.h Show resolved Hide resolved
libunwind/src/Unwind-wasm.c Outdated Show resolved Hide resolved
libunwind/src/Unwind-wasm.c Outdated Show resolved Hide resolved
libunwind/src/libunwind.cpp Show resolved Hide resolved
@trcrsired trcrsired force-pushed the wasmlibunwindfix branch 2 times, most recently from de59577 to 13abdad Compare March 17, 2024 11:46
@trcrsired
Copy link
Author

@ldionne Hi. I think i have fixed as many issues as I can. can you review it again? Thank you so much!

@trcrsired trcrsired changed the title [libunwind][libcxx][libcxxabi] Fix Exception Handling build for wasm [libunwind][libcxx][libcxxabi][compiler-rt-builtins] Fix Exception Handling build for wasm Mar 18, 2024
@trcrsired trcrsired requested a review from ldionne March 18, 2024 21:00
Copy link
Member

@EricWF EricWF 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 there are some pretty non-idiomatic workarounds for WASM here. I think in general this approach could use a more structured approach to handling where WASM differs.

Also, I know we have the mandatory code-formatter now, but a lot of this diff is just formatting noise. Could we find a way to extract that out?

@@ -12,6 +12,7 @@
#ifndef __LIBUNWIND_EXT__
#define __LIBUNWIND_EXT__

#ifndef __wasm__
Copy link
Member

Choose a reason for hiding this comment

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

This feels like the wrong way to do this. We don't typically just if-def out a header for a particular platform.

Maybe track down the users of this header, and make them work under WASM?

Copy link
Author

@trcrsired trcrsired Mar 26, 2024

Choose a reason for hiding this comment

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

The problem is that WASM_EXCEPTIONS macro is not defined by clang, so using any other the detection macro without wasm will trigger ODR violation. Remote unwinding is not a thing for wasm, so i do not see what's wrong here

Copy link
Member

Choose a reason for hiding this comment

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

I would much rather you guard the usage of this header in each file.

That way it's clear to the reader in context that the include is unused under a certain configuration.
If the entire file contains no WASM code, then it probably shouldn't mention WASM at all.

Can you please explain what the __WASM_EXCEPTIONS__ or __USING_WASM_EXCEPTIONS__ was?

Copy link
Author

Choose a reason for hiding this comment

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

wasm has its own exception handling mechanism. i do not know why USING_WASM_EXCEPTION is not defined, because the author of the code did not change the upstream code correctly i guess.

@@ -12,6 +12,7 @@
#include <libunwind.h>

#include "config.h"
#ifndef __wasm__
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we simply don't want this file to build under WASM.

Copy link
Author

@trcrsired trcrsired Mar 26, 2024

Choose a reason for hiding this comment

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

then it has to change cmake file, which is annoying for users since they have to set cmake system name and cmake architecture name.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we could detect __wasm__ or similar in CMake. And it feels like the right tool, though maybe not the most trivial to implement.

Copy link
Author

Choose a reason for hiding this comment

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

I feel like we could detect __wasm__ or similar in CMake. And it feels like the right tool, though maybe not the most trivial to implement.

Tbh i would always avoid wrapping around cmake, if platform macro works out.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that it makes the file a lot harder to read, each #ifdef block enclosed within becomes harder to read and reason about its scope.

Though I see that maybe WASM uses the log functions declared at the bottom? Do you know if it does?

@@ -12,7 +12,7 @@
// functions. We need to ensure that the return value is sign-extended in the
// same way as GCC expects (since otherwise GCC-generated __builtin_isinf
// returns true for finite 128-bit floating-point numbers).
#ifdef __aarch64__
#if defined(__aarch64__) || defined(__wasm__)
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the comment above to mention why WASM needs this too?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, please leave an explicit comment here for WASM. When I ran into this issue it was really difficult to track down, so I really appreciate having explicit comments that show the rational for the special cases.

@@ -10,6 +10,7 @@
#ifndef LIBUNWIND_CET_UNWIND_H
#define LIBUNWIND_CET_UNWIND_H

#ifndef __wasm__
Copy link
Member

Choose a reason for hiding this comment

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

Again, I don't think #ifdef-ing out entire headers is the best way to do this, or is even necessary.

@@ -431,6 +432,7 @@ int __unw_remove_find_dynamic_unwind_sections(
}

#endif // __APPLE__
#endif
Copy link
Member

Choose a reason for hiding this comment

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

At minimum this needs a comment matching it to the specific block it's closing.

@@ -12,6 +12,7 @@
#include <libunwind.h>

#include "config.h"
#ifndef __wasm__
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that it makes the file a lot harder to read, each #ifdef block enclosed within becomes harder to read and reason about its scope.

Though I see that maybe WASM uses the log functions declared at the bottom? Do you know if it does?

@@ -15,6 +15,7 @@
#ifndef UNWIND_ASSEMBLY_H
#define UNWIND_ASSEMBLY_H

#ifndef __wasm__
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is not OK.

First, this file just defines macros, so even under WASM it should be safe to include.
If you're hitting the #error case in the #else, then add an #elif __wasm__.

While his style of code may not cause issues for you, because you don't use it. It makes things more complicated for every change that actually uses these files.

@@ -12,6 +12,7 @@
#ifndef __LIBUNWIND_EXT__
#define __LIBUNWIND_EXT__

#ifndef __wasm__
Copy link
Member

Choose a reason for hiding this comment

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

I would much rather you guard the usage of this header in each file.

That way it's clear to the reader in context that the include is unused under a certain configuration.
If the entire file contains no WASM code, then it probably shouldn't mention WASM at all.

Can you please explain what the __WASM_EXCEPTIONS__ or __USING_WASM_EXCEPTIONS__ was?

@@ -39,3 +40,5 @@ extern void *__libunwind_cet_get_registers(unw_cursor_t *);
extern void *__libunwind_cet_get_jump_target(void);
Copy link
Member

Choose a reason for hiding this comment

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

What is the problem with exposing these two declarations, even if they're not defined?

Copy link
Author

Choose a reason for hiding this comment

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

there are no registers. so what would you expect for cet_get_jump_target? there is no setjmp/longjmp

@trcrsired
Copy link
Author

trcrsired commented Apr 9, 2024

@trcrsired Could you please describe what bits of libunwind WASM uses? It seems to me almost all of the functionality is stripped out?

Could you also provide instructions to build and test libunwind under WASM so we could add CI coverage? And so I can test this change myself.

The only file that libunwind really needs is the Unwind-wasm.c. However it also needs the logapi from libunwind. So that is the case here. They include the header from the libunwind.h something for some macros

ok. i will add CI to libunwind. Also i think probably need to add CI for libcxxabi,libcxx too

@trcrsired trcrsired force-pushed the wasmlibunwindfix branch 2 times, most recently from f46e206 to 4f1ce89 Compare June 2, 2024 08:07
@trcrsired trcrsired requested a review from EricWF June 2, 2024 08:09
The wasm unwind build appears to be dysfunctional, likely because the author has only supplied a customized LLVM build on request, rather than a fully functional patch.

This patch fixes the build

Apply formatting patch proposed by github bot

use "" to prevent CMAKE_SYSTEM_PROCESSOR not defined

[libunwind] logAPI functions should also be built

[libcxxabi] Fix function signatures for wasm

wasm does not define the function signatures correctly for cxxabi
Fix them

Fix formatting issues for libcxxabi's wasm eh change

Merge remote-tracking branch 'parent/main' into wasmlibunwindfix

remove unwanted changes in unwind-wasm.c

Make Unwind-wasm.c compile correctly without workaround in
CMakeLists.txt

using __wasm__ macro to guard against all wasm eh build

fix UnwindLevel.c's formatting issue

ISO C requires a translation unit to contain at least one declaration [-Werror,-Wempty-translation-unit]

compiler-rt does not define CMP_RESULT correct on wasm64
Fixed

Merge code
@trcrsired
Copy link
Author

trcrsired commented Oct 1, 2024

@EricWF You need to build it with wasi-libc

@@ -29,22 +29,21 @@

namespace __cxxabiv1 {

# if defined(__wasm__)
typedef void* (*__libcpp_exception_destructor_func)(void*);
Copy link
Member

Choose a reason for hiding this comment

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

Could keep the existing comment here // In Wasm, a destructor returns its argument to explain why it is different?

extern _LIBCXXABI_FUNC_VIS _LIBCXXABI_ALWAYS_COLD void __cxa_guard_release(uint64_t *);
extern _LIBCXXABI_FUNC_VIS _LIBCXXABI_ALWAYS_COLD void __cxa_guard_abort(uint64_t *);
#endif
# if defined(_LIBCXXABI_GUARD_ABI_ARM)
Copy link
Member

Choose a reason for hiding this comment

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

There are a lot of formatting changes here that make this hard to review - could you commit the clang-format changes first and then rebase this PR on top?

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

I started reviewing this but all the unrelated formatting changes made this quite difficult. Would appreciate a minimal diff with just the changes that are needed for WASM.

Copy link
Member

@ldionne ldionne 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 support for wasm mandates a short RFC, like most projects to support new platforms that require non-trivial changes. There's not a huge amount of changes in this patch, but it's also unclear to me that this is sufficient to do anything.

In particular, the following should be covered:

  • What are the particularities of this platform, if any
  • Who is supporting this platform and what is the expected support story
  • What is the story for testing. How do you run tests on that platform? Do we need to make changes to the libc++ conformance test suite for things to work?

That would be the minimum. We have a policy of not accepting changes to add support for new platforms for platforms that are not officially supported. It would be amazing to support wasm and I'm sure it can be done, but we need to do it the right way, not only whip out a patch and call it a day.

I know we've accepted some isolated wasm changes in the past so this may sound surprising, however given the scope of the changes here I think this deserves to be done properly.

@@ -10,15 +10,13 @@
//
//===----------------------------------------------------------------------===//

// clang-format off
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Adding another newline should avoid the reordering. Alternatively, config.h should include stdbool.h to be self-contained.

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

There are tons of unrelated changes that make this really hard to review. Please do the formatting part of the changes separately and address the outstanding comments.

@arichardson
Copy link
Member

Could you address the raised issues instead of constantly updating the branch? This wastes CI resources and spams everyone subscribed to this PR. Please feel free to reopen it once you have addressed the issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt:builtins compiler-rt libc++abi libc++abi C++ Runtime Library. Not libc++. libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libunwind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants