Exception in `WasmState.getLocals`

[tolauwae]

The following index can be out of range:

https://github.com/TOPLLab/WARDuino-VSCode/blob/be8fe97a6f112a9b69be998f4662b87c1bece33a/src/State/AllState.ts#L157

@carllocos Why is there a plus one?

---

More information on the bug:

fp: 0 stack: 5 items

dump:

{"pc":4172,"breakpoints":[4687],"callstack":[{"type":0,"fidx":"0x25","sp":-1,"fp":-1,"idx":0,"block_key":0,"ra":5917},{"type":0,"fidx":"0x1f","sp":-1,"fp":0,"idx":1,"block_key":0,"ra":5082},{"type":0,"fidx":"0x1e","sp":-1,"fp":0,"idx":2,"block_key":0,"ra":4317}],"globals":[{"idx":0,"type":"i32","value":1648},{"idx":1,"type":"i32","value":3248},{"idx":2,"type":"i32","value":3280},{"idx":3,"type":"i32","value":3312},{"idx":4,"type":"i32","value":3344},{"idx":5,"type":"i32","value":3376},{"idx":6,"type":"i32","value":3472},{"idx":7,"type":"i32","value":1},{"idx":8,"type":"i32","value":3248},{"idx":9,"type":"i32","value":0},{"idx":10,"type":"i32","value":1},{"idx":11,"type":"i32","value":1568}],"stack":[{"idx":0,"type":"i32","value":0},{"idx":1,"type":"i32","value":0},{"idx":2,"type":"i32","value":0},{"idx":3,"type":"i32","value":0},{"idx":4,"type":"i32","value":0}],"events": []}

