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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions quickjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40974,16 +40974,16 @@ static double js_math_fround(double a)
static JSValue js_math_imul(JSContext *ctx, JSValue this_val,
int argc, JSValue *argv)
{
int a, b;
uint32_t a, b, c;
int32_t d;

if (JS_ToInt32(ctx, &a, argv[0]))
if (JS_ToUint32(ctx, &a, argv[0]))
return JS_EXCEPTION;
if (JS_ToInt32(ctx, &b, argv[1]))
if (JS_ToUint32(ctx, &b, argv[1]))
return JS_EXCEPTION;
/* TODO(bnoordhuis) Signed integral narrowing has implementation-defined
* 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 •
edited
Loading

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;

memcpy(&d, &c, sizeof(d));
return js_int32(d);
}

static JSValue js_math_clz32(JSContext *ctx, JSValue this_val,
Expand Down
Loading