fixes 7833 (#8533)

* fixes #7833; still to-do: fix setLen
* make tests green again
* also fixes setLen and string concats; refs #7833
* change formating to avoid a compiler warning
* emit the write barrier also for addChar
* fixes yet another regression
* make setLengthStr compile for the old version
* make growobjcrash complete earlier

compiler/ccgexprs.nim

@@ -1063,7 +1063,7 @@ proc genStrAppend(p: BProc, e: PNode, d: var TLoc) =
   #    appendChar(s, 'z');
   #  }
   var
-    a, dest: TLoc
+    a, dest, call: TLoc
     appends, lens: Rope
   assert(d.k == locNone)
   var L = 0
@@ -1082,8 +1082,9 @@ proc genStrAppend(p: BProc, e: PNode, d: var TLoc) =
         addf(lens, "($1 ? $1->$2 : 0) + ", [rdLoc(a), lenField(p)])
       add(appends, ropecg(p.module, "#appendString($1, $2);$n",
                         rdLoc(dest), rdLoc(a)))
-  linefmt(p, cpsStmts, "$1 = #resizeString($1, $2$3);$n",
+  initLoc(call, locCall, e, OnHeap)
-          rdLoc(dest), lens, rope(L))
+  call.r = ropecg(p.module, "#resizeString($1, $2$3)", [rdLoc(dest), lens, rope(L)])
+  genAssignment(p, dest, call, {})
   add(p.s(cpsStmts), appends)
   gcUsage(p.config, e)
 
@@ -1092,17 +1093,20 @@ proc genSeqElemAppend(p: BProc, e: PNode, d: var TLoc) =
   #    seq = (typeof seq) incrSeq(&seq->Sup, sizeof(x));
   #    seq->data[seq->len-1] = x;
   let seqAppendPattern = if not p.module.compileToCpp:
-                           "$1 = ($2) #incrSeqV3(&($1)->Sup, $3);$n"
+                           "($2) #incrSeqV3(&($1)->Sup, $3)"
                          else:
-                           "$1 = ($2) #incrSeqV3($1, $3);$n"
+                           "($2) #incrSeqV3($1, $3)"
-  var a, b, dest, tmpL: TLoc
+  var a, b, dest, tmpL, call: TLoc
   initLocExpr(p, e.sons[1], a)
   initLocExpr(p, e.sons[2], b)
   let seqType = skipTypes(e.sons[1].typ, {tyVar})
-  lineCg(p, cpsStmts, seqAppendPattern, [
+  initLoc(call, locCall, e, OnHeap)
-      rdLoc(a),
+  call.r = ropecg(p.module, seqAppendPattern, [rdLoc(a),
-      getTypeDesc(p.module, e.sons[1].typ),
+    getTypeDesc(p.module, e.sons[1].typ),
-      genTypeInfo(p.module, seqType, e.info)])
+    genTypeInfo(p.module, seqType, e.info)])
+  # emit the write barrier if required, but we can always move here, so
+  # use 'genRefAssign' for the seq.
+  genRefAssign(p, a, call, {})
   #if bt != b.t:
   #  echo "YES ", e.info, " new: ", typeToString(bt), " old: ", typeToString(b.t)
   initLoc(dest, locExpr, e.sons[2], OnHeap)
@@ -1509,7 +1513,7 @@ proc genArrayLen(p: BProc, e: PNode, d: var TLoc, op: TMagic) =
   else: internalError(p.config, e.info, "genArrayLen()")
 
 proc genSetLengthSeq(p: BProc, e: PNode, d: var TLoc) =
-  var a, b: TLoc
+  var a, b, call: TLoc
   assert(d.k == locNone)
   var x = e.sons[1]
   if x.kind in {nkAddr, nkHiddenAddr}: x = x[0]
@@ -1517,17 +1521,27 @@ proc genSetLengthSeq(p: BProc, e: PNode, d: var TLoc) =
   initLocExpr(p, e.sons[2], b)
   let t = skipTypes(e.sons[1].typ, {tyVar})
   let setLenPattern = if not p.module.compileToCpp:
-      "$1 = ($3) #setLengthSeqV2(&($1)->Sup, $4, $2);$n"
+      "($3) #setLengthSeqV2(&($1)->Sup, $4, $2)"
     else:
-      "$1 = ($3) #setLengthSeqV2($1, $4, $2);$n"
+      "($3) #setLengthSeqV2($1, $4, $2)"
 
-  lineCg(p, cpsStmts, setLenPattern, [
+  initLoc(call, locCall, e, OnHeap)
+  call.r = ropecg(p.module, setLenPattern, [
       rdLoc(a), rdLoc(b), getTypeDesc(p.module, t),
       genTypeInfo(p.module, t.skipTypes(abstractInst), e.info)])
+  genAssignment(p, a, call, {})
   gcUsage(p.config, e)
 
 proc genSetLengthStr(p: BProc, e: PNode, d: var TLoc) =
-  binaryStmt(p, e, d, "$1 = #setLengthStr($1, $2);$n")
+  var a, b, call: TLoc
+  if d.k != locNone: internalError(p.config, e.info, "genSetLengthStr")
+  initLocExpr(p, e.sons[1], a)
+  initLocExpr(p, e.sons[2], b)
+
+  initLoc(call, locCall, e, OnHeap)
+  call.r = ropecg(p.module, "#setLengthStr($1, $2)", [
+      rdLoc(a), rdLoc(b)])
+  genAssignment(p, a, call, {})
   gcUsage(p.config, e)
 
 proc genSwap(p: BProc, e: PNode, d: var TLoc) =
@@ -1844,7 +1858,13 @@ proc genMagicExpr(p: BProc, e: PNode, d: var TLoc, op: TMagic) =
         getTypeDesc(p.module, ranged), res])
 
   of mConStrStr: genStrConcat(p, e, d)
-  of mAppendStrCh: binaryStmt(p, e, d, "$1 = #addChar($1, $2);$n")
+  of mAppendStrCh:
+    var dest, b, call: TLoc
+    initLoc(call, locCall, e, OnHeap)
+    initLocExpr(p, e.sons[1], dest)
+    initLocExpr(p, e.sons[2], b)
+    call.r = ropecg(p.module, "#addChar($1, $2)", [rdLoc(dest), rdLoc(b)])
+    genAssignment(p, dest, call, {})
   of mAppendStrStr: genStrAppend(p, e, d)
   of mAppendSeqElem: genSeqElemAppend(p, e, d)
   of mEqStr: genStrEquals(p, e, d)

compiler/condsyms.nim

@@ -72,3 +72,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimNoZeroTerminator")
   defineSymbol("nimNotNil")
   defineSymbol("nimVmExportFixed")
+  defineSymbol("nimIncrSeqV3")

compiler/docgen.nim

@@ -259,7 +259,7 @@ proc nodeToHighlightedHtml(d: PDoc; n: PNode; result: var Rope; renderFlags: TRe
     of tkSpaces, tkInvalid:
       add(result, literal)
     of tkCurlyDotLe:
-      dispA(d.conf, result, "<span>" &  # This span is required for the JS to work properly
+      dispA(d.conf, result, "<span>" & # This span is required for the JS to work properly
         """<span class="Other">{</span><span class="Other pragmadots">...</span><span class="Other">}</span>
 </span>
 <span class="pragmawrap">

lib/system/gc.nim

@@ -548,7 +548,10 @@ proc growObj(old: pointer, newsize: int, gch: var GcHeap): pointer =
   gcTrace(res, csAllocated)
   track("growObj old", ol, 0)
   track("growObj new", res, newsize)
-  when reallyDealloc:
+  when defined(nimIncrSeqV3):
+    # since we steal the old seq's contents, we set the old length to 0.
+    cast[PGenericSeq](old).len = 0
+  elif reallyDealloc:
     sysAssert(allocInv(gch.region), "growObj before dealloc")
     if ol.refcount shr rcShift <=% 1:
       # free immediately to save space:

lib/system/sysstr.nim

@@ -184,8 +184,13 @@ proc addChar(s: NimString, c: char): NimString =
     result = s
     if result.len >= result.space:
       let r = resize(result.space)
-      result = cast[NimString](growObj(result,
+      when defined(nimIncrSeqV3):
-        sizeof(TGenericSeq) + r + 1))
+        result = rawNewStringNoInit(r)
+        result.len = s.len
+        copyMem(addr result.data[0], unsafeAddr(s.data[0]), s.len+1)
+      else:
+        result = cast[NimString](growObj(result,
+          sizeof(TGenericSeq) + r + 1))
       result.reserved = r
   result.data[result.len] = c
   result.data[result.len+1] = '\0'
@@ -228,8 +233,13 @@ proc resizeString(dest: NimString, addlen: int): NimString {.compilerRtl.} =
   elif dest.len + addlen <= dest.space:
     result = dest
   else: # slow path:
-    var sp = max(resize(dest.space), dest.len + addlen)
+    let sp = max(resize(dest.space), dest.len + addlen)
-    result = cast[NimString](growObj(dest, sizeof(TGenericSeq) + sp + 1))
+    when defined(nimIncrSeqV3):
+      result = rawNewStringNoInit(sp)
+      result.len = dest.len
+      copyMem(addr result.data[0], unsafeAddr(dest.data[0]), dest.len+1)
+    else:
+      result = cast[NimString](growObj(dest, sizeof(TGenericSeq) + sp + 1))
     result.reserved = sp
     #result = rawNewString(sp)
     #copyMem(result, dest, dest.len + sizeof(TGenericSeq))
@@ -246,13 +256,21 @@ proc appendChar(dest: NimString, c: char) {.compilerproc, inline.} =
   inc(dest.len)
 
 proc setLengthStr(s: NimString, newLen: int): NimString {.compilerRtl.} =
-  var n = max(newLen, 0)
+  let n = max(newLen, 0)
   if s == nil:
     result = mnewString(newLen)
   elif n <= s.space:
     result = s
   else:
-    result = resizeString(s, n)
+    let sp = max(resize(s.space), newLen)
+    when defined(nimIncrSeqV3):
+      result = rawNewStringNoInit(sp)
+      result.len = s.len
+      copyMem(addr result.data[0], unsafeAddr(s.data[0]), s.len+1)
+      zeroMem(addr result.data[s.len], newLen - s.len)
+      result.reserved = sp
+    else:
+      result = resizeString(s, n)
   result.len = n
   result.data[n] = '\0'
 
@@ -282,6 +300,9 @@ proc incrSeqV2(seq: PGenericSeq, elemSize: int): PGenericSeq {.compilerProc.} =
                                GenericSeqSize))
     result.reserved = r
 
+template `+!`(p: pointer, s: int): pointer =
+  cast[pointer](cast[int](p) +% s)
+
 proc incrSeqV3(s: PGenericSeq, typ: PNimType): PGenericSeq {.compilerProc.} =
   if s == nil:
     result = cast[PGenericSeq](newSeq(typ, 1))
@@ -290,9 +311,16 @@ proc incrSeqV3(s: PGenericSeq, typ: PNimType): PGenericSeq {.compilerProc.} =
     result = s
     if result.len >= result.space:
       let r = resize(result.space)
-      result = cast[PGenericSeq](growObj(result, typ.base.size * r +
+      when defined(nimIncrSeqV3):
+        result = cast[PGenericSeq](newSeq(typ, r))
+        result.len = s.len
+        copyMem(result +! GenericSeqSize, s +! GenericSeqSize, s.len * typ.base.size)
+        # since we steal the content from 's', it's crucial to set s's len to 0.
+        s.len = 0
+      else:
+        result = cast[PGenericSeq](growObj(result, typ.base.size * r +
                                 GenericSeqSize))
-      result.reserved = r
+        result.reserved = r
 
 proc setLengthSeq(seq: PGenericSeq, elemSize, newLen: int): PGenericSeq {.
     compilerRtl, inl.} =
@@ -336,10 +364,43 @@ proc setLengthSeq(seq: PGenericSeq, elemSize, newLen: int): PGenericSeq {.
 
 proc setLengthSeqV2(s: PGenericSeq, typ: PNimType, newLen: int): PGenericSeq {.
     compilerRtl.} =
+  sysAssert typ.kind == tySequence, "setLengthSeqV2: type is not a seq"
   if s == nil:
     result = cast[PGenericSeq](newSeq(typ, newLen))
   else:
-    result = setLengthSeq(s, typ.base.size, newLen)
+    when defined(nimIncrSeqV3):
+      let elemSize = typ.base.size
+      if s.space < newLen:
+        let r = max(resize(s.space), newLen)
+        result = cast[PGenericSeq](newSeq(typ, r))
+        copyMem(result +! GenericSeqSize, s +! GenericSeqSize, s.len * elemSize)
+        # since we steal the content from 's', it's crucial to set s's len to 0.
+        s.len = 0
+      elif newLen < s.len:
+        result = s
+        # we need to decref here, otherwise the GC leaks!
+        when not defined(boehmGC) and not defined(nogc) and
+            not defined(gcMarkAndSweep) and not defined(gogc) and
+            not defined(gcRegions):
+          if ntfNoRefs notin typ.base.flags:
+            for i in newLen..result.len-1:
+              forAllChildrenAux(cast[pointer](cast[ByteAddress](result) +%
+                                GenericSeqSize +% (i*%elemSize)),
+                                extGetCellType(result).base, waZctDecRef)
+
+        # XXX: zeroing out the memory can still result in crashes if a wiped-out
+        # cell is aliased by another pointer (ie proc parameter or a let variable).
+        # This is a tough problem, because even if we don't zeroMem here, in the
+        # presence of user defined destructors, the user will expect the cell to be
+        # "destroyed" thus creating the same problem. We can destoy the cell in the
+        # finalizer of the sequence, but this makes destruction non-deterministic.
+        zeroMem(cast[pointer](cast[ByteAddress](result) +% GenericSeqSize +%
+              (newLen*%elemSize)), (result.len-%newLen) *% elemSize)
+      else:
+        result = s
+      result.len = newLen
+    else:
+      result = setLengthSeq(s, typ.base.size, newLen)
 
 # --------------- other string routines ----------------------------------
 proc add*(result: var string; x: int64) =

tests/closure/tclosure3.nim

@@ -15,7 +15,7 @@ proc main =
       let val = s[i]()
       if val != $(i*i): echo "bug  ", val
 
-    if getOccupiedMem() > 3000_000: quit("still a leak!")
+    if getOccupiedMem() > 5000_000: quit("still a leak!")
   echo "success"
 
 main()

tests/gc/growobjcrash.nim

@@ -14,7 +14,7 @@ proc handleRequest(query: string): StringTableRef =
   let x = foo
   result = x()
 
-const Limit = when compileOption("gc", "markAndSweep"): 5*1024*1024 else: 700_000
+const Limit = 5*1024*1024
 
 proc main =
   var counter = 0