PackFile_Constant_unpack_pmc should call Parrot_thaw_constants(), not Parrot_thaw()

[Infinoid]

There is a TODO comment in this function, which has been there for quite some time. We need to figure out if this is still valid, and either make the change, or remove the comment.

From src/packfile.c:

/*
* TODO use thaw_constants
* current issue: a constant Sub with attached properties
* doesn't DOD mark the properties
* for a constant PMC *all* contents have to be in the constant pools */
pmc = Parrot_thaw(interp, image);

I noticed this comment while looking into that Parrot_thaw line for other reasons... specifically, valgrind is reporting a leak from this line.

So I was curious. I changed Parrot_thaw to Parrot_thaw_constant, and it built fine. But the change causes the following tests to fail:

Failed Test Stat Wstat Total Fail List of Failed
-------------------------------------------------------------------------------
t/library/iter.t 0 11 47 42 27-47
t/library/range.t 0 11 78 80 39-78
t/op/cmp-nonbranch.t 0 11 88 58 60-88
t/pmc/hash.t 1 256 147 242 27-147
t/pmc/n_arithmetics.t 0 11 73 70 39-73


4 out of 5 of those failures are caused by a segfault in mmd_distance():

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x2ac127b99cf0 (LWP 22652)]
0x00002ac1229cf74d in mmd_distance (interp=0x60a080, pmc=0x9e2f58, arg_tuple=0x9c7880) at src/mmd.c:1520
1520 if (multi_sig->vtable->base_type == enum_class_FixedPMCArray) {
(gdb) bt
#0 0x00002ac1229cf74d in mmd_distance (interp=0x60a080, pmc=0x9e2f58, arg_tuple=0x9c7880) at src/mmd.c:1520
#1 0x00002ac1229cfa38 in mmd_sort_candidates (interp=0x60a080, arg_tuple=0x9c7880, cl=0x9c6ea8) at src/mmd.c:1649
#2 0x00002ac1229cebcf in Parrot_mmd_sort_candidate_list (interp=0x60a080, candidates=0x9c6ea8) at src/mmd.c:1124
#3 0x00002ac122b26111 in Parrot_MultiSub_invoke (interp=0x60a080, pmc=0xa16428, next=0xaa31e8) at ./src/pmc/multisub.pmc:61
#4 0x00002ac12294080c in Parrot_invokecc_p (cur_opcode=0xaa31d8, interp=0x60a080) at src/ops/core.ops:413
#5 0x00002ac1229f4767 in runops_slow_core (interp=0x60a080, pc=0xaa31d8) at src/runops_cores.c:219
#6 0x00002ac1229c605d in runops_int (interp=0x60a080, offset=0) at src/interpreter.c:916
#7 0x00002ac1229c6ae0 in runops (interp=0x60a080, offs=0) at src/inter_run.c:104
#8 0x00002ac1229c6d78 in runops_args (interp=0x60a080, sub=0xa175a8, obj=0x65ac00, meth_unused=0x0, sig=0x2ac122bb3133 "vP", ap=0x7fff88525230) at src/inter_run.c:230
#9 0x00002ac1229c6f6b in Parrot_runops_fromc_args (interp=0x60a080, sub=0xa175a8, sig=0x2ac122bb3133 "vP") at src/inter_run.c:299
#10 0x00002ac1229ac92e in Parrot_runcode (interp=0x60a080, argc=1, argv=0x7fff88525520) at src/embed.c:941
#11 0x00002ac122b8e3f8 in imcc_run_pbc (interp=0x60a080, obj_file=0, output_file=0x0, argc=1, argv=0x7fff88525520) at compilers/imcc/main.c:781
#12 0x00002ac122b8ecf7 in imcc_run (interp=0x60a080, sourcefile=0x7fff88527350 "t/pmc/n_arithmetics.t", argc=1, argv=0x7fff88525520) at compilers/imcc/main.c:1069
#13 0x0000000000400c34 in main (argc=1, argv=0x7fff88525520) at src/main.c:61
(gdb) print multi_sig
$3 = (PMC *) 0xae3a90
(gdb) print multi_sig->vtable
$4 = (VTABLE *) 0xdeadbeef

[coke]

Originally reported in  http://rt.perl.org/rt3/Ticket/Display.html?id=53754

Followup comment by chromatic.

Parrot_thaw() and whatever it calls to create new PMCs doesn't know to 
allocate all GCable elements reachable from a constant PMC from constant 
pools themselves.

Constant GCable elements get marked during the mark phase if and only if 
they're reachable from the root set AND (most importantly) if there's a path 
through the graph to them that goes through a PMC that's not already marked 
as live for this mark phase.

Constant GCable elements never get their live flags flipped back to unknown 
during the sweep phase. They never get collected, so this is only a problem 
when...

... any non-constant GCable element reachable only from a constant PMC will be 
marked as live when its parent is first marked as live. The sweep phase will 
unmark the non-constant element. The next mark phase will *not* mark the 
non-constant element as live (because its parent is still live), and the 
subsequent sweep phase will collect that element...

''see backtrace''

... which is exactly what you see here.

--chromatic

[whiteknight]

Taking into account chromatic's assessment above, it seems to me that there are two options forward to resolve this issue:

• Treat the pool of Constant PMCs as a rootset and always mark the children of constant PMCs on each mark phase
• When marking, if we reach a Constant PMC as a child of a non-constant PMC that we are marking, we should ignore it's "I'm already alive" flag and mark it's children anyway.

I worry about situations where we have a constant PMC with non-constant children that are not getting marked. The children are collected, the pointers in the constant PMC are never updated, and the PMC itself is likely unreachable but its storage is never reclaimed. A small issue, to be sure, but definitely has an effect on Parrot's memory footprint.

[plobsing]

In the pmc_freeze_cleanup branch, due to this issue (and some nasty workarounds in src/pmc_freeze.c, which had made the two roughly equivalent anyways), Parrot_thaw_constants() became a wrapper over Parrot_thaw().

The small issue of calling Parrot_thaw_constants() is solved (go ahead and change the call, it will do the same thing). The larger issue of what PackFile constants are and how they should be treated remains open AFAICT.