fix: use correct index as shadowstack slot key (#2719)

[HerrCai0907]

Fixes #2719.

numLocals + this.callSlotOffset + operandIndex will cause crash in nested call.

function f1(a: i32, t: T, b: i32): void;
function f2(t: T): i32;

f1(1, new T(), f2(new T()));

In arguments of f1, key of the second argument (the first new T()) is 0(callSlotOffset) + 1(operandIndex). And the key of the argument of f2 (the second new T()) is 1(callSlotOffset) + 0(operandIndex). It means there use the same slot in shadow stack.
So if gc happened in f2, the first new T() will be cleaned but it should be alive.

This PR change numLocals + this.callSlotOffset + operandIndex to numLocals + this.callSlotOffset + numSlots, to avoid the same key in nested call.

• I've read the contributing guidelines
• I've added my name and email to the NOTICE file

[HerrCai0907]

@dcodeIO @MaxGraey Could you take a look about this PR? thanks!

[CountBleck]

I wish I knew what this was doing so I could review this properly. @dcodeIO is the most familiar with the shadowstack pass.

[CountBleck]

I did some basic logging and it seems like you fixed something, although one function (I can only identify it by the location in binaryen.js's memory, 5587016) seems to reuse an index/slot/whatever it is.

old:

currentFunction 5587016 numLocals 1 callSlotOffset 0 numSlots 0 i 0; total = 1
currentFunction 5587016 numLocals 1 callSlotOffset 0 numSlots 0 i 0; total = 1

currentFunction 5620480 numLocals 2 callSlotOffset 0 numSlots 0 i 0; total = 2

currentFunction 5621664 numLocals 3 callSlotOffset 0 numSlots 0 i 0; total = 3
currentFunction 5621664 numLocals 3 callSlotOffset 1 numSlots 0 i 0; total = 4

currentFunction 5621976 numLocals 0 callSlotOffset 0 numSlots 0 i 1; total = 1
currentFunction 5621976 numLocals 0 callSlotOffset 1 numSlots 0 i 0; total = 1

new:

currentFunction 5587016 numLocals 1 callSlotOffset 0 numSlots 0 i 0; total = 1
currentFunction 5587016 numLocals 1 callSlotOffset 0 numSlots 0 i 0; total = 1

currentFunction 5620480 numLocals 2 callSlotOffset 0 numSlots 0 i 0; total = 2

currentFunction 5621664 numLocals 3 callSlotOffset 0 numSlots 0 i 0; total = 3
currentFunction 5621664 numLocals 3 callSlotOffset 1 numSlots 0 i 0; total = 4

currentFunction 5621976 numLocals 0 callSlotOffset 0 numSlots 0 i 1; total = 0
currentFunction 5621976 numLocals 0 callSlotOffset 1 numSlots 0 i 0; total = 1

[dcodeIO]

I'm a little confused by the diff, as it also touches roundSize and other things that I wouldn't have expected from this change. Are these somehow leftovers from the previous PR?

[CountBleck]

Are these somehow leftovers from the previous PR?

Wouldn't CI have caught those in the previous PR, or on the main branch itself after it was merged?

I'd guess some other parts of the diff triggered some heuristic to uninline roundSize, but I have no clue. For instance...

[MaxGraey]

@HerrCai0907 Hmm, try to sync with "main" and force to reinstall deps

[CountBleck]

Okay, I think I understand a bit of what's going on. Maybe. I hope so.

I've seen the change to tests/compiler/bindings/noExportRuntime.debug.js (the addition of a trailing comma) for a few days before this PR, at least. That change seems to occur every time that test is run.

If you run yarn test:compiler --create on the main branch, it seems to affect tests/compiler/bindings/noExportRuntime.release.js and (at least on my machine) tests/compiler/math.release.wat (which isn't part of the diff here for whatever reason).

1. tests/compiler/math.release.wat, which once again isn't part of the diff here, is only different in that f64.const nan:0x8000000000000 is changed to f64.const -nan:0x8000000000000. I'm not sure if this is exclusive to my Node version and/or OS. I'll pretend this doesn't exist.
2. tests/compiler/bindings/noExportRuntime.release.js is only generated on --create for whatever reason, according to the following outputs and checking git status after each command:

(without --create)

- compile release
  Skipped TypeScript binding (no output path)
  Skipped JavaScript binding (no output path)
  SUCCESS (468.653 ms)

(with --create)

- compile release
  SUCCESS (492.313 ms)

3. The bindings/{esm,raw} tests are skipped on yarn test:compiler and yarn test:compiler --create by default since they require the reference-types feature. Once ASC_FEATURES="*" is used, the test failures become evident.

@HerrCai0907 must have been using ASC_FEATURES="*" and yarn test:compiler --create to recreate all fixtures, including ones that aren't normally checked for in CI.

[HerrCai0907]

The reason why change in tests/compiler/math.release.wat which isn't part of the diff here is in last change in main branch, I also re-create all testcase. And the behavior of wasm egine in nodejs is a little bit different between x86 and arm64(maybe, I am not sure, but when I replace my laptop from x86 linux to aarch64 mac)

[HerrCai0907]

I will revert other change and waiting for @CountBleck fix.

[CountBleck]

And the behavior of wasm egine in nodejs is a little bit different between x86 and arm64

I'm currently on an x86 Macbook running Node v20.0.0.

[CountBleck]

@HerrCai0907 You should probably rebase onto/merge from main now. And of course, who needs to resolve merge conflicts by hand when npm run test:compiler --create exists? :D

[HerrCai0907]

@dcodeIO Could this PR merge?

[HerrCai0907]

Thanks!