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

Attempt to warn when use of ternary op might fail due to side effects #241

Closed
andythenorth opened this issue Oct 10, 2021 · 1 comment
Closed

Comments

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Oct 10, 2021

Discovered via https://gist.github.com/andythenorth/64f3532ee4c148c5d7f9bf21ba36cda1#file-gistfile1-txt

FIRS code using procedures and storage access in a ternary op appeared to be incorrectly evaluating as true

e.g. foo() < bar() ? a : b was returning True for a case where value of foo() and bar() should have been 12 and 9, e.g. 12 < 9 ? a : b returned a.

glx diagnosed this as a case where temp register 0x100 (?) is re-used within the chain, which causes incorrect results.

The fix is to not use ternary op in these cases. glx has a commit which might function as a warning in these cases.

@glx22
Copy link
Contributor

@glx22 glx22 commented Oct 10, 2021

No the issue is not the reuse of temp register 0x100, but the way ternary are compiled.

For guard ? expr_true : expr_false, first guard is evaluated, then stored as guard, and it's opposite stored as !guard. Then some magic happens for the result, it's translated as !guard * expr_false + guard * expr_true, meaning both expressions are evaluated in any case.

If any of both expression writes to a register, you have an unexpected side effect.

Loading

@andythenorth andythenorth changed the title Attempt to warn when use of ternary op might fail due to over-writing temp register (?) Attempt to warn when use of ternary op might fail due to side effects Oct 10, 2021
@glx22 glx22 closed this in #243 Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants