Bug with TypedArray.set() when the code is optimized?

[alexvictoor]

Hello all,
I have found a strange behaviour with TypedArray.set().
The following code fragment works fine in an as-pect test or when the code is compiled without the "--optimize" flag:

  let buffer = new Uint8Array(1);
  buffer[0] = 42;
  const oldBuffer = buffer;
  buffer = new Uint8Array(2);
  buffer.set(oldBuffer);
  buffer[1] = 1;
  // buffer[0] === 42

However, when the "--optimize" flag is used, buffer[0] is set to 0 at the end...
Here is a link to see this bug in action: https://webassembly.studio/?f=wtzfbyvd2q
Let me know if you need anything else!

[MaxGraey]

It seems issue with ARC optimizer.
If add extra __retain(changetype<usize>(buffer)) this problem disappear:
https://webassembly.studio/?f=p1c75u3i9yr

Perhaps it could be fixed in WebAssembly/binaryen#2833
cc @dcodeIO

[alexvictoor]

@MaxGraey thanks a lot for the workaround!

[MaxGraey]

Don't forget remove this when issue will fixed. Otherwise you got memory leakage

[alexvictoor]

Thanks @MaxGraey . Just to be sure tu understand: the workaround leaks memory already with assemblyscript v0.10 or just in upcoming versions of assemblyscript?

[MaxGraey]

not sure about leaknes with this workaround but after fixing it definitely could so just leave TODO: / NOTE: comment under workaround in your code I guess

[dcodeIO]

Very strange. To recap, on

{
  let buffer = new Uint8Array(1);
  buffer[0] = 42;
  const oldBuffer = buffer;
  buffer = new Uint8Array(2);
  buffer.set(oldBuffer);
  buffer[1] = 1;
  assert(buffer[0] === 42);
}

before optimizations we get

  i32.const 0
  i32.const 1
  call $~lib/typedarray/Uint8Array#constructor
  local.set $0
  local.get $0
  i32.const 0
  i32.const 42
  call $~lib/typedarray/Uint8Array#__set
  local.get $0
  call $~lib/rt/pure/__retain ;; (1)
  local.set $1
  i32.const 0
  i32.const 2
  call $~lib/typedarray/Uint8Array#constructor
  local.set $2
  local.get $0
  call $~lib/rt/pure/__release
  local.get $2
  local.set $0
  local.get $0
  local.get $1
  i32.const 0
  call $~lib/typedarray/Uint8Array#set<~lib/typedarray/Uint8Array>
  local.get $0
  i32.const 1
  i32.const 1
  call $~lib/typedarray/Uint8Array#__set
  local.get $0
  i32.const 0
  call $~lib/typedarray/Uint8Array#__get
  i32.const 42
  i32.eq
  i32.eqz
  if
   i32.const 0
   i32.const 432
   i32.const 8
   i32.const 3
   call $~lib/builtins/abort
   unreachable
  end
  local.get $0
  call $~lib/rt/pure/__release
  local.get $1
  call $~lib/rt/pure/__release ;; (2)

expecting (2) to eliminate (1), which appears to happen, but after optimizations we are left with (disabled inlining here)

  i32.const 1
  call $~lib/typedarray/Uint8Array#constructor
  local.tee $1
  i32.const 0
  i32.const 42
  call $~lib/typedarray/Uint8Array#__set
  i32.const 2
  call $~lib/typedarray/Uint8Array#constructor
  local.set $0
  local.get $1
  call $~lib/rt/pure/__release
  local.get $0
  local.get $1
  call $~lib/typedarray/Uint8Array#set<~lib/typedarray/Uint8Array>
  local.get $0
  i32.const 1
  i32.const 1
  call $~lib/typedarray/Uint8Array#__set
  local.get $0
  call $~lib/typedarray/Uint8Array#__get
  i32.const 42
  i32.ne
  if
   i32.const 1440
   i32.const 1472
   i32.const 8
   i32.const 3
   call $~lib/builtins/abort
   unreachable
  end
  local.get $0
  call $~lib/rt/pure/__release

In a nutshell, the problem is

  let buffer = new Uint8Array(1);
  buffer[0] = 42;
    const oldBuffer = buffer;
  buffer = new Uint8Array(2);
    buffer.set(oldBuffer);

which is fine as long as the indented parts either aren't there, since on reassigning buffer = ... the old reference is done and is allowed to reach RC=0, while with indented parts the same reference is still live, kept alive by another set of __retain/__release which the pass eliminates too early. Need to think about this, but I don't see a proper solution yet, just shitty ones.

1. Whenever a reference enters a scope, always release it at the end and never in between, i.e. on reassign, potentially using a ton of autorelease locals Doesn't work with loops
2. autorelease buffer that is processed when dropping out of Wasm, probably ugly codegen
3. Let the ARC pass count references and do alias analysis?!

[dcodeIO]

To be sure, I also tested with the Binaryen PR linked above, and found that it doesn't solve the issue.

[dcodeIO]

Have been looking into a fix that would have been a breaking change, but after inspecting the fixtures figured that the particular approach I tried is not worth it compared to just disabling the pass. So, the linked PR above instead disables aggressive ARC optimizations for now, which seems reasonable to do given that there's still no better fix in sight.