Bug #9609: [PATCH] vm_eval.c: fix misplaced RB_GC_GUARDs

• Status: Rejected
• Assignee: normalperson (Eric Wong)
• Target version: -
• ruby -v: -

[normalperson (Eric Wong)]

RB_GC_GUARD needs to be placed after RARRAY_PTR usages to portably
prevent compilers from optimizing the VALUE away.

[ko1 (Koichi Sasada)]

• ruby -v changed from ruby 2.2.0dev (2014-03-07 trunk 45281) [x86_64-linux] to -

(2014/03/07 19:09), [e-mail] wrote:

RB_GC_GUARD needs to be placed after RARRAY_PTR usages to portably
prevent compilers from optimizing the VALUE away.

And also it should be RARRAY_CONST_PTR().

--
// SASADA Koichi at atdot dot net

[normalperson (Eric Wong)]

SASADA Koichi [e-mail] wrote:

And also it should be RARRAY_CONST_PTR().

Thanks (r45283). Btw, I noticed rb_apply uses RARRAY_PTR, too,
but also with OBJ_FREEZE.

Should we ignore WB unprotect for frozen objects?

[ko1 (Koichi Sasada)]

(2014/03/07 19:38), Eric Wong wrote:

Thanks (r45283). Btw, I noticed rb_apply uses RARRAY_PTR, too,
but also with OBJ_FREEZE.

Should we ignore WB unprotect for frozen objects?

No relation between frozen and wb unprotected flag. We can use
RARRAY_CONST_PTR() for this case. I'll commit it with other patches.

