Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #2730 compound assignment with post increment in lhs miscompiles #2774

Closed
wants to merge 1 commit into from

Conversation

Changqing-JING
Copy link
Contributor

fix #2730

@Changqing-JING Changqing-JING changed the title fix #2730 ompound assignment with post increment in lhs miscompiles fix #2730 compound assignment with post increment in lhs miscompiles Oct 17, 2023
@CountBleck
Copy link
Member

what the heck

That looks bigger than it's supposed to be.

@MaxGraey
Copy link
Member

Also, it looks like produces a lot of unnecessary extra locals which do only local.set $x / local.get $x sequentially and nothing more

@Changqing-JING
Copy link
Contributor Author

@CountBleck @MaxGraey
Thank you for review. I also agree that this fix is not very beautiful. But unfortunately, after aligned with @HerrCai0907, we think there is no way better than this, except we make big refactor on current code.

This bug can be easily fixed as supposed because current compile flow emit instruction as pesudo code

setter(this, indexStatement, getter(indexStatement));

The problem here is, assemblyscript + binary organize statements as tree and compiles from top to down. When compile the setter function, the getter(indexStatement) is just a statement. The compiler can't easily know there is a common indexStatement which need to be compiled in advanced.

To fix this bug, we need

index = indexStatement;
tmp = getter(index)
setter(this, index, tmp);

To achieve this, we need to hack a bit into the compiled getter(indexStatement). Otherwise, in assemblyscript's top down compile flow, it's difficult to let indexStatement be compiled in advanced.

This also explained by additional temps are created. But this shall not be a big problem because binary optimization pass can remove them.

@HerrCai0907
Copy link
Member

I think it is a more generic issue for compound assignment instead of only for IndexSignature

This code will call getA() twice.

class A {
  a: i32 = 0;
}

function getA(): A {
  trace("get A");
  return new A();
}

export function _start(): void {
  getA().a |= 10;
}

@CountBleck
Copy link
Member

Unless someone else finds a cleaner alternative to whatever this mess1 is, this is something I'll definitely have to look into myself, once I have the time.

Footnotes

  1. Attaching a cache to an AST node is straight up weird, and the way it's used seems like a hack around expressions being compiled twice. Another hack is the patching of previously generated Binaryen IR so that it uses a local. CompiledExpression in general seems like something that should/could be removed (it's not used in any other part of AS, nor can it be effectively used in transforms AFAIK).

@Changqing-JING
Copy link
Contributor Author

Heacking the previsouly generated IR problem is solved, which makes the commit clearer. But this still can't solve the getA().a |= 10; problem. So I didn't update this to PR. I will also think about a better solution.
Changqing-JING@a3cb730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compound assignment with post increment in lhs miscompiles
4 participants