Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix js_math_imul #356

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Fix js_math_imul #356

merged 1 commit into from
Apr 8, 2024

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented Apr 7, 2024

  • follow ECMA specification
  • remove implementation defined signed conversion

- follow ECMA specification
- remove implementation defined signed conversion
@saghul saghul requested a review from bnoordhuis April 7, 2024 15:38
* behavior but that's a step up from the undefined behavior it replaced.
*/
return js_int32((int64_t)a * (int64_t)b);
c = a * b;
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a while to figure out how this could possibly be correct but the trick is that JS_ToUint32 is just a call to JS_ToInt32 with the argument cast to int32_t * 🤦

I think we're in a state of sin^Wimplementation/undefined behavior here but okay, it's no worse than before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's correct. The correct solution is to implement JS_ToInt32 as a hack around JS_ToUint32 as specified in ECMA, not the other way around as is done right now.

Copy link
Collaborator Author

@chqrlie chqrlie Apr 7, 2024

Choose a reason for hiding this comment

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

More specifically, JS_ToInt32 has this code with ret defined as int ret;:

                ret = v >> 32;
                /* take the #to account */
                if (u.u64 >> 63)
                    if (ret != INT32_MIN)
                        ret = -ret;

For JS_ToUint32, with ret defined as uint32_t, the code could be made simpler and without implementation defined behavior:

                uint32_t ret = v >> 32;
                /* take the #to account */
                if (u.u64 >> 63)
                     ret = -ret;

Or this branchless alternative:

                uint32_t ret = v >> 32;
                uint32_t sign = u.u64 >> 63;
                ret = (ret ^ (-sign)) + sign;

@chqrlie chqrlie merged commit 0658d9c into quickjs-ng:master Apr 8, 2024
46 checks passed
@chqrlie chqrlie deleted the fix-js-math-imul branch April 8, 2024 20:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants