Fix #106 index out of range in WasmState.getLocals

[tolauwae]

The buggy behaviour seems to be fixed now for the demo. But there is still an issue, which may possibly be on the vm side.

Getting locals in the plugin from the stack had a hardcoded +1. This caused a index out-of-range exception that causes the plugin not to update and breakpoints to not work.

Getting the locals from the stack rather than directly from the debugger dump, means we are reimplementing vm logic. So this comes down doing the same as as i_instr_get_local, which does not include the +1.

https://github.com/TOPLLab/WARDuino/blob/006128f8ba027b54f55d0354b1e4cc0fbb6b915e/src/Interpreter/instructions.cpp#L516-L524

Therefore I think the code right now is correct.

Unfortunately, now the index can be -1, so we get another out of range index. I fixed this by simply returning NaN. However, how can there be locals if the index is -1?

This seems impossible. I would expect that a -1 means there are no locals. So is the dump wrong? Is the vm wrong?

@carllocos Is my thinking right on this? any idea what is going on?

[carllocos]

Getting the locals from the stack rather than directly from the debugger dump, means we are reimplementing vm logic. So this comes down doing the same as as i_instr_get_local, which does not include the +1.

I realise indeed that there is already an implementation to retrieve the locals but the reason why I refrained myself from using it is because we can derive the locals from the stack. If we would have to request the locals via a separate interrupt it would require additional message interchange which in future work may be something that we want to limit (e.g., due to network delays). Also if my understanding is correct on the implementation of the local interrupt, the locals interrupt only gives us information about the locals of the current function. This is a problem when trying to view the locals of a function lower in the callstack. So trying to derive the locals from the stack is in my opinion a more general approach which has the additional benefit to reduce network communication.

@carllocos Is my thinking right on this? any idea what is going on?

Unfortunately removing the +1 does not really fix the issue since the problem stems from using the fp as a way to derive the locals from the stack. The reason on why using the fp is an issue is better explained in comment https://github.com/TOPLLab/WARDuino-VSCode/issues/106#issuecomment-1602813219

If fine by you, I propose to take over this PR. I will fix this issue.

[tolauwae]

Ok sounds good.

[chscholl]

I thought that the whole point of the changes to the VM were that we can now ask very detailed information instead of getting a whole dump? This seems to be the same kind of thing instead of having to retrieve the whole stack we can just ask the VM to give us the locals. In any case it would be worthwhile to have both, i.e. that we can simply ask the VM for locals and that we can get the whole stack when needed.

[tolauwae]

Yes that makes more sense. Also the stack view will probably be hidden whenever you are debugging Typescript or Rust, so you don't need that info for that view either. Only for locals of functions lower in the callstack, which can be requested when necessary.

[tolauwae]

However, for those locals of other functions. We still need this code to work. So I think we should fix that in this PR. And have a separate PR that refactors the code to request the locals directly.

[carllocos]

Indeed we could have both: the whole stack or only the locals of any function in the callstack. However, if we would only request the locals and no longer the full stack we would lose the view on the arguments provided to a function call. Therefore I believe that in the separate PR, as Tom suggests, we should also make it possible to request both kinds of values: the locals and arguments per function in the callstack.

[tolauwae]

Wouldn't it make more sense that when you request the locals of the current function you also get the arguments? After all in the wasm spec, there is no difference between the two.

I see no real need to request locals from functions other than the current one. They can't update anyway, so we don't need to fetch them.

But specifically for this PR. What still needs to be done so we can merge the fix for the index out of range?

[carllocos]

Yes agree, we should be able to request both locals and arguments. And this as a single request.

To be precise, I'm talking about giving access to the locals and arguments of only the functions parts of the callstack. So not any function. The users could see the locals and arguments of the other functions in the callstack by clicking on any item in the call stack view of the plugin. By default the plugin would only show the locals and arguments of the current function but through clicks, the users could choose to see the locals and args of other function not yet completed. And for optimisation purposes, we would also only fetch the locals and args of the other functions in the callstack after clicking by the user.

But specifically for this PR. What still needs to be done so we can merge the fix for the index out of range?

Manual testing by me

[tolauwae]

Ah of course, I was wrong:

I see no real need to request locals from functions other than the current one. They can't update anyway, so we don't need to fetch them.

There are possibly functions on the callstack we haven't "seen" yet, after a breakpoint or running.

[carllocos]

@tolauwae so I just committed the fix and tested the fix manually. Everything seems to work now. However, for this I created a custom WAST file and did not test on the WAST that you experienced the issue with. So maybe as a last review step, you can provide me with that file.

Also, I cannot change the reviewer to become you so I propose that if the fix works on your WAST file I then approve the PR.

[tolauwae]

I actually experienced it with almost any file, also handwritten. But this one definitely had it: code.zip

[carllocos]

I do not experience the issue with the code above. But also I do not think that the code that I got does something with locals. I see a main function called $blink/main which loops and blinks the led. The main function does not do any call to other functions that define locals. I see however some functions that do have locals (e.g., ~lib/rt/pure/decrement) but are not called from the main.

When time allows, could you maybe test the fix on a file for which you know that the bug occurs? That is probably the easiest because for me to test this I have to manually modify the wast to make it work on the m5stickc.

[tolauwae]

I'll double check on my end.

[tolauwae]

Issue still doesn't manifest. Looks good :+1: @carllocos