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

A certain combination of iterators and procs crashes the compiler #19703

Closed
EyeCon opened this issue Apr 8, 2022 · 1 comment · Fixed by #24333
Closed

A certain combination of iterators and procs crashes the compiler #19703

EyeCon opened this issue Apr 8, 2022 · 1 comment · Fixed by #24333

Comments

@EyeCon
Copy link

EyeCon commented Apr 8, 2022

Code to reproduce

import std / sequtils

iterator combinations*[T](s: seq[T], r: Positive): seq[T] =
  yield @[s[0], s[1]]

iterator pairwise*[T](s: openArray[T]): seq[T] =
  yield @[s[0], s[1]]

proc checkSpecialSubset5(s: seq[int]): bool =
  toSeq(
    toSeq(
      s.combinations(2)
    ).map(proc(a: auto): int = a[0]).pairwise()
  ).any(proc(a: auto): bool = true)

echo checkSpecialSubset5 @[1, 2]

Current Output

> nim c -r "d:\[REDACTED].nim"
Hint: used config file 'C:\Users\EyeCon\.choosenim\toolchains\nim-#devel\config\nim.cfg' [Conf]
Hint: used config file 'C:\Users\EyeCon\.choosenim\toolchains\nim-#devel\config\config.nims' [Conf]
Hint: used config file 'd:\prog\Nim\pe\config.nims' [Conf]
...............................................................
[REDACTED](1, 12) Warning: imported and not used: 'algorithm' [UnusedImport]
CC: pe0103.nim
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c: In function 'checkSpecialSubset5__pe48494851_13':
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c:629:55: error: redeclaration of 'resultX60gensym6_' with no linkage
  629 |                 tySequence__PSP8snSsRoFs9cDiya9bd7UQ* resultX60gensym6_;
      |                                                       ^~~~~~~~~~~~~~~~~
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c:626:55: note: previous declaration of 'resultX60gensym6_' with type 'tySequence__PSP8snSsRoFs9cDiya9bd7UQ *'      
  626 |                 tySequence__PSP8snSsRoFs9cDiya9bd7UQ* resultX60gensym6_;
      |                                                       ^~~~~~~~~~~~~~~~~
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c:633:55: error: redeclaration of 'resultX60gensym6_' with no linkage
  633 |                 tySequence__PSP8snSsRoFs9cDiya9bd7UQ* resultX60gensym6_;
      |                                                       ^~~~~~~~~~~~~~~~~
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c:629:55: note: previous declaration of 'resultX60gensym6_' with type 'tySequence__PSP8snSsRoFs9cDiya9bd7UQ *'      
  629 |                 tySequence__PSP8snSsRoFs9cDiya9bd7UQ* resultX60gensym6_;
      |                                                       ^~~~~~~~~~~~~~~~~
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c:636:55: error: redeclaration of 'resultX60gensym6_' with no linkage
  636 |                 tySequence__PSP8snSsRoFs9cDiya9bd7UQ* resultX60gensym6_;
      |                                                       ^~~~~~~~~~~~~~~~~
C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c:633:55: note: previous declaration of 'resultX60gensym6_' with type 'tySequence__PSP8snSsRoFs9cDiya9bd7UQ *'      
  633 |                 tySequence__PSP8snSsRoFs9cDiya9bd7UQ* resultX60gensym6_;
      |                                                       ^~~~~~~~~~~~~~~~~
Error: execution of an external compiler program 'gcc.exe -c  -w -fmax-errors=3 -mno-ms-bitfields -O3 -fno-strict-aliasing -fno-ident   -IC:\Users\EyeCon\.choosenim\toolchains\nim-#devel\lib -Id:\prog\Nim\pe -o C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c.o C:\Users\EyeCon\nimcache\pe0103_d\@mpe0103.nim.c' failed with exit code: 1

Expected Output

That the file is compiled without problems

Possible Solution

From the discussion in Discord:

Solitude: i can change inner iterator to seq and it will still break
Solitude: but pairwise needs to be openArray to not work

Additional Information

>nim -v
Nim Compiler Version 1.7.1 [Windows: amd64]
Compiled at 2022-03-27
Copyright (c) 2006-2022 by Andreas Rumpf

