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

Emit load_addr and load_size if WAMR_ENABLE_COMPILER is set #3835

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

sjamesr
Copy link
Contributor

@sjamesr sjamesr commented Oct 2, 2024

Currently, the open-source builds set WASM_ENABLE_DUMP_CALL_STACK, which causes these two fields to be emitted. They are required by aot_emit_exception.cc.

Internally at Google, we don't enable call stack dumps, so we've been using the attached patch to make sure the fields are emitted anyway.

Does it make sense to build the compiler without ENABLE_DUMP_CALL_STACK? This could just be an oversight on our part when we import WAMR.

Currently, the open-source builds set WASM_ENABLE_DUMP_CALL_STACK, which
causes these two fields to be emitted. They are required by
aot_emit_exception.cc.

Internally at Google, we don't enable call stack dumps, so we've been
using the attached patch to make sure the fields are emitted anyway.
@sjamesr sjamesr force-pushed the add_additional_check branch from 8739d35 to e9da33c Compare October 2, 2024 22:41
@loganek
Copy link
Collaborator

loganek commented Oct 4, 2024

@sjamesr could you tell a bit about your usecase? WAMR compiler usually is used as a developer tools and rarely in production, so size micro optimizations like this one is usually not a problem. You can still enable dump callstacks feature for the runtime itself though. So my recommended approach would be to:

  • compile the aot without specifying --enable-dump-call-stack
  • compile the runtime with -DWAMR_BUILD_DUMP_CALL_STACK=0 CMake flag (which is the default one IIRC).
    Would that work for you?

@sjamesr
Copy link
Contributor Author

sjamesr commented Oct 4, 2024

Currently we build the compiler as you suggest, without WASM_ENABLE_DUMP_CALL_STACK defined. The change I sent is required to get the compiler to build in this case, and we apply this patch locally before building.

Upstream, the compiler is built with the option set. If you apply the following patch to disable it, I think you'll find wamrc doesn't build. I was thinking about enabling WASM_ENABLE_DUMP_CALL_STACK for the compiler, because that's what is done upstream, but it's good to know that leaving it off should be OK.

diff --git a/wamr-compiler/CMakeLists.txt b/wamr-compiler/CMakeLists.txt
index 2ab0462b..210ff481 100644
--- a/wamr-compiler/CMakeLists.txt
+++ b/wamr-compiler/CMakeLists.txt
@@ -42,7 +42,7 @@ add_definitions(-DWASM_ENABLE_SIMD=1)
 add_definitions(-DWASM_ENABLE_REF_TYPES=1)
 add_definitions(-DWASM_ENABLE_CUSTOM_NAME_SECTION=1)
 add_definitions(-DWASM_ENABLE_AOT_STACK_FRAME=1)
-add_definitions(-DWASM_ENABLE_DUMP_CALL_STACK=1)
+add_definitions(-DWASM_ENABLE_DUMP_CALL_STACK=0)
 add_definitions(-DWASM_ENABLE_PERF_PROFILING=1)
 add_definitions(-DWASM_ENABLE_LOAD_CUSTOM_SECTION=1)
 add_definitions(-DWASM_ENABLE_MODULE_INST_CONTEXT=1)
/home/sjr/wasm-micro-runtime/core/iwasm/compilation/aot_compiler.c: In function ‘aot_gen_commit_sp_ip’:
/home/sjr/wasm-micro-runtime/core/iwasm/compilation/aot_compiler.c:666:66: error: ‘WASMModule’ has no member named ‘load_addr’
  666 |                 value = I64_CONST((uint64)(uintptr_t)(ip - module->load_addr));
      |                                                                  ^~

@loganek
Copy link
Collaborator

loganek commented Oct 4, 2024

Got it. Yeah personally I don't see why the WAMR_BUILD_DUMP_CALL_STACK flag would be disabled for the wamrc itself, ideally this should be untouched, and only define/undefined it when building the runtime.

Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit 6b4d8aa into bytecodealliance:main Oct 8, 2024
383 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants