Skip to content

Fix local confusion in builtin min/max/rotl/rotr #1540

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

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Nov 9, 2020

The emulated min/max builtins for integer types compile to a select expression with two temporary locals

(select
 (t1 = arg1)
 (t2 = arg2)
 (t1 > t2)
)

where arg2 may itself utilize a temporary local, and since arg2 is compiled before the temporary locals for the select are obtained, it may have used the same then-free'd local, in turn overwriting the value of t1 when executing arg2, ultimately breaking the condition executed last as reported.

This PR makes sure that t1 is not a local used in arg2.

Fixes #1537

  • I've read the contributing guidelines

@@ -1484,7 +1485,7 @@ function builtin_max(ctx: BuiltinContext): ExpressionRef {
if (op != -1) {
let flow = compiler.currentFlow;
let nativeType = type.toNativeType();
let temp1 = flow.getTempLocal(type);
let temp1 = flow.getTempLocal(type, findUsedLocals(arg1));
Copy link
Member

@MaxGraey MaxGraey Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about make getUnusedLocal which try to find unused local in already allocated locals or allocate new one otherwise? And use flow.getUnusedLocal(type) here and in other places?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findUsedLocals is what other code typically uses to keep the number of necessary locals low. Otherwise we'd always get a new local, never a reused one, I guess?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also builtin_abs, builtin_rotl / builtin_rotr I guess also required this fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, at some point I expect us to move away from free'ing temporary locals, but we'll have to get rid of any kind of recompiling expressions first. There are still a few places doing this, and getUnusedLocal would return a new local upon recompile, yielding different code, while findUsedLocals doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also builtin_abs, builtin_rotl / builtin_rotr I guess also required this fix

Abs is a unary expression not executing input code in between assignment and usage of the temporary locals. Should be safe. Going to take a look at rotl/rotr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findUsedLocals is what other code typically uses to keep the number of necessary locals low. Otherwise we'd always get a new local, never a reused one, I guess?

I guess better always alloc new loacal and let binaryen optimize all of this afterwards

Copy link
Member Author

@dcodeIO dcodeIO Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect us to move into that direction, yeah, but we cannot do that as long as we are recompiling code (and not resetting the local pool with a new mechanism) because it would confuse local flags (adding new locals on each recompile, leaving earlier ones unused), then for example leading to infinite loops when attempting to unify local flags in loops.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that rotl for some reason does not exhibit the behavior on rotl(a, rotl(b, c)), but should be susceptible just as rotr is, which trivially breaks without the fix. Probably coincidence. Applied the same fix for both.

Copy link
Member

@MaxGraey MaxGraey Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then for example leading to infinite loops when attempting to unify local flags in loops.

I see. Ok, in this case PR LGTM

@dcodeIO dcodeIO changed the title Fix local confusion in builtin min/max Fix local confusion in builtin min/max/rotl/rotr Nov 9, 2020
@dcodeIO dcodeIO merged commit d0a2c9e into master Nov 9, 2020
@MaxGraey MaxGraey deleted the issue-1537 branch November 29, 2020 17:02
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.

Incorrect compiler output for nested min/max builtins
2 participants