Steve Williams (Oct 08 2021 at 18:16): Steve Williams (Oct 08 2021 at 18:16):rust side: there's a resolve_vmctx_memory_ptr that seems to do what I want, its just not exported.
as that's not messing about with string tables, I might even be able to get that running.
copy set export template, figure out how to add the integer parameter & done, maybe.
at least to get something working.
Steve Williams (Oct 08 2021 at 18:17):https://github.com/bytecodealliance/wasmtime/blob/main/crates/runtime/src/debug_builtins.rs
Steve Williams (Oct 08 2021 at 20:38):oops, wrong thread, sorry.
bjorn3 (Oct 08 2021 at 21:13):If you chose edit for the first commit in the wrong topic, you can change the topic title to match the topic you actually wanted to post it in. Zulip will then ask you if you want to move all later messages too.
bjorn3 (Oct 08 2021 at 21:14):I would do this for you if not for the fact that the mobile app doesn't support this.
Steve Williams (Oct 09 2021 at 09:47):issue summary: lldb expects c++ "this" to be a standard system memory pointer. when its not as is the case with current wasmtime wrapped pointer delvered via jit debug connection, we run into difficulties. we are prevented from being able to access 'this' as we would other variables because the evaluate fails because lldb is hardwired to expect "this" to be a standard pointer.
(continued from detail in lldbg post - its not an lldbg issue, its core)
Steve Williams (Oct 09 2021 at 13:55):I've got some kind of regression or config error on windows. switching to linux where things appear more sane right now.
running wasmtime trunk, lldb 12.0.1
(lldb) target create "wasmtime"
Current executable set to 'wasmtime' (x86_64).
(lldb) settings set -- target.run-args "-g" "./test.wasm"
(lldb) b test.cpp:7
Breakpoint 1: no locations (pending).
WARNING: Unable to resolve breakpoint to any actual locations.
(lldb) r
Process 21261 launched: '/home/x/dev/tools/wasmtime' (x86_64)
1 location added to breakpoint 1
Process 21261 stopped
Thing::Thing(this=(__ptr = 67216), i=5) at test.cpp:8:13
5 public:
6 Thing(int i)
7 {
-> 8 m_i = i;
9 }
10
11 int m_i;
(lldb) p __vmctx->set()
warning: this' is not accessible (substituting 0) Steve Williams (Oct 09 2021 at 14:06):resolve_vmctx_memory_ptr seems to want address of sandbox pointer, not its direct value, so that's probably why it's faulting.
Steve Williams (Oct 09 2021 at 14:22):tried :
#[no_mangle]
pub unsafe extern "C" fn resolve_vmctx_memory(ptr: usize) -> *const u8 {
let handle = InstanceHandle::from_vmctx(VMCTX_AND_MEMORY.0);
assert!(
VMCTX_AND_MEMORY.1 < handle.module().memory_plans.len(),
"memory index for debugger is out of bounds"
);
let index = MemoryIndex::new(VMCTX_AND_MEMORY.1);
let mem = handle.instance().get_memory(index);
mem.base.add(ptr)
}
but not found so there's something else I need to tweak somewhere to add a new reserved function.
Steve Williams (Oct 09 2021 at 14:32):ahh, ensure_exported section :)
Steve Williams (Oct 09 2021 at 14:48):Looks promising:
(lldb) target create "wasmtime"
Current executable set to 'wasmtime' (x86_64).
(lldb) settings set -- target.run-args "-g" "./test.wasm"
(lldb) b test.cpp:7
Breakpoint 1: no locations (pending).
WARNING: Unable to resolve breakpoint to any actual locations.
(lldb) r
Process 29127 launched: '/home/x/dev/tools/wasmtime' (x86_64)
1 location added to breakpoint 1
Process 29127 stopped
thread #1, name = 'wasmtime', stop reason = breakpoint 1.1
frame #0: 0x00007ffff7fa732c JIT(0x555556045210)Thing::Thing(this=(__ptr = 67216), i=5) at test.cpp:8:13
5 public:
6 Thing(int i)
7 {
-> 8 m_i = i;
9 }
10
11 int m_i;
(lldb) p __vmctx->set
warning: this' is not accessible (substituting 0)
Fix-it applied, fixed expression was:
__vmctx->set()
(lldb) p (Thing*) resolve_vmctx_memory(67216)
warning: this' is not accessible (substituting 0)
(Thing *) $0 = 0x00007ffe40010690
(lldb) p $0->m_i
warning: this' is not accessible (substituting 0)
(int) $1 = 0
(lldb) n
Process 29127 stopped
thread #1, name = 'wasmtime', stop reason = step over
frame #0: 0x00007ffff7fa7336 JIT(0x555556045210)Thing::Thing(this=(__ptr = 0), i=0) at test.cpp:9:4
6 Thing(int i)
7 {
8 m_i = i;
-> 9 }
10
11 int m_i;
12 };
(lldb) p $0->m_i
warning: this' is not accessible (substituting 0)
(int) $2 = 5
Steve Williams (Oct 09 2021 at 14:51):Is there an actual need for the resolve in here to be a pointer to a sandbox pointer rather than the pointer itself ?
https://github.com/bytecodealliance/wasmtime/blob/main/crates/runtime/src/debug_builtins.rs
if we can mod the one that's there, there's no need to add another method. usize rather than u32 though ideally so we have 64 bit covered too.
Steve Williams (Oct 09 2021 at 14:53):if can perform above from lldbg seems I've got a solution. lldb doesn't 'like' the invalid this pointer but it seems to let us pass.
Steve Williams (Oct 09 2021 at 15:00):& we shouldn't be sending that as const as debuggers will sometimes mess with it & that's valid.
Steve Williams (Oct 09 2021 at 15:01):done, I think but won't know for sure until I have it fixed up in the GUI & there's a more elegant solution in there with a valid this pointer to keep lldb happier.
Steve Williams (Oct 10 2021 at 15:00):updated : https://github.com/adv-sw/lldbg
includes 'this' workaround with all detail hidden in gui so it behaves as expected.
requires minor wasmtime tweak to add resolve_vmctx_memory function - ready to push that.
Steve Williams (Oct 11 2021 at 09:10):resolves "this" pointer cannot be evaluated by lldb issue.
https://github.com/bytecodealliance/wasmtime/commit/92a10d1ace70fe75c3e2c646ea613168fc417851
https://github.com/bytecodealliance/wasmtime/issues/3419
plz review/merge.
Steve Williams (Oct 11 2021 at 17:20):more specifically, could I have a reviewer for this, please.
no huge rush but then it's done. mod is trivial, doesn't affect anything else.
Steve Williams (Oct 11 2021 at 17:22):security: only invoked under debugger which legitimately can access/modify anything in the sandbox.
bjorn3 (Oct 11 2021 at 17:24):cc @Yury Delendik
Steve Williams (Oct 11 2021 at 17:25):could factor resolve_ptr into resolve if you'd like.
Steve Williams (Oct 11 2021 at 17:27):to answer my own q: resolve_ptr is required for * operators injected by wasmtime. so yes, we do need both unless that code is to be modified (unnecessary imo - lets keep this simple for now)
Steve Williams (Oct 11 2021 at 17:41):cfallon : Would you mind creating a PR for the review and merging of your commit?
ummm sure ... how ?
:)
The usual flow is that one will create this PR with the text "Fixes #ISSUE-NUMBER" in the commit message, which links it back to this issue and closes it when merged)
Steve Williams (Oct 11 2021 at 17:42):PR = pull request. thought I did. that's what this is, isn't it ?
https://github.com/bytecodealliance/wasmtime/commit/92a10d1ace70fe75c3e2c646ea613168fc417851
git collab noob also :)
bjorn3 (Oct 11 2021 at 17:43):On your fork there is a button "contribute" that you can click on.
bjorn3 (Oct 11 2021 at 17:44):Normally you would create a branch and push the changes there though instead of push to the main branch.
bjorn3 (Oct 11 2021 at 17:44):This allows you to have multiple PR's at the same time.
bjorn3 (Oct 11 2021 at 17:45):Also I think you will want to remove the merge commit by rebasing on top of the bytecodealliance/wasmtime main branch.
Steve Williams (Oct 11 2021 at 17:45):@bjorn3 - right ... thought that's what I did, though via pull requests, create pull request.
didn't label anything with issue # happy to do so, going forwards.
Steve Williams (Oct 11 2021 at 17:50):unsure what else you want other than "fixes (indirectly) #3419 by providing debugger with ability to resolve sandbox pointers"
Chris Fallin (Oct 11 2021 at 17:56):@Steve Williams that is just a git commit, not a PR; a PR is a separate entity on GitHub that creates a discussion thread, a space for code review, and asks the maintainers to merge your commit into the mainline. You can click on "Pull Requests" in the main bytecodealliance/wasmtime repo and then the "New Pull Request" button (green, upper right), and select your repo fork and branch with the commit(s) you want to contribute
Steve Williams (Oct 11 2021 at 18:04):Thanks, Chris. Will see if I can present patch as requested.
Steve Williams (Oct 11 2021 at 18:08):Like this ?
https://github.com/bytecodealliance/wasmtime/pull/3443
Steve Williams (Oct 11 2021 at 18:30):The direct fix is lldbg which provides high level this access - appears to be working nicely with minimal testing. Equivalent could be presented as macro for lldb direct, but haven't done that as don't currently need it. Quite liking the new front end. Could be a bytecode alliance project if desired, though as its largely generic lldb front end, perhaps better over there. Dunno, don't care. Just pleased we have it.
Looking forwards to being able to present my globals :)
https://github.com/adv-sw/lldbg
Direct fix requires indirect fix in PR 3443 & also lldb patch on windows to fix their JIT interface.
Other platforms should work out of the box.
Chris Fallin (Oct 11 2021 at 18:35):Got it, will review -- thanks!
Steve Williams (Oct 11 2021 at 18:39):LLDB/Windows JIT patch fix here : https://reviews.llvm.org/D110066
Other platforms should interoperate with above out of the box.
Steve Williams (Oct 11 2021 at 18:41):Dan - do you have commit over there ? Can it go in plz. LLDB is currently broken on Windows, that's not clever wrt wide spread adoption which is surely part of the goal here. Test can wait, it works. -my 2c.
Steve Williams (Oct 11 2021 at 18:45):Test is lldb or lldbg :) Without patch - JIT debugging fails. With patch, JIT debugging connects.
Steve Williams (Oct 11 2021 at 18:49):That comes out sounding like a demand. Sorry. I humbly request LLVM D110066 is accepted as is following confirmation it functions as stated. Can maintain out of tree, but I think its better for all that it goes in. Eliminate the wonky.
Steve Williams (Oct 11 2021 at 18:55):or anyone else with commit - not trying to put anyone on the spot with my nonsense :)
Steve Williams (Oct 11 2021 at 19:00):also, sooner its in, sooner its into a standard release regular folks have access to.
Steve Williams (Oct 11 2021 at 20:01):Thanks Chris - think that's done as requested now. Assistance appreciated.
LLDB aside, perhaps the wrong place, certainly the wrong thread :)
Last updated: Nov 22 2024 at 17:03 UTC