From: Jacek Caban <jacek@codeweavers.com>
Subject: Re: [PATCH v3 3/6] jscript: Support block scope variables.
Message-Id: <4d60bbcc-ea02-36cf-1d93-2554ab09c51d@codeweavers.com>
Date: Wed, 16 Jun 2021 21:35:35 +0200
In-Reply-To: <20210615150739.150691-3-pgofman@codeweavers.com>
References: <20210615150739.150691-1-pgofman@codeweavers.com> <20210615150739.150691-3-pgofman@codeweavers.com>

On 6/15/21 5:07 PM, Paul Gofman wrote:
> Signed-off-by: Paul Gofman <pgofman@codeweavers.com>
> ---
> v3:
>      - initialize new scope's locals tree in visit_statement( STAT_WITH case):
>        fixes assertion in test from patch 6.
>
>   dlls/jscript/compile.c     | 235 +++++++++++++++++++++++++++----------
>   dlls/jscript/engine.c      | 105 ++++++++++++++---
>   dlls/jscript/engine.h      |   5 +-
>   dlls/jscript/parser.h      |   2 +
>   dlls/jscript/parser.y      |   2 +
>   dlls/jscript/tests/lang.js |  16 +++
>   dlls/mshtml/tests/es5.js   |  48 +++++++-
>   7 files changed, 334 insertions(+), 79 deletions(-)


It would be nice to split it a bit. For example, it seems to me that 
visiting phrase changes could be a separated patch.


> diff --git a/dlls/jscript/compile.c b/dlls/jscript/compile.c
> index 46efd53cdb5..9ac3d221ff0 100644
> --- a/dlls/jscript/compile.c
> +++ b/dlls/jscript/compile.c
> @@ -39,6 +39,8 @@ typedef struct _statement_ctx_t {
>   
>       const labelled_statement_t *labelled_stat;
>   
> +    unsigned int scope_index;
> +    BOOL block_scope;
>       struct _statement_ctx_t *next;
>   } statement_ctx_t;
>   
> @@ -48,6 +50,8 @@ typedef struct {
>       int ref;
>   } function_local_t;
>   
> +#define MAX_SCOPE_COUNT 1024


Such arbitrary limit seems bad. Scripts can be really long, so it's not 
unrealistic to hit the limit. At the same time, it's wasteful for short 
scripts like "runOnload()".


>   typedef struct _compiler_ctx_t {
>       parser_ctx_t *parser;
>       bytecode_t *code;
> @@ -61,8 +65,14 @@ typedef struct _compiler_ctx_t {
>       unsigned labels_size;
>       unsigned labels_cnt;
>   
> -    struct wine_rb_tree locals;
> -    unsigned locals_cnt;
> +    struct
> +    {
> +        struct wine_rb_tree locals;
> +        unsigned int locals_cnt;
> +        unsigned int *ref_index;
> +    }
> +    *local_scopes;
> +    unsigned local_scope_count;
>   
>       statement_ctx_t *stat_ctx;
>       function_code_t *func;
> @@ -253,6 +263,19 @@ static HRESULT push_instr_int(compiler_ctx_t *ctx, jsop_t op, LONG arg)
>       return S_OK;
>   }
>   
> +static HRESULT push_instr_uint_uint(compiler_ctx_t *ctx, jsop_t op, unsigned arg1, unsigned arg2)
> +{
> +    unsigned instr;
> +
> +    instr = push_instr(ctx, op);
> +    if(!instr)
> +        return E_OUTOFMEMORY;
> +
> +    instr_ptr(ctx, instr)->u.arg[0].uint = arg1;
> +    instr_ptr(ctx, instr)->u.arg[1].uint = arg2;
> +    return S_OK;
> +}
> +
>   static HRESULT push_instr_str(compiler_ctx_t *ctx, jsop_t op, jsstr_t *str)
>   {
>       unsigned instr;
> @@ -439,10 +462,19 @@ static BOOL bind_local(compiler_ctx_t *ctx, const WCHAR *identifier, int *ret_re
>   
>       for(iter = ctx->stat_ctx; iter; iter = iter->next) {
>           if(iter->using_scope)
> -            return FALSE;
> +        {
> +            if (!iter->block_scope)
> +                return FALSE;
> +            TRACE("iter->scope_index %d, ctx->func->local_scope_count %d.\n", iter->scope_index, ctx->func->local_scope_count);


Is it a debugging leftover? I wouldn't mind adding some debug traces, 
but this one seems kind of random.


> +            if ((ref = lookup_local(ctx->func, identifier, iter->scope_index)))
> +            {
> +                *ret_ref = ref->ref;
> +                return TRUE;
> +            }
> +        }
>       }
>   
> -    ref = lookup_local(ctx->func, identifier);
> +    ref = lookup_local(ctx->func, identifier, 0);
>       if(!ref)
>           return FALSE;
>   
> @@ -1111,18 +1143,35 @@ static inline BOOL is_loop_statement(statement_type_t type)
>   }
>   
>   /* ECMA-262 3rd Edition    12.1 */
> -static HRESULT compile_block_statement(compiler_ctx_t *ctx, statement_t *iter)
> +static HRESULT compile_block_statement(compiler_ctx_t *ctx, block_statement_t *block, statement_t *iter)
>   {
> +    statement_ctx_t stat_ctx = {0, TRUE};
> +    BOOL needs_scope;
>       HRESULT hres;
>   
> +    needs_scope = block && block->scope_index;
> +    if (needs_scope)
> +    {
> +        if(!push_instr(ctx, OP_new_obj))
> +            return E_OUTOFMEMORY;
> +        if(FAILED(hres = push_instr_uint_uint(ctx, OP_push_scope, block->scope_index, TRUE)))
> +            return hres;


Creating a new object on each block entry is a rather expensive. Could 
we create those object on demand instead (like in 
detach_variable_object())? Given that, it seems to me that a new 
dedicated opcode would appropriate here.


Thanks,

Jacek