Set compile symbol visibility to hidden in cmake #3655

bnason-nf · 2024-07-22T17:47:40Z

No description provided.

CMakeLists.txt

lum1n0us · 2024-08-05T07:25:03Z

@bnason-nf @wenyongh It will fail WAMR_BUILD_SHARED enabling. might need to reconsider the PR.

$ cmake -S . -B build-shared -DWAMR_BUILD_SHARED=1

$ cmake --build build-shared

/usr/bin/ld: CMakeFiles/iwasm.dir/main.c.o: in function `libc_wasi_init':
main.c:(.text.libc_wasi_init+0x32): undefined reference to `wasm_runtime_set_wasi_args'
/usr/bin/ld: main.c:(.text.libc_wasi_init+0x4b): undefined reference to `wasm_runtime_set_wasi_addr_pool'
/usr/bin/ld: main.c:(.text.libc_wasi_init+0x66): undefined reference to `wasm_runtime_set_wasi_ns_lookup_pool'
/usr/bin/ld: CMakeFiles/iwasm.dir/main.c.o: in function `load_native_lib':
main.c:(.text.load_native_lib+0x10): undefined reference to `wasm_runtime_malloc'
/usr/bin/ld: main.c:(.text.load_native_lib+0xe2): undefined reference to `wasm_runtime_register_natives'
/usr/bin/ld: main.c:(.text.load_native_lib+0x10e): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x133): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.load_native_lib+0x183): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x1a9): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x1ce): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x1f4): undefined reference to `bh_log'
/usr/bin/ld: CMakeFiles/iwasm.dir/main.c.o: in function `main':
main.c:(.text.startup.main+0x186): undefined reference to `wasm_runtime_full_init'
/usr/bin/ld: main.c:(.text.startup.main+0x197): undefined reference to `bh_log_set_verbose_level'
/usr/bin/ld: main.c:(.text.startup.main+0x203): undefined reference to `wasm_runtime_is_xip_file'
/usr/bin/ld: main.c:(.text.startup.main+0x22c): undefined reference to `wasm_runtime_load'
/usr/bin/ld: main.c:(.text.startup.main+0x26a): undefined reference to `wasm_runtime_instantiate'
/usr/bin/ld: main.c:(.text.startup.main+0x2b0): undefined reference to `wasm_application_execute_func'
/usr/bin/ld: main.c:(.text.startup.main+0x2b8): undefined reference to `wasm_runtime_get_exception'
/usr/bin/ld: main.c:(.text.startup.main+0x2d9): undefined reference to `wasm_runtime_deinstantiate'
/usr/bin/ld: main.c:(.text.startup.main+0x2e3): undefined reference to `wasm_runtime_unload'
/usr/bin/ld: main.c:(.text.startup.main+0x2f7): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x33c): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x351): undefined reference to `wasm_runtime_unregister_natives'
/usr/bin/ld: main.c:(.text.startup.main+0x36e): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.startup.main+0x378): undefined reference to `wasm_runtime_destroy'
/usr/bin/ld: main.c:(.text.startup.main+0x53d): undefined reference to `os_mmap'
/usr/bin/ld: main.c:(.text.startup.main+0x55e): undefined reference to `b_memcpy_s'
/usr/bin/ld: main.c:(.text.startup.main+0x566): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x6d5): undefined reference to `wasm_application_execute_func'
/usr/bin/ld: main.c:(.text.startup.main+0x6dd): undefined reference to `wasm_runtime_get_exception'
/usr/bin/ld: main.c:(.text.startup.main+0x6f9): undefined reference to `os_munmap'
/usr/bin/ld: main.c:(.text.startup.main+0x75c): undefined reference to `wasm_runtime_get_wasi_exit_code'
/usr/bin/ld: main.c:(.text.startup.main+0x7ef): undefined reference to `wasm_application_execute_main'
/usr/bin/ld: main.c:(.text.startup.main+0x80d): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x84d): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.startup.main+0x884): undefined reference to `wasm_runtime_get_version'
/usr/bin/ld: CMakeFiles/iwasm.dir/workspaces/wasm-micro-runtime/core/shared/utils/uncommon/bh_read_file.c.o: in function `bh_read_file_to_buffer':
bh_read_file.c:(.text.bh_read_file_to_buffer+0x62): undefined reference to `wasm_runtime_malloc'
/usr/bin/ld: bh_read_file.c:(.text.bh_read_file_to_buffer+0x113): undefined reference to `wasm_runtime_free'
collect2: error: ld returned 1 exit status
gmake[2]: *** [CMakeFiles/iwasm.dir/build.make:134: iwasm] Error 1
gmake[2]: Leaving directory '/workspaces/wasm-micro-runtime/product-mini/platforms/linux/build-shared'
gmake[1]: *** [CMakeFiles/Makefile2:99: CMakeFiles/iwasm.dir/all] Error 2
gmake[1]: Leaving directory '/workspaces/wasm-micro-runtime/product-mini/platforms/linux/build-shared'
gmake: *** [Makefile:149: all] Error 2

yamt · 2024-08-05T07:58:57Z

@bnason-nf @wenyongh It will fail WAMR_BUILD_SHARED enabling. might need to reconsider the PR.

$ cmake -S . -B build-shared -DWAMR_BUILD_SHARED=1

$ cmake --build build-shared

/usr/bin/ld: CMakeFiles/iwasm.dir/main.c.o: in function `libc_wasi_init':
main.c:(.text.libc_wasi_init+0x32): undefined reference to `wasm_runtime_set_wasi_args'
/usr/bin/ld: main.c:(.text.libc_wasi_init+0x4b): undefined reference to `wasm_runtime_set_wasi_addr_pool'
/usr/bin/ld: main.c:(.text.libc_wasi_init+0x66): undefined reference to `wasm_runtime_set_wasi_ns_lookup_pool'
/usr/bin/ld: CMakeFiles/iwasm.dir/main.c.o: in function `load_native_lib':
main.c:(.text.load_native_lib+0x10): undefined reference to `wasm_runtime_malloc'
/usr/bin/ld: main.c:(.text.load_native_lib+0xe2): undefined reference to `wasm_runtime_register_natives'
/usr/bin/ld: main.c:(.text.load_native_lib+0x10e): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x133): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.load_native_lib+0x183): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x1a9): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x1ce): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.load_native_lib+0x1f4): undefined reference to `bh_log'
/usr/bin/ld: CMakeFiles/iwasm.dir/main.c.o: in function `main':
main.c:(.text.startup.main+0x186): undefined reference to `wasm_runtime_full_init'
/usr/bin/ld: main.c:(.text.startup.main+0x197): undefined reference to `bh_log_set_verbose_level'
/usr/bin/ld: main.c:(.text.startup.main+0x203): undefined reference to `wasm_runtime_is_xip_file'
/usr/bin/ld: main.c:(.text.startup.main+0x22c): undefined reference to `wasm_runtime_load'
/usr/bin/ld: main.c:(.text.startup.main+0x26a): undefined reference to `wasm_runtime_instantiate'
/usr/bin/ld: main.c:(.text.startup.main+0x2b0): undefined reference to `wasm_application_execute_func'
/usr/bin/ld: main.c:(.text.startup.main+0x2b8): undefined reference to `wasm_runtime_get_exception'
/usr/bin/ld: main.c:(.text.startup.main+0x2d9): undefined reference to `wasm_runtime_deinstantiate'
/usr/bin/ld: main.c:(.text.startup.main+0x2e3): undefined reference to `wasm_runtime_unload'
/usr/bin/ld: main.c:(.text.startup.main+0x2f7): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x33c): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x351): undefined reference to `wasm_runtime_unregister_natives'
/usr/bin/ld: main.c:(.text.startup.main+0x36e): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.startup.main+0x378): undefined reference to `wasm_runtime_destroy'
/usr/bin/ld: main.c:(.text.startup.main+0x53d): undefined reference to `os_mmap'
/usr/bin/ld: main.c:(.text.startup.main+0x55e): undefined reference to `b_memcpy_s'
/usr/bin/ld: main.c:(.text.startup.main+0x566): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x6d5): undefined reference to `wasm_application_execute_func'
/usr/bin/ld: main.c:(.text.startup.main+0x6dd): undefined reference to `wasm_runtime_get_exception'
/usr/bin/ld: main.c:(.text.startup.main+0x6f9): undefined reference to `os_munmap'
/usr/bin/ld: main.c:(.text.startup.main+0x75c): undefined reference to `wasm_runtime_get_wasi_exit_code'
/usr/bin/ld: main.c:(.text.startup.main+0x7ef): undefined reference to `wasm_application_execute_main'
/usr/bin/ld: main.c:(.text.startup.main+0x80d): undefined reference to `wasm_runtime_free'
/usr/bin/ld: main.c:(.text.startup.main+0x84d): undefined reference to `bh_log'
/usr/bin/ld: main.c:(.text.startup.main+0x884): undefined reference to `wasm_runtime_get_version'
/usr/bin/ld: CMakeFiles/iwasm.dir/workspaces/wasm-micro-runtime/core/shared/utils/uncommon/bh_read_file.c.o: in function `bh_read_file_to_buffer':
bh_read_file.c:(.text.bh_read_file_to_buffer+0x62): undefined reference to `wasm_runtime_malloc'
/usr/bin/ld: bh_read_file.c:(.text.bh_read_file_to_buffer+0x113): undefined reference to `wasm_runtime_free'
collect2: error: ld returned 1 exit status
gmake[2]: *** [CMakeFiles/iwasm.dir/build.make:134: iwasm] Error 1
gmake[2]: Leaving directory '/workspaces/wasm-micro-runtime/product-mini/platforms/linux/build-shared'
gmake[1]: *** [CMakeFiles/Makefile2:99: CMakeFiles/iwasm.dir/all] Error 2
gmake[1]: Leaving directory '/workspaces/wasm-micro-runtime/product-mini/platforms/linux/build-shared'
gmake: *** [Makefile:149: all] Error 2

