I’ve been looking at RelationKind.i6t, and there is a curiosity that I don’t understand in the routine DoubleHashSetRelationHandler():
[ DoubleHashSetRelationHandler rel task X Y sym kov kx ky at tmp v;
kov = BlkValueRead(rel, RRV_KIND);
kx = KindBaseTerm(kov, 0); ky = KindBaseTerm(kov, 1);
if (task == RELS_SET_VALENCY) {
return RELATION_TY_SetValency(rel, X);
} else if (task == RELS_DESTROY) {
! clear
kx = KOVIsBlockValue(kx); ky = KOVIsBlockValue(ky);
if (~~(kx || ky)) return;
for (at = BlkValueRead(rel, RRV_STORAGE): at >= 0: at--) { ! <--- field index decremented here
tmp = BlkValueRead(rel, RRV_DATA_BASE + 3*at);
if (tmp & RRF_USED) {
if (kx) BlkValueFree(BlkValueRead(rel, RRV_DATA_BASE + 3*at + 1));
if (ky) BlkValueFree(BlkValueRead(rel, RRV_DATA_BASE + 3*at + 2));
}
at--; ! <-- why also decrement here?
}
return;
} else if (task == RELS_COPY)
...
In short: While handling a RELS_DESTROY task, the for loop decrements the field index at twice in each cycle. Similar loops (both for and while) appear in several other places in the template, but this is the only one that decrements twice in this manner.
Wouldn’t double-decrementing each pass result in skipped entries, and wouldn’t freeing only every other entry of the relation result in orphan block values due to the skipped entries?
2 Likes
Compare the above to the handling of the RELS_EMPTY task later in the routine:
...
} else if (task == RELS_EMPTY) {
if (BlkValueRead(rel, RRV_USED) == 0) rtrue;
if (X == 1) {
DoubleHashSetRelationHandler(rel, RELS_DESTROY); ! <--- applies RELS_DESTROY logic
for (at = BlkValueRead(rel, RRV_STORAGE): at >= 0: at--) { ! <--- field index decremented once per loop
tmp = RRV_DATA_BASE + 3*at;
BlkValueWrite(rel, tmp, 0);
BlkValueWrite(rel, tmp + 1, 0); ! <--- assumes X block value already freed
BlkValueWrite(rel, tmp + 2, 0); ! <--- assumes Y block value already freed
}
BlkValueWrite(rel, RRV_USED, 0);
BlkValueWrite(rel, RRV_FILLED, 0);
rtrue;
}
rfalse;
} else if (task == RELS_LOOKUP_ANY) {
...
Also compare to the RELS_COPY clause in the same routine, which contains a while loop that handles each entry individually and creates new block values for each entry:
...
} else if (task == RELS_COPY) {
X = KOVIsBlockValue(kx); Y = KOVIsBlockValue(ky);
if (~~(X || Y)) return;
at = BlkValueRead(rel, RRV_STORAGE);
while (at >= 0) {
tmp = BlkValueRead(rel, RRV_DATA_BASE + 3*at);
if (tmp & RRF_USED) {
if (X) {
tmp = BlkValueRead(rel, RRV_DATA_BASE + 3*at + 1);
tmp = BlkValueCopy(BlkValueCreate(kx), tmp); ! <--- new block value for X
BlkValueWrite(rel, RRV_DATA_BASE + 3*at + 1, tmp);
}
if (Y) {
tmp = BlkValueRead(rel, RRV_DATA_BASE + 3*at + 2);
tmp = BlkValueCopy(BlkValueCreate(ky), tmp); ! <--- new block value for Y
BlkValueWrite(rel, RRV_DATA_BASE + 3*at + 2, tmp);
}
}
at--; ! <--- field index decremented once per loop
}
return;
} else if (task == RELS_SHOW) {
...
Am I correct in thinking that if relation B is copied to B’, then even if the skipped-entries logic is valid for B, it would not be valid for B’?
I worked up an example to demonstrate that the problem is real:
"Leak Demo"
Place is a room.
[temporary relation will be automatically destroyed at end of phrase]
After jumping:
let R be a relation of numbers to texts;
now R relates 4 to "score and seven days ago";
now R relates 6 to "ways to Sunday";
now R relates 7 to "up";
now R relates 7 to "eleven";
now R relates 8 to "days a week";
show relation R;
now R is empty. [demonstrates mismatch between loop index handling]
Test me with "showheap / jump / showheap / jump / showheap / jump / g / g / g / showheap".
Stopping the decrement of at at the end of the RELS_DESTROY loop seems to fix the issue:
[ DoubleHashSetRelationHandler rel task X Y sym kov kx ky at tmp v;
kov = BlkValueRead(rel, RRV_KIND);
kx = KindBaseTerm(kov, 0); ky = KindBaseTerm(kov, 1);
if (task == RELS_SET_VALENCY) {
return RELATION_TY_SetValency(rel, X);
} else if (task == RELS_DESTROY) {
! clear
kx = KOVIsBlockValue(kx); ky = KOVIsBlockValue(ky);
if (~~(kx || ky)) return;
for (at = BlkValueRead(rel, RRV_STORAGE): at >= 0: at--) {
tmp = BlkValueRead(rel, RRV_DATA_BASE + 3*at);
if (tmp & RRF_USED) {
if (kx) BlkValueFree(BlkValueRead(rel, RRV_DATA_BASE + 3*at + 1));
if (ky) BlkValueFree(BlkValueRead(rel, RRV_DATA_BASE + 3*at + 2));
}
at--; ! <--- COMMENTING OUT OR REMOVING THIS LINE FIXES LEAK
}
return;
} else if (task == RELS_COPY) {
...
2 Likes
Cool spot and fix.
Although surely that should be “score and seven years ago”?
Even speaking from the old side of the pond 
1 Like
Actually, it should have been “score and seven minutes ago”… I was thinking of Lincoln’s lesser-known San Dimas address.
5 Likes
FYI for those coming across this post, avatar of civic-minded diligence Zed has submitted this as an official bug (I7-2374).
(Thank you, @Zed.)
1 Like