nimforum mirror - How to use a string in an allocShared object

winksaville (orginal) [2015-04-23T00:14:32+02:00] view original

I'm exploring message passing using nim and I've created a MsgQueue that has a "name: string" field:

MsgQueue* = object of Queue
  name*: string
  head*: MsgPtr
  tail*: MsgPtr
MsgQueuePtr* = ptr MsgQueue

And then this is allocated and deleted using code like this:

proc newMpscFifo*(name: string): MsgQueuePtr =
  ## Create a new Fifo
  var mq = cast[MsgQueuePtr](allocShared(sizeof(MsgQueue)))
  
  mq.name = name  ## < SIGSEGV here
  GC_ref(mq.name)
  mq.arena = arena
  var mn = getMsg()
  mq.head = mn
  mq.tail = mn
  result = mq

proc delMpscFifo*(qp: QueuePtr) =
  var mq = cast[MsgQueuePtr](qp)
  doAssert(mq.isEmpty())
  retMsg(mq.head)
  mq.head = nil
  mq.tail = nil
  GcUnref(mq.name)
  deallocShared(mq)

And used like so:

var fifo = newMpscFifo("fifo1")

# Use the fifo

delMpscFifo(fifo)

Sometimes the assignment "mq.name = name" crashes with a SIGSEGV like below:


Traceback (most recent call last)
bm1.nim(46)              bm1
mpscfifo.nim(115)        newMpscFifo
mpscfifo.nim(83)         newMpscFifo
gc.nim(269)              unsureAsgnRef
gc.nim(167)              decRef
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
error running: file=tests/bm1

