I believe that MVM_file_set_encoding() needs to root oshandle (patch attached)
as MVM_string_find_encoding() can allocate and hence can cause a GC run.

There are two other uses of MVM_string_find_encoding(). One is:

    /* At least find_encoding may allocate on first call, so root just
     * in case. */
    MVMROOT(tc, buf, {
        encoded = MVM_string_encode(tc, s, 0, NUM_GRAPHS(s), &output_size,
            MVM_string_find_encoding(tc, enc_name));
    });


The other is:

    /* Decode. */
    return MVM_string_decode(tc, tc->instance->VMString,
        ((MVMArray *)buf)->body.slots.i8 + ((MVMArray *)buf)->body.start,
        ((MVMArray *)buf)->body.elems * elem_size,
        MVM_string_find_encoding(tc, enc_name));


Is this one buggy, because tc->instance->VMString is a GC-able object, and
so might move if MVM_string_find_encoding() triggers a GC collection?

ie, should that MVM_string_find_encoding(tc, enc_name) be yanked to a line
above and assigned to a local variable, so that any GC run has happened
before the arguments to MVM_string_decode() start to be put onto the CPU
stack for the C function call?

Nicholas Clark
>From b54de7c404e5bd0121f13be811e638581374b0ad Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Sat, 28 Dec 2013 16:58:45 +0100
Subject: [PATCH] MVM_file_set_encoding() needs to root oshandle

MVM_string_find_encoding() can allocate, and hence can cause a GC run.
---
 src/io/fileops.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/io/fileops.c b/src/io/fileops.c
index 870fb84..62b9f83 100644
--- a/src/io/fileops.c
+++ b/src/io/fileops.c
@@ -834,9 +834,10 @@ MVMint64 MVM_file_eof(MVMThreadContext *tc, MVMObject *oshandle) {
 
 void MVM_file_set_encoding(MVMThreadContext *tc, MVMObject *oshandle, MVMString *encoding_name) {
     MVMOSHandle *handle;
-    const MVMuint8 encoding_flag = MVM_string_find_encoding(tc, encoding_name);
+    MVMROOT(tc, oshandle, {
+            const MVMuint8 encoding_flag = MVM_string_find_encoding(tc, encoding_name);
 
-    verify_filehandle_type(tc, oshandle, &handle, "setencoding");
-
-    handle->body.encoding_type = encoding_flag;
+            verify_filehandle_type(tc, oshandle, &handle, "setencoding");
+            handle->body.encoding_type = encoding_flag;
+        });
 }
-- 
1.7.1

Reply via email to