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.

Sign up for GitHub

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

Jump to bottom

Regression in Nim 2.x handling of destructors/copies/moves for discarded variables #24586

Open
elcritch opened this issue Dec 29, 2024 · 1 comment
Open

Regression in Nim 2.x handling of destructors/copies/moves for discarded variables #24586

elcritch opened this issue Dec 29, 2024 · 1 comment
Labels
ARC/ORC Memory Management

Comments

@elcritch
Copy link
Contributor

Description

A modified version of #24579 (comment) results in a double-free with ARC in test1. The second test using variables appears to be fine.

It's not clear if =destroy(x.fref) shouldn't be called in =destroy*(x: var Baz) in Nim 2.x but it does work with 1.6.20. This occurs on both Mac/arm64 and Linux/arm64.

My take it that it looks like the discarded variables have lots of extra copies and destructions occuring. Also of note is that adding proc =copy(x: var Bar) {.error.} doesn't error. Similarly =wasMoved doesn't appear to be called either.

Run with:
nim c -d:debug -d:useMalloc --mm:arc -d:traceArc -d:nimArcDebug -r "destroytest.nim"

import std/strutils
# destroytest.nim

type
  Foo = object
    id: int
  Bar = object
    id: int
    foo: Foo
  Baz = object
    id: int
    fref: ref Foo

var depth = 1

template printDestroying(x) = 
  depth.inc
  echo "\t".repeat(depth), "Destroying: ", x.repr, " addr: 0x", addr(x).pointer.repr 
template printDestroyed(x) = 
  echo "\t".repeat(depth), "Destroyed: ", x.repr, " addr: 0x", addr(x).pointer.repr 
  depth.dec

proc `=destroy`*(x: var Foo) =
  printDestroying(x)
  addr(x)[].id = -1
  printDestroyed(x)

proc `=destroy`*(x: var Bar) =
  printDestroying(x)
  `=destroy`(x.foo)
  addr(x)[].id = -1
  printDestroyed(x)

proc `=destroy`*(x: var Baz) =
  printDestroying(x)
  if not x.fref.isNil:
    `=destroy`(x.fref) # no issue if this is commented
  addr(x)[].id = -1
  printDestroyed(x)

proc test1() =
  echo "\n=== Test1 ==="
  discard Bar(id: 2, foo: Foo(id: 1))
  discard Baz(id: 4, fref: (ref Foo)(id: 3))

proc test2() =
  echo "\n=== Test2 ==="
  let x1 = Foo(id: 1)
  let x2 = Bar(id: 2, foo: x1)
  let x3 = (ref Foo)(id: 3)
  let x4 = Baz(id: 4, fref: x3)
  echo "addr x1: ", $(x1), " ", x1.unsafeAddr.pointer.repr
  echo "addr x2: ", $(x2), " ", x2.unsafeAddr.pointer.repr
  echo "addr x2.foo: ", $(x2.foo), " ", x2.foo.unsafeAddr.pointer.repr
  echo "addr x3: ref Foo: ", $(x3[]), " ", cast[pointer](x3).repr
  echo "addr x4: Baz #", $(x4), " ", x4.unsafeAddr.pointer.repr
  echo "addr x4.fref: ", $(x4.fref[]), " ", cast[pointer](x4.fref).repr
  echo ""

test1()
test2()

Nim Version

Nim Compiler Version 2.0.14 [Linux: arm64]
Compiled at 2024-12-23
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: bf4de6a
active boot switches: -d:release -d:danger

Current Output

## Note that `[Freed] 0xc42bba8e36b0` occurs twice, without -d:nimArcDebug it segfaults

=== Test1 ===
[Allocated] 0xc42bba8e36b0 result: 0xc42bba8e36c0
                Destroying: Baz(id: 4, fref: ref Foo(id: 3)) addr: 0x0000FFFFFF4D98F8
[ABOUT TO DESTROY] 0xc42bba8e36b0
                        Destroying: Foo(id: 3) addr: 0x0000C42BBA8E36C0
                        Destroyed: Foo(id: -1) addr: 0x0000C42BBA8E36C0
[Freed] 0xc42bba8e36b0
                Destroyed: Baz(id: -1, fref: ref Foo(id: -1)) addr: 0x0000FFFFFF4D98F8
[ABOUT TO DESTROY] 0xc42bba8e36b0
                Destroying: Foo(id: -1) addr: 0x0000C42BBA8E36C0
                Destroyed: Foo(id: -1) addr: 0x0000C42BBA8E36C0
