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

ref(wasm-split): Simplify varint encoding prefix #621

Merged
merged 4 commits into from Jan 5, 2022

Conversation

bkotsopoulossc
Copy link
Contributor

@bkotsopoulossc bkotsopoulossc commented Jan 4, 2022

#skip-changelog

Follow-up to #619.

I wasn't quite sure at the time how to reuse the LEB128 / varint encoding prefix from the wasmbin library. It turns out that it is easy to do with the Encode trait.

The library has some basic encoding support for builtin types. Strings get encoded as binary contents, and binary contents (RawBlob) automatically get encoded with the length before the contents, and the length integer gets encoded as a LEB128 / varint.

Tested by comparing the emscripten output with the wasm-split flag.

% emcc -gseparate-dwarf=foobar bazel/hello-world/hello-world.cc -o test.wasm
% wasm-objdump -j external_debug_info -s test.wasm

test.wasm:	file format wasm 0x1

Contents of section Custom:
0050c3c: 1365 7874 6572 6e61 6c5f 6465 6275 675f  .external_debug_
0050c4c: 696e 666f 0666 6f6f 6261 72              info.foobar
% cargo build --release --package wasm-split 
% ./target/release/wasm-split --strip-names --strip test.debug.wasm -o test.wasm -d test.symbols.wasm --external-dwarf-url=foobar
% wasm-objdump -j external_debug_info -s test.wasm

test.wasm:	file format wasm 0x1

Contents of section Custom:
0957a13: 1365 7874 6572 6e61 6c5f 6465 6275 675f  .external_debug_
0957a23: 696e 666f 0666 6f6f 6261 72              info.foobar

PTAL @Swatinem

@bkotsopoulossc bkotsopoulossc requested a review from a team as a code owner January 4, 2022 23:41
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

One less dependency is always a good thing ;-)

@Swatinem
Copy link
Member

Swatinem commented Jan 5, 2022

The failing unit test is again a test missing credentials, so I will just ignore that and merge.

@Swatinem Swatinem merged commit a943468 into getsentry:master Jan 5, 2022
@bkotsopoulossc bkotsopoulossc deleted the simpler_custom_section branch January 5, 2022 14:12
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.

None yet

2 participants