What is the correct way to use a string in an allocShared object? (The actual code is here: https://github.com/winksaville/nim-mpscfifo/blob/master/mpscfifo.nim)

winksaville (orginal) [2015-04-23T03:52:44+02:00] view original

I've narrowed down the problem and I don't believe the problem is directly related to the assignment of mq.name to name in line 20. Instead it appears to be memory corruption. Below is the the simplest code I can get to fail:

$ cat -n as.nim ; nim c -r --hints:off as
     1	import locks
     2
     3	type
     4	  MsgQueuePtr* = ptr MsgQueue
     5	  MsgQueue* = object of RootObj
     6	    name*: string
     7	    cond*: ptr TCond
     8
     9	proc newMpscFifo*(name: string, blocking: bool): MsgQueuePtr =
    10	  var
    11	    mq: MsgQueuePtr = nil
    12	    cond: ptr TCond = nil
    13
    14	  if blocking:
    15	    cond = cast[ptr TCond](allocShared(sizeof(TCond)))
    16	    cond[].initCond()
    17
    18	  mq = cast[MsgQueuePtr](allocShared(sizeof(MsgQueue)))
    19
    20	  mq.name = name
    21	  GC_ref(mq.name)
    22	  mq.cond = cond
    23
    24	  result = mq
    25
    26	proc delMpscFifo*(mq: MsgQueuePtr) =
    27	  if mq.cond != nil:
    28	    mq.cond[].deinitCond()
    29	    deallocShared(mq.cond)
    30	  GC_unref(mq.name)
    31	  deallocShared(mq)
    32
    33	var mq = newMpscFifo("mq", true)
    34	mq.delMpscFifo()
    35
    36	mq = newMpscFifo("mq", false)
    37	mq.delMpscFifo()
lib/core/locks.nim(22, 43) Warning: LockEffect is deprecated [Deprecated]
lib/core/locks.nim(25, 44) Warning: LockEffect is deprecated [Deprecated]
gcc -c  -w -pthread  -I/opt/nim/lib -o /home/wink/prgs/nim/mpscfifo/nimcache/as.o /home/wink/prgs/nim/mpscfifo/nimcache/as.c
[Linking]
/home/wink/prgs/nim/mpscfifo/as
Traceback (most recent call last)
as.nim(36)               as
as.nim(20)               newMpscFifo
gc.nim(269)              unsureAsgnRef
gc.nim(167)              decRef
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Error: execution of an external program failed

Several changes can make this code succeed, the 3 most interesting cases I've included below, but there are others. For instance, only calling newMpscFifo with the second parameter true followed by calling it with false fails. Any other order does NOT SIGSEV.

First, don't deinit mq.cond by commenting out line 28, this is a poor solution, but interesting that it does work leads me to believe its memory corruption.

$ cat -n as.nim ; nim c -r --hints:off as
     1	import locks
     2
     3	type
     4	  MsgQueuePtr* = ptr MsgQueue
     5	  MsgQueue* = object of RootObj
     6	    name*: string
     7	    cond*: ptr TCond
     8
     9	proc newMpscFifo*(name: string, blocking: bool): MsgQueuePtr =
    10	  var
    11	    mq: MsgQueuePtr = nil
    12	    cond: ptr TCond = nil
    13
    14	  if blocking:
    15	    cond = cast[ptr TCond](allocShared(sizeof(TCond)))
    16	    cond[].initCond()
    17
    18	  mq = cast[MsgQueuePtr](allocShared(sizeof(MsgQueue)))
    19
    20	  mq.name = name
    21	  GC_ref(mq.name)
    22	  mq.cond = cond
    23
    24	  result = mq
    25
    26	proc delMpscFifo*(mq: MsgQueuePtr) =
    27	  if mq.cond != nil:
    28	    #mq.cond[].deinitCond() <<<< DON'T deinit mq.cond[]
    29	    deallocShared(mq.cond)
    30	  GC_unref(mq.name)
    31	  deallocShared(mq)
    32
    33	var mq = newMpscFifo("mq", true)
    34	mq.delMpscFifo()
    35
    36	mq = newMpscFifo("mq", false)
    37	mq.delMpscFifo()
lib/core/locks.nim(22, 43) Warning: LockEffect is deprecated [Deprecated]
lib/core/locks.nim(25, 44) Warning: LockEffect is deprecated [Deprecated]
[Linking]
/home/wink/prgs/nim/mpscfifo/as

Second, don't have MsgQueue inherit from RootObj, so change line 5 as below. This is reasonable and there is no driving reason to inherit from RootObj, in addition it saves 8 bytes of memory.

$ cat -n as.nim ; nim c -r --hints:off as
     1	import locks
     2
     3	type
     4	  MsgQueuePtr* = ptr MsgQueue
     5	  MsgQueue* = object # of RootObj <<<< REMOVE RootObj
     6	    name*: string
     7	    cond*: ptr TCond
     8
     9	proc newMpscFifo*(name: string, blocking: bool): MsgQueuePtr =
    10	  var
    11	    mq: MsgQueuePtr = nil
    12	    cond: ptr TCond = nil
    13
    14	  if blocking:
    15	    cond = cast[ptr TCond](allocShared(sizeof(TCond)))
    16	    cond[].initCond()
    17
    18	  mq = cast[MsgQueuePtr](allocShared(sizeof(MsgQueue)))
    19
    20	  mq.name = name
    21	  GC_ref(mq.name)
    22	  mq.cond = cond
    23
    24	  result = mq
    25
    26	proc delMpscFifo*(mq: MsgQueuePtr) =
    27	  if mq.cond != nil:
    28	    mq.cond[].deinitCond()
    29	    deallocShared(mq.cond)
    30	  GC_unref(mq.name)
    31	  deallocShared(mq)
    32
    33	var mq = newMpscFifo("mq", true)
    34	mq.delMpscFifo()
    35
    36	mq = newMpscFifo("mq", false)
    37	mq.delMpscFifo()
lib/core/locks.nim(22, 43) Warning: LockEffect is deprecated [Deprecated]
lib/core/locks.nim(25, 44) Warning: LockEffect is deprecated [Deprecated]
gcc -c  -w -pthread  -I/opt/nim/lib -o /home/wink/prgs/nim/mpscfifo/nimcache/as.o /home/wink/prgs/nim/mpscfifo/nimcache/as.c
[Linking]
/home/wink/prgs/nim/mpscfifo/as

And third, allocate MsgQueue with allocShared0 at line 18 and seems "best solution". I suggest that its "best" because I notice that sizeof(MsgQueue) without inheriting from RootObj is 16 bytes and its 24 bytes when it does inherit from RootObj. So even though I'm initializing all visible fields of MsgQueue there is 8 bytes I'm not when it inherits from RootObj unless I use allocShared0. So using allocShared0 seems the most robust and means if in the future MsgQueue needs to inherit from RootObj everything looks like it will continue to work.

$ cat -n as.nim ; nim c -r --hints:off as
     1	import locks
     2
     3	type
     4	  MsgQueuePtr* = ptr MsgQueue
     5	  MsgQueue* = object of RootObj
     6	    name*: string
     7	    cond*: ptr TCond
     8
     9	proc newMpscFifo*(name: string, blocking: bool): MsgQueuePtr =
    10	  var
    11	    mq: MsgQueuePtr = nil
    12	    cond: ptr TCond = nil
    13
    14	  if blocking:
    15	    cond = cast[ptr TCond](allocShared(sizeof(TCond)))
    16	    cond[].initCond()
    17
    18	  mq = cast[MsgQueuePtr](allocShared0(sizeof(MsgQueue))) # <<<< allocShared0
    19
    20	  mq.name = name
    21	  GC_ref(mq.name)
    22	  mq.cond = cond
    23
    24	  result = mq
    25
    26	proc delMpscFifo*(mq: MsgQueuePtr) =
    27	  if mq.cond != nil:
    28	    mq.cond[].deinitCond()
    29	    deallocShared(mq.cond)
    30	  GC_unref(mq.name)
    31	  deallocShared(mq)
    32
    33	var mq = newMpscFifo("mq", true)
    34	mq.delMpscFifo()
    35
    36	mq = newMpscFifo("mq", false)
    37	mq.delMpscFifo()
lib/core/locks.nim(22, 43) Warning: LockEffect is deprecated [Deprecated]
lib/core/locks.nim(25, 44) Warning: LockEffect is deprecated [Deprecated]
gcc -c  -w -pthread  -I/opt/nim/lib -o /home/wink/prgs/nim/mpscfifo/nimcache/as.o /home/wink/prgs/nim/mpscfifo/nimcache/as.c
[Linking]
/home/wink/prgs/nim/mpscfifo/as

So a rule of thumb appears to be always use allocShared0 for nim objects. In fact maybe there should be a "allocObject" and "deallocObject" and using others would generate a warning or maybe even an error.

Does anyone know if my analysis and conclusions are reasonable? And any other thoughts or observations would be welcome.

pwernersbach (orginal) [2015-04-23T06:08:22+02:00] view original

Araq can comment on this authoritatively, but I can confirm that this SIGSEGV's on OS X Yosemite. It looks like it's a bug in the garbage collector.

As a suggestion, why not use ref's instead of ptr's? ptr's are unsafe and should generally only be used when you're interfacing with unsafe code (C/C++), or need the unsafe properties for some reason. This is a version that uses ref's, it's much simpler:


import locks
type
    MsgQueuePtr* = ref MsgQueue
    MsgQueue* = object of RootObj
        name*: string
        cond*: ref TCond

proc newMpscFifo*(name: string, blocking: bool): MsgQueuePtr =
    var
        mq: MsgQueuePtr = nil
        cond: ref TCond = nil
    
    if blocking:
        new(cond)
        cond[].initCond()
    
    new(mq)
    
    mq.name = name
    GC_ref(mq.name)
    mq.cond = cond
    
    result = mq

proc delMpscFifo*(mq: MsgQueuePtr) =
    if mq.cond != nil:
        mq.cond[].deinitCond()
    
    GC_unref(mq.name)

var mq = newMpscFifo("mq", true)
mq.delMpscFifo()

mq = newMpscFifo("mq", false)
mq.delMpscFifo()

pwernersbach (orginal) [2015-04-23T06:13:09+02:00] view original

Also, I left the GC_ref and GC_unref calls in there, but you should not use them unless you have a specific reason why you don't want the GC to collect mq.name.

winksaville (orginal) [2015-04-23T07:24:52+02:00] view original

The reason I was using pointers was because the code is to be used by multiple threads so it is my understanding was the alloc/dealloc had to be used.

Jehan (orginal) [2015-04-23T09:07:03+02:00] view original

There are a number of problems that you have, but the biggest one is that you use allocShared() instead of allocShared0(). This means that when you assign to mq.name and the existing value of mq.name is garbage (because it's not initialized), then the program will crash as you observed, because assignment to mq.name comprises three steps:

Decrement the reference count of the old mq.name.

Set the new value of mq.name.

Increment the reference count of the new mq.name.

Note that while this makes calls to GC_ref and GC_unref superfluous for the reference counting GC, you may need them for the mark-and-sweep GC still.

Further problems: If you use objects with inheritance (i.e. object of ... or object {.inheritable.}, then neither allocShared() nor allocShared0() will initialize the type field of these objects correctly. You need to set them on your own (e.g. by creating them locally and using copyMem).

Also, if you want to actually have multiple threads use the mq.name field, this is generally not safe. If you want to use shared memory to have it accessed by multiple threads, then you generally do not want to mix in GCed memory with that unless you know exactly what you are doing. My recommendation would be to have a simple library that allows for explicit manual allocation and deallocation of objects (including special shared strings and seqs if you need those).

Araq (orginal) [2015-04-23T09:33:39+02:00] view original

http://nim-lang.org/manual.html#reference-and-pointer-types

"Note: The example only works because the memory is initialized to zero (alloc0 instead of alloc does this): d.s is thus initialized to nil which the string assignment can handle. One needs to know low level details like this when mixing garbage collected data with unmanaged memory."

pwernersbach (orginal) [2015-04-23T19:43:14+02:00] view original

I've filed an issue for this at https://github.com/Araq/Nim/issues/2591.

Mirror of forum.nim-lang.org

1154 :: How to use a string in an allocShared object