Index: enum.c
===================================================================
--- enum.c	(revision 45283)
+++ enum.c	(working copy)
@@ -1163,7 +1163,7 @@ static void
 nmin_filter(struct nmin_data *data)
 {
     long n;
-    VALUE *beg;
+    const VALUE *beg;
     int eltsize;
     long numelts;
 
@@ -1175,7 +1175,7 @@ nmin_filter(struct nmin_data *data)
 	return;
 
     n = data->n;
-    beg = RARRAY_PTR(data->buf);
+    beg = RARRAY_CONST_PTR(data->buf);
     eltsize = data->by ? 2 : 1;
     numelts = data->curlen;
 
Index: enumerator.c
===================================================================
--- enumerator.c	(revision 45283)
+++ enumerator.c	(working copy)
@@ -422,13 +422,13 @@ static VALUE
 enumerator_block_call(VALUE obj, rb_block_call_func *func, VALUE arg)
 {
     int argc = 0;
-    VALUE *argv = 0;
+    const VALUE *argv = 0;
     const struct enumerator *e = enumerator_ptr(obj);
     ID meth = e->meth;

     if (e->args) {
 	argc = RARRAY_LENINT(e->args);
-	argv = RARRAY_PTR(e->args);
+	argv = RARRAY_CONST_PTR(e->args);
     }
     return rb_block_call(e->obj, meth, argc, argv, func, arg);
 }
Index: internal.h
===================================================================
--- internal.h	(revision 45283)
+++ internal.h	(working copy)
@@ -963,9 +963,9 @@ VALUE rb_int_positive_pow(long x, unsign
 /* process.c */
 int rb_exec_async_signal_safe(const struct rb_execarg *e, char *errmsg, size_t errmsg_buflen);
 rb_pid_t rb_fork_async_signal_safe(int *status, int (*chfunc)(void*, char *, size_t), void *charg, VALUE fds, char *errmsg, size_t errmsg_buflen);
-VALUE rb_execarg_new(int argc, VALUE *argv, int accept_shell);
+VALUE rb_execarg_new(int argc, const VALUE *argv, int accept_shell);
 struct rb_execarg *rb_execarg_get(VALUE execarg_obj); /* dangerous. needs GC guard. */
-VALUE rb_execarg_init(int argc, VALUE *argv, int accept_shell, VALUE execarg_obj);
+VALUE rb_execarg_init(int argc, const VALUE *argv, int accept_shell, VALUE execarg_obj);
 int rb_execarg_addopt(VALUE execarg_obj, VALUE key, VALUE val);
 void rb_execarg_fixup(VALUE execarg_obj);
 int rb_execarg_run_options(const struct rb_execarg *e, struct rb_execarg *s, char* errmsg, size_t errmsg_buflen);
Index: io.c
===================================================================
--- io.c	(revision 45283)
+++ io.c	(working copy)
@@ -6227,7 +6227,7 @@ rb_io_s_popen(int argc, VALUE *argv, VAL
 #endif
 	tmp = rb_ary_dup(tmp);
 	RBASIC_CLEAR_CLASS(tmp);
-	execarg_obj = rb_execarg_new((int)len, RARRAY_PTR(tmp), FALSE);
+	execarg_obj = rb_execarg_new((int)len, RARRAY_CONST_PTR(tmp), FALSE);
 	rb_ary_clear(tmp);
     }
     else {
Index: numeric.c
===================================================================
--- numeric.c	(revision 45283)
+++ numeric.c	(working copy)
@@ -1887,7 +1887,7 @@ num_step_size(VALUE from, VALUE args, VA
     VALUE to, step, hash;
     int desc;
     int argc = args ? RARRAY_LENINT(args) : 0;
-    VALUE *argv = args ? RARRAY_PTR(args) : 0;
+    const VALUE *argv = args ? RARRAY_CONST_PTR(args) : 0;
 
     NUM_STEP_SCAN_ARGS(argc, argv, to, step, hash, desc);
 
Index: process.c
===================================================================
--- process.c	(revision 45283)
+++ process.c	(working copy)
@@ -2167,7 +2167,7 @@ rb_exec_fillarg(VALUE prog, int argc, VA
 }
 
 VALUE
-rb_execarg_new(int argc, VALUE *argv, int accept_shell)
+rb_execarg_new(int argc, const VALUE *argv, int accept_shell)
 {
     VALUE execarg_obj;
     struct rb_execarg *eargp;
@@ -2186,7 +2186,7 @@ rb_execarg_get(VALUE execarg_obj)
 }
 
 VALUE
-rb_execarg_init(int argc, VALUE *argv, int accept_shell, VALUE execarg_obj)
+rb_execarg_init(int argc, const VALUE *argv, int accept_shell, VALUE execarg_obj)
 {
     struct rb_execarg *eargp = rb_execarg_get(execarg_obj);
     VALUE prog, ret;
Index: vm_eval.c
===================================================================
--- vm_eval.c	(revision 45283)
+++ vm_eval.c	(working copy)
@@ -751,7 +751,7 @@ rb_apply(VALUE recv, ID mid, VALUE args)
 	args = rb_ary_subseq(args, 0, argc);
 	RBASIC_CLEAR_CLASS(args);
 	OBJ_FREEZE(args);
-	ret = rb_call(recv, mid, argc, RARRAY_PTR(args), CALL_FCALL);
+	ret = rb_call(recv, mid, argc, RARRAY_CONST_PTR(args), CALL_FCALL);
 	RB_GC_GUARD(args);
 	return ret;
     }
@@ -1482,7 +1482,7 @@ rb_eval_cmd(VALUE cmd, VALUE arg, int le
 	rb_set_safe_level_force(level);
 	if ((state = EXEC_TAG()) == 0) {
 	    val = rb_funcall2(cmd, rb_intern("call"), RARRAY_LENINT(arg),
-			      RARRAY_PTR(arg));
+			      RARRAY_CONST_PTR(arg));
 	}
 	POP_TAG();
 

---

// SASADA Koichi at atdot dot net

[ko1 (Koichi Sasada)]

(2014/03/07 20:03), SASADA Koichi wrote:

Should we ignore WB unprotect for frozen objects?
No relation between frozen and wb unprotected flag. We can use
RARRAY_CONST_PTR() for this case. I'll commit it with other patches.

Ah, I understand what you mean. As you say, we can ignore unprotect.
But I think the checking cost is bigger.

--
// SASADA Koichi at atdot dot net

[normalperson (Eric Wong)]

• Status changed from Open to Rejected
• Backport deleted (1.9.3: REQUIRED, 2.0.0: REQUIRED, 2.1: REQUIRED)