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

Add DWARF debugging information to all artifacts by default #422

Merged
merged 3 commits into from
May 22, 2024

Conversation

alexcrichton
Copy link
Collaborator

This commit adds DWARF debugging information with the -g compiler flag to all WASI artifacts for wasi-sdk. The LLVM build itself does not have debugging information, only the sysroot artifacts. This is intended to assist with debugging. The main downside to this is binary size of generated artifacts will, by default, be larger. Stripping debug information from an artifact though involves removing custom sections which is generally pretty easy to do through wasm tooling.

This commit adds DWARF debugging information with the `-g` compiler flag
to all WASI artifacts for wasi-sdk. The LLVM build itself does not have
debugging information, only the sysroot artifacts. This is intended to
assist with debugging. The main downside to this is binary size of
generated artifacts will, by default, be larger. Stripping debug
information from an artifact though involves removing custom sections
which is generally pretty easy to do through wasm tooling.
@alexcrichton alexcrichton marked this pull request as ready for review May 21, 2024 20:07
@TerrorJack
Copy link
Contributor

TerrorJack commented May 21, 2024

Have you actually verified wasm modules can be debugged if the wasi-sdk installation path is different from the build path? The wasm objects dwarf sections contain absolute paths of their build path but does not embed the sources, I doubt if that can be useful at all if the user is not building wasi-sdk themselves.

One example assembly source on godbolt. Even with -g3, you still need the original C/C++ source in place to properly debug, and we do not ship sources. -fdebug-prefix-map= also seems to be a no-op that doesn't affect assembly output.

@alexcrichton
Copy link
Collaborator Author

Yes, I've verified this. My pre-existing install of wasi-sdk is at $WASI_SDK_PATH and for this source:

#include <stdio.h>
#include <stdlib.h>

int main() {
  printf("%s\n", "hello");
  abort();
}

I get:

$ $WASI_SDK_PATH/bin/wasm32-wasi-clang foo.c -o foo.wasm -g
$ WASMTIME_BACKTRACE_DETAILS=1 wasmtime foo.wasm
hello
Error: failed to run main module `foo.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0:  0x283 - foo.wasm!abort
           1:  0x21f - main
                           at /home/alex/code/wasi-sdk/foo.c:6:3
           2:  0x1b2 - foo.wasm!_start
    2: wasm trap: wasm `unreachable` instruction executed

Note the lack of filename/line number in frame 0 and frame 2.

With this PR:

$ ./build/install/opt/wasi-sdk/bin/wasm32-wasi-clang foo.c -o foo.wasm -g
$ WASMTIME_BACKTRACE_DETAILS=1 wasmtime foo.wasm
hello
Error: failed to run main module `foo.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0:  0x50f - abort
                           at wasisdk://v22.4g8fbfe1875dec+m/src/wasi-libc/libc-bottom-half/sources/abort.c:5:5
           1:  0x2a8 - main
                           at /home/alex/code/wasi-sdk/foo.c:6:3
           2:  0x213 - _start
                           at wasisdk://v22.4g8fbfe1875dec+m/src/wasi-libc/libc-bottom-half/crt/crt1-command.c:43:13
    2: wasm trap: wasm `unreachable` instruction executed

Note the presence of wasisdk://... sources.

That's a significant improvement for crashes in wasi-libc or other bugs relative to today.

@TerrorJack
Copy link
Contributor

Ah thanks for the check! I also just figured out that something like -fdebug-prefix-map=$PWD=wasisdk://v114514 makes it work, my previous invocation missed the $PWD part and probably confused the clang driver. Looks like this patch is indeed an improvement of status quo!

Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I think this makes a lot of sense, thanks! For those worried about binary size, it should be true that this additional DWARF information does not come along into the the final binary unless one opts in with -g, right? (Of course, one can always just leave wasm-opt in the PATH to resolve that 😉).

I was about to merge this but:

  • is the wasi-libc submodule update intentional?
  • we probably need to update some expected test output or something to make CI pass?

@sbc100
Copy link
Member

sbc100 commented May 21, 2024

I think this makes a lot of sense, thanks! For those worried about binary size, it should be true that this additional DWARF information does not come along into the the final binary unless one opts in with -g, right?

I think its the other way around, the linker (wasm-ld) will include debug info from any object files unless you link with -s/--strip-all or -S/--strip-debug.

@abrown
Copy link
Collaborator

abrown commented May 21, 2024

Hm, well this change does seem in general "a good thing." Perhaps it's not too much to ask for users interested in small binaries to compile with extra flags or run wasm-opt?

@sbc100
Copy link
Member

sbc100 commented May 21, 2024

Hm, well this change does seem in general "a good thing." Perhaps it's not too much to ask for users interested in small binaries to compile with extra flags or run wasm-opt?

Anyone shipping release binaries should always be stripping their binaries one way or another (either at link time, or post-link using llvm-strip or wasm-opt).

Even without this change unstripped binaries contain a "name" section which can be pretty huge.

* Update some expected error messages and remove some files with
  duplicate error messages that are no longer needed.
* Remove undefined behavior in `stat.c` where padding bytes were being
  compared.
@alexcrichton
Copy link
Collaborator Author

is the wasi-libc submodule update intentional?

It is indeed! Sorry should have clarified but the update pulls in a few minor commit to fix build issues to ensure wasi-libc builds correctly.

we probably need to update some expected test output or something to make CI pass?

I believe I've handled the test failures now. Some were just updating a few error messages but I had to also fix some undefined behavior in the stat.c test where padding bytes were being compared and the padding bytes were differing (presumably enabling debuginfo shuffled some things around or similar)


Also yes thanks @sbc100 I forgot to mention but wasm-ld flags are indeed sufficient to strip debuginfo from any binary in addition to other stripping tools.

@alexcrichton alexcrichton enabled auto-merge (squash) May 22, 2024 16:00
@alexcrichton alexcrichton merged commit 307694b into WebAssembly:main May 22, 2024
5 checks passed
@alexcrichton alexcrichton deleted the debug branch May 22, 2024 17:07
alexcrichton added a commit to alexcrichton/wasi-sdk that referenced this pull request May 22, 2024
I forgot in WebAssembly#422 that by specifying `EXTRA_CFLAGS` to the wasi-libc
build that it would override the defaults of wasi-libc which is to pass
these two flags in. This passes them back in to ensure the default build
still has the same flags.
alexcrichton added a commit that referenced this pull request May 22, 2024
I forgot in #422 that by specifying `EXTRA_CFLAGS` to the wasi-libc
build that it would override the defaults of wasi-libc which is to pass
these two flags in. This passes them back in to ensure the default build
still has the same flags.
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