Fix fully initializing JSStackFrame

[saghul]

Fixes: #323

[saghul]

@bnoordhuis @chqrlie PTAL.

[chqrlie]

This is not the correct fix: you should instead initialize sf->cur_pc along with pc on line 14603, before the stack frame gets linked.

See the explanation in the issue conversation #323

[saghul]

Will do!

[saghul]

You mean to initialize it to NULL? Or to find all places in the opcode handling where it needs to be set to pc? Both?

[chqrlie]

No, not to NULL:

    pc = b->byte_code_buf;
    s->cur_pc = pc;

This allows build_backtrace() to find the source position for the code that ultimately caused the error to be thrown even if it is triggered by an opcode that did not set sf->cur prior to calling some function in JS_CallInternal() and no other opcode did it before said call. Your minimal reproducible example only generated 3 OP_get_xxx opcodes.

[chqrlie]

Many more opcodes may need to set sf->cur_pc... almost all of those that test for exceptions and goto exception if we want a debugger to track the call chain properly when triggered by the JS_Error constructor.

Without such a debugger (wish I could spend more time on that) we should set sf->cur_pc = pc for all opcodes that may cause a recursive call to JS_CallInternal().

• OP_get_length
• OP_push_this before the call to JS_ToObject
• OP_throw_error
• OP_apply_eval
• OP_import
• OP_get_var_undef, OP_get_var, OP_put_var, OP_put_var_init, OP_put_var_strict
• OP_define_var, OP_define_func
• OP_for_in_start, OP_for_in_next, OP_for_of_start, OP_for_of_next, OP_for_await_of_start
• OP_iterator_next
• OP_iterator_call
• OP_get_private_field, OP_put_private_field
• OP_get_array_el, OP_get_array_el2, OP_get_ref_value, OP_get_super_value
• OP_put_array_el, OP_put_ref_value, OP_put_super_value
• OP_append, OP_copy_data_properties
• OP_add before the call to js_add_slow()
• OP_add_loc before the calls to JS_ToPrimitiveFree() and js_add_slow()
• at the label binary_arith_slow:
• OP_plus, OP_neg, OP_inc, OP_dec before the calls to js_unary_arith_slow()
• OP_post_inc, OP_post_dec
• OP_inc_loc, OP_dec_loc before the calls to js_unary_arith_slow()
• OP_not before the call to js_not_slow()
• OP_shl, OP_shr, OP_sar, OP_and, OP_or, OP_xor before the calls to js_binary_logic_slow()
• if the OP_CMP macro before the slow_call
• OP_to_object
• all the OP_with_xxx opcodes

Instead of carefully, I should have written painstakingly :)

[chqrlie]

I wondered if setting cur_pc before the dispatch could have adverse affects.

I don't think so. The stack frame is just uninitialized there and it is a potential problem.
The alternate path where the generator function is restarted gets pc from sf->cur_pc and branches to restart:, so no conflict there either.

[saghul]

Alright. So then it looks like we could get away with doing that instead of setting it in each opcode. I'll dig some more tonight!

[chqrlie]

No, initializing it before the switch is a safety measure to avoid a potential crash if sf->cur_pc gets used without having been set, but it is not sufficient for build_backtrace() to get the correct source location when the stack trace is built.
We use pc instead of sf->cur_pc for the dispatch to optimize the code and let the compiler use a register instead of updating the stack frame for each opcode. Same for sp and sf->cur_sp.
sf->cur_pc must be synchronized with pc before a recursive call to JS_CallInternal so the stack frame contains the proper address for build_backtrace() to compute the source location of the caller.

[saghul]

I see. In that case it might be better to initialize it to NULL and add an asset inside build_backtrace so we can catch 'em all. WDYT?

[chqrlie]

We could do that. sf->cur_pc should not be null for bytecoded functions, but is null for C functions.

[saghul]

Valgrind run on this branch: https://github.com/quickjs-ng/quickjs/actions/runs/8404692156

[saghul]

Caught some more, new CI: https://github.com/quickjs-ng/quickjs/actions/runs/8405035727

[chqrlie]

Caught some more, new CI: https://github.com/quickjs-ng/quickjs/actions/runs/8405035727

The culprit is js_call_c_function where sf->cur_pc is not initialized. You should set sf->cur_pc and sf->cur_sp to NULL in this case, after setting sf->cur_func.

[saghul]

Alright, got the tests green!

There might me more places missing, but the test suite does not exercise those :-/

I kicked off a Valgrind run too, let's see 😅

[saghul]

Valgrind is green too: https://github.com/quickjs-ng/quickjs/actions/runs/8409873927

[chqrlie]

Interesting to see that this fixes a v8 test...