WAT code:

 (func $game/initSnake
  (local $0 i32)
  (local $1 i32)
  (local $2 i32)
  (local $3 i32)
  (local $4 i32)
  ;;@ game.ts:38:1
  (global.set $game/s_length
   ;;@ game.ts:38:12
   (i32.const 1)  ;; <---------------------------- pc
  )
  (local.set $0
   (global.get $game/snake)
  )
  (i32.store
   (local.tee $1
    (call $game/Coordinate#constructor)
   )
  ;; ...

Instruction 4172 is the (i32.const 1).

---

[tolauwae]

This seems to happen at the start of every function.

[carllocos]

@carllocos Why is there a plus one?

TL;DR

I made a stupid copy-paste mistake when inspiring myself from my Python code where I already had a similar implementation. The code that you reference above should be modified to this:

const nrArgs = this.sourceMap.typeInfos.get(func.type)!.parameters.length;
const fp = frame.sp + 1 + nrArgs;
return func.locals.map((local, idx) => {
     const sv = stack[fp + idx];
     ...

However, I still need to test my solution thouroughly. I'll make a PR for this.

More details Now you may be more confused about why do I use sp instead of fp. And this in my opinion is an interesting conversation 😂 .

Something that you may or may not know is that the fp of a function frame corresponds with the frame pointer of the caller of the current function. And this fp is restored after completion of the function call. So for a particular function frame, we cannot rely on the fp stored on that function frame to retrieve the arguments and locals of the associated function, which explains the occasional out-of-range exceptions.

Fortunately, it is still possible to derive the fp that corresponds to the function frame which is also what I do in the snippet above. The reason why we can use the sp to derive the fp is because of the way the VM pushes function frames on the callstack. In this line, you can see that the sp saved on a function frame points to a stackvalue just before the first argument of the function called. So sp+1 should point to the first argument of the function. And thus const fp = frame.sp + 1 + nrArgs; would make us point to the first local value, if any.

Hopefully this makes some sense.

[tolauwae]

@carllocos so #107 is an incorrect fix?

[carllocos]

@tolauwae Indeed. But I'll handle the fix :)

[tolauwae]

@carllocos I didn't do a good job of reviewing PR #107. This is still broken. We get the same off-by-one error.

I have added a breaking example: 3c0a38e It connects 2 buttons to the microcontroller to respectively brighten and dim the LED.

When I put a breakpoint in the callbacks and run the program. Whenever I press one of the buttons, vs code doesn't enter the functions because of this error:

SerialConnection (/dev/ttyUSB0): AT 308!
SerialConnection (/dev/ttyUSB0): unhandled line "received interrupt 1"
SerialConnection (/dev/ttyUSB0): unhandled line "Interrupt: 1"
BP reached at line 32 (addr=308)
SerialConnection (/dev/ttyUSB0): received interrupt 9
SerialConnection (/dev/ttyUSB0): Interrupt: 9
SerialConnection (/dev/ttyUSB0): DUMP!
SerialConnection (/dev/ttyUSB0): unhandled line "received interrupt 9"
SerialConnection (/dev/ttyUSB0): unhandled line "Interrupt: 9"
SerialConnection (/dev/ttyUSB0): unhandled line "DUMP!"
SerialConnection (/dev/ttyUSB0): {"pc":308,"breakpoints":[308,319,401],"callstack":[{"type":0,"fidx":"0x8","sp":-1,"fp":-1,"idx":0,"block_key":0,"ra":293},{"type":3,"fidx":"0x0","sp":0,"fp":0,"idx":1,"block_key":366,"ra":368},{"type":0,"fidx":"0x6","sp":2,"fp":0,"idx":2,"block_key":0,"ra":401},{"type":255,"fidx":"0x0","sp":7,"fp":3,"idx":3,"block_key":0,"ra":308}],"globals":[{"idx":0,"type":"i32","value":2},{"idx":1,"type":"i32","value":25},{"idx":2,"type":"i32","value":33},{"idx":3,"type":"i32","value":26},{"idx":4,"type":"i32","value":250},{"idx":5,"type":"i32","value":0},{"idx":6,"type":"i32","value":250}],"stack":[{"idx":0,"type":"i32","value":250},{"idx":1,"type":"i32","value":26},{"idx":2,"type":"i32","value":250},{"idx":3,"type":"i32","value":10000},{"idx":4,"type":"i32","value":12},{"idx":5,"type":"i32","value":10012},{"idx":6,"type":"i32","value":0},{"idx":7,"type":"i32","value":0}],"events": [{"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_25", "payload": ""}, {"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_33", "payload": ""}, {"topic": "interrupt_33", "payload": ""}]}
Hardware: refresh Error TypeError: Cannot read properties of undefined (reading 'value')

The error happens on line 161 because sv is undefined. This is due to the initial value of the framepointer const fp = 2 + 1 + 5, while there are only 8 values on the stack. In other words stack[8 + 0] is undefined.

https://github.com/TOPLLab/WARDuino-VSCode/blob/6937c66b6c0118221fcc928807d28826fae66ba2/src/State/AllState.ts#L157-L162

[tolauwae]

@carllocos I understand the issue. The arguments are part of the func objects locals. The code tries to get 5 locals, but with the + nrArgs skips them. I have a fix in the works.

[carllocos]

@tolauwae I also noticed that this was still broken while working on the AssemblyScript source mapping but failed to report it in time as you did here above.

I'm afraid that your PR https://github.com/TOPLLab/WARDuino-VSCode/pull/115 does not suffice to fix the problem the reason is simply that the locals may also contain parameters or exclusively parameters making the plugin, in that case, show either twice the same argument information or have an out-of-range issue that leads to the undefined that you experienced. To properly solve this issue it essentially boils down to properly distinguishing between locals that are parameters and the others that are locals declared in the function. I also explain this in issue #114. I already figured out a way how to solve this issue, which also solves #114 and dependents on PR.

Once again if fine by you I will push my solution to PR https://github.com/TOPLLab/WARDuino-VSCode/pull/115, I should have reported this issue in time in that way you would have not lost valuable time, apologies for that.

[tolauwae]

If my commit is not useful I would make a new PR for your changes.

Can you push them on a branch? That way I know what you are working on.

[tolauwae]

I'm actaully not clear on why #115 doesn't fix this issue and why the wabt tools need to be updated.

the locals may also contain parameters or exclusively parameters making the plugin, in that case, show either twice the same argument information or have an out-of-range issue that leads to the undefined that you experienced.

But the breaking example has precisely this case. A function with only arguments and it works fine.

it essentially boils down to properly distinguishing between locals that are parameters

I actually think the code you wrote in WasmState.getLocals already does this properly, no? Parameters by the Wasm spec are the first locals. All you need to know is the number of arguments, which we already know thanks to the type. That is why this code works:

const fp = frame.sp + 1 + nrArgs; 

[carllocos]

Apologies for the lengthy explanation that will follow but I think it is needed given the subtleties of this issue.

I will try to explain the issue with a simple example. Suppose the following snippet:

(func $f (param $par1 i32) (param $par2 i32)
    (local $loc1 i32)
    ;; some code
)

What function $f does here is not important. What is important here to observe is that function $f has two parameters that are given a name and has one locally declared variable. Because the parameters have a name the source mapping will include those names in the locals for function $f. Thus accessing the locals for $f as func.locals gives us three elements ['$parq', '$par2', '$loc1'].

This is an issue for the current version of the plugin because it does not expect any parameters to be part of the locals properties. And in this example, it leads to an index out of range and consequently the Cannot read properties of undefined. The reason is because we will try to access 3 local variables from the stack instead of only 1 see line 159

The solve this issue the plugin needs to know precisely which elements from the func.locals are parameters so that it can then filter those out so to only keeps the locals (in the example above only ['$loc1']). We cannot simply slice the func.locals to remove the amount of arguments the function expects (e.g., func.locals.slice(nargs, func.locals.length) because sometimes the func.locals may not contain the parameters. If a function is defined as follow

(type $t1 (param i32 i32) (result))
(func $g (type $t1)
    (local $loc1 i32)
    ;; some code
)

then the func.locals for $g will be ['$loc1']. Thus slicing will not work.

The only solution that I saw, (although there may be easier solutions), is to ensure that when wat2wasm prints the json that contains the locals per function. The indexes of the locals get also included in the json. Thanks to those indexes we can precisely know whether a local in the func.locals is a parameter or a local declared in the function. In the case of function $g, despite that func.locals contains 1 element the index of that element will be equal to 2 so func.locals[0].index === 2 will be true.

You may wonder why we need to change the WABT to include the indexes since the VariableInfo interface already contains an index property which is assigned when generating the source maps in line https://github.com/TOPLLab/WARDuino-VSCode/blob/6937c66b6c0118221fcc928807d28826fae66ba2/src/Parsers/ParseUtils.ts#L177

This is because that index is assigned based on the length of the locals printed by the JSON. In the case of $g local $loc1 would obtain as index 0 instead of 2. The plugin cannot know based on that index whether $loc1 is the first parameter or a local.

To apply my solution I thus extended the wat2wasm in (commit) to include the indexes of locals and in PR https://github.com/TOPLLab/WARDuino-VSCode/pull/116 the plugin makes use of that information.

Unfortunately, I cannot provide a concrete comparison of my solution https://github.com/TOPLLab/WARDuino-VSCode/pull/116 with yours in PR https://github.com/TOPLLab/WARDuino-VSCode/pull/115 because I think there is an issue with your code. I believe you implemented something else than what you indented to implement. In this line https://github.com/TOPLLab/WARDuino-VSCode/blob/4edcfae38b2926dfb139bea6870d9737cdf130c5/src/State/AllState.ts#L159 You take the slice of the func.locals starting from the fp and until the number of locals. The fp is an index within the stack and is used for accessing the stack. So maybe you wanted to take the slice from the stack, something on the lines stack.slice(fp, fp + func.locals.length)? Regardless, I believe that using func.locals as part of the slice argument is still incorrect because that would, as explained here above, include the parameters and lead to out-of-bound exception. In my solution, I filter the func.locals to only contain the elements that have an index greater or equal to the number of arguments that the function expects. In doing so, we obtain only the locals and not the parameters (see line).

Also, the reason why the breaking example here above works as shown in the image of https://github.com/TOPLLab/WARDuino-VSCode/issues/106#issuecomment-1621544459 is because the callbacks do not declare locals as (local $loc i32) nor parameters as (param $par1) (param $par2).

[tolauwae]

because sometimes the func.locals may not contain the parameters

Ok I understand. This is the real problem. That is annoying.

I think the code would be easier to understand if it worked in the opposite direction. Go over locals one-by-one on the stack and look up their name etc. in the sourcemapping with their index. Instead of going over the sourcemapping, since its content is unreliable.

We can revisit this later, since we'd want to change the internal sourcemap structure in that case imo. In other words, the fix in #116 looks good for now in that case.

Thank you for explaining this so patiently.

[carllocos]

I think the code would be easier to understand if it worked in the opposite direction. Go over locals one-by-one on the stack and look up their name etc. in the sourcemapping with their index. Instead of going over the sourcemapping, since its content is unreliable.

agree

Thank you for explaining this so patiently.

No problem :)