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

SIGSEGV when return type depends on a static[T] parameter #8545

Closed
timotheecour opened this issue Aug 6, 2018 · 5 comments · Fixed by #17432
Closed

SIGSEGV when return type depends on a static[T] parameter #8545

timotheecour opened this issue Aug 6, 2018 · 5 comments · Fixed by #17432

Comments

@timotheecour
Copy link
Member

timotheecour commented Aug 6, 2018

BUG: case2 below should work, but it gives SIGSEGV

template bar(a: static[bool]): untyped = int

when defined(case1):
  # ok
  proc foo(a: static[bool]): auto = 1

when defined(case2):
  # SIGSEGV: see stacktrace in [1]
  proc foo(a: static[bool]): bar(a) = 1

when defined(case3):
  # Error: type mismatch: got <bool> but expected ... static[bool]
  proc foo(a: static[bool]): bar(static[bool](a)) = 1

when defined(case4):
  # Error: expression cannot be cast to static[bool]
  proc foo(a: static[bool]): bar(cast[static[bool]](a)) = 1

echo foo(true)

#[
[1]
/Users/timothee/git_clone/nim/Nim/compiler/nim.nim(133) nim
/Users/timothee/git_clone/nim/Nim/compiler/nim.nim(97) handleCmdLine
/Users/timothee/git_clone/nim/Nim/compiler/main.nim(162) mainCommand
/Users/timothee/git_clone/nim/Nim/compiler/main.nim(73) commandCompileToC
/Users/timothee/git_clone/nim/Nim/compiler/modules.nim(124) compileProject
/Users/timothee/git_clone/nim/Nim/compiler/modules.nim(71) compileModule
/Users/timothee/git_clone/nim/Nim/compiler/passes.nim(194) processModule
/Users/timothee/git_clone/nim/Nim/compiler/passes.nim(103) processTopLevelStmt
/Users/timothee/git_clone/nim/Nim/compiler/sem.nim(592) myProcess
/Users/timothee/git_clone/nim/Nim/compiler/sem.nim(560) semStmtAndGenerateGenerics
/Users/timothee/git_clone/nim/Nim/compiler/semstmts.nim(1891) semStmt
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(912) semExprNoType
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(2444) semExpr
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(2050) semWhen
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(2520) semExpr
/Users/timothee/git_clone/nim/Nim/compiler/semstmts.nim(1832) semStmtList
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(2538) semExpr
/Users/timothee/git_clone/nim/Nim/compiler/semstmts.nim(1682) semProc
/Users/timothee/git_clone/nim/Nim/compiler/semstmts.nim(1508) semProcAux
/Users/timothee/git_clone/nim/Nim/compiler/semstmts.nim(1119) semParamList
/Users/timothee/git_clone/nim/Nim/compiler/semtypes.nim(1110) semProcTypeNode
/Users/timothee/git_clone/nim/Nim/compiler/semtypes.nim(1492) semTypeNode
/Users/timothee/git_clone/nim/Nim/compiler/semtypes.nim(1276) semTypeExpr
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(59) semExprWithType
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(2413) semExpr
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(893) semDirectOp
/Users/timothee/git_clone/nim/Nim/compiler/semexprs.nim(745) semOverloadedCallAnalyseEffects
/Users/timothee/git_clone/nim/Nim/compiler/semcall.nim(467) semOverloadedCall
/Users/timothee/git_clone/nim/Nim/compiler/semcall.nim(435) semResolvedCall
/Users/timothee/git_clone/nim/Nim/compiler/ast.nim(1037) add
/Users/timothee/git_clone/nim/Nim/lib/system.nim(3753) failedAssertImpl
/Users/timothee/git_clone/nim/Nim/lib/system.nim(3746) raiseAssert
/Users/timothee/git_clone/nim/Nim/lib/system.nim(2807) sysFatal
Error: unhandled exception:
not (son == nil)  [AssertionError]

 ]#

NOTE: use case reduced from #8531 (comment)

@mratsim
Copy link
Collaborator

mratsim commented Aug 6, 2018

The SIGSEGV crash is a regression due to the not nil changes.

On 0.18.0 the output is:

template bar(a: static[bool]): untyped = int
proc foo(a: static[bool]): bar(a) = 1
echo foo(true)

# static.nim(3, 31) Error: type mismatch: got <static[bool]>
# but expected one of:
# template bar(a: static[bool]): untyped
# 
# expression: bar(a)

which is another issue which maybe should be tracked separately? (this one for the not nil regression, another one for passing static for result types).

This reminds me of #7719 / nim-lang/RFCs#44, except for static types instead of generics.

Note that I can also reproduce this with static[int], this is important as static[bool] (and static[enum]) can get silently converted to int at compile time #7375

I think the issue on stable was fixed already by the flurry of static[int] fixes and there is no need to log it. (On stable this fails with the same error but compiles properly on devel)

@timotheecour
Copy link
Member Author

@mratsim just curious: what's the policy on logging and fixing bugs in stable? IMO, given scarce resources, shouldn't we just focus on logging / fixing bugs in devel? (maybe at exception of critical security bugs, idk)

I think the issue on stable was fixed already by the flurry of static[int] fixes and there is no need to log it.

what do you mean by that?
last commit on stable (aka master) is 855956b on 2018-03-01 21:39

@mratsim
Copy link
Collaborator

mratsim commented Aug 6, 2018

Sorry I was completely unclear.

