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.

Sign up for GitHub

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

Jump to bottom

WASI: enable uvwasi debug logging in debug mode #310

Merged
merged 1 commit into from
Dec 9, 2024
Merged

WASI: enable uvwasi debug logging in debug mode #310

merged 1 commit into from
Dec 9, 2024

Conversation

matetokodi
Copy link
Contributor

No description provided.

zherczeg
zherczeg reviewed Dec 3, 2024
View reviewed changes
third_party/uvwasi/CMakeLists.txt Outdated
@@ -42,6 +42,10 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
list(APPEND uvwasi_defines _GNU_SOURCE _POSIX_C_SOURCE=200112)
endif()

if (WALRUS_MODE STREQUAL "debug")
list(APPEND uvwasi_defines UVWASI_DEBUG_LOG)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does debug log contain -O0 and -g ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked; it does not, uvwasi only had them enabled when coverage reporting is enabled. Updated the patch so they are also enabled when walrus is in debug mode.

Copy link
Collaborator

@clover2123 clover2123 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what about using USWASI_DEBUG_LOG option defined in CMakeList.txt instead?

zherczeg
zherczeg reviewed Dec 3, 2024
View reviewed changes
third_party/uvwasi/CMakeLists.txt Outdated
@@ -42,6 +42,12 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
list(APPEND uvwasi_defines _GNU_SOURCE _POSIX_C_SOURCE=200112)
endif()

if (WALRUS_MODE STREQUAL "debug")
list(APPEND uvwasi_defines UVWASI_DEBUG_LOG)
add_compile_options( -O0 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need two steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it did not occur to me that it can take multiple options at the same time.

zherczeg
zherczeg approved these changes Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@matetokodi
Copy link
Contributor Author

Are you sure we want full wasi debug logging all the time in walrus debug builds though?
I do think it will clutter up the stderr output quite substantially.

for example:

uvwasi_args_sizes_get(uvwasi=0x7ffcb389f310, argc=0x79236f00f000, argv_buf_size=0x79236f00f00c)
uvwasi_args_sizes_get(uvwasi=0x7ffcb389f310, argc=0x7ffcb3899e88, argv_buf_size=0x7ffcb3899e8c)
uvwasi_args_get(uvwasi=0x7ffcb389f310, argv=0x7ffcb3899ed0, argv_buf=0x79236f00f258)
uvwasi_fd_write(uvwasi=0x7ffcb389f310, fd=1, iovs=0x7ffcb3899e60, iovs_len=1, nwritten=0x79236f00f0c8)

would just having add_compile_options( -O0 -g ) be enough?

clover2123
clover2123 requested changes Dec 4, 2024
View reviewed changes
third_party/uvwasi/CMakeLists.txt Outdated
@@ -42,6 +42,10 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
list(APPEND uvwasi_defines _GNU_SOURCE _POSIX_C_SOURCE=200112)
endif()

if (WALRUS_MODE STREQUAL "debug")
list(APPEND uvwasi_defines UVWASI_DEBUG_LOG)
Copy link
Collaborator

@clover2123 clover2123 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what about using USWASI_DEBUG_LOG option defined in CMakeList.txt instead?

@matetokodi
WASI: Enable debug build if walrus is a debug build
35b5689 
Add option to enable wasi debug logging with `-DUVWASI_DEBUG_LOG=ON`

Signed-off-by: Máté Tokodi mate.tokodi@szteszoftver.hu
@matetokodi
Copy link
Contributor Author

Updated the patch to only enable uvwasi debug logging when -DUVWASI_DEBUG_LOG=ON is used; and to use uvwasi_cflags for the debug build options.

@clover2123 clover2123 merged commit 866c995 into Samsung:main Dec 9, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clover2123 clover2123 clover2123 approved these changes

@zherczeg zherczeg zherczeg approved these changes

@ksh8281 ksh8281 Awaiting requested review from ksh8281 ksh8281 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matetokodi @zherczeg @clover2123