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

Bad code and parser confusion with closure, upval and assignment with modification #344

Closed
sfromis opened this issue Jun 17, 2023 · 4 comments · Fixed by #345
Closed

Bad code and parser confusion with closure, upval and assignment with modification #344

sfromis opened this issue Jun 17, 2023 · 4 comments · Fixed by #345

Comments

@sfromis
Copy link
Contributor

sfromis commented Jun 17, 2023

Debugging some surprising behavior when coding in Berry, I whittled the anomaly experienced down to a couple of simple test cases easy to repeat. The basis is that I have a local var expected to be captured and modified as an upval in a closure.

def test()
  var nv = 1
  tasmota.set_timer(500,
    def()
      nv += 2*1
      print(nv)
    end)
end
test()
BRY: Exception> 'syntax_error' - input:6: register overflow (more than 255)

The "perpetrator" seems to be the line nv += 2*1 - it works if I reduce it to nv += 2 or "flatten" it to nv = nv + 2*1.

The above was one result of reducing code from a more concerning case with bad code generated. Here's a "less reduced" variant exhibiting that behavior.

def set_timer_repeat(delay,f,id)
  tasmota.set_timer(delay, def() set_timer_repeat(delay,f,id) f() end, id)
end

def test()
  var nv = 1
  set_timer_repeat(500,
    def()
      print(nv)
      nv += 1*1
    end, 'test')
end
tasmota.set_timer(5000,/->tasmota.remove_timer('test'))
test()
1
2
4
8
16
32
64
128
256

Expected outcome of this "stupid" code is that nv simply should be incremented each iteration. Instead it is doubled. If I move the print(nv) after the assignment, I get the register overflow error instead, basically the first test case. Does not seem to depend on the use of the set_timer_repeat convenience function.

To further simplify, here's the same situation without involving any Tasmota extensions:

def test()
  var nv = 1
  var f = def() print(nv) nv += 1*1 end
  f() f() f() f() f() f() f() f() f()
end
test()

If I instead of the test() function enter the body 3 lines in the REPL, I get the correct increments instead of the bad doubling. Seems to make a difference with a local vs global variable.

@s-hadinger
Copy link
Contributor

Thanks. As discussed on Discord this will not be a high priority, especially if fixing this risks breaking something else.

@sfromis
Copy link
Contributor Author

sfromis commented Jun 17, 2023

Yeah, for now I'll cut back on using the assignment with modification combined operator.

@s-hadinger
Copy link
Contributor

s-hadinger commented Jun 18, 2023

There is definitely a problem in register allocation:

def test()
  var nv = 1
  var f =
    def()
      nv += 2*1
    end
end

import solidify
solidify.dump(test)

Here is the output:

/********************************************************************
** Solidified function: test
********************************************************************/
be_local_closure(test,   /* name */
  be_nested_proto(
    2,                          /* nstack */
    0,                          /* argc */
    0,                          /* varg */
    0,                          /* has upvals */
    NULL,                       /* no upvals */
    1,                          /* has sup protos */
    ( &(const struct bproto*[ 1]) {
      be_nested_proto(
        1,                          /* nstack */
        0,                          /* argc */
        0,                          /* varg */
        1,                          /* has upvals */
        ( &(const bupvaldesc[ 1]) {  /* upvals */
          be_local_const_upval(1, 0),
        }),
        0,                          /* has sup protos */
        NULL,                       /* no sub protos */
        1,                          /* has constants */
        ( &(const bvalue[ 2]) {     /* constants */
        /* K0   */  be_const_int(2),
        /* K1   */  be_const_int(1),
        }),
        &be_const_str__anonymous_,
        &be_const_str_solidified,
        ( &(const binstruction[ 5]) {  /* code */
          0x08020101,  //  0000  MUL	R0	K0	K1
          0x68000000,  //  0001  GETUPV	R0	U0
          0x00000000,  //  0002  ADD	R0	R0	R0
          0x6C000000,  //  0003  SETUPV	R0	U0
          0x80000000,  //  0004  RET	0
        })
      ),
    }),
    1,                          /* has constants */
    ( &(const bvalue[ 1]) {     /* constants */
    /* K0   */  be_const_int(1),
    }),
    &be_const_str_test,
    &be_const_str_solidified,
    ( &(const binstruction[ 4]) {  /* code */
      0x58000000,  //  0000  LDCONST	R0	K0
      0x84040000,  //  0001  CLOSURE	R1	P0
      0xA0000000,  //  0002  CLOSE	R0
      0x80000000,  //  0003  RET	0
    })
  )
);
/*******************************************************************/

The culprit is obviously

          0x08020101,  //  0000  MUL	R0	K0	K1
          0x68000000,  //  0001  GETUPV	R0	U0             <---- R0 is being overriden
          0x00000000,  //  0002  ADD	R0	R0	R0     <---- WRONG!!!
          0x6C000000,  //  0003  SETUPV	R0	U0

It should be something like

          0x08020101,  //  0000  MUL	R0	K0	K1
          0x68000000,  //  0001  GETUPV	R1	U0
          0x00000000,  //  0002  ADD	R1	R0	R1
          0x6C000000,  //  0003  SETUPV	R1	U0

To double check:

def test()
  var nv = 1
  var f =
    def()
      nv += 2
    end
end

import solidify
solidify.dump(test)

Now the output is correct:

[...]
        ( &(const bvalue[ 1]) {     /* constants */
        /* K0   */  be_const_int(2),
        }),
        &be_const_str__anonymous_,
        &be_const_str_solidified,
        ( &(const binstruction[ 4]) {  /* code */
          0x68000000,  //  0000  GETUPV	R0	U0
          0x00000100,  //  0001  ADD	R0	R0	K0
          0x6C000000,  //  0002  SETUPV	R0	U0
          0x80000000,  //  0003  RET	0
        })
[...]

@sfromis
Copy link
Contributor Author

sfromis commented Jun 18, 2023

Unsurprisingly, testing did no longer produce errors.

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

Successfully merging a pull request may close this issue.

2 participants