Regarding the following snippet:

template bar(a: static[bool]): untyped = int
proc foo(a: static[bool]): bar(a) = 1
echo foo(true)
  1. On 0.18.0, it throws:
# static.nim(3, 31) Error: type mismatch: got <static[bool]>
# but expected one of:
# template bar(a: static[bool]): untyped
# 
# expression: bar(a)
  1. After Static[T] fixes #7333 / 121b9e2 I'm pretty sure it would work

  2. After what I suppose all the not nil commits from April 27 to April 30: https://github.com/nim-lang/Nim/commits/599b5d6dcbd8b73d98d1aaf2de94d3360229a699?after=599b5d6dcbd8b73d98d1aaf2de94d3360229a699+34, your regression was introduced.

It reminds me of #7730 and #7746 where less traveled compiler codepaths crashed with SIGSEGV due to the new not nil (compiling with linedir or debugger-native).


Regarding bugs in stable, the solution is just to release either 0.x.2 or 0.x+1.0 depending on the changes.

@konsumlamm
Copy link
Contributor

Update:
On latest devel, cases 1, 2 and 4 now work (all print 1), case 3 still gives the same error message, but that seems to be expected (case 4 could be used as a workaround). So I think this issue can be closed (specially since it doesn't SIGSEGV anymore, if case 3 is an issue, that should go into a new issue imo).

$ nim -v
Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2021-03-18
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: de5a8265384c2f23eca744b4fc9feda9721faefd
active boot switches: -d:release

@timotheecour
Copy link
Member Author

timotheecour commented Mar 21, 2021

also, this works too:

when defined(case5):
  proc foo(a: static[bool]): bar(static(a)) = 1

before closing, we need a test case with case1,2,4,5 (can be simply tests/misc/t8545.nim)
if you volunteer a PR, I can review it fast

konsumlamm added a commit to konsumlamm/Nim that referenced this issue Mar 21, 2021
timotheecour added a commit that referenced this issue Mar 21, 2021
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
ringabout pushed a commit to ringabout/Nim that referenced this issue Mar 22, 2021
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
Araq pushed a commit that referenced this issue Aug 26, 2024
…n fixes (#24005)

fixes #4228, fixes #4990, fixes #7006, fixes #7008, fixes #8406, fixes
#8551, fixes #11112, fixes #20027, fixes #22647, refs #23854 and #23855
(remaining issue fixed), refs #8545 (works properly now with
`cast[static[bool]]` changed to `cast[bool]`), refs #22342 and #22607
(disabled tests added), succeeds #23194

Parameter and return type nodes in generic procs now undergo the same
`inGenericContext` treatment that nodes in generic type bodies do. This
allows many of the fixes in #22029 and followups to also apply to
generic proc signatures. Like #23983 however this needs some more
compiler fixes, but this time mostly in `sigmatch` and type
instantiations.

1. `tryReadingGenericParam` no longer treats `tyCompositeTypeClass` like
a concrete type anymore, so expressions like `Foo.T` where `Foo` is a
generic type don't look for a parameter of `Foo` in non-generic code
anymore. It also doesn't generate `tyFromExpr` in non-generic code for
any generic LHS. This is to handle a very specific case in `asyncmacro`
which used `FutureVar.astToStr` where `FutureVar` is generic.
2. The `tryResolvingStaticExpr` call when matching `tyFromExpr` in
sigmatch now doesn't consider call nodes in general unresolved, only
nodes with `tyFromExpr` type, which is emitted on unresolved expressions
by increasing `c.inGenericContext`. `c.inGenericContext == 0` is also
now required to attempt instantiating `tyFromExpr`. So matching against
`tyFromExpr` in proc signatures works in general now, but I'm
speculating it depends on constant folding in `semExpr` for statics to
match against it properly.
3. `paramTypesMatch` now doesn't try to change nodes with `tyFromExpr`
type into `tyStatic` type when fitting to a static type, because it
doesn't need to, they'll be handled the same way (this was a workaround
in place of the static type instantiation changes, only one of the
fields in the #22647 test doesn't work with it).
4. `tyStatic` matching now uses `inferStaticParam` instead of just range
type matching, so `Foo[N div 2]` can infer `N` in the same way `array[N
div 2, int]` can. `inferStaticParam` also disabled itself if the
inferred static param type already had a node, but `makeStaticExpr`
generates static types with unresolved nodes, so we only disable it if
it also doesn't have a binding. This might not work very well but the
static type instantiation changes should really lower the amount of
cases where it's encountered.
5. Static types now undergo type instantiation. Previously the branch
for `tyStatic` in `semtypinst` was a no-op, now it acts similarly to
instantiating any other type with the following differences:
- Other types only need instantiation if `containsGenericType` is true,
static types also get instantiated if their value node isn't a literal
node. Ideally any value node that is "already evaluated" should be
ignored, but I'm not sure of a better way to check this, maybe if
`evalConstExpr` emitted a flag. This is purely for optimization though.
- After instantiation, `semConstExpr` is called on the value node if
`not cl.allowMetaTypes` and the type isn't literally a `static` type.
Then the type of the node is set to the base type of the static type to
deal with `semConstExpr` stripping abstract types.
We need to do this because calls like `foo(N)` where `N` is `static int`
and `foo`'s first parameter is just `int` do not generate `tyFromExpr`,
they are fully typed and so `makeStaticExpr` is called on them, giving a
static type with an unresolved node.
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.

4 participants