[libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm #73196

aheejin · 2023-11-23T02:22:16Z

Wasm doesn't use most of that file; Wasm does not allow access of system registers and those functionalities are provided from the VM.

llvmbot · 2023-11-23T02:22:43Z

@llvm/pr-subscribers-libunwind

Author: Heejin Ahn (aheejin)

Changes

Wasm doesn't use that file; Wasm does not allow access of system registers and those functionalities are provided from the VM. Wasm only uses
https://github.com/llvm/llvm-project/blob/main/libunwind/src/Unwind-wasm.c, which implements a few interface functions.

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

1 Files Affected:

(modified) libunwind/src/libunwind.cpp (+3-2)

diff --git a/libunwind/src/libunwind.cpp b/libunwind/src/libunwind.cpp
index 1bd18659b7860c0..cd610377b63de8d 100644
--- a/libunwind/src/libunwind.cpp
+++ b/libunwind/src/libunwind.cpp
@@ -26,7 +26,7 @@
 #include <sanitizer/asan_interface.h>
 #endif
 
-#if !defined(__USING_SJLJ_EXCEPTIONS__)
+#if !defined(__USING_SJLJ_EXCEPTIONS__) || !defined(__USING_WASM_EXCEPTIONS__)
 #include "AddressSpace.hpp"
 #include "UnwindCursor.hpp"
 
@@ -347,7 +347,8 @@ void __unw_remove_dynamic_eh_frame_section(unw_word_t eh_frame_start) {
 }
 
 #endif // defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
-#endif // !defined(__USING_SJLJ_EXCEPTIONS__)
+#endif // !defined(__USING_SJLJ_EXCEPTIONS__) ||
+       // !defined(__USING_WASM_EXCEPTIONS__)
 
 #ifdef __APPLE__

arichardson

The commit message is slightly misleading, it appears there are some parts of the file that are actually used?

arichardson · 2023-11-27T20:53:00Z

The commit message is slightly misleading, it appears there are some parts of the file that are actually used?

Other than this, the change looks good to me.

aheejin · 2023-11-28T00:39:55Z

The commit message is slightly misleading, it appears there are some parts of the file that are actually used?

As the commit message says, we only use https://github.com/llvm/llvm-project/blob/main/libunwind/src/Unwind-wasm.c and not libunwind.cpp. We don't use any parts of libunwind.cpp. Do you have any suggestions on how to improve the message to make it clearer?

arichardson · 2023-11-28T15:10:24Z

The commit message is slightly misleading, it appears there are some parts of the file that are actually used?

As the commit message says, we only use https://github.com/llvm/llvm-project/blob/main/libunwind/src/Unwind-wasm.c and not libunwind.cpp. We don't use any parts of libunwind.cpp. Do you have any suggestions on how to improve the message to make it clearer?

Looking at the file, there is some code that will still be compiled for wasm (at least for debug builds). If those functions are also not used for wasm they should be ifdef'd out as well?

Commit message could be something like Omit unused parts of libunwind.cpp for WASM.

aheejin · 2023-11-28T19:19:07Z

Hmm, yes, I thought we didn't build libunwind.cpp because the code next to the part I excluded was wrapped with #ifdef __APPLE__:

llvm-project/libunwind/src/libunwind.cpp

Lines 352 to 432 in a8ac930

    
           #ifdef __APPLE__ 
        
           namespace libunwind { 
        
           static constexpr size_t MAX_DYNAMIC_UNWIND_SECTIONS_FINDERS = 8; 
        
           static RWMutex findDynamicUnwindSectionsLock; 
        
           static size_t numDynamicUnwindSectionsFinders = 0; 
        
           static unw_find_dynamic_unwind_sections 
        
               dynamicUnwindSectionsFinders[MAX_DYNAMIC_UNWIND_SECTIONS_FINDERS] = {0}; 
        
           bool findDynamicUnwindSections(void *addr, unw_dynamic_unwind_sections *info) { 
        
             bool found = false; 
        
             findDynamicUnwindSectionsLock.lock_shared(); 
        
             for (size_t i = 0; i != numDynamicUnwindSectionsFinders; ++i) { 
        
               if (dynamicUnwindSectionsFinders[i]((unw_word_t)addr, info)) { 
        
                 found = true; 
        
                 break; 
        
               } 
        
             } 
        
             findDynamicUnwindSectionsLock.unlock_shared(); 
        
             return found; 
        
           } 
        
           } // namespace libunwind 
        
           int __unw_add_find_dynamic_unwind_sections( 
        
               unw_find_dynamic_unwind_sections find_dynamic_unwind_sections) { 
        
             findDynamicUnwindSectionsLock.lock(); 
        
             // Check that we have enough space... 
        
             if (numDynamicUnwindSectionsFinders == MAX_DYNAMIC_UNWIND_SECTIONS_FINDERS) { 
        
               findDynamicUnwindSectionsLock.unlock(); 
        
               return UNW_ENOMEM; 
        
             } 
        
             // Check for value already present... 
        
             for (size_t i = 0; i != numDynamicUnwindSectionsFinders; ++i) { 
        
               if (dynamicUnwindSectionsFinders[i] == find_dynamic_unwind_sections) { 
        
                 findDynamicUnwindSectionsLock.unlock(); 
        
                 return UNW_EINVAL; 
        
               } 
        
             } 
        
             // Success -- add callback entry. 
        
             dynamicUnwindSectionsFinders[numDynamicUnwindSectionsFinders++] = 
        
               find_dynamic_unwind_sections; 
        
             findDynamicUnwindSectionsLock.unlock(); 
        
             return UNW_ESUCCESS; 
        
           } 
        
           int __unw_remove_find_dynamic_unwind_sections( 
        
               unw_find_dynamic_unwind_sections find_dynamic_unwind_sections) { 
        
             findDynamicUnwindSectionsLock.lock(); 
        
             // Find index to remove. 
        
             size_t finderIdx = numDynamicUnwindSectionsFinders; 
        
             for (size_t i = 0; i != numDynamicUnwindSectionsFinders; ++i) { 
        
               if (dynamicUnwindSectionsFinders[i] == find_dynamic_unwind_sections) { 
        
                 finderIdx = i; 
        
                 break; 
        
               } 
        
             } 
        
             // If no such registration is present then error out. 
        
             if (finderIdx == numDynamicUnwindSectionsFinders) { 
        
               findDynamicUnwindSectionsLock.unlock(); 
        
               return UNW_EINVAL; 
        
             } 
        
             // Remove entry. 
        
             for (size_t i = finderIdx; i != numDynamicUnwindSectionsFinders - 1; ++i) 
        
               dynamicUnwindSectionsFinders[i] = dynamicUnwindSectionsFinders[i + 1]; 
        
             dynamicUnwindSectionsFinders[--numDynamicUnwindSectionsFinders] = nullptr; 
        
             findDynamicUnwindSectionsLock.unlock(); 
        
             return UNW_ESUCCESS; 
        
           } 
        
           #endif // __APPLE__

But yeah then we build the logAPI part:

llvm-project/libunwind/src/libunwind.cpp

Lines 434 to 474 in a8ac930

    
           // Add logging hooks in Debug builds only 
        
           #ifndef NDEBUG 
        
           #include <stdlib.h> 
        
           _LIBUNWIND_HIDDEN 
        
           bool logAPIs() { 
        
             // do manual lock to avoid use of _cxa_guard_acquire or initializers 
        
             static bool checked = false; 
        
             static bool log = false; 
        
             if (!checked) { 
        
               log = (getenv("LIBUNWIND_PRINT_APIS") != NULL); 
        
               checked = true; 
        
             } 
        
             return log; 
        
           } 
        
           _LIBUNWIND_HIDDEN 
        
           bool logUnwinding() { 
        
             // do manual lock to avoid use of _cxa_guard_acquire or initializers 
        
             static bool checked = false; 
        
             static bool log = false; 
        
             if (!checked) { 
        
               log = (getenv("LIBUNWIND_PRINT_UNWINDING") != NULL); 
        
               checked = true; 
        
             } 
        
             return log; 
        
           } 
        
           _LIBUNWIND_HIDDEN 
        
           bool logDWARF() { 
        
             // do manual lock to avoid use of _cxa_guard_acquire or initializers 
        
             static bool checked = false; 
        
             static bool log = false; 
        
             if (!checked) { 
        
               log = (getenv("LIBUNWIND_PRINT_DWARF") != NULL); 
        
               checked = true; 
        
             } 
        
             return log; 
        
           } 
        
           #endif // NDEBUG

Will fix the message accordingly.

Wasm doesn't use most of that file; Wasm does not allow access of system registers and those functionalities are provided from the VM.

aheejin · 2023-11-30T02:03:51Z

I think the CI failures are unrelated. Merging.

trcrsired · 2023-12-13T05:28:53Z

Not working

aheejin · 2023-12-13T05:49:40Z

@trcrsired When we use it in Emscripten, we build it with -D__USING_WASM_EXCEPTIONS__. Does that work? I'm not sure which toolchain and libraries you are using though. (It looks you are using WASI, but does WASI have the EH support? Does it allow usage with llvm libunwind?)

kaz7

Please correct this code. Thank you!

kaz7 · 2024-01-15T10:42:39Z

libunwind/src/libunwind.cpp

@@ -26,7 +26,7 @@
 #include <sanitizer/asan_interface.h>
 #endif

-#if !defined(__USING_SJLJ_EXCEPTIONS__)
+#if !defined(__USING_SJLJ_EXCEPTIONS__) || !defined(__USING_WASM_EXCEPTIONS__)


This must be #if !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__USING_WASM_EXCEPTIONS__) since we don't want to compile this code for either sjlj or wasm users. not (sjlj || wasm) is equal to not(sjlj) && not(wasm) as you know...

VE use only SjLj, so it should be warnned by the buildbot for VE, but our buildbot has been stopped last 2 months. I've noticed this problem when I've been trying to recover our buildbot. ;-)

