Njs: bind() segfault.

[xeioex]

From https://github.com/nginx/njs/issues/106#issuecomment-480480675

function foo() {
    var t = 2;

    function baz() {
        t = 3;
    }

    baz.bind()()
}

foo();

./build/njs bind_bug.js                                                                                                       
AddressSanitizer:DEADLYSIGNAL                                                                                                   
=================================================================                                                                
==2195==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004c6c12 bp 0x7ffec1e80c70 sp 0x7ffec1e80418 T0
)                                                                                                                             
==2195==The signal is caused by a READ memory access.                                                                            
==2195==Hint: address points to the zero page.                                                                             
    #0 0x4c6c11 in __asan::QuickCheckForUnpoisonedRegion(unsigned long, unsigned long) (/home/xeioex/workspace/nginx/nginScript/n
js/build/njs+0x4c6c11)                                                                                                          
    #1 0x4c6b71 in __asan_memcpy (/home/xeioex/workspace/nginx/nginScript/njs/build/njs+0x4c6b71)                                
    #2 0x515973 in njs_vmcode_interpreter /home/xeioex/workspace/nginx/nginScript/njs/njs/njs_vm.c:176:27                      
    #3 0x513db4 in njs_vm_start /home/xeioex/workspace/nginx/nginScript/njs/njs/njs.c:594:11                                     
...

[hongzhidao]

@xeioex

Take a look.

diff -r 1bce636aed3d njs/njs_function.c
--- a/njs/njs_function.c    Sun Mar 31 22:59:04 2019 +0800
+++ b/njs/njs_function.c    Sat Apr 06 17:05:32 2019 +0800
@@ -83,7 +83,7 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return copy;
     }

-    copy->closure = 1;
+    copy->closure = 1; /* It seems wrong. */

     n = 0;

@@ -434,6 +434,10 @@ njs_function_lambda_call(njs_vm_t *vm, n
     n = 0;
     nesting = lambda->nesting;

+    /*
+     * explain why do we need function->closure?
+     * what's the diffenence of function->closures and vm->active_frame->closures?
+     */
     if (nesting != 0) {
         closures = (function->closure) ? function->closures
                                        : vm->active_frame->closures;

Another place.

static njs_function_t *
njs_parser_function_alloc(njs_vm_t *vm, njs_parser_t *parser,
    njs_variable_t *var)
{
    njs_value_t            *value;
    njs_function_t         *function;
    njs_function_lambda_t  *lambda;

    lambda = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_function_lambda_t));
    if (nxt_slow_path(lambda == NULL)) {
        njs_memory_error(vm);
        return NULL;
    }

    /* TODO:
     *  njs_function_t is used to pass lambda to
     *  njs_generate_function_declaration() and is not actually needed.
     *  real njs_function_t is created by njs_vmcode_function() in runtime.
     */
    /* So it still need more work? */

    function = njs_function_alloc(vm, lambda, NULL, 1);
    if (nxt_slow_path(function == NULL)) {
        return NULL;
    }

[hongzhidao]

This bug also happens in expression function.

function foo() {
    var a;
    (function() {
        a = 1;
    }).bind()()
}
foo();

[hongzhidao]

@xeioex

Review the patch, please.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1554566949 -28800
# Node ID 824c384406bf93708d78fd5d3981f0858e4bb01d
# Parent  7dba758fda64edab90d80a7b44f4a29fc3ce5f91
Fixed bind() segfault.

diff -r 7dba758fda64 -r 824c384406bf njs/njs_function.c
--- a/njs/njs_function.c    Fri Apr 05 17:49:22 2019 +0300
+++ b/njs/njs_function.c    Sun Apr 07 00:09:09 2019 +0800
@@ -8,6 +8,8 @@
 #include <string.h>


+static njs_function_t *njs_function_copy(njs_vm_t *vm,
+    njs_function_t *function);
 static njs_native_frame_t *njs_function_frame_alloc(njs_vm_t *vm, size_t size);
 static njs_ret_t njs_normalize_args(njs_vm_t *vm, njs_value_t *args,
     uint8_t *args_types, nxt_uint_t nargs);
@@ -71,8 +73,6 @@ fail:
 njs_function_t *
 njs_function_value_copy(njs_vm_t *vm, njs_value_t *value)
 {
-    size_t          size;
-    nxt_uint_t      n, nesting;
     njs_function_t  *function, *copy;

     function = value->data.u.function;
@@ -81,6 +81,25 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return function;
     }

+    copy = njs_function_copy(vm, function);
+    if (nxt_slow_path(copy == NULL)) {
+        njs_memory_error(vm);
+        return NULL;
+    }
+
+    value->data.u.function = copy;
+
+    return copy;
+}
+
+
+static njs_function_t *
+njs_function_copy(njs_vm_t *vm, njs_function_t *function)
+{
+    size_t          size;
+    nxt_uint_t      n, nesting;
+    njs_function_t  *copy;
+
     nesting = (function->native) ? 0 : function->u.lambda->nesting;

     size = sizeof(njs_function_t) + nesting * sizeof(njs_closure_t *);
@@ -91,8 +110,6 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return NULL;
     }

-    value->data.u.function = copy;
-
     *copy = *function;
     copy->object.__proto__ = &vm->prototypes[NJS_PROTOTYPE_FUNCTION].object;
     copy->object.shared = 0;
@@ -1050,18 +1067,12 @@ njs_function_prototype_bind(njs_vm_t *vm
         return NXT_ERROR;
     }