i guess you need attribute((visibility("default"))) for WASM_RUNTIME_API_EXTERN.
i'm not sure about bh_log.

@bnason-nf have you tested your change at all?

bnason-nf · 2024-08-05T16:48:17Z

Hi @yamt, thanks for finding this, I wasn't aware of it because I almost exclusively use WAMR in static build mode. It looks like the fix might be pretty simple:

diff --git a/core/iwasm/include/wasm_export.h b/core/iwasm/include/wasm_export.h
index 569c4dea..b7284761 100644
--- a/core/iwasm/include/wasm_export.h
+++ b/core/iwasm/include/wasm_export.h
@@ -24,7 +24,7 @@
 #define WASM_RUNTIME_API_EXTERN __declspec(dllimport)
 #endif
 #else
-#define WASM_RUNTIME_API_EXTERN
+#define WASM_RUNTIME_API_EXTERN __attribute__((visibility("default")))
 #endif
 #endif

However, that still leaves these functions undefined: b_memcpy_s, bh_log, os_mmap, os_munmap. We could change the visibility of those also, but I'm wondering if it would be better to change main.cpp of the product-mini application to not call those directly? Do we want those functions (or other similar ones) exposed across the library boundary?

yamt · 2024-08-06T00:39:40Z

I wasn't aware of it because I almost exclusively use WAMR in static build mode.

