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

arc bug using iterator parentDirs returning inside a try #22001

Closed
c-blake opened this issue Jun 4, 2023 · 7 comments
Closed

arc bug using iterator parentDirs returning inside a try #22001

c-blake opened this issue Jun 4, 2023 · 7 comments

Comments

@c-blake
Copy link
Contributor

c-blake commented Jun 4, 2023

Description

import std/os

iterator parentDirs*(path: string, fromRoot=false, inclusive=true): string =
# Nim-devel-f552618d6b1cddee3fdad7b4cf1917481ca346d4 w/arc breaks here
# finding `current` to be "" prematurely.  Nim-1.6.12 works.  iterator
# lifted from `lib/std/private/ospaths2.nim` to be more self-contained.
  if not fromRoot:  # Commenting out the if & else: when.. hides bug
    var current = path
    if inclusive: yield path
    while true:
      if current.isRootDir: break # <- current=="" => current.isRootDir
      current = current.parentDir
      yield current
  else: # Any simplification below that drops `for` hides bug
    when doslikeFileSystem:
      let start = path.splitDrive.drive.len
    else:
      const start = 0
    for i in countup(start, path.len - 2): # ignore the last /
      if path[i] in {DirSep, AltSep} and
          (i == 0 or path[i-1] notin {DirSep, AltSep}):
        yield path.substr(0, i)
    if inclusive: yield path

proc finOp(wd, name: string): (string,File) = # Find & open FIRST `name`
  for dir in parentDirs(wd):                  #.. going up to the root.
    echo dir  # Commenting out try/except below hides bug
    try: result[0] = dir/name; result[1] = open(result[0]); return
    except CatchableError: discard

let (path, f) = finOp("/1/2/3", "4")  # All same if this->inside a proc

Nim Version

I tried both Nim-devel f552618 as well as Nim-devel 767fec1 .

Current Output

/1/2/3
/1/2

Expected Output

/1/2/3
/1/2
/1
/

Possible Solution

I just worked around this by not doing some of the things where the comments say it hides the bug.

Additional Information

The problem does not seem to manifest on Nim-1.6.12 or with either --mm:markAndSweep or --mm:refc.

So, someone with more patience than I have at the moment could run nim c --expandArc:finOp --mm:arc this.nim or maybe do a git bisect first.

EDIT: I first tested this code about 3..4 weeks prior to this issue filing and I did not notice this problem then.

c-blake added a commit to c-blake/bu that referenced this issue Jun 4, 2023
@ringabout
Copy link
Member

It's quite infuriating to bisect this problem because I have to switch to 1.6.12 compiler and do some git stash tricks constantly.

Nevertheless, here is probably the regression that causes this issue => #20471

@c-blake
Copy link
Contributor Author

c-blake commented Jun 5, 2023

Thank you for having the patience to track that down (to an admittedly large set of changes).

@Araq
Copy link
Member

Araq commented Jun 5, 2023

Nonsense, the new move analyser is perfect. The problem is Kauffman's string tunneling device.

@Araq
Copy link
Member

Araq commented Jun 27, 2023

Reduced to:

proc finOp2(path, name: string): (string,File) = # Find & open FIRST `name`
  var current = path
  while true:
    if current.isRootDir: break # <- current=="" => current.isRootDir
    current = current.parentDir
    let dir = current
    echo dir  # Commenting out try/except below hides bug
    try: result[0] = dir/name; result[1] = open(result[0]); return
    except CatchableError: discard

let (path, f) = finOp2("/1/2/3", "4")  # All same if this->inside a proc

@Araq
Copy link
Member

Araq commented Jun 27, 2023

The control flow graph move analyser misinterprets the try except.

@c-blake
Copy link
Contributor Author

c-blake commented Jun 27, 2023

I am glad you looked into it! FWIW, I cannot get current nim-devel to break with that same code. So, we may need a more robustly breaking test case. I suppose one could bisect again the current head with one of those two revisions I mentioned to see what perturbed things, but unless it was intentionally fixed there is probably still a bug lurking.

Araq added a commit that referenced this issue Jun 27, 2023
@Araq
Copy link
Member

Araq commented Jun 27, 2023

It's all trivial to reproduce for me and a fix is in the CI now. No worries.

@Araq Araq closed this as completed in 427ad17 Jun 27, 2023
bung87 pushed a commit to bung87/Nim that referenced this issue Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants