TL;DR - fix at end.
If I change the MoarVM nursery size to 2034K, then the NQP build fails with
a SEGV in the garbage collector. There's an item on the worklist which is
an utterly bogus pointer. Adding a bit more code to try to see how it is
getting onto the worklist shows it to be coming from the temproots. In turn,
a bit more code shows it to be coming from the bootstrap. In all, my
annotated MoarVM looks like this:
diff --git a/3rdparty/dyncall b/3rdparty/dyncall
--- a/3rdparty/dyncall
+++ b/3rdparty/dyncall
@@ -1 +1 @@
-Subproject commit a9e6eec70785f43f63ef17189fc2733d4ceb8446
+Subproject commit a9e6eec70785f43f63ef17189fc2733d4ceb8446-dirty
diff --git a/src/core/args.c b/src/core/args.c
index db6e6bb..ba3eb83 100644
--- a/src/core/args.c
+++ b/src/core/args.c
@@ -286,10 +286,13 @@ MVMArgInfo MVM_args_get_named_num(MVMThreadContext *tc,
MVMArgProcContext *ctx,
autounbox(tc, MVM_CALLSITE_ARG_NUM, "number", result);
return result;
}
+static int dummy;
MVMArgInfo MVM_args_get_named_str(MVMThreadContext *tc, MVMArgProcContext
*ctx, MVMString *name, MVMuint8 required) {
MVMArgInfo result;
args_get_named(tc, ctx, name, required, "string");
autounbox(tc, MVM_CALLSITE_ARG_STR, "string", result);
+ if (result.arg.s)
+ ++dummy;
return result;
}
MVMint64 MVM_args_has_named(MVMThreadContext *tc, MVMArgProcContext *ctx,
MVMString *name) {
diff --git a/src/gc/collect.h b/src/gc/collect.h
index d6fd456..a9b9893 100644
--- a/src/gc/collect.h
+++ b/src/gc/collect.h
@@ -1,6 +1,6 @@
/* How big is the nursery area? Note that since it's semi-space copying, we
* actually have double this amount allocated. Also it is per thread. */
-#define MVM_NURSERY_SIZE 2097152
+#define MVM_NURSERY_SIZE (1024 * 2034)
/* How often do we collect the second generation? This is specified as the
* number of nursery runs that happen per full collection. For example, if
diff --git a/src/gc/roots.c b/src/gc/roots.c
index a1e67fc..eb0b9eb 100644
--- a/src/gc/roots.c
+++ b/src/gc/roots.c
@@ -85,11 +85,15 @@ void MVM_gc_root_add_tc_roots_to_worklist(MVMThreadContext
*tc, MVMGCWorklist *w
MVM_gc_worklist_add(tc, worklist, &tc->cur_dispatcher);
}
+static int dummy;
+
/* Pushes a temporary root onto the thread-local roots list. */
void MVM_gc_root_temp_push(MVMThreadContext *tc, MVMCollectable **obj_ref) {
/* Ensure the root is not null. */
if (obj_ref == NULL)
MVM_panic(MVM_exitcode_gcroots, "Illegal attempt to add null object
address as a temporary root");
+ if (*obj_ref)
+ ++dummy;
/* Allocate extra temporary root space if needed. */
if (tc->num_temproots == tc->alloc_temproots) {
diff --git a/src/gc/worklist.h b/src/gc/worklist.h
index a27cd9a..bc8e0df 100644
--- a/src/gc/worklist.h
+++ b/src/gc/worklist.h
@@ -32,7 +32,7 @@ struct MVMGCWorklist {
* functions since perhaps they become them in the future if needed. */
#define MVM_gc_worklist_add(tc, worklist, item) \
do { \
- if (worklist->items == worklist->alloc) \
+ if (!*item || worklist->items == worklist->alloc) \
MVM_gc_worklist_add_slow(tc, worklist, (MVMCollectable **)(item));
\
else \
worklist->list[worklist->items++] = (MVMCollectable **)(item); \
With that, valgrind does this:
==7352== Memcheck, a memory error detector
==7352== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==7352== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==7352== Command: /home/nicholas/Sandpit/burndown2/bin/moar
--libpath=src/vm/moar/stage0 src/vm/moar/stage0/nqp.moarvm --bootstrap
--setting=NULL --no-regex-lib --target=mbc
--output=gen/moar/stage1/nqpmo.moarvm gen/moar/stage1/nqpmo.nqp
==7352==
==7352== Conditional jump or move depends on uninitialised value(s)
==7352== at 0x4EF17D4: MVM_args_get_named_str (args.c:294)
==7352== by 0x4F67FAC: new_type (bootstrap.c:61)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E8DB: MVM_vm_run_file (moar.c:165)
==7352== by 0x400D2D: main (main.c:137)
==7352==
==7352== Conditional jump or move depends on uninitialised value(s)
==7352== at 0x4F3B2B4: MVM_gc_root_temp_push (roots.c:95)
==7352== by 0x4F68077: new_type (bootstrap.c:70)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E8DB: MVM_vm_run_file (moar.c:165)
==7352== by 0x400D2D: main (main.c:137)
==7352==
==7352== Conditional jump or move depends on uninitialised value(s)
==7352== at 0x4EF17D4: MVM_args_get_named_str (args.c:294)
==7352== by 0x4F67F59: new_type (bootstrap.c:60)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352== by 0x400D2D: main (main.c:137)
==7352==
==7352== Conditional jump or move depends on uninitialised value(s)
==7352== at 0x4F3B2B4: MVM_gc_root_temp_push (roots.c:95)
==7352== by 0x4F69B4A: attr_new (bootstrap.c:381)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352== by 0x400D2D: main (main.c:137)
==7352==
==7352== Conditional jump or move depends on uninitialised value(s)
==7352== at 0x4F3B466: MVM_gc_root_add_temps_to_worklist (roots.c:138)
==7352== by 0x4F41002: MVM_gc_collect (collect.c:84)
==7352== by 0x4F39C57: run_gc (orchestrate.c:272)
==7352== by 0x4F3A05F: MVM_gc_enter_from_allocator (orchestrate.c:371)
==7352== by 0x4F3A231: MVM_gc_allocate_nursery (allocation.c:32)
==7352== by 0x4F3A2B6: MVM_gc_allocate_zeroed (allocation.c:49)
==7352== by 0x4F3A4F9: MVM_gc_allocate_object (allocation.c:85)
==7352== by 0x4F524AA: allocate (KnowHOWAttributeREPR.c:22)
==7352== by 0x4F69B81: attr_new (bootstrap.c:385)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352==
==7352== Conditional jump or move depends on uninitialised value(s)
==7352== at 0x4F41278: process_worklist (collect.c:160)
==7352== by 0x4F4101E: MVM_gc_collect (collect.c:86)
==7352== by 0x4F39C57: run_gc (orchestrate.c:272)
==7352== by 0x4F3A05F: MVM_gc_enter_from_allocator (orchestrate.c:371)
==7352== by 0x4F3A231: MVM_gc_allocate_nursery (allocation.c:32)
==7352== by 0x4F3A2B6: MVM_gc_allocate_zeroed (allocation.c:49)
==7352== by 0x4F3A4F9: MVM_gc_allocate_object (allocation.c:85)
==7352== by 0x4F524AA: allocate (KnowHOWAttributeREPR.c:22)
==7352== by 0x4F69B81: attr_new (bootstrap.c:385)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352==
==7352== Use of uninitialised value of size 8
==7352== at 0x4F41282: process_worklist (collect.c:165)
==7352== by 0x4F4101E: MVM_gc_collect (collect.c:86)
==7352== by 0x4F39C57: run_gc (orchestrate.c:272)
==7352== by 0x4F3A05F: MVM_gc_enter_from_allocator (orchestrate.c:371)
==7352== by 0x4F3A231: MVM_gc_allocate_nursery (allocation.c:32)
==7352== by 0x4F3A2B6: MVM_gc_allocate_zeroed (allocation.c:49)
==7352== by 0x4F3A4F9: MVM_gc_allocate_object (allocation.c:85)
==7352== by 0x4F524AA: allocate (KnowHOWAttributeREPR.c:22)
==7352== by 0x4F69B81: attr_new (bootstrap.c:385)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352==
==7352== Invalid read of size 2
==7352== at 0x4F41282: process_worklist (collect.c:165)
==7352== by 0x4F4101E: MVM_gc_collect (collect.c:86)
==7352== by 0x4F39C57: run_gc (orchestrate.c:272)
==7352== by 0x4F3A05F: MVM_gc_enter_from_allocator (orchestrate.c:371)
==7352== by 0x4F3A231: MVM_gc_allocate_nursery (allocation.c:32)
==7352== by 0x4F3A2B6: MVM_gc_allocate_zeroed (allocation.c:49)
==7352== by 0x4F3A4F9: MVM_gc_allocate_object (allocation.c:85)
==7352== by 0x4F524AA: allocate (KnowHOWAttributeREPR.c:22)
==7352== by 0x4F69B81: attr_new (bootstrap.c:385)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352== Address 0x4000300400005 is not stack'd, malloc'd or (recently) free'd
==7352==
==7352==
==7352== Process terminating with default action of signal 11 (SIGSEGV):
dumping core
==7352== General Protection Fault
==7352== at 0x4F41282: process_worklist (collect.c:165)
==7352== by 0x4F4101E: MVM_gc_collect (collect.c:86)
==7352== by 0x4F39C57: run_gc (orchestrate.c:272)
==7352== by 0x4F3A05F: MVM_gc_enter_from_allocator (orchestrate.c:371)
==7352== by 0x4F3A231: MVM_gc_allocate_nursery (allocation.c:32)
==7352== by 0x4F3A2B6: MVM_gc_allocate_zeroed (allocation.c:49)
==7352== by 0x4F3A4F9: MVM_gc_allocate_object (allocation.c:85)
==7352== by 0x4F524AA: allocate (KnowHOWAttributeREPR.c:22)
==7352== by 0x4F69B81: attr_new (bootstrap.c:385)
==7352== by 0x4F519FA: invoke_handler (MVMCFunction.c:9)
==7352== by 0x4EF6D3F: MVM_interp_run (interp.c:444)
==7352== by 0x4F8E924: MVM_vm_run_file (moar.c:173)
==7352==
==7352== HEAP SUMMARY:
==7352== in use at exit: 49,550,486 bytes in 158,734 blocks
==7352== total heap usage: 412,647 allocs, 253,913 frees, 265,716,645 bytes
allocated
==7352==
==7352== LEAK SUMMARY:
==7352== definitely lost: 576,280 bytes in 8,100 blocks
==7352== indirectly lost: 55,872 bytes in 776 blocks
==7352== possibly lost: 216 bytes in 3 blocks
==7352== still reachable: 48,918,118 bytes in 149,855 blocks
==7352== suppressed: 0 bytes in 0 blocks
==7352== Rerun with --leak-check=full to see details of leaked memory
==7352==
==7352== For counts of detected and suppressed errors, rerun with: -v
==7352== Use --track-origins=yes to see where uninitialised values come from
==7352== ERROR SUMMARY: 60 errors from 8 contexts (suppressed: 6 from 6)
The problem turns out to be that the bootstrap code is unconditionally
pushing MVM registers onto the temp roots, where those registers have not
been initialised, because they are from MVM arguments which do not exist. ie
this bit of new_type() in bootstrap.c:
name_arg = MVM_args_get_named_str(tc, &arg_ctx, str_name, MVM_ARG_OPTIONAL);
...
MVM_gc_root_temp_push(tc, (MVMCollectable **)&name_arg);
If the argument doesn't exist, then (currently) only exists is set to 0 in:
/* Struct used for returning information about an argument. */
struct MVMArgInfo {
MVMRegister arg;
MVMCallsiteEntry flags;
MVMuint8 exists;
};
with arg uninitialised. The GC doesn't know this, so it treats it as a valid
pointer to chase, and explodes.
I think that the more robust (but micro-slower) solution would be to ensure
that args is always NULL if it doesn't exist, so that other code doesn't need
to explicitly take care of always checking exists. But it might be that the
number of places where this happens is minimal, so it would be better for
the calls of code in args.c to know that they have to be careful with exists
before pushing to the GC. Or maybe there should be a macro which takes a
MVMArgInfo to temporarily root, and encapsulates the conditional check for
exists.
Anyway, for now, I'm applying the attached patch locally, and continuing to
test.
Nicholas Clark
>From e3b867213b855506682aa6a5956ac691a227479d Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Fri, 20 Dec 2013 17:47:43 +0100
Subject: [PATCH] Set arg in MVMArgInfo to NULL if the argument doesn't exist.
If arg is left uninitialised, then it's too easy for other code to push it
onto the GC temp roots, and the GC later chase a pointer to SEGV-land.
---
src/core/args.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/core/args.c b/src/core/args.c
index db6e6bb..88068fb 100644
--- a/src/core/args.c
+++ b/src/core/args.c
@@ -94,6 +94,7 @@ void MVM_args_checkarity(MVMThreadContext *tc, MVMArgProcContext *ctx, MVMuint16
result.exists = 1; \
} \
else { \
+ result.arg.s = NULL; \
result.exists = 0; \
} \
} while (0)
@@ -249,6 +250,7 @@ MVMArgInfo MVM_args_get_pos_str(MVMThreadContext *tc, MVMArgProcContext *ctx, MV
#define args_get_named(tc, ctx, name, required, _type) do { \
\
MVMuint32 flag_pos, arg_pos; \
+ result.arg.s = NULL; \
result.exists = 0; \
\
for (flag_pos = arg_pos = ctx->num_pos; arg_pos < ctx->arg_count; flag_pos++, arg_pos += 2) { \
--
1.7.1