-    function = nxt_mp_alloc(vm->mem_pool, sizeof(njs_function_t));
+    function = njs_function_copy(vm, args[0].data.u.function);
     if (nxt_slow_path(function == NULL)) {
         njs_memory_error(vm);
         return NXT_ERROR;
     }

-    *function = *args[0].data.u.function;
-
-    function->object.__proto__ = &vm->prototypes[NJS_PROTOTYPE_FUNCTION].object;
-    function->object.shared = 0;
-    function->object.extensible = 1;
-
     if (nargs == 1) {
         args = (njs_value_t *) &njs_value_undefined;

diff -r 7dba758fda64 -r 824c384406bf njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c  Fri Apr 05 17:49:22 2019 +0300
+++ b/njs/test/njs_unit_test.c  Sun Apr 07 00:09:09 2019 +0800
@@ -6356,6 +6356,12 @@ static njs_unit_test_t  njs_test[] =
                  "var b = f.bind('1', '2', '3'); b.apply()"),
       nxt_string("123") },

+    { nxt_string("function f() { var a; return (function() { a = 1; return a; }).bind()() } f()"),
+      nxt_string("1") },
+
+    { nxt_string("function f() { var a; function baz() { a = 1; return a; } return baz.bind()(); } f()"),
+      nxt_string("1") },
+
     { nxt_string("var obj = {prop:'abc'}; "
                  "var func = function(x) { "
                  "    return this === obj && x === 1 && arguments[0] === 1 "

@drsm Welcome to test. Thanks again.

[xeioex]

@hongzhidao

+    /*
+     * explain why do we need function->closure?
+     * what's the diffenence of function->closures and vm->active_frame->closures?
+     */

good question, I also need to dig in.

/* So it still need more work? */

I think so. But it can be improved later, not a problem now.

The patch looks good. Committed, thanks!

[igorsysoev]

 * explain why do we need function->closure?
 * what's the diffenence of function->closures and vm->active_frame->closures?

vm->active_frame->closures is array of parent functions scopes that are referenced in nested functions.
function->closure means that the function has closures (function->closures) captured on function creation.

When parent function calls nested function, the nested function should use current active frame closures. When parent function creates closure from nested function (by returning it or by assigning it to variable or property) the nested function should capture current active frame closures. And when the nested closure function will later be called after parent function will complete, the nested closure function should use the captured closures.

[hongzhidao]

@xeioex @drsm

function foo() {
    var t;

    function baz() {
        return t;
    }

    return baz;
}

foo().bind()();

Not completely fixed. On it.

[hongzhidao]

@igorsysoev @xeioex

Review again.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1554615484 -28800
# Node ID 049800e92ef6fb0b216c2a5351031b0eab5680d5
# Parent  84cb61f23b7e3075dd21ff0ead228157bc0dd0e9
Fixed njs_function_copy().

diff -r 84cb61f23b7e -r 049800e92ef6 njs/njs_function.c
--- a/njs/njs_function.c    Sun Apr 07 00:09:09 2019 +0800
+++ b/njs/njs_function.c    Sun Apr 07 13:38:04 2019 +0800
@@ -9,7 +9,7 @@


 static njs_function_t *njs_function_copy(njs_vm_t *vm,
-    const njs_function_t *function);
+    const njs_function_t *function, njs_closure_t *closures[]);
 static njs_native_frame_t *njs_function_frame_alloc(njs_vm_t *vm, size_t size);
 static njs_ret_t njs_normalize_args(njs_vm_t *vm, njs_value_t *args,
     uint8_t *args_types, nxt_uint_t nargs);
@@ -81,7 +81,7 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return function;
     }

-    copy = njs_function_copy(vm, function);
+    copy = njs_function_copy(vm, function, vm->active_frame->closures);
     if (nxt_slow_path(copy == NULL)) {
         njs_memory_error(vm);
         return NULL;
@@ -94,7 +94,8 @@ njs_function_value_copy(njs_vm_t *vm, nj


 static njs_function_t *
-njs_function_copy(njs_vm_t *vm, const njs_function_t *function)
+njs_function_copy(njs_vm_t *vm, const njs_function_t *function,
+    njs_closure_t *closures[])
 {
     size_t          size;
     nxt_uint_t      n, nesting;
@@ -106,7 +107,6 @@ njs_function_copy(njs_vm_t *vm, const nj

     copy = nxt_mp_alloc(vm->mem_pool, size);
     if (nxt_slow_path(copy == NULL)) {
-        njs_memory_error(vm);
         return NULL;
     }

@@ -124,7 +124,7 @@ njs_function_copy(njs_vm_t *vm, const nj

     do {
         /* GC: retain closure. */
-        copy->closures[n] = vm->active_frame->closures[n];
+        copy->closures[n] = closures[n];
         n++;
     } while (n < nesting);

@@ -1060,14 +1060,16 @@ njs_function_prototype_bind(njs_vm_t *vm
 {
     size_t          size;
     njs_value_t     *values;
-    njs_function_t  *function;
+    njs_function_t  *src, *function;

     if (!njs_is_function(&args[0])) {
         njs_type_error(vm, "\"this\" argument is not a function");
         return NXT_ERROR;
     }

-    function = njs_function_copy(vm, args[0].data.u.function);
+    src = args[0].data.u.function;
+
+    function = njs_function_copy(vm, src, src->closures);
     if (nxt_slow_path(function == NULL)) {
         njs_memory_error(vm);
         return NXT_ERROR;
diff -r 84cb61f23b7e -r 049800e92ef6 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c  Sun Apr 07 00:09:09 2019 +0800
+++ b/njs/test/njs_unit_test.c  Sun Apr 07 13:38:04 2019 +0800
@@ -6362,6 +6362,12 @@ static njs_unit_test_t  njs_test[] =
     { nxt_string("function f() { var a; function baz() { a = 1; return a; } return baz.bind()(); } f()"),
       nxt_string("1") },

+    { nxt_string("(function(){ var a = 1; return (function() { return a; })})().bind()()"),
+      nxt_string("1") },
+
+    { nxt_string("function f() { var a = 1; function baz() { return a; } return baz; } f().bind()()"),
+      nxt_string("1") },
+
     { nxt_string("function f(a, b) { return a + b }"
                  "f(3,4) === f.bind()(3,4)"),
       nxt_string("true") },

1. I think it's right that copy closures from function->closures in bind.
2. I'm not sure what closures should be used in OBJECT COPY, is function->closures or vm->active_frame->closures?

BTW, this also fixes the below issue happened in arrow function.

#if 0
    { nxt_string("(function() { return (() => this); })"
                 ".call('abc').bind('bca')()"),
      nxt_string("abc") },
#endif

[xeioex]

@hongzhidao

I think it's right that copy closures from function->closures in bind.

agree.

I'm not sure what closures should be used in OBJECT COPY, is function->closures or vm->active_frame->closures

I looks like to me, it should also be function->closures, but the following test fails for example (if we replace vm->active_frame->closures -> function->closures):

> function f(a) { function g(b) { return a + b } return g }var k = f('a'); k('b')
'ab'

But, maybe it is related to another issue.

[igorsysoev]

I think it's right that copy closures from function->closures in bind.
agree.

As I understand using nested function in bind() should create JS closure. So:

copy_closures = (original_function->closure) ? original_function->closures : vm->active_frame->closures

[igorsysoev]

I looks like to me, it should also be function->closures, but the following test fails for example

No, it should depend on function->closure flag. If it is set, then use function->closures, otherwise vm->active->frame->closures.

[hongzhidao]

@igorsysoev @xeioex

Another issue (coredump).

function f(a) { var g; function g(b) { return a + b } return g }
var k = f('a');
k('b')

typedef struct njs_generator_patch_s   njs_generator_patch_t;
@@ -536,7 +537,11 @@ njs_generate_name(njs_vm_t *vm, njs_gene
         return NXT_ERROR;
     }

-    if (var->type == NJS_VARIABLE_FUNCTION) {
+    /*
+     * Easily making var->type not NJS_VARIABLE_FUNCTION
+     * var f;
+     */
+    if (var->type == NJS_VARIABLE_FUNCTION) {

         node->index = njs_generate_dest_index(vm, generator, node);

It seems the design of variable (including function declare) need to be improved.

BTW, we can hoist function declaration if necessary like import statement.
My thoughts.

1. Don't set variable any value at parser phase, even at the generation phase. Only allowed at runtime phase.
2. function foo() {} => var foo = anonymous function expression().

[hongzhidao]

I have a question about hoist in JS.

It says hoist happens in variable and function declaration.
In njs, as I understand, variable declartion doesn't generate code.
So I think we only consider function declaration hoist if we want to introduce hoist in njs.

@drsm can you explain hoist, that you are good at JS?

[xeioex]

@hongzhidao

Another issue (coredump).

how is that different from https://github.com/nginx/njs/issues/126#issuecomment-480568609 example?

[hongzhidao]

@hongzhidao

Another issue (coredump).

how is that different from #126 (comment) example?

1. bind bug.

function foo() {
    var t = 2;

    function baz() {
        t = 3;
    }

    baz.bind()()
}

foo();

1. Notice that variable g is declared before function declaration. It has no concern with bind.

function f(a) { var g; function g(b) { return a + b } return g }
var k = f('a');
k('b')

[drsm]

@hongzhidao

BTW, we can hoist function declaration if necessary like import statement.

var name; and function name() {} should be hoisted.

https://tc39.github.io/ecma262/#sec-variable-statement

A var statement declares variables that are scoped to the running execution context's VariableEnvironment. Var variables are created when their containing Lexical Environment is instantiated and are initialized to undefined when created. Within the scope of any VariableEnvironment a common BindingIdentifier may appear in more than one VariableDeclaration but those declarations collectively define only one variable. A variable defined by a VariableDeclaration with an Initializer is assigned the value of its Initializer's AssignmentExpression when the VariableDeclaration is executed, not when the variable is created.

https://tc39.github.io/ecma262/#sec-block-static-semantics-toplevelvardeclarednames

NOTE
At the top level of a function or script, inner function declarations are treated like var declarations.

[hongzhidao]

@hongzhidao

BTW, we can hoist function declaration if necessary like import statement.

var name; and function name() {} should be hoisted.

https://tc39.github.io/ecma262/#sec-variable-statement

A var statement declares variables that are scoped to the running execution context's VariableEnvironment. Var variables are created when their containing Lexical Environment is instantiated and are initialized to undefined when created. Within the scope of any VariableEnvironment a common BindingIdentifier may appear in more than one VariableDeclaration but those declarations collectively define only one variable. A variable defined by a VariableDeclaration with an Initializer is assigned the value of its Initializer's AssignmentExpression when the VariableDeclaration is executed, not when the variable is created.

https://tc39.github.io/ecma262/#sec-block-static-semantics-toplevelvardeclarednames

NOTE
At the top level of a function or script, inner function declarations are treated like var declarations.

Thanks. So it seems we needn't consider variable hoist, since all variables belongs to scope->variables in njs.

[drsm]

@hongzhidao

> ((a) => { var s = typeof g, q = g; var g = 1; s += typeof g; function g(b) { return a + b }; console.log(s); return q; })(1)(2)
'functionnumber'
Segmentation fault

[hongzhidao]

@hongzhidao

> ((a) => { var s = typeof g, q = g; var g = 1; s += typeof g; function g(b) { return a + b }; console.log(s); return q; })(1)(2)
'functionnumber'
Segmentation fault

It looks an issue related to variable.

(function(a) { var s = typeof g, q = g; var g = 1; s += typeof g; function g(b) { return a + b }; console.log(s); return q; })(1)(2)

[drsm]

@hongzhidao

the problem arise when we assign a value to the variable generated by the function definition:

>> function f(a) { var q = g; function g(b) { return a + b; }; return q; }; f(1)(2);
3
>> function f(a) { var q = g; var g = 1; function g(b) { return a + b; }; return q; }; f(1)(2);
Segmentation fault
>> function f(a) { var g = g; function g(b) { return a + b; }; return g; }; f(1)(2);
Segmentation fault

the code generated is somewhat different (but should be identical):

>> function f(a) { var q = g; g = 1; function g(b) { return a + b; }; return q; }; f(1)(2);
shell:g
00000 ADD               0004 0015 0013
00040 RETURN            0004
shell:f
00000 MOVE              0015 0013
**00032 OBJECT COPY       0004 0014**
00064 MOVE              0014 556462282A60
00096 RETURN            0004
shell:main
00000 FUNCTION FRAME    556462282A50 1
00032 MOVE              0002 556462282A60
00064 FUNCTION CALL     5564622A3780
00088 FUNCTION FRAME    5564622A3780 1
00120 MOVE              0002 5564622A3790
00152 FUNCTION CALL     5564622A3780
00176 STOP              5564622A3780

3
>> function f(a) { var q = g; var g = 1; function g(b) { return a + b; }; return q; }; f(1)(2);
shell:g
00000 ADD               0004 0015 0013
00040 RETURN            0004
shell:f
00000 MOVE              0015 0013
**00032 MOVE              0004 0014**
00064 MOVE              0014 556462282A60
00096 RETURN            0004
shell:main
00000 FUNCTION FRAME    556462282A50 1
00032 MOVE              0002 556462282A60
00064 FUNCTION CALL     5564622A38B0
00088 FUNCTION FRAME    5564622A38B0 1
00120 MOVE              0002 5564622A3790
00152 FUNCTION CALL     5564622A38B0
00176 STOP              5564622A38B0

Segmentation fault

test:

(function(a) { var g = f; var f = 1; function f() { return a; } return g; })(42)()

[hongzhidao]

@drsm

In the below case of required hoist.
I want to hoist statements of function declaration and import.

...

function foo() {}
import ...;

static nxt_int_t
njs_parser_statement_hoist(njs_vm_t *vm, njs_parser_t *parser,
    njs_parser_node_t *new_node)
{
    njs_parser_node_t  *node, *stmt, *right, **child;

    child = &njs_parser_chain_top(parser);

    while (*child != NULL) {
        node = *child;

        if (node->right != NULL) {

            right = node->right;

            /* Is the order of hoist statements important? */
            if (right->token == NJS_TOKEN_IMPORT
                || right->token == NJS_TOKEN_FUNCTION)
            {
                break;
            }
        }

        child = &node->left;
    }

    stmt = njs_parser_node_new(vm, parser, NJS_TOKEN_STATEMENT);
    if (nxt_slow_path(stmt == NULL)) {
        return NXT_ERROR;
    }

    stmt->left = *child;
    stmt->right = new_node;

    *child = stmt;

    return NXT_OK;
}

1. Is the order of hoist statements important?
2. Does function expression hoist too?
3. And shim expression? var g = function f(a) { return (a > 1) ? a * f(a - 1) : 1 };

[hongzhidao]

@igorsysoev

Take a look, here's the draft patch.
Check the design of reimplementing of function declaration.
https://gist.github.com/hongzhidao/a07d8809234be934fab6a508affc8863

What is left:

1. shim function expression.

@drsm welcome to test again.

[drsm]

@hongzhidao

Does function expression hoist too?
And shim expression? var g = function f(a) { return (a > 1) ? a * f(a - 1) : 1 };

no, it's just a statement that generates a function object.
the name f in the example above is bound to the scope of the function object created:

>> var f = 'abc', g = function f(a) { return (a > 1) ? a * f(a - 1) : 1 }; (typeof f) + g(3);
'string6'

[drsm]

@hongzhidao

Is the order of hoist statements important?

I think imports should go first, as they are just links from one module's scope to another (in general).
and we cannot rebind them:

root@node:~# head main.mjs mod.mjs 
==> main.mjs <==
import mod from './mod.mjs';

function mod() {
    console.log('local');
}

console.log(mod());

==> mod.mjs <==
export default function() {
    console.log('the only case of anonymous function declaration');
}
root@node:~# node --experimental-modules main.mjs 
(node:3425) ExperimentalWarning: The ESM module loader is experimental.
file:///root/main.mjs:3
function mod() {
^

SyntaxError: Identifier 'mod' has already been declared
    at translators.set (internal/modules/esm/translators.js:48:18)

[hongzhidao]

I think imports should go first, as they are just links from one module's scope to another (in general).
and we cannot rebind them:

OK, This can belongs to module improvement.

[hongzhidao]

@drsm

Does function expression hoist too?
And shim expression? var g = function f(a) { return (a > 1) ? a * f(a - 1) : 1 };
no, it's just a statement that generates a function object.
the name f in the example above is bound to the scope of the function object created:

In the below code.

// test.js
var g = function f() { }

njs -d test.js

test.js:anonymous   (i)
00000 RETURN            14862A0
test.js:main
00000 FUNCTION          0141 1485C80
00032 STOP              14862A0

See (i), do you agree it's an anonymous function? Not test.js:f.

[hongzhidao]

@igorsysoev

I faced a problem while digging into function expression.

diff -r 84cb61f23b7e njs/njs_generator.c
--- a/njs/njs_generator.c   Sun Apr 07 00:09:09 2019 +0800
+++ b/njs/njs_generator.c   Tue Apr 09 11:27:00 2019 +0800
@@ -7,6 +7,7 @@

 #include <njs_core.h>
 #include <string.h>
+#include <stdio.h>


 typedef struct njs_generator_patch_s   njs_generator_patch_t;
@@ -3094,6 +3095,11 @@ njs_generate_object_dest_index(njs_vm_t
     njs_index_t        index;
     njs_parser_node_t  *dest;

+    /*
+     * Why not need to call the function?
+     * njs_generate_children_indexes_release(vm, generator, node);
+    */
+
     dest = node->dest;

     if (dest != NULL && dest->index != NJS_INDEX_NONE) {

Why not need to call njs_generate_children_indexes_release in njs_generate_object_dest_index?
Thanks.

[drsm]

@hongzhidao

// test.js
var g = function f() { }

njs -d test.js

test.js:anonymous   (i)
00000 RETURN            14862A0
test.js:main
00000 FUNCTION          0141 1485C80
00032 STOP              14862A0

See (i), do you agree it's an anonymous function? Not test.js:f.

no. it is a named function expression. but, as for now, Function.prototype.name
is not supported.
and the name f is not bound to any variable in the module scope of test.js.

[hongzhidao]

@xeioex @igorsysoev

Take a look, what about this?

diff -r dd44fdfef2de njs/njs_function.c
--- a/njs/njs_function.c    Wed Apr 10 17:46:14 2019 +0300
+++ b/njs/njs_function.c    Thu Apr 11 02:24:13 2019 +0800
@@ -31,7 +31,6 @@ njs_function_alloc(njs_vm_t *vm, njs_fun
         goto fail;
     }

-
     /*
      * nxt_mp_zalloc() does also:
      *   nxt_lvlhsh_init(&function->object.hash);
@@ -328,7 +327,7 @@ njs_function_lambda_frame(njs_vm_t *vm,

     max_args = nxt_max(nargs, lambda->nargs);

-    closures = lambda->nesting + lambda->block_closures;
+    closures = lambda->nesting + (lambda->closure_size > 0);

     size = njs_frame_size(closures)
            + (function->args_offset + max_args) * sizeof(njs_value_t)
@@ -485,7 +484,7 @@ njs_function_lambda_call(njs_vm_t *vm, n
             closure = *closures++;

             frame->closures[n] = closure;
-            vm->scopes[NJS_SCOPE_CLOSURE + n] = &closure->u.values;
+            vm->scopes[NJS_SCOPE_CLOSURE + n] = closure->values;

             n++;
         } while (n < nesting);
@@ -493,44 +492,42 @@ njs_function_lambda_call(njs_vm_t *vm, n

     /* Function closure values. */

-    if (lambda->block_closures > 0) {
-        closure = NULL;
+    size = lambda->closure_size;

-        size = lambda->closure_size;
-
-        if (size != 0) {
-            closure = nxt_mp_align(vm->mem_pool, sizeof(njs_value_t), size);
-            if (nxt_slow_path(closure == NULL)) {
-                njs_memory_error(vm);
-                return NXT_ERROR;
-            }
-
-            size -= sizeof(njs_value_t);
-            closure->u.count = 0;
-            dst = closure->values;
-
-            src = lambda->closure_scope;
-
-            do {
-                *dst++ = *src++;
-                size -= sizeof(njs_value_t);
-            } while (size != 0);
+    if (size > 0) {
+        closure = nxt_mp_align(vm->mem_pool, sizeof(njs_value_t), size);
+        if (nxt_slow_path(closure == NULL)) {
+            goto fail;
         }

+        dst = closure->values;
+        src = lambda->closure_scope;
+
+        do {
+            *dst++ = *src++;
+            size -= sizeof(njs_value_t);
+        } while (size != 0);
+
         frame->closures[n] = closure;
-        vm->scopes[NJS_SCOPE_CLOSURE + n] = &closure->u.values;
+        vm->scopes[NJS_SCOPE_CLOSURE + n] = closure->values;
     }

     if (lambda->rest_parameters) {
         ret = njs_function_rest_parameters_init(vm, &frame->native);
         if (nxt_slow_path(ret != NXT_OK)) {
-            return NXT_ERROR;
+            goto fail;
         }
     }

     vm->active_frame = frame;

     return NJS_APPLIED;
+
+fail:
+
+    njs_memory_error(vm);
+
+    return NXT_ERROR;
 }


diff -r dd44fdfef2de njs/njs_function.h
--- a/njs/njs_function.h    Wed Apr 10 17:46:14 2019 +0300
+++ b/njs/njs_function.h    Thu Apr 11 02:24:13 2019 +0800
@@ -27,9 +27,6 @@ struct njs_function_lambda_s {
     /* Function nesting level. */
     uint8_t                        nesting;           /* 4 bits */

-    /* Function internal block closures levels. */
-    uint8_t                        block_closures;    /* 4 bits */
-
     uint8_t                        rest_parameters;   /* 1 bit */

     /* Initial values of local scope. */
diff -r dd44fdfef2de njs/njs_generator.c
--- a/njs/njs_generator.c   Wed Apr 10 17:46:14 2019 +0300
+++ b/njs/njs_generator.c   Thu Apr 11 02:24:13 2019 +0800
@@ -2282,7 +2282,6 @@ static nxt_int_t
 njs_generate_function_scope(njs_vm_t *vm, njs_function_lambda_t *lambda,
     njs_parser_node_t *node, const nxt_str_t *name)
 {
-    size_t           size;
     nxt_int_t        ret;
     nxt_array_t      *closure;
     njs_generator_t  generator;
@@ -2294,17 +2293,14 @@ njs_generate_function_scope(njs_vm_t *vm
     ret = njs_generate_scope(vm, &generator, node->scope, name);

     if (nxt_fast_path(ret == NXT_OK)) {
-        size = 0;
+
         closure = node->scope->values[1];

         if (closure != NULL) {
-            lambda->block_closures = 1;
             lambda->closure_scope = closure->start;
-            size = (1 + closure->items) * sizeof(njs_value_t);
+            lambda->closure_size = closure->items * sizeof(njs_value_t);
         }

-        lambda->closure_size = size;
-
         lambda->nesting = node->scope->nesting;

         lambda->start = generator.code_start;
diff -r dd44fdfef2de njs/njs_parser.c
--- a/njs/njs_parser.c  Wed Apr 10 17:46:14 2019 +0300
+++ b/njs/njs_parser.c  Thu Apr 11 02:24:13 2019 +0800
@@ -200,8 +200,7 @@ njs_parser_scope_begin(njs_vm_t *vm, njs

     if (type == NJS_SCOPE_FUNCTION) {
         scope->next_index[0] = type;
-        scope->next_index[1] = NJS_SCOPE_CLOSURE + nesting
-                               + sizeof(njs_value_t);
+        scope->next_index[1] = NJS_SCOPE_CLOSURE + nesting;

     } else {
         if (type == NJS_SCOPE_GLOBAL) {
diff -r dd44fdfef2de njs/njs_vm.c
--- a/njs/njs_vm.c  Wed Apr 10 17:46:14 2019 +0300
+++ b/njs/njs_vm.c  Thu Apr 11 02:24:13 2019 +0800
@@ -2242,7 +2242,7 @@ njs_vm_scopes_restore(njs_vm_t *vm, njs_
     nesting = (function != NULL) ? function->u.lambda->nesting : 0;

     for (n = 0; n <= nesting; n++) {
-        vm->scopes[NJS_SCOPE_CLOSURE + n] = &frame->closures[n]->u.values;
+        vm->scopes[NJS_SCOPE_CLOSURE + n] = frame->closures[n]->values;
     }

     while (n < NJS_MAX_NESTING) {
diff -r dd44fdfef2de njs/njs_vm.h
--- a/njs/njs_vm.h  Wed Apr 10 17:46:14 2019 +0300
+++ b/njs/njs_vm.h  Thu Apr 11 02:24:13 2019 +0800
@@ -286,11 +286,6 @@ struct njs_array_s {


 typedef struct {
-    union {
-        uint32_t                      count;
-        njs_value_t                   values;
-    } u;
-
     njs_value_t                       values[1];
 } njs_closure_t;

@@ -316,6 +311,7 @@ struct njs_function_s {
     } u;

     njs_value_t                       *bound;
+
 #if (NXT_SUNC)
     njs_closure_t                     *closures[1];
 #else

[xeioex]

@hongzhidao

 typedef struct {
-    union {
-        uint32_t                      count;
-        njs_value_t                   values;
-    } u;
-
     njs_value_t                       values[1];
 } njs_closure_t;

These fields should be preserved for the future garbage collector.

[hongzhidao]

These fields should be preserved for the future garbage collector.

OK, Get it.

[hongzhidao]

@xeioex take a look.

Simplified function creation.
https://gist.github.com/hongzhidao/397fdb1611101fe088535f5d97ed7f09

Now.

1. Fixed segfault.
2. Introduced hoist in parser.
3. Eliminate function creation in parser.
4. Remove shim variable type and shim scope type.
5. Remove NJS_TOKEN_FUNCTION_EXPRESSION.

Left question.

1. Why not need to call njs_generate_children_indexes_release in njs_generate_object_dest_index?

[hongzhidao]

@xeioex

Here are patches.
https://gist.github.com/hongzhidao/b670852214c946add89c53749893c24e

Done in.

1. Making njs_parser_node_t more generic.
2. Making parser hoist more generic.
3. Simplified shim variable.
4. Fixed function declaration.

@drsm welcome to test. Thanks.

[xeioex]

@hongzhidao

Here are patches.
https://gist.github.com/hongzhidao/b670852214c946add89c53749893c24e

Thanks in the pipeline.
BTW, what is wrong with node->label? :)

[hongzhidao]

@hongzhidao

Here are patches.
https://gist.github.com/hongzhidao/b670852214c946add89c53749893c24e

Thanks in the pipeline.
BTW, what is wrong with node->label? :)

It's OK, but I want to re-use it with node->name for function name without introducing newly field.

[drsm]

@hongzhidao

njs/njs_variable.c: In function ‘njs_variable_reference_resolve’:
njs/njs_variable.c:452:34: error: variable ‘previous’ set but not used [-Werror=unused-but-set-variable]
     njs_parser_scope_t  *scope, *previous;

[hongzhidao]

@drsm thanks.

diff -r 1a2377af7ca0 njs/njs_variable.c
--- a/njs/njs_variable.c    Fri Apr 12 22:15:25 2019 +0800
+++ b/njs/njs_variable.c    Fri Apr 12 22:25:12 2019 +0800
@@ -449,14 +449,13 @@ njs_variable_reference_resolve(njs_vm_t
     njs_parser_scope_t *node_scope)
 {
     nxt_lvlhsh_query_t  lhq;
-    njs_parser_scope_t  *scope, *previous;
+    njs_parser_scope_t  *scope;

     lhq.key_hash = vr->hash;
     lhq.key = vr->name;
     lhq.proto = &njs_variables_hash_proto;

     scope = node_scope;
-    previous = NULL;

     for ( ;; ) {
         if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
@@ -490,7 +489,6 @@ njs_variable_reference_resolve(njs_vm_t
             return NXT_DECLINED;
         }

-        previous = scope;
         scope = scope->parent;
     }
 }

BTW, @xeioex plan to commit arrow function patch, so these patches will be recommited.

[hongzhidao]

@drsm @xeioex

-Werror=unused-but-set-variable

Good gcc option, we can add into njs. And I think it's also needed in UNIT :)

[drsm]

@hongzhidao

>> var i = 0; (function x() { if (i < 10) setImmediate(x); i++; })()
undefined
TypeError: first arg must be a function
    at setImmediate (native)
    at anonymous (shell:1)
    at main (native)

var i = 0; (function x() { if (i < 10) {console.log('type:', typeof x); setImmediate(x) }; i++; })();
type: function
undefined
type: undefined
TypeError: first arg must be a function
    at setImmediate (native)
    at anonymous (shell:1)
    at main (native)

[hongzhidao]

undefined

I get it.

test.js:anonymous
00000 MOVE              0004 0141   // x = function. But the node of function maybe temporary.
00032 OBJECT COPY       0014 0151
00064 FUNCTION FRAME    0014 1
00096 MOVE              0002 0004
00128 FUNCTION CALL     0014
00152 RETURN            74A2C0
test.js:main
00000 FUNCTION          0141 749C00
00032 FUNCTION FRAME    0141 0
00064 FUNCTION CALL     0141
00088 STOP              0141

Here's the patch.

diff -r 1a2377af7ca0 njs/njs_generator.c
--- a/njs/njs_generator.c   Fri Apr 12 22:15:25 2019 +0800
+++ b/njs/njs_generator.c   Fri Apr 12 22:59:42 2019 +0800
@@ -1904,7 +1904,7 @@ njs_generate_function(njs_vm_t *vm, njs_
     njs_function_lambda_t  *lambda;
     njs_vmcode_function_t  *function;

-    node->index = njs_generate_object_dest_index(vm, generator, node);
+    node->index = njs_generate_temp_index_get(vm, generator, node);
     if (nxt_slow_path(node->index == NJS_INDEX_ERROR)) {
         return NXT_ERROR;
     }
diff -r 1a2377af7ca0 njs/njs_variable.c
--- a/njs/njs_variable.c    Fri Apr 12 22:15:25 2019 +0800
+++ b/njs/njs_variable.c    Fri Apr 12 22:59:42 2019 +0800
@@ -449,14 +449,13 @@ njs_variable_reference_resolve(njs_vm_t
     njs_parser_scope_t *node_scope)
 {
     nxt_lvlhsh_query_t  lhq;
-    njs_parser_scope_t  *scope, *previous;
+    njs_parser_scope_t  *scope;

     lhq.key_hash = vr->hash;
     lhq.key = vr->name;
     lhq.proto = &njs_variables_hash_proto;

     scope = node_scope;
-    previous = NULL;

     for ( ;; ) {
         if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
@@ -490,7 +489,6 @@ njs_variable_reference_resolve(njs_vm_t
             return NXT_DECLINED;
         }

-        previous = scope;
         scope = scope->parent;
     }
 }

@drsm @xeioex

(function x() {})()

The key point which phase set x value, on phasing or runtime?
The ideal is on runtime phase.

The patch makes sense. Since.

1. x is a variable.
2. It's ok that the anonymous function occupies a permanent value.

[hongzhidao]

@xeioex

Done in.
Making njs_parser_node_t more generic.
Making parser hoist more generic.
Simplified shim variable.
Fixed function declaration.

1. Making njs_parser_node_t more generic.

2. Making parser hoist more generic.
   https://gist.github.com/hongzhidao/1733a63d20b4accc2490ba35ea51bc92

3. Simplified shim variable and scope.
   https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802
   Check whether this looks clear, please.

[xeioex]

@hongzhidao

Simplified shim variable and scope.
https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802

BTW, this patch breaks the following test: https://gist.github.com/xeioex/10d915edfbf7b5395dcf22fcb7fc1aa2
(The test itself is at the bottom, everything else is test harness).

It fixes the following test: https://gist.github.com/64099ab9ef7f8f5f63a8e0771766c98f
Everything else is not changed.

[hongzhidao]

https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802

fixed.
https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802

[hongzhidao]

@drsm

There is one place I'm not sure about function creation.

1. see function expression first.

(function() {})
test.js:anonymous
00000 RETURN            18998F0
test.js:main
00000 FUNCTION          0141 1899280   # shows the instruction of function expression here.
00032 STOP              0141

1. declare function without instructions

function foo() {}        # does not show the instruction of function declaration below.
test.js:foo
00000 RETURN            1FBB910
test.js:main
00000 STOP              1FBB910

1. the same as above.

(function foo() {})        # the instruction of function expression is shown below, but the information of binding foo to function is not given.
test.js:anonymous
00000 RETURN            1F6F900
test.js:main
00000 FUNCTION          0141 1F6F2C0    # shows the instruction of function expression here like 1.
00032 STOP              0141

Do you prefer to see the instructions of function declaration and binding shim variable to function?

Actually, the key point is whether it's worth to show the instructions of variable declaration.
We know the assignment is. So does function declaration include assignment behavior?

[hongzhidao]

@xeioex

Fixed segfault.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1555169933 -28800
# Node ID 48d2c60ec0690a1a784ae630d93f88af9461169d
# Parent  ace6f73dff8d51f79cde1c14aa17d26bb3fb70f0
Fixed function declaration.

diff -r ace6f73dff8d -r 48d2c60ec069 njs/njs_variable.c
--- a/njs/njs_variable.c    Sat Apr 13 01:11:49 2019 +0800
+++ b/njs/njs_variable.c    Sat Apr 13 23:38:53 2019 +0800
@@ -65,6 +65,11 @@ njs_variable_add(njs_vm_t *vm, njs_parse

     if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
         var = lhq.value;
+
+        if (type == NJS_VARIABLE_FUNCTION) {
+            var->type = type;
+        }
+
         return var;
     }

diff -r ace6f73dff8d -r 48d2c60ec069 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c  Sat Apr 13 01:11:49 2019 +0800
+++ b/njs/test/njs_unit_test.c  Sat Apr 13 23:38:53 2019 +0800
@@ -6413,6 +6413,16 @@ static njs_unit_test_t  njs_test[] =
     { nxt_string("function f() { var a = 1; function baz() { return a; } return baz; } f().bind()()"),
       nxt_string("1") },

+    { nxt_string("function f() { var t = 1; function baz() { return t; } return baz; }"
+                 "f().bind()();"),
+      nxt_string("1") },
+
+    { nxt_string("(function(a) { var s = typeof g, q = g; var g = 1; s += typeof g; function g(b) { return a + b }; return q; })(1)(2)"),
+      nxt_string("3")},
+
+    { nxt_string("(function(a) { var g = f; var f = 1; function f() { return a; } return g; })(42)()"),
+      nxt_string("42") },
+
     { nxt_string("function f(a, b) { return a + b }"
                  "f(3,4) === f.bind()(3,4)"),
       nxt_string("true") },

BTW, it seems the code related to shim and function declaration can be simplified, it is a separate topic. I think this ticket can be closed.
Finally, the answer to the question is helpful, thanks.
Why not need to call njs_generate_children_indexes_release in njs_generate_object_dest_index?

[drsm]

@hongzhidao

fixed.
https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802

the patch above fails the following test:

$ build/njs
interactive njs 0.3.1

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> if (true) { var i = 0; ++i } typeof i
'undefined'
>>

[hongzhidao]

@drsm

Can you try it again? It seems work well.

changeset:   892:48d2c60ec069
tag:         tip
user:        hongzhidao <[email protected]>
date:        Sat Apr 13 23:38:53 2019 +0800
summary:     Fixed function declaration.

changeset:   891:ace6f73dff8d
user:        hongzhidao <[email protected]>
date:        Sat Apr 13 01:11:49 2019 +0800
summary:     Making parser hoist more generic.

./build/njs
interactive njs 0.3.1

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> if (true) { var i = 0; ++i } typeof i
'number'

[xeioex]

@hongzhidao

BTW, it seems the code related to shim and function declaration can be simplified, it is a separate topic. I think this ticket can be closed.

Agree. The patch 'Fixed function declaration.' looks good.

Finally, the answer to the question is helpful, thanks.
Why not need to call njs_generate_children_indexes_release in njs_generate_object_dest_index?

Why do you think it should be called here?

We release tmp indexes to reuse them for other expressions at the moment we know for sure that we do not need them.

[drsm]

@hongzhidao

Can you try it again? It seems work well.

I mean this patch:

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1555098777 -28800
# Node ID 76c58f358b013f51f6183f75750bb1b825055d27
# Parent  ace6f73dff8d51f79cde1c14aa17d26bb3fb70f0
Simplified shim variable and scope.

[hongzhidao]

@hongzhidao

Can you try it again? It seems work well.

I mean this patch:

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1555098777 -28800
# Node ID 76c58f358b013f51f6183f75750bb1b825055d27
# Parent  ace6f73dff8d51f79cde1c14aa17d26bb3fb70f0
Simplified shim variable and scope.

We plan to process it later, it's not clear now.

[hongzhidao]

We release tmp indexes to reuse them for other expressions at the moment we know for sure that we do not need them.

OK.