Sorry, fixed in #78230.

Thanks a lot!!

Most of the changes seem irrelevant to us given that we only use `Uniwnd-wasm.cpp`. Changes related to Wasm are what I submitted upstream for adding the file to `CMakeLists.txt` and fixing some guards: llvm/llvm-project#67770 llvm/llvm-project#73196 llvm/llvm-project#78230

aheejin requested a review from a team as a code owner November 23, 2023 02:22

llvmbot added the libunwind label Nov 23, 2023

aheejin mentioned this pull request Nov 23, 2023

How to build libunwind for wasm? #72771

Closed

ldionne approved these changes Nov 23, 2023

View reviewed changes

arichardson requested changes Nov 27, 2023

View reviewed changes

aheejin changed the title ~~[libunwind][WebAssembly] Don't build libunwind.cpp~~ [libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm Nov 28, 2023

[libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm

cb110ab

Wasm doesn't use most of that file; Wasm does not allow access of system registers and those functionalities are provided from the VM.

aheejin force-pushed the wasm_exclude_libunwind branch from f26f6ec to cb110ab Compare November 28, 2023 19:24

arichardson approved these changes Nov 28, 2023

View reviewed changes

aheejin merged commit e88a2f1 into llvm:main Nov 30, 2023
35 of 38 checks passed

aheejin deleted the wasm_exclude_libunwind branch November 30, 2023 02:04

kaz7 reviewed Jan 15, 2024

View reviewed changes

aheejin mentioned this pull request Mar 25, 2024

Update libunwind to LLVM 18.1.2 emscripten-core/emscripten#21607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm #73196

[libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm #73196

aheejin commented Nov 23, 2023 •

edited

Loading

llvmbot commented Nov 23, 2023

arichardson left a comment

arichardson commented Nov 27, 2023

aheejin commented Nov 28, 2023

arichardson commented Nov 28, 2023

aheejin commented Nov 28, 2023

aheejin commented Nov 30, 2023

trcrsired commented Dec 13, 2023

aheejin commented Dec 13, 2023

kaz7 left a comment

kaz7 Jan 15, 2024

aheejin Jan 16, 2024

kaz7 Jan 16, 2024

[libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm #73196

[libunwind][WebAssembly] Omit unused parts of libunwind.cpp for Wasm #73196

Conversation

aheejin commented Nov 23, 2023 • edited Loading

llvmbot commented Nov 23, 2023

arichardson left a comment

Choose a reason for hiding this comment

arichardson commented Nov 27, 2023

aheejin commented Nov 28, 2023

arichardson commented Nov 28, 2023

aheejin commented Nov 28, 2023

aheejin commented Nov 30, 2023

trcrsired commented Dec 13, 2023

aheejin commented Dec 13, 2023

kaz7 left a comment

Choose a reason for hiding this comment

kaz7 Jan 15, 2024

Choose a reason for hiding this comment

aheejin Jan 16, 2024

Choose a reason for hiding this comment

kaz7 Jan 16, 2024

Choose a reason for hiding this comment

aheejin commented Nov 23, 2023 •

edited

Loading