summary refs log tree commit diff stats
path: root/lib
diff options
context:
space:
mode:
authorzah <zahary@gmail.com>2021-04-14 13:10:01 +0300
committerGitHub <noreply@github.com>2021-04-14 12:10:01 +0200
commit3b47a689cfa56d9b54cd1b92dc1b57d3e4094937 (patch)
tree23dff05345e436c1a2d16e78ddf241e96be61ef3 /lib
parent25b4a0ab0a03f35c58bc0dbfff51a344e108bfe3 (diff)
downloadNim-3b47a689cfa56d9b54cd1b92dc1b57d3e4094937.tar.gz
Remove the use of usrToCell in gcMark [backport:1.2] (#17709)
* Remove the use of usrToCell in gcMark [backport:1.2]

Recently, we've discovered a GC crash resulting from inlining of
the memory allocation procs that allowed the compiler to avoid
maintaining any references to the "user pointer" on the stack.
Instead, a "cell pointer" appeared there and all field accesses
were performed with adjusted offsets. This interfered with the
ability of the GC to mark the correct cell in the conservative
stack scans which lead to premature collection of objects.

More details here:
https://github.com/status-im/Nim/commit/af69b3ceae16281efd45cbee4ce1bedd14282304

This commit closes another theoretical loophole that may lead to
the same problem. If a short proc is accessing both the object and
its reference count in a short sequence of instructions, the compiler
may be enticed to reduce the number of registers being used by storing
only a single pointer to the object and using offsets when reading
and writing fields. A perfectly good strategy would be to store only
the cell pointer, so the reference count updates can be performed
without applying offsets. Accessing the fields of the object requires
offsets anyway, but these can be adjusted at compile-time without any
loss. Following this strategy will lead to the same problem of marking
a wrong cell during the conservative stack scan, leading to premature
collection.

The problem is avoided by not using `usrToCell` in `gcMark`. Since
the cell discovery logic can already handle interior pointers, the
user pointers don't need to be adjusted for the GC to function correctly.
Diffstat (limited to 'lib')
-rw-r--r--lib/system/gc.nim6
-rw-r--r--lib/system/gc_ms.nim5
2 files changed, 5 insertions, 6 deletions
diff --git a/lib/system/gc.nim b/lib/system/gc.nim
index 039a627ba..e50e80f11 100644
--- a/lib/system/gc.nim
+++ b/lib/system/gc.nim
@@ -710,16 +710,16 @@ proc collectCycles(gch: var GcHeap) {.raises: [].} =
 proc gcMark(gch: var GcHeap, p: pointer) {.inline.} =
   # the addresses are not as cells on the stack, so turn them to cells:
   sysAssert(allocInv(gch.region), "gcMark begin")
-  var cell = usrToCell(p)
-  var c = cast[ByteAddress](cell)
+  var c = cast[ByteAddress](p)
   if c >% PageSize:
     # fast check: does it look like a cell?
-    var objStart = cast[PCell](interiorAllocatedPtr(gch.region, cell))
+    var objStart = cast[PCell](interiorAllocatedPtr(gch.region, p))
     if objStart != nil:
       # mark the cell:
       incRef(objStart)
       add(gch.decStack, objStart)
     when false:
+      let cell = usrToCell(p)
       if isAllocatedPtr(gch.region, cell):
         sysAssert false, "allocated pointer but not interior?"
         # mark the cell:
diff --git a/lib/system/gc_ms.nim b/lib/system/gc_ms.nim
index b4221f7fd..c20e93699 100644
--- a/lib/system/gc_ms.nim
+++ b/lib/system/gc_ms.nim
@@ -455,11 +455,10 @@ proc markGlobals(gch: var GcHeap) =
 
 proc gcMark(gch: var GcHeap, p: pointer) {.inline.} =
   # the addresses are not as cells on the stack, so turn them to cells:
-  var cell = usrToCell(p)
-  var c = cast[ByteAddress](cell)
+  var c = cast[ByteAddress](p)
   if c >% PageSize:
     # fast check: does it look like a cell?
-    var objStart = cast[PCell](interiorAllocatedPtr(gch.region, cell))
+    var objStart = cast[PCell](interiorAllocatedPtr(gch.region, p))
     if objStart != nil:
       mark(gch, objStart)
 
oang/Nim/commit/tests/gctest.nim?h=devel&id=8b2a9401a147bd0b26cd2976ae71a1022fbde8cc'>8b2a9401a ^
405b86068







8b2a9401a ^
07d5a8085 ^

405b86068













































8b2a9401a ^
405b86068
8b2a9401a ^
405b86068


8b2a9401a ^

405b86068

8b2a9401a ^
405b86068







8b2a9401a ^
405b86068
8b2a9401a ^
405b86068


8b2a9401a ^
405b86068


8b2a9401a ^
405b86068

db4f617af ^




405b86068
db4f617af ^




405b86068














8b2a9401a ^
405b86068




405b86068

8b2a9401a ^





405b86068

8b2a9401a ^

405b86068



8b2a9401a ^

db4f617af ^
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197