Wasmtime GitHub notifications bot (Mar 09 2023 at 17:48): Wasmtime GitHub notifications bot (Mar 09 2023 at 17:48):fitzgen opened PR #5973 from compiled-funciton-info to main:
This will allow us to build developer tools for Wasmtime and Cranelift like WAT and asm side-by-side viewers (a la Godbolt).
These are not proper public APIs, so they are marked
doc(hidden)and have comments saying they are only for use within this repo's workspace.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Mar 09 2023 at 17:48):fitzgen requested alexcrichton for a review on PR #5973.
Wasmtime GitHub notifications bot (Mar 09 2023 at 17:49):fitzgen edited PR #5973 from compiled-funciton-info to main:
This will allow us to build developer tools for Wasmtime and Cranelift like WAT and asm side-by-side viewers (a la Godbolt).
These are not proper public APIs, so they are marked
doc(hidden)and have comments saying they are only for use within this repo's workspace.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Mar 09 2023 at 18:04):fitzgen updated PR #5973 from compiled-funciton-info to main.
Wasmtime GitHub notifications bot (Mar 09 2023 at 18:37):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 18:37):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 18:37):alexcrichton created PR review comment:
In practice I've found that comments like this tend to not hold any real weight in practice, especially for "juicy" functionality like this that can't be replicated anywhere else. Given that the high-level functionality exposed here is always going to be supported in Wasmtime in one way or another (e.g. via backtraces) I think it's ok to make this an "official" API with some brief docs. I don't think it's necessarily important to spend a ton of time figuring out the perfect API here either, as that's why we bump major versions still.
Wasmtime GitHub notifications bot (Mar 09 2023 at 18:37):alexcrichton created PR review comment:
If possible I think it would be best to keep these private. Could iterators be exposed which peel away these types when exposed to outside users?
Wasmtime GitHub notifications bot (Mar 09 2023 at 18:37):alexcrichton created PR review comment:
I'm only seeing half the design here in this PR since I'm not sure how this will be consumed, but could the functionality on this structure be folded directly into
wasmtime::Moduleitself? For example there's not really any need to create a duplicateVec<FunctionLoc>when it already lives withinwasmtime::Module. Additionally this structure may grow somewhat significantly over time to end up looking like a lot of interior pointers intowasmtime::Moduleand otherwise it may not need its own state.That being said though your future plans for this structure may motivate a separate type.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:12):fitzgen updated PR #5973 from compiled-funciton-info to main.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:13):fitzgen requested alexcrichton for a review on PR #5973.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:22):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:22):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:22):alexcrichton created PR review comment:
I think this'll want to handle
Noneand return an empty iterator?
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:22):alexcrichton created PR review comment:
One thing I might recommend for this signature is that this would be a good place to convert from
u32tousizesince these are indexes intoself.text(). Do you think it'd be worth returning&[u8]slices here though?
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:22):alexcrichton created PR review comment:
I think it might be good to explain a bit in the docs what the
Option<u32>is here, namely it's an instruction that doesn't map to any known source location in the original wasm file. I think we use this to avoid corresponding instructions to places they didn't come from but I sort of forget as well.Also IIRC I think that the map here is based here on ranges where if you get
(1, Some(2))then(5, Some(4))then that means bytes 1, 2, 3, and 4 come from source position 2 and bytes 5 to the next range come from source position 4. Basically we're not going to have an entry per instruction but rather for ranges of instructions (which is what theNoneis used for, to terminate a range).Finally the first element of the pair returned here might be good to change to a
usizeto assist with indexing into.text()?Oh and one more, for the docs, I think it would be good to mention that the address map is not guaranteed to be available as there's a
Configoption which can disable it.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:31):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:31):fitzgen created PR review comment:
I guess the address map data is an empty slice when we've been configured not to have address maps? I was assuming that the only way it wouldn't be the correct format was if we messed up plumbing data around.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:32):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:32):fitzgen created PR review comment:
The function slices themselves? These aren't actually that useful because everything is always text section relative offset wise, so I won't really ever be using the individual slices except when I slice them out myself and remember what offset the slices begin at.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:33):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:33):fitzgen created PR review comment:
Yeah I can add this stuff to the docs.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:45):fitzgen updated PR #5973 from compiled-funciton-info to main.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:45):fitzgen requested alexcrichton for a review on PR #5973.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:50):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 09 2023 at 19:51):fitzgen has enabled auto merge for PR #5973.
Wasmtime GitHub notifications bot (Mar 09 2023 at 20:39):fitzgen merged PR #5973.
Last updated: Nov 22 2024 at 17:03 UTC