[Freed] 0xc42bba8e36b0
                Destroying: Bar(id: 2, foo: Foo(id: 1)) addr: 0x0000FFFFFF4D98E8
                        Destroying: Foo(id: 1) addr: 0x0000FFFFFF4D98F0
                        Destroyed: Foo(id: -1) addr: 0x0000FFFFFF4D98F0
                Destroyed: Bar(id: -1, foo: Foo(id: -1)) addr: 0x0000FFFFFF4D98E8
                Destroying: Foo(id: 1) addr: 0x0000FFFFFF4D98C8
                Destroyed: Foo(id: -1) addr: 0x0000FFFFFF4D98C8

=== Test2 ===
[Allocated] 0xc42bba8e36d0 result: 0xc42bba8e36e0
[INCREF] 0xc42bba8e36d0
addr x1: (id: 1) 0000FFFFFF4D9678
addr x2: (id: 2, foo: (id: 1)) 0000FFFFFF4D96A8
addr x2.foo: (id: 1) 0000FFFFFF4D96B0
addr x3: ref Foo: (id: 3) 0000C42BBA8E36E0
addr x4: Baz #(id: 4, fref: ...) 0000FFFFFF4D96B8
addr x4.fref: (id: 3) 0000C42BBA8E36E0

                Destroying: Baz(id: 4, fref: ref Foo(id: 3)) addr: 0x0000FFFFFF4D96B8
[DECREF] 0xc42bba8e36d0
                Destroyed: Baz(id: -1, fref: ref Foo(id: 3)) addr: 0x0000FFFFFF4D96B8
[ABOUT TO DESTROY] 0xc42bba8e36d0
                Destroying: Foo(id: 3) addr: 0x0000C42BBA8E36E0
                Destroyed: Foo(id: -1) addr: 0x0000C42BBA8E36E0
[Freed] 0xc42bba8e36d0
                Destroying: Bar(id: 2, foo: Foo(id: 1)) addr: 0x0000FFFFFF4D96A8
                        Destroying: Foo(id: 1) addr: 0x0000FFFFFF4D96B0
                        Destroyed: Foo(id: -1) addr: 0x0000FFFFFF4D96B0
                Destroyed: Bar(id: -1, foo: Foo(id: -1)) addr: 0x0000FFFFFF4D96A8
                Destroying: Foo(id: 1) addr: 0x0000FFFFFF4D9678
                Destroyed: Foo(id: -1) addr: 0x0000FFFFFF4D9678

Expected Output

# Run with Nim 1.6.20 

=== Test1 ===
[Allocated] 0x144605f10 result: 0x144605f20
[ABOUT TO DESTROY] 0x144605f10
                Destroying: Foo(id: 3) addr: 0x0000000144605F20
                Destroyed: Foo(id: -1) addr: 0x0000000144605F20
[Freed] 0x144605f10

=== Test2 ===
[Allocated] 0x144606080 result: 0x144606090
[INCREF] 0x144606080
addr x1: (id: 1) 000000016DAD6DE8
addr x2: (id: 2, foo: (id: 1)) 000000016DAD6DD8
addr x2.foo: (id: 1) 000000016DAD6DE0
addr x3: ref Foo: (id: 3) 0000000144606090
addr x4: Baz #(id: 4, fref: ...) 000000016DAD6DB8
addr x4.fref: (id: 3) 0000000144606090

                Destroying: Baz(id: 4, fref: ref Foo(id: 3)) addr: 0x000000016DAD6DB8
[DeCREF] 0x144606080
                Destroyed: Baz(id: -1, fref: ref Foo(id: 3)) addr: 0x000000016DAD6DB8
[ABOUT TO DESTROY] 0x144606080
                Destroying: Foo(id: 3) addr: 0x0000000144606090
                Destroyed: Foo(id: -1) addr: 0x0000000144606090
[Freed] 0x144606080
                Destroying: Bar(id: 2, foo: Foo(id: 1)) addr: 0x000000016DAD6DD8
                        Destroying: Foo(id: 1) addr: 0x000000016DAD6DE0
                        Destroyed: Foo(id: -1) addr: 0x000000016DAD6DE0
                Destroyed: Bar(id: -1, foo: Foo(id: -1)) addr: 0x000000016DAD6DD8
                Destroying: Foo(id: 1) addr: 0x000000016DAD6DE8
                Destroyed: Foo(id: -1) addr: 0x000000016DAD6DE8

Known Workarounds

No response

Additional Information

Spawns from #24579
@elcritch elcritch mentioned this issue Dec 29, 2024
Open
@elcritch
Copy link
Contributor Author

Actually looks like 1.6 missed some destroys of Bar/Baz in test1 as well? Or maybe it's an optimization gone wrong.

On 1.6 without calling =destroy(x.fref) frees Baz.fref in test1 but not in test2. While 1.6 with calling =destroy(x.fref) free Baz.fref in test1 and `test2.

Output from Test1 on 1.6 without calling =destroy(x.fref):

=== Test1 ===
[Allocated] 0x137605f10 result: 0x137605f20
[ABOUT TO DESTROY] 0x137605f10
                Destroying: Foo(id: 3) addr: 0x0000000137605F20
                Destroyed: Foo(id: -1) addr: 0x0000000137605F20
[Freed] 0x137605f10

=== Test2 ===
[Allocated] 0x12a606080 result: 0x12a606090
[INCREF] 0x12a606080
addr x1: (id: 1) 000000016B97ADE8
addr x2: (id: 2, foo: (id: 1)) 000000016B97ADD8
addr x2.foo: (id: 1) 000000016B97ADE0
addr x3: ref Foo: (id: 3) 000000012A606090
addr x4: Baz #(id: 4, fref: ...) 000000016B97ADB8
addr x4.fref: (id: 3) 000000012A606090

                Destroying: Baz(id: 4, fref: ref Foo(id: 3)) addr: 0x000000016B97ADB8
                Destroyed: Baz(id: -1, fref: ref Foo(id: 3)) addr: 0x000000016B97ADB8
[DeCREF] 0x12a606080
                Destroying: Bar(id: 2, foo: Foo(id: 1)) addr: 0x000000016B97ADD8
                        Destroying: Foo(id: 1) addr: 0x000000016B97ADE0
                        Destroyed: Foo(id: -1) addr: 0x000000016B97ADE0
                Destroyed: Bar(id: -1, foo: Foo(id: -1)) addr: 0x000000016B97ADD8
                Destroying: Foo(id: 1) addr: 0x000000016B97ADE8
                Destroyed: Foo(id: -1) addr: 0x000000016B97ADE8

Output on 1.6 with calling =destroy(x.fref):

=== Test1 ===
[Allocated] 0x137605f10 result: 0x137605f20
[ABOUT TO DESTROY] 0x137605f10
                Destroying: Foo(id: 3) addr: 0x0000000137605F20
                Destroyed: Foo(id: -1) addr: 0x0000000137605F20
[Freed] 0x137605f10

=== Test2 ===
[Allocated] 0x154e06080 result: 0x154e06090
[INCREF] 0x154e06080
addr x1: (id: 1) 000000016B33ADE8
addr x2: (id: 2, foo: (id: 1)) 000000016B33ADD8
addr x2.foo: (id: 1) 000000016B33ADE0
addr x3: ref Foo: (id: 3) 0000000154E06090
addr x4: Baz #(id: 4, fref: ...) 000000016B33ADB8
addr x4.fref: (id: 3) 0000000154E06090

                Destroying: Baz(id: 4, fref: ref Foo(id: 3)) addr: 0x000000016B33ADB8
[DeCREF] 0x154e06080
                Destroyed: Baz(id: -1, fref: ref Foo(id: 3)) addr: 0x000000016B33ADB8
[ABOUT TO DESTROY] 0x154e06080
                Destroying: Foo(id: 3) addr: 0x0000000154E06090
                Destroyed: Foo(id: -1) addr: 0x0000000154E06090
[Freed] 0x154e06080
                Destroying: Bar(id: 2, foo: Foo(id: 1)) addr: 0x000000016B33ADD8
                        Destroying: Foo(id: 1) addr: 0x000000016B33ADE0
                        Destroyed: Foo(id: -1) addr: 0x000000016B33ADE0
                Destroyed: Bar(id: -1, foo: Foo(id: -1)) addr: 0x000000016B33ADD8
                Destroying: Foo(id: 1) addr: 0x000000016B33ADE8
                Destroyed: Foo(id: -1) addr: 0x000000016B33ADE8

@elcritch elcritch mentioned this issue Dec 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ARC/ORC Memory Management
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@elcritch @ringabout