active boot switches: -d:release

# make sure to include the git hash if not using a tagged release

For easier testing, here a slightly different version with templates:

import std / [algorithm, sequtils, sugar]

template bar(): untyped =
    toSeq(toSeq(s.combinations(2)).map(a => a).pairwise()).any(a => true) # Exhibits the bug
    # toSeq(toSeq(s.combinations(2)).map(proc(p: auto): auto = p).pairwise()).any(a => true) # Exhibits the bug
    # toSeq(toSeq(s.combinations(2)).pairwise()).any(a => true) # Everything OK

iterator combinations*[T](s: openArray[T], r: Positive): seq[T] =
  yield @[s[0], s[1]]
  yield @[s[0], s[1]]

iterator pairwise*[T](s: openArray[T]): seq[T] =
  yield @[s[0], s[1]]
  yield @[s[0], s[1]]

proc checkSpecialSubset5(s: seq[int]): bool = bar()

when isMainModule:
    let s = @[3,4,5,6]
    echo bar() # Works
    if bar(): # Works
        echo "Foo"
    echo checkSpecialSubset5(s) # With the bug: doesn't work
@EyeCon EyeCon changed the title A certain combination of iterators and procs crash the compiler A certain combination of iterators and procs crashes the compiler Apr 8, 2022
@metagn
Copy link
Collaborator

metagn commented Oct 20, 2024

The transformed AST is:

proc checkSpecialSubset5(s: seq[int]): bool =
  result = any(
    var result`gensym0: seq[typeof(map(
      var result`gensym4: seq[typeof(s.combinations(2))] = @[]
      for x`gensym4 in combinations(s, 2):
        add(result`gensym4, x`gensym4)
      result`gensym4, proc (a: auto): int = result = a[0]).pairwise())] = []
    block :tmp:
      var x`gensym0
      x`gensym0 = @[map(
        var result`gensym5: seq[typeof(s.combinations(2))] = []
        block :tmp:
          var x`gensym5
          x`gensym5 = @[s[0], s[1]]
          add(result`gensym5, x`gensym5)
        result`gensym5, (proc (a: auto): int = result = a[0], nil))[0], map(
        var result`gensym5: seq[typeof(s.combinations(2))] = []
        block :tmp:
          var x`gensym5
          x`gensym5 = @[s[0], s[1]]
          add(result`gensym5, x`gensym5)
        result`gensym5,
          (proc (a: auto): int = result = a[0], nil))[1]]
      add(result`gensym0, x`gensym0)
    result`gensym0,
               (proc (a: auto): bool = result = true, nil))

So a case of #24023 triggered by iterators duplicating the expression of openarray values (#13417).

@Araq Araq closed this as completed in d303c28 Oct 25, 2024
narimiran pushed a commit that referenced this issue Jan 14, 2025
…4333)

fixes #13417, fixes #19703

When passing an expression to an `openarray` iterator parameter: If the
expression is a statement list (considered "complex"), it's assigned in
a non-deep-copying way to a temporary variable first, then this variable
is used as a parameter. If it's not a statement list, i.e. a call or a
symbol, the parameter is substituted directly with the given expression.
In the case of calls, this results in the call potentially being
executed more than once, or can cause redefined variables in the
codegen.

To fix this, calls are also considered as "complex" assignments to
openarrays, as long as the return type of the call is not `openarray` as
the generated assignment in that case has issues/is unimplemented
(caused a segfault [here in
datamancer](https://github.com/SciNim/Datamancer/blob/47ba4d81bf240a7755b73bc48c1cec9b638d18ae/src/datamancer/dataframe.nim#L1580)).

As for why creating a temporary isn't the default only with exceptions
for things like `nkSym`, the "non-deep-copying" way of assignment
apparently still causes arrays to be copied according to a comment in
the code. I'm not sure to what extent this is true: if it still happens
on ARC/ORC, if it happens for every array length, or if we can fix it by
passing arrays by reference. Otherwise, a more general way to assign to
openarrays might be needed, but I'm not sure if the compiler can easily
do this.

(cherry picked from commit d303c28)
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.

2 participants