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

add(a, b) buggy in RT js or in VM when a or b is a nil cstring #16686

Closed
timotheecour opened this issue Jan 11, 2021 · 1 comment · Fixed by #16979
Closed

add(a, b) buggy in RT js or in VM when a or b is a nil cstring #16686

timotheecour opened this issue Jan 11, 2021 · 1 comment · Fixed by #16979
Labels
Javascript VM see also `const` label

Comments

@timotheecour
Copy link
Member

timotheecour commented Jan 11, 2021

Example 1

proc main()=
  block:
    var a: cstring = nil
    a.add "ba"
    echo (a,)
  block:
    var b: cstring = "ba"
    var b2: cstring
    b.add b2
    echo (b,)
  block:
    var a: cstring = nil
    a.add a
    echo (a,)
  block:
    var a: cstring = "ab"
    a.add a
    echo (a,)
  block:
    var a: cstring = nil
    echo (a,)

static: main()
main()

Current Output

vm js:
("ba",)
("ba",)
("",)
("abab",)
("",) # this is a separate issue, fixed in #16026

rt js:
("nullba",)
("banull",)
("",)
("abab",)
(nil,)

Expected Output

vm js:
("ba",)
("ba",)
("",)
("abab",)
(nil,)

rt js:
("ba",)
("ba",)
("",)
("abab",)
(nil,)

Example 2

when true:
  proc main()=
    block:
      var a: cstring = nil
      a.add "ba".cstring
      echo (a,)
    block:
      var b = "ba"
      var b2: cstring
      b.add b2
      echo (b,)
  # static: main()
  main()

prints:
("nullba",)
/Users/timothee/git_clone/nim/timn/build/nimcache/t11680.js:376
x_33556413[x_33556413_Idx].length += y_33556414.length;
(2 bugs here)

Example 3: js vm

uncomment static: main() in Example 2
gives CT error:

system.nim(1987, 5) Error: cannot generate VM code for asm "      if ("x

Example 4: vm (eg c vm)

when true:
  proc main=
    var a = "abc"
    a.add nil
    echo (a,)
  static: main()
  main()

VM:
Error: index out of bounds, the container is empty
while y[i] != '\0':

Additional Information

1.5.1 be6e891

@timotheecour timotheecour changed the title add(a, b: cstring) buggy in RT js when a or b is nil add(a, b) buggy in RT js when a or b is a nil cstring Jan 11, 2021
@timotheecour timotheecour added the VM see also `const` label label Jan 11, 2021
@timotheecour timotheecour changed the title add(a, b) buggy in RT js when a or b is a nil cstring add(a, b) buggy in RT js or in VM when a or b is a nil cstring Jan 11, 2021
@metagn
Copy link
Collaborator

metagn commented Jan 12, 2021

This overload add(var cstring, cstring) is not in the system docs. The block that declares it does not check for defined(nimdoc). The overload here uses AppendStrStr, for which I, without thinking of the possibility that cstrings might be nil, deleted the null checks for and simply turned into a += b for some reason in #14158. Dead easy fix, just revert this line and add back the deleted temp to the cstring branch (#16674 is also introduced by this PR though so you might need to look for more problems), but also add or defined(nimdoc) to the when statement in the system module and maybe look for other declarations that might not have docs in the standard library.

As for VM, maybe when nimvm: add(x, string(y)) else: asm ... in the same proc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Javascript VM see also `const` label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants