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

[Question] Aligment of arguments in CFunctionData Calling for 32bits target #802

Closed
Icemic opened this issue Jan 8, 2025 · 8 comments
Closed

Comments

@Icemic
Copy link
Contributor

Icemic commented Jan 8, 2025

Background:

  1. Only talking about 32bits targets, or more specific as x86 Windows. (JS_NAN_BOXING=1)
  2. Using JS_NewCFunctionData to expose a native function for javascript calling
  3. In the callback of my native function, argv (uint64_t *) is always aligned to 8 in previous versions

I noticed that argv is no longer align to 8 but 4 after this commit.

I cannot find something special in this commit but if I modifies the new JS_FreeValue from

void JS_FreeValueRT(JSRuntime *rt, JSValue v)
{
    if (JS_VALUE_HAS_REF_COUNT(v)) {
        JSRefCountHeader *p = (JSRefCountHeader *)JS_VALUE_GET_PTR(v);
        if (--p->ref_count <= 0) {
            js_free_value_rt(rt, v);
        }
    }
}

void JS_FreeValue(JSContext *ctx, JSValue v)
{
    JS_FreeValueRT(ctx->rt, v);
}

to

void JS_FreeValueRT(JSRuntime *rt, JSValue v)
{
    if (JS_VALUE_HAS_REF_COUNT(v)) {
        JSRefCountHeader *p = (JSRefCountHeader *)JS_VALUE_GET_PTR(v);
        if (--p->ref_count <= 0) {
            js_free_value_rt(rt, v);
        }
    }
}

void JS_FreeValue(JSContext *ctx, JSValue v)
{
    if (JS_VALUE_HAS_REF_COUNT(v)) {
        JSRefCountHeader *p = (JSRefCountHeader *)JS_VALUE_GET_PTR(v);
        if (--p->ref_count <= 0) {
            js_free_value_rt(ctx->rt, v);
        }
    }
}

argv is aligned to 8 back again.

And those are my questions:

  1. Should argv be aligned to 8? (when JSValue is uint64_t)
  2. If yes, is it a bug or something magic that causing this behaviour?
@Icemic
Copy link
Contributor Author

Icemic commented Jan 8, 2025

Not any custom function can reproduce this problem, I'm trying to make a minimal reproduction demo.

@Icemic
Copy link
Contributor Author

Icemic commented Jan 8, 2025

#include "quickjs.c"

JSValue custom_func(JSContext *ctx, JSValue this_val, int argc, JSValue *argv, int magic, JSValue *func_data)
{
  printf("custom_func\n");
  JSValue data = func_data[0];
  int32_t data_val = JS_VALUE_GET_INT(data);
  printf("data: %d\n", data_val);

  printf("argc: %d\n", argc);
  printf("argv: %d, argv is aligned to 8: %d\n", argv, (uintptr_t)argv % 8 == 0);

  const char *ret_str = "fulfilled";
  JSValue ret = JS_NewString(ctx, ret_str);
  return ret;
}

JSValue JS_PromiseWithResolvers(JSContext *ctx)
{
  return js_promise_withResolvers(ctx, ctx->promise_ctor, 0, NULL);
}

JSValue JS_PromiseThen(JSContext *ctx, JSValue promise, JSValue on_fulfilled_func)
{
  JSValue argv[1] = {on_fulfilled_func};
  return js_promise_then(ctx, promise, 1, argv);
}

int main(void)
{
  JSRuntime *rt = JS_NewRuntime();
  JSContext *ctx = JS_NewContext(rt);

  // follwing code is similar to JavaScript code:
  // ```
  // let obj = {};
  // let {promise, resolve, reject} = new Promise(() => {});
  // let data = 42;
  // let on_fulfilled_func = (v) => {
  //   console.log(data);
  //   console.log(v);
  //   return "fulfilled";
  // };
  // let promise2 = promise.then(on_fulfilled_func);
  // resolve();
  // ```


  JSValue obj = JS_PromiseWithResolvers(ctx);
  JSValue promise = JS_GetPropertyStr(ctx, obj, "promise");
  JSValue resolve = JS_GetPropertyStr(ctx, obj, "resolve");
  JSValue reject = JS_GetPropertyStr(ctx, obj, "reject");

  JSValue data = JS_NewInt32(ctx, 42);
  JSValue on_fulfilled_func = JS_NewCFunctionData(ctx, &custom_func, 1, 0, 1, &data);

  JSValue promise2 = JS_PromiseThen(ctx, promise, on_fulfilled_func);

  JS_Call(ctx, resolve, JS_MKVAL(JS_TAG_NULL, 0), 0, NULL);

  // create pctx
  JSContext *pctx = NULL;
  while (1)
  {
    int ret = JS_ExecutePendingJob(rt, &pctx);
    if (ret <= 0)
    {
      printf("ret: %d\n", ret);
      break;
    }
  }

  JS_FreeValue(ctx, obj);
  JS_FreeValue(ctx, promise);
  JS_FreeValue(ctx, resolve);
  JS_FreeValue(ctx, reject);
  JS_FreeValue(ctx, data);
  JS_FreeValue(ctx, on_fulfilled_func);
  JS_FreeValue(ctx, promise2);

  JS_FreeContext(ctx);
  JS_FreeRuntime(rt);
  return 0;
}

In this case, I get both align and non-align output randomly.

// which is ok

custom_func
data: 42
argc: 1
argv: 8322016, argv is aligned to 8: 1
ret: 0

// which is problematic

custom_func
data: 42
argc: 1
argv: 9436132, argv is aligned to 8: 0
ret: 0

@Icemic
Copy link
Contributor Author

Icemic commented Jan 8, 2025

BTW, the magic "replacing JS_FreeValue body" not work on this case. It just works for my Rust version case...

@saghul
Copy link
Contributor

saghul commented Jan 8, 2025

I'm not sure it was guaranteed to be aligned in the first place...

That said, I'm out of my depth so maybe @bnoordhuis has am idea here.

@bnoordhuis
Copy link
Contributor

See rust-lang/rust#112480 - tl;dr msvc quirk or, depending how you look at it, bug.

Are you running into a situation where C and Rust disagree on alignment or ...?

@Icemic
Copy link
Contributor Author

Icemic commented Jan 9, 2025

See rust-lang/rust#112480 - tl;dr msvc quirk or, depending how you look at it, bug.

Are you running into a situation where C and Rust disagree on alignment or ...?

Yeah, I used to acquiring arguments by Rust's std::slice::from_raw_parts which needs the pointer to be aligned. It panics now so I find this issue.

This problem is easy to get around, but I think it's some kind unusual and may cause a little inefficiency, so I came to ask.

As it can be a bug (of MSVC though), will you fix it or just leave it for some time?

@bnoordhuis
Copy link
Contributor

I'll go ahead and close this because I don't think we can work around it if we wanted to, not robustly or reliably. That it used to align was just a happy accident. You'll have to take this up with either the MSVC or the Rust people.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2025
@Icemic
Copy link
Contributor Author

Icemic commented Jan 9, 2025

That it used to align was just a happy accident.

Yeah, I'm afraid so.

Thanks for your help!

For someone encounters the same problem as me, there are two choices to get around it:

  1. Avoid using std::slice::from_raw_parts but read elements by pointer offsets manually. A little inefficiency but it's ok. Though we don't known if there's still some more happy accidents MSVC prepared for us...
  2. Switch to use Clang on Windows and get consistent outputs across platforms.

I've chosen 2.

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

No branches or pull requests

3 participants