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

Use logger for runtime prints #3097

Conversation

eloparco
Copy link
Contributor

When reporting logs from the runtime, it's better to use BH_LOG (since it can be overridden with WAMR_BH_LOG) instead of os_printf. In this way, we can distinguish runtime output from wasm execution output.

From #3070:

Also, I think there's a few places where os_printf is used in the runtime where actually we should bh_log
#3070 (comment)

Occurrences of os_printf have been replaced, apart from a few files (e.g. the platform-specific ones in core/shared/platforms/).

@eloparco eloparco force-pushed the eloparco/use-logger-for-runtime-prints branch 3 times, most recently from 50446e4 to ca9ffbe Compare January 31, 2024 17:39
@eloparco eloparco force-pushed the eloparco/use-logger-for-runtime-prints branch from ca9ffbe to 8eacc28 Compare January 31, 2024 18:15
@wenyongh
Copy link
Contributor

wenyongh commented Feb 1, 2024

@eloparco thanks for the PR, I am not sure whether it is a good practice to replace os_printf to LOG_VERBOSE, there are some concerns: (1) LOG_VERBOSE only outputs logs when -v=5 is specified for iwasm, by default there is no output, (2) LOG_VERBOSE dumps extra info, e.g. timestamp and thread id, (3) developer may be confused which function to use, LOG_VERBOSE, LOG_WARNING or LOG_ERROR?

I think we had better collect more input from other developers and check what is the best choice.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

Suggest to only change the dump of error/failure to LOG_ERROR, and leave others unchanged first?

core/iwasm/aot/aot_runtime.c Outdated Show resolved Hide resolved
@@ -376,7 +376,7 @@ runtime_exception_handler(EXCEPTION_POINTERS *exce_info)
#endif
}

os_printf("Unhandled exception thrown: exception code: 0x%lx, "
LOG_ERROR("Unhandled exception thrown: exception code: 0x%lx, "
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the output of error/exception in wasm loader/instantiation/interpreter, I think it is an important info for developer, had better not use bh_log which adds timestamp and thread id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like the output of error/exception in wasm loader/instantiation/interpreter

Which one are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant printing the error/exception in the product-mini main.c:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/product-mini/platforms/posix/main.c#L899
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/product-mini/platforms/posix/main.c#L970

They are printed with printf, I am not sure whether here we should also use printf to dump "Unhandled exception thrown: xxx" like them.

core/iwasm/common/wasm_runtime_common.c Outdated Show resolved Hide resolved
core/iwasm/compilation/aot_llvm.c Outdated Show resolved Hide resolved
core/iwasm/compilation/aot_llvm.c Outdated Show resolved Hide resolved
core/iwasm/compilation/aot_llvm.c Outdated Show resolved Hide resolved
core/iwasm/fast-jit/cg/x86-64/jit_codegen_x86_64.cpp Outdated Show resolved Hide resolved
core/iwasm/fast-jit/jit_dump.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_runtime.c Outdated Show resolved Hide resolved
@eloparco eloparco force-pushed the eloparco/use-logger-for-runtime-prints branch from 35887b9 to 5a19f05 Compare February 5, 2024 17:17
@eloparco eloparco force-pushed the eloparco/use-logger-for-runtime-prints branch from 5a19f05 to 7818444 Compare February 5, 2024 17:19
@eloparco
Copy link
Contributor Author

eloparco commented Feb 5, 2024

Suggest to only change the dump of error/failure to LOG_ERROR, and leave others unchanged first?

Yes, makes sense, I reverted all the replacements for LOG_VERBOSE.

Copy link
Contributor

@wenyongh wenyongh 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 cfa90ca into bytecodealliance:main Feb 6, 2024
395 checks passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Change runtime internal error/debug prints from using `os_printf()`
to using `LOG_ERROR()`/`LOG_DEBUG()`.
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.

2 participants