why do you care the visibility setting then?

However, that still leaves these functions undefined: b_memcpy_s, bh_log, os_mmap, os_munmap. We could change the visibility of those also, but I'm wondering if it would be better to change main.cpp of the product-mini application to not call those directly? Do we want those functions (or other similar ones) exposed across the library boundary?

i don't have an answer.
i wonder how it's handled for windows.
@wenyongh do you know the policy?

bnason-nf · 2024-08-06T01:01:37Z

The situation I encountered where this mattered was when linking wamr into a shared library. This shared library had default visibility, which then exposed all of the wamr functions to users of the shared library. In this particular application, there was another shared library that was also exposing some of the same functions that wamr was exposing, such as os_malloc(). This was causing conflicting allocators and resulted in a crash.

lum1n0us · 2024-08-06T02:08:26Z

I guess there are two cases need to handle differently.

a library for iwasm. It is where above failure happens. In this case, iwasm(posix/main.c) is linked with libiwasm.so and it requires not just export functions in wasm_export.h but also some internal shared functions(like b_memcpy_s, bh_log and so on). It requires default visibility.

 174   │ target_link_libraries(iwasm
 175 ~ │   PRIVATE
 176 ~ │     $<$<BOOL:${WAMR_BUILD_SHARED}>:libiwasm> $<$<NOT:$<BOOL:${WAMR_BUILD_SHARED}>>:vmlib>
 177 ~ │     ${LLVM_AVAILABLE_LIBS}
 178 ~ │     ${UV_A_LIBS}
 179 ~ │     -lm
 180 ~ │     -ldl
 181 ~ │     -lpthread
 182   │ )

a library for a project(embedded). In this case, only APIs in wasm_export.h should be exposed. Other symbols should be hidden.

Therefore, a suggestion is:

keep the change in CMakeLists.txt at the root, but revert the change in product-mini/platforms/linux/CMakeLists.txt. And suggest people to use CMakeLists.txt at the root to integrate WAMR.

bnason-nf · 2024-08-06T18:11:31Z

I'm fine with that. @wenyongh are you ok with that suggestion? You requested the product-mini change:

#3655 (comment)

wenyongh · 2024-08-06T23:54:51Z

I wasn't aware of it because I almost exclusively use WAMR in static build mode.

why do you care the visibility setting then?

However, that still leaves these functions undefined: b_memcpy_s, bh_log, os_mmap, os_munmap. We could change the visibility of those also, but I'm wondering if it would be better to change main.cpp of the product-mini application to not call those directly? Do we want those functions (or other similar ones) exposed across the library boundary?

i don't have an answer. i wonder how it's handled for windows. @wenyongh do you know the policy?

The main.c of windows also calls these APIs. I think they are part of APIs in the core/shared/platform/utils and core/shared/platform/include. We haven't discussed whether to expose these APIs in the shared lib before. It is difficult to avoid calling these APIs in product-mini's main.c, maybe we can also expose part of these APIs e.g., in wasm_export.h or another header file, for easier integration?

But per @lum1n0us's suggestion, we had better revert CMakeLists.txt in product-mini first.

wenyongh · 2024-08-06T23:56:18Z

I'm fine with that. @wenyongh are you ok with that suggestion? You requested the product-mini change:

#3655 (comment)

Agree, let's revert CMakeLists.txt in product-mini first.

Reverts commit 6d8d60d: "Set linux iwasm default visibility to hidden also" Addresses comments for pull request bytecodealliance#3655.

bnason-nf · 2024-08-07T01:27:59Z

Thanks, the pull request for this is #3691.

yamt · 2024-08-07T05:12:30Z

The main.c of windows also calls these APIs.

how does it work w/o dllexport on windows?

wenyongh · 2024-08-07T05:30:56Z

The main.c of windows also calls these APIs.

how does it work w/o dllexport on windows?

The windows product-mini's iwasm links libvmlib.a instead of libiwasm.dll. The APIs in core/shared are not exported in libiwasm.dll, I used dumpbin.exe /EXPORTS libiwasm.dll to view the exported APIs, it seems that only APIs in wasm_export.h and wasm_c_api.h are exported.

yamt · 2024-08-07T05:45:58Z

The main.c of windows also calls these APIs.

how does it work w/o dllexport on windows?

The windows product-mini's iwasm links libvmlib.a instead of libiwasm.dll. The APIs in core/shared are not exported in libiwasm.dll, I used dumpbin.exe /EXPORTS libiwasm.dll to view the exported APIs, it seems that only APIs in wasm_export.h and wasm_c_api.h are exported.

ok. thank you for explanation.

Revert commit 6d8d60d: "Set linux iwasm default visibility to hidden also" Address comments for pull request #3655.

This was originally fixed in bytecodealliance#3655, but regressed in bytecodealliance#3762 which removed the -fvisibility=hidden flag from the CMakeLists.txt file. This also removes extraneous ending whitespace from the file.

This was originally fixed in #3655, but regressed in #3762 which removed the `-fvisibility=hidden` flag from the CMakeLists.txt file. This also removes extraneous ending whitespace from the file.

Set compile symbol visibility to hidden in cmake

ab76127

wenyongh reviewed Jul 23, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

wenyongh approved these changes Jul 23, 2024

View reviewed changes

Set linux iwasm default visibility to hidden also

6d8d60d

wenyongh merged commit 5be8f35 into bytecodealliance:main Jul 24, 2024
379 checks passed

bnason-nf deleted the default-visibility-hidden branch July 24, 2024 16:47

yamt mentioned this pull request Aug 5, 2024

CI doesn't cover shared library #3687

Open

bnason-nf added a commit to bnason-nf/wasm-micro-runtime that referenced this pull request Aug 7, 2024

Restore linux iwasm default visibility

ab65047

Reverts commit 6d8d60d: "Set linux iwasm default visibility to hidden also" Addresses comments for pull request bytecodealliance#3655.

bnason-nf mentioned this pull request Aug 7, 2024

Restore linux iwasm default visibility #3691

Merged

wenyongh pushed a commit that referenced this pull request Aug 7, 2024

Restore linux iwasm default visibility (#3691)

1362a30

Revert commit 6d8d60d: "Set linux iwasm default visibility to hidden also" Address comments for pull request #3655.

bnason-nf mentioned this pull request Sep 16, 2024

Restore cmake hidden compile symbol visibility #3796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set compile symbol visibility to hidden in cmake #3655

Set compile symbol visibility to hidden in cmake #3655

bnason-nf commented Jul 22, 2024

lum1n0us commented Aug 5, 2024

yamt commented Aug 5, 2024

bnason-nf commented Aug 5, 2024

yamt commented Aug 6, 2024

bnason-nf commented Aug 6, 2024

lum1n0us commented Aug 6, 2024 •

edited

Loading

bnason-nf commented Aug 6, 2024

wenyongh commented Aug 6, 2024

wenyongh commented Aug 6, 2024

bnason-nf commented Aug 7, 2024

yamt commented Aug 7, 2024

wenyongh commented Aug 7, 2024

yamt commented Aug 7, 2024

Set compile symbol visibility to hidden in cmake #3655

Set compile symbol visibility to hidden in cmake #3655

Conversation

bnason-nf commented Jul 22, 2024

lum1n0us commented Aug 5, 2024

yamt commented Aug 5, 2024

bnason-nf commented Aug 5, 2024

yamt commented Aug 6, 2024

bnason-nf commented Aug 6, 2024

lum1n0us commented Aug 6, 2024 • edited Loading

bnason-nf commented Aug 6, 2024

wenyongh commented Aug 6, 2024

wenyongh commented Aug 6, 2024

bnason-nf commented Aug 7, 2024

yamt commented Aug 7, 2024

wenyongh commented Aug 7, 2024

yamt commented Aug 7, 2024

lum1n0us commented Aug 6, 2024 •

edited

Loading