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

Regression: SIGSEGV caused by using is with string #8129

Closed
Vindaar opened this issue Jun 27, 2018 · 3 comments · Fixed by #8130
Closed

Regression: SIGSEGV caused by using is with string #8129

Vindaar opened this issue Jun 27, 2018 · 3 comments · Fixed by #8130
Assignees

Comments

@Vindaar
Copy link
Contributor

Vindaar commented Jun 27, 2018

I had code similar to

let d = "aaa"
if d is "":
  echo "help"

in a library from when I was learning Nim. Trying to compile on the most recent devel causes a SIGSEGV during compilation.
koch outputs:

Traceback (most recent call last)
nim.nim(133)             nim
nim.nim(97)              handleCmdLine
main.nim(162)            mainCommand
main.nim(73)             commandCompileToC
modules.nim(124)         compileProject
modules.nim(71)          compileModule
passes.nim(194)          processModule
passes.nim(103)          processTopLevelStmt
sem.nim(587)             myProcess
sem.nim(555)             semStmtAndGenerateGenerics
semstmts.nim(1894)       semStmt
semexprs.nim(908)        semExprNoType
semexprs.nim(2510)       semExpr
semstmts.nim(160)        semIf
semexprs.nim(59)         semExprWithType
semexprs.nim(2440)       semExpr
semexprs.nim(1988)       semMagic
semexprs.nim(410)        semIs
semexprs.nim(345)        isOpImpl
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
FAILURE

While is and isnot should be used on types as I know nowadays, this should still not crash the compiler. The regression is also introduced by the recent typedesc merge of: #7681

@Vindaar
Copy link
Contributor Author

Vindaar commented Jun 27, 2018

The issue seems to be that the string "" doesn't have the kind field set. In line 345 in semexprs.nim:

  if t1.kind == tyTypeDesc and t2.kind != tyTypeDesc:

trying to access t2.kind is what causes the SIGSEGV.

edit: as a matter of fact t2 is simply nil ( t2.isNil returns true ).

@LemonBoy
Copy link
Contributor

That check is guarded by

  internalAssert c.config,
    n.sonsLen == 3 and
    n[1].typ != nil and
    n[2].kind in {nkStrLit..nkTripleStrLit, nkType}

This means that t2 cannot be a tyTypeDesc. The solutions are:

  • Remove the second check in the and
  • Run semExpr on the n.sons[2]node anyway before callingisOpImpl`

CC @zah since he touched this proc last

@Vindaar
Copy link
Contributor Author

Vindaar commented Jun 27, 2018

Ah, thanks @LemonBoy! I'll create a PR for that.
I'm still pretty unsure about the compiler. As in how to fix issues / what procs to call etc. :) I fixed it locally by doing something like

    let t2 = semTypeNode(c, n[2], nil)
    n.sons[2] = newNodeIT(nkType, n[2].info, t2)

in the else branch of the

  if n[2].kind notin {nkStrLit..nkTripleStrLit}:

Should the call be to semExprWithType (semExpr is not enough, is it?) and only for the case where n.sons[2] is a strLit?

edit: ok, I see now that semExpr works just fine too. I really need to get a better understanding of all these procs :P

Vindaar added a commit to Vindaar/Nim that referenced this issue Jun 27, 2018
…#8129

In case the second son of the node in `semIs` is of kind `strLit`, we
now call `semExprWithType` to set the `typ` field of that node.

Also removes the `t2 != tyTypeDesc` check in `isOpImpl`, since the
kind of `n[2]` is already assertet with the `internalAssert`.
Vindaar added a commit to Vindaar/Nim that referenced this issue Jun 27, 2018
In case the second son of the node in `semIs` is of kind `strLit`, we
now call `semExpr` to set the `typ` field of that node.

Also removes the `t2 != tyTypeDesc` check in `isOpImpl`, since the
kind of `n[2]` is already assertet with the `internalAssert`.
@Araq Araq assigned zah Jun 27, 2018
Vindaar added a commit to Vindaar/Nim that referenced this issue Jun 27, 2018
In case the second son of the node in `semIs` is of kind `strLit`, we
now call `semExpr` to set the `typ` field of that node.

Also removes the `t2 != tyTypeDesc` check in `isOpImpl`, since the
kind of `n[2]` is already assertet with the `internalAssert`.
Araq pushed a commit that referenced this issue Jun 29, 2018
* call `semExpr` in `semIs` if node is `strLit`, fixes #8129

In case the second son of the node in `semIs` is of kind `strLit`, we
now call `semExpr` to set the `typ` field of that node.

Also removes the `t2 != tyTypeDesc` check in `isOpImpl`, since the
kind of `n[2]` is already assertet with the `internalAssert`.

* reintroduce check for `t2.kind != tyTypeDesc` to fix test case

The `internalAssert` in the `isOpImpl` doesn't check
`n.sons[2].typ.kind` as I previously read, but rather
`n.sons[2].kind`. Therefore the check for `tyTypeDesc` here is
useful. Otherwise the last test case in `isopr.nim` fails.

Also removes the flag `efDetermineType` from the call to `semExpr`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants