[ARC] Memory zeroed after =sink even for self assignment

[Vindaar]

Working on mratsim/Arraymancer#477 I've chased a weird segfault originating for self assignments of tensors, which only happens for tensors stored in objects.
I've reduced it to a relatively simple piece of code now, which mostly reproduces the bug. However, in my actual code the issue also happens if compiled without --gc:arc(sink is used in that case even for default GC. Does that make sense? Or should the =sink procs always be guarded by defined(gcDestructors)?).

Or maybe I still misunderstand how to use ARC?

Example

type
  CpuStorage*[T] = ref CpuStorageObj[T]
  CpuStorageObj[T] = object
    size*: int
    raw_buffer*: ptr UncheckedArray[T]
  Tensor[T] = object
    buf*: CpuStorage[T]
  TestObject = object
    x: Tensor[float]

proc `=destroy`[T](s: var CpuStorageObj[T]) =
  s.raw_buffer.deallocShared()
  s.size = 0

proc `=destroy`[T](t: var Tensor[T]) =
  `=destroy`(t.buf)

proc `=destroy`(t: var TestObject) =
  `=destroy`(t.x)

proc `=`[T](a: var CpuStorageObj[T]; b: CpuStorageObj[T]) {.error.}

#proc `=sink`[T](a: var CpuStorageObj[T], b: CpuStorageObj[T]) =
#  if a.raw_buffer != b.raw_buffer:
#    `=destroy`(a)
#    wasMoved(a)
#    a.raw_buffer = b.raw_buffer
#    a.size = b.size
#
#proc `=sink`[T](a: var Tensor[T], b: Tensor[T]) =
#  if a.buf != b.buf:
#    `=destroy`(a)
#    wasMoved(a)
#    `=sink`(a.buf, b.buf)

proc allocCpuStorage[T](s: var CpuStorage[T], size: int) =
  new(s)
  s.raw_buffer = cast[ptr UncheckedArray[T]](allocShared0(sizeof(T) * size))
  s.size = size

proc newTensor[T](size: int): Tensor[T] =
  allocCpuStorage(result.buf, size)

proc `[]`[T](t: Tensor[T], idx: int): T = t.buf.raw_buffer[idx]
proc `[]=`[T](t: Tensor[T], idx: int, val: T) = t.buf.raw_buffer[idx] = val

proc toTensor[T](s: seq[T]): Tensor[T] =
  result = newTensor[T](s.len)
  for i, x in s:
    result[i] = x

proc main() =
  var t: TestObject
  t.x = toTensor(@[1.0, 2, 3, 4])
  t.x = t.x
  # the following line holds true even if it shouldn't of course
  doAssert t.x.buf.isNil

main()

# does ``not`` happen if the code is at top level!
#var t: TestObject
#t.x = toTensor(@[1.0, 2, 3, 4])
#t.x = t.x
#doAssert t.x.buf.isNil

Current Output

Nothing, because the doAssert falsely holds.

Expected Output

doAssert should not hold, thus program should fail.

Additional Information

It seems to be a more complex version of #10689 to me.

The generated C code (using --gc:arc -d:danger):

	nimZeroMem((void*)(&T1_), sizeof(tyObject_Tensor__IQAwMnmu4uBKMjqXsM9adCA));
	toTensor__UpFt1bYA1ehyF1G4AabWcQ(colontmpD_, (&T1_));
	if (NIM_UNLIKELY(*nimErr_)) goto BeforeRet_;
	eqsink___UU2NWj0I19chxYozquBBXMA((&t.x), T1_);
	eqsink___UU2NWj0I19chxYozquBBXMA((&t.x), t.x);
	nimZeroMem((void*)(&t.x), sizeof(tyObject_Tensor__IQAwMnmu4uBKMjqXsM9adCA));
	{
		if (!!((t.x.buf == 0))) goto LA4_;
		failedAssertImpl__W9cjVocn1tjhW7p7xohJj6A(TM__zIQ3yoETjuVGvXrPQFKeSQ_3);
		if (NIM_UNLIKELY(*nimErr_)) goto BeforeRet_;
	}

The latter nimZeroMem shouldn't be there, because the eqsink before is a self assignment. It doesn't matter if I define the =sink in the code or not.

I just pulled the newest devel:

Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2020-11-29
Copyright (c) 2006-2020 by Andreas Rumpf

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

[cooldome]

FYI, Expected way of writing this example:

type
  CpuStorage*[T] = ref CpuStorageObj[T]
  CpuStorageObj[T] = object
    size*: int
    raw_buffer*: ptr UncheckedArray[T]
  Tensor[T] = object
    buf*: CpuStorage[T]
  TestObject = object
    x: Tensor[float]

proc `=destroy`[T](s: var CpuStorageObj[T]) =
  if s.raw_buffer != nil:
    s.raw_buffer.deallocShared()
    s.size = 0
    s.raw_buffer = nil

proc `=`[T](a: var CpuStorageObj[T]; b: CpuStorageObj[T]) {.error.}

proc allocCpuStorage[T](s: var CpuStorage[T], size: int) =
  new(s)
  s.raw_buffer = cast[ptr UncheckedArray[T]](allocShared0(sizeof(T) * size))
  s.size = size

proc newTensor[T](size: int): Tensor[T] =
  allocCpuStorage(result.buf, size)

proc `[]`[T](t: Tensor[T], idx: int): T = t.buf.raw_buffer[idx]
proc `[]=`[T](t: Tensor[T], idx: int, val: T) = t.buf.raw_buffer[idx] = val

proc toTensor[T](s: seq[T]): Tensor[T] =
  result = newTensor[T](s.len)
  for i, x in s:
    result[i] = x

proc main() =
  var t: TestObject
  t.x = toTensor(@[1.0, 2, 3, 4])
  t.x = t.x
  # the following line holds true even if it shouldn't of course
  echo t.x.buf.isNil

main()

Bug, i will take a look

[Vindaar]

Thanks for the super quick response and the example!

The additional =destroy I wrote are what's used by default anyway, right? And yes, completely forgot to nil the pointer.

[Vindaar]

I've just stumbled on a much simpler version of this:

type
  ValueKind* = enum
    VNull,
    VFloat,
    VString

  Value* = object
    case kind*: ValueKind
    of VString:
      str*: string
    of VFloat:
      fnum*: float
    of VNull:
      discard

proc `%~`*(v: string): Value =
  result = Value(kind: VString, str: v)

proc `%~`*(v: SomeFloat): Value =
  result = Value(kind: VFloat, fnum: v.float)

proc main =
  var p: tuple[x: Value, y: Value]
  p.x = %~ 1.2
  p.y = %~ 5.5
  echo p # all fine
  p.y = if p.y.kind == VNull: %~ 0.0 else: p.y
  echo p.y # p.y is going to be `VNull` after this self assignment

main()

If the string branch of the Value object is removed the problem also disappears (but I can add another int branch just fine!).