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.
-->
fitzgen requested alexcrichton for a review on PR #5973.
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.
-->
fitzgen updated PR #5973 from compiled-funciton-info
to main
.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
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.
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?
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::Module
itself? 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::Module
and otherwise it may not need its own state.That being said though your future plans for this structure may motivate a separate type.
fitzgen updated PR #5973 from compiled-funciton-info
to main
.
fitzgen requested alexcrichton for a review on PR #5973.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I think this'll want to handle
None
and return an empty iterator?
alexcrichton created PR review comment:
One thing I might recommend for this signature is that this would be a good place to convert from
u32
tousize
since these are indexes intoself.text()
. Do you think it'd be worth returning&[u8]
slices here though?
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 theNone
is used for, to terminate a range).Finally the first element of the pair returned here might be good to change to a
usize
to 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
Config
option which can disable it.
fitzgen submitted PR review.
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.
fitzgen submitted PR review.
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.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah I can add this stuff to the docs.
fitzgen updated PR #5973 from compiled-funciton-info
to main
.
fitzgen requested alexcrichton for a review on PR #5973.
alexcrichton submitted PR review.
fitzgen has enabled auto merge for PR #5973.
fitzgen merged PR #5973.
Last updated: Dec 23 2024 at 12:05 UTC