Stream: git-wasmtime

Topic: wasmtime / PR #7858 Add cloning for DWARF String attributes

Wasmtime GitHub notifications bot (Feb 01 2024 at 22:16):

yowl opened PR #7858 from yowl:dwarf-att-string to bytecodealliance:main:

This PR (my first here), adds cloning for DWARF String attributes, copying how Block was done. This allows wasmtime -D debug-info to work with programs compiled with emscripten:

emcc 1.c -o 1.wasm -g -gdwarf-5 -sWASM_BIGINT=1 -sSTANDALONE_WASM=1

And consequently that they can be debugged in lldb:


Wasmtime GitHub notifications bot (Feb 01 2024 at 22:16):

yowl requested pchickey for a review on PR #7858.

Wasmtime GitHub notifications bot (Feb 01 2024 at 22:16):

yowl requested wasmtime-core-reviewers for a review on PR #7858.

Wasmtime GitHub notifications bot (Feb 01 2024 at 22:25):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2024 at 22:25):

bjorn3 created PR review comment:

I think you can use into_owned() instead of to_vec().

Wasmtime GitHub notifications bot (Feb 01 2024 at 22:53):

yowl created PR review comment:

Thanks, shall I change AttributeValue::Block(d) => write::AttributeValue::Block(d.to_slice()?.to_vec()), as well?

Wasmtime GitHub notifications bot (Feb 01 2024 at 22:53):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2024 at 23:46):

yowl updated PR #7858.

Wasmtime GitHub notifications bot (Feb 01 2024 at 23:47):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2024 at 23:47):

yowl created PR review comment:

I went ahead and changed Block, I have limited experience of Cow and slicing, but it seemed logical being as how similar Block is to String

Wasmtime GitHub notifications bot (Feb 05 2024 at 16:31):

alexcrichton submitted PR review:

I won't really pretend to know what's going on here but I don't think many of us know much about the native dwarf transform this is doing. That being said it seems not obviously wrong at least and the same as everything else in this area.

Ideally we'd have tests for changes like this but we don't have many preexisting tests here yet so I think it's ok to land this as-is and we can look to improve testing later.

Wasmtime GitHub notifications bot (Feb 05 2024 at 17:03):

alexcrichton merged PR #7858.

Last updated: Nov 22 2024 at 17:03 UTC