https://redmine.ruby-lang.org/https://redmine.ruby-lang.org/favicon.ico?17113305112017-01-23T16:58:33ZRuby Issue Tracking SystemRuby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=626442017-01-23T16:58:33Znaruse (Yui NARUSE)naruse@airemix.jp
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Applied in changeset r57410.</p>
<hr>
<p>Prevent GC by volatile [Bug <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: TestMarshal failures on FreeBSD with gcc7 because of GC (Closed)" href="https://redmine.ruby-lang.org/issues/13150">#13150</a>]</p>
<p>test/ruby/test_marshal.rb test_context_switch (load) and test_gc (dump)<br>
are failed on FreeBSD 10.3 and gcc7 (FreeBSD Ports Collection) 7.0.0<br>
20170115 (experimental); RB_GC_GUARD looks not worked well.</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=626472017-01-23T19:41:55Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:naruse@ruby-lang.org" class="email">naruse@ruby-lang.org</a> wrote:</p>
<blockquote>
<pre><code>Prevent GC by volatile [Bug #13150]
test/ruby/test_marshal.rb test_context_switch (load) and test_gc (dump)
are failed on FreeBSD 10.3 and gcc7 (FreeBSD Ports Collection) 7.0.0
20170115 (experimental); RB_GC_GUARD looks not worked well.
</code></pre>
</blockquote>
<p>Have you tried to experiment with making RB_GC_GUARD stronger<br>
for gcc7, instead? There may be similar bugs, and I would rather<br>
improve RB_GC_GUARD than add volatiles (which are more subtle<br>
and have more variance between implementations).</p>
<p>Thanks</p>
<blockquote>
<p>Modified files:<br>
trunk/marshal.c</p>
</blockquote> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=627292017-01-31T01:41:55Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Has duplicate</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/13168">Bug #13168</a>: Marshaling broken with GCC 7.x</i> added</li></ul> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=627302017-01-31T01:42:38Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/62730/diff?detail_id=43787">diff</a>)</li></ul> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=628422017-02-03T14:31:42Zvo.x (Vit Ondruch)v.ondruch@tiscali.cz
<ul><li><strong>Backport</strong> changed from <i>2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: UNKNOWN</i> to <i>2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: REQUIRED</i></li></ul><p>It would be nice to backport this, so we don't need to carry the patch around in Fedora.</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=629592017-02-13T01:08:18Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>Eric Wong <a href="mailto:normalperson@yhbt.net" class="email">normalperson@yhbt.net</a> wrote:</p>
<blockquote>
<p><a href="mailto:naruse@ruby-lang.org" class="email">naruse@ruby-lang.org</a> wrote:</p>
<blockquote>
<pre><code>Prevent GC by volatile [Bug #13150]
test/ruby/test_marshal.rb test_context_switch (load) and test_gc (dump)
are failed on FreeBSD 10.3 and gcc7 (FreeBSD Ports Collection) 7.0.0
20170115 (experimental); `RB_GC_GUARD` looks not worked well.
</code></pre>
</blockquote>
<p>Have you tried to experiment with making <code>RB_GC_GUARD</code> stronger<br>
for gcc7, instead? There may be similar bugs, and I would rather<br>
improve <code>RB_GC_GUARD</code> than add volatiles (which are more subtle<br>
and have more variance between implementations).</p>
</blockquote>
<p>I still support fixing <code>RB_GC_GUARD</code> to be stronger for GCC7.</p>
<p>But in marshal.c, I think we can use <code>klass==0</code> to hide the object<br>
and use <code>rb_gc_force_recycle</code>, instead. AFAIK,<br>
<code>rb_gc_force_recycle</code> is safe if the object has <code>klass==0</code> for its<br>
entire lifetime.</p>
<p>How about the following?</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git a/marshal.c b/marshal.c
index d628daa4de..b9c9e6a03e 100644
</span><span class="gd">--- a/marshal.c
</span><span class="gi">+++ b/marshal.c
</span><span class="p">@@ -1024,9 +1024,9 @@</span> VALUE
rb_marshal_dump_limited(VALUE obj, VALUE port, int limit)
{
struct dump_arg *arg;
<span class="gd">- volatile VALUE wrapper; /* used to avoid memory leak in case of exception */
</span><span class="gi">+ VALUE wrapper; /* used to avoid memory leak in case of exception */
</span>
<span class="gd">- wrapper = TypedData_Make_Struct(rb_cData, struct dump_arg, &dump_arg_data, arg);
</span><span class="gi">+ wrapper = TypedData_Make_Struct(0, struct dump_arg, &dump_arg_data, arg);
</span> arg->dest = 0;
arg->symbols = st_init_numtable();
arg->data = rb_init_identtable();
<span class="p">@@ -1053,8 +1053,8 @@</span> rb_marshal_dump_limited(VALUE obj, VALUE port, int limit)
rb_io_write(arg->dest, arg->str);
rb_str_resize(arg->str, 0);
}
<span class="gd">- clear_dump_arg(arg);
- RB_GC_GUARD(wrapper);
</span><span class="gi">+ free_dump_arg(arg);
+ rb_gc_force_recycle(wrapper);
</span>
return port;
}
<span class="p">@@ -2038,7 +2038,7 @@</span> rb_marshal_load_with_proc(VALUE port, VALUE proc)
{
int major, minor, infection = 0;
VALUE v;
<span class="gd">- volatile VALUE wrapper; /* used to avoid memory leak in case of exception */
</span><span class="gi">+ VALUE wrapper; /* used to avoid memory leak in case of exception */
</span> struct load_arg *arg;
v = rb_check_string_type(port);
<span class="p">@@ -2053,7 +2053,7 @@</span> rb_marshal_load_with_proc(VALUE port, VALUE proc)
else {
io_needed();
}
<span class="gd">- wrapper = TypedData_Make_Struct(rb_cData, struct load_arg, &load_arg_data, arg);
</span><span class="gi">+ wrapper = TypedData_Make_Struct(0, struct load_arg, &load_arg_data, arg);
</span> arg->infection = infection;
arg->src = port;
arg->offset = 0;
<span class="p">@@ -2084,8 +2084,8 @@</span> rb_marshal_load_with_proc(VALUE port, VALUE proc)
if (!NIL_P(proc)) arg->proc = proc;
v = r_object(arg);
<span class="gd">- clear_load_arg(arg);
- RB_GC_GUARD(wrapper);
</span><span class="gi">+ free_load_arg(arg);
+ rb_gc_force_recycle(wrapper);
</span>
return v;
}
</code></pre> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=629662017-02-13T10:08:13Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>On 2017/02/13 10:04, Eric Wong wrote:</p>
<blockquote>
<p>I still support fixing <code>RB_GC_GUARD</code> to be stronger for GCC7.</p>
</blockquote>
<p>I think it is stronger than GCC7 now.</p>
<blockquote>
<p>But in marshal.c, I think we can use <code>klass==0</code> to hide the object<br>
and use <code>rb_gc_force_recycle</code>, instead. AFAIK,<br>
<code>rb_gc_force_recycle</code> is safe if the object has <code>klass==0</code> for its<br>
entire lifetime.</p>
<p>How about the following?</p>
</blockquote>
<p>Seems nice.</p>
<p>--<br>
Nobu Nakada</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=629812017-02-15T07:32:04Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>On 2017/02/13 19:05, Nobuyoshi Nakada wrote:</p>
<blockquote>
<blockquote>
<p>But in marshal.c, I think we can use <code>klass==0</code> to hide the object<br>
and use <code>rb_gc_force_recycle</code>, instead. AFAIK,<br>
<code>rb_gc_force_recycle</code> is safe if the object has <code>klass==0</code> for its<br>
entire lifetime.</p>
<p>How about the following?</p>
</blockquote>
<p>Seems nice.</p>
</blockquote>
<p>Sorry, I missed that <code>arg</code> may be dereferenced in <code>check_dump_arg()</code><br>
in the case continuation is used. Hiding wrapper objects is fine, but<br>
freeing <code>arg</code> and recycling <code>wrapper</code> causes a dangling pointer and<br>
can segfault on some environments, compilers and options, with the<br>
following pacth.</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git a/test/ruby/test_marshal.rb b/test/ruby/test_marshal.rb
index bc22b5fd3a..bfc3f6df25 100644
</span><span class="gd">--- a/test/ruby/test_marshal.rb
</span><span class="gi">+++ b/test/ruby/test_marshal.rb
</span><span class="p">@@ -644,6 +644,9 @@</span>
c = Bug9523.new
assert_raise_with_message(RuntimeError, /Marshal\.dump reentered at marshal_dump/) do
Marshal.dump(c)
<span class="gi">+ GC.start
+ 1000.times {"x"*1000}
+ GC.start
</span> c.cc.call
end
end
</code></pre> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=629822017-02-15T08:51:38Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>Nobuyoshi Nakada <a href="mailto:nobu@ruby-lang.org" class="email">nobu@ruby-lang.org</a> wrote:</p>
<blockquote>
<p>On 2017/02/13 19:05, Nobuyoshi Nakada wrote:</p>
<blockquote>
<blockquote>
<p>But in marshal.c, I think we can use <code>klass==0</code> to hide the object<br>
and use <code>rb_gc_force_recycle</code>, instead. AFAIK,<br>
<code>rb_gc_force_recycle</code> is safe if the object has <code>klass==0</code> for its<br>
entire lifetime.</p>
<p>How about the following?</p>
</blockquote>
<p>Seems nice.</p>
</blockquote>
<p>Sorry, I missed that <code>arg</code> may be dereferenced in <code>check_dump_arg()</code><br>
in the case continuation is used. Hiding wrapper objects is fine, but<br>
freeing <code>arg</code> and recycling <code>wrapper</code> causes a dangling pointer and<br>
can segfault on some environments, compilers and options, with the<br>
following pacth.</p>
</blockquote>
<p>Ah, thanks. I forgot this :x I saw you already made r57634.<br>
Since callcc is obsolete, is the following patch OK; or can Fiber<br>
have the same problem?</p>
<p>(Admittedly, I never fully understood callcc :x)</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git a/cont.c b/cont.c
index 50fa45e96e..363e05fb91 100644
</span><span class="gd">--- a/cont.c
</span><span class="gi">+++ b/cont.c
</span><span class="p">@@ -149,7 +149,7 @@</span> struct rb_fiber_struct {
};
static const rb_data_type_t cont_data_type, fiber_data_type;
<span class="gd">-static VALUE rb_cContinuation;
</span><span class="gi">+VALUE rb_cContinuation;
</span> static VALUE rb_cFiber;
static VALUE rb_eFiberError;
<span class="gh">diff --git a/marshal.c b/marshal.c
index 2a10b98100..57acf8c30c 100644
</span><span class="gd">--- a/marshal.c
</span><span class="gi">+++ b/marshal.c
</span><span class="p">@@ -20,6 +20,8 @@</span>
#include "encindex.h"
#include "id_table.h"
<span class="gi">+extern VALUE rb_cContinuation;
+
</span> #include <math.h>
#ifdef HAVE_FLOAT_H
#include <float.h>
<span class="p">@@ -1053,8 +1055,14 @@</span> rb_marshal_dump_limited(VALUE obj, VALUE port, int limit)
rb_io_write(arg->dest, arg->str);
rb_str_resize(arg->str, 0);
}
<span class="gd">- clear_dump_arg(arg);
- RB_GC_GUARD(wrapper);
</span><span class="gi">+ if (rb_cContinuation) {
+ clear_dump_arg(arg);
+ RB_GC_GUARD(wrapper);
+ }
+ else {
+ free_dump_arg(arg);
+ rb_gc_force_recycle(wrapper);
+ }
</span>
return port;
}
<span class="p">@@ -2084,8 +2092,14 @@</span> rb_marshal_load_with_proc(VALUE port, VALUE proc)
if (!NIL_P(proc)) arg->proc = proc;
v = r_object(arg);
<span class="gd">- free_load_arg(arg);
- rb_gc_force_recycle(wrapper);
</span><span class="gi">+ if (rb_cContinuation) {
+ clear_load_arg(arg);
+ RB_GC_GUARD(wrapper);
+ }
+ else {
+ free_load_arg(arg);
+ rb_gc_force_recycle(wrapper);
+ }
</span>
return v;
}
</code></pre> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=635332017-03-13T07:30:34Znaruse (Yui NARUSE)naruse@airemix.jp
<ul><li><strong>Backport</strong> changed from <i>2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: REQUIRED</i> to <i>2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: DONE</i></li></ul><p>ruby_2_4 r57954 merged revision(s) 57410,57619,57621,57631,57634.</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=635812017-03-14T03:40:25Znagachika (Tomoyuki Chikanaga)nagachika00@gmail.com
<ul><li><strong>Backport</strong> changed from <i>2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: DONE</i> to <i>2.2: UNKNOWN, 2.3: REQUIRED, 2.4: DONE</i></li></ul> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=635942017-03-14T13:35:07Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>normalperson (Eric Wong) wrote:</p>
<blockquote>
<p>Ah, thanks. I forgot this :x I saw you already made r57634.<br>
Since callcc is obsolete, is the following patch OK; or can Fiber<br>
have the same problem?</p>
</blockquote>
<p>I guess <code>Fiber</code> would too.</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=643822017-04-19T02:20:09Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Has duplicate</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/13319">Bug #13319</a>: GC issues seen with GCC7</i> added</li></ul> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=658412017-07-19T03:04:05Zhsbt (Hiroshi SHIBATA)hsbt@ruby-lang.org
<ul><li><strong>File</strong> <a href="/attachments/6655">ruby_2_3_gcc7.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/6655/ruby_2_3_gcc7.patch">ruby_2_3_gcc7.patch</a> added</li></ul><blockquote>
<p>usa</p>
</blockquote>
<p>I got a report for this issue about ruby-build repository.</p>
<p><a href="https://github.com/rbenv/ruby-build/issues/1115" class="external">https://github.com/rbenv/ruby-build/issues/1115</a></p>
<p>I created a patch for ruby_2_3 branch with gcc7. this patch contains r54158, r57410, r57631 and r57954.</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=659012017-07-24T03:14:49ZMSP-Greg (Greg L)
<ul></ul><p>Hiroshi,</p>
<p>Thank you for the patch. I just decided to start doing regular MinGW builds with ruby_2_3 & ruby_2_4. I also recently updated to gcc 7.1.0 from 6.3.0. With 7.1.0, ruby_2_4 was fine, but ruby_2_3 didn't get very far. Your patch seems to have solved the issue with MinGW & 7.1.0. Below are initial test results -</p>
<p>ruby 2.3.5p342 (2017-07-07 revision 59277) [x64-mingw32]<br>
test-btest passed<br>
test-all 15487 tests, 2192347 assertions, 4 failures, 1 errors, 216 skips</p>
<p>Haven't hooked up spec tests yet...</p> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=660902017-08-09T08:41:14Zusa (Usaku NAKAMURA)usa@garbagecollect.jp
<ul><li><strong>Backport</strong> changed from <i>2.2: UNKNOWN, 2.3: REQUIRED, 2.4: DONE</i> to <i>2.2: UNKNOWN, 2.3: DONE, 2.4: DONE</i></li></ul> Ruby master - Bug #13150: TestMarshal failures on FreeBSD with gcc7 because of GChttps://redmine.ruby-lang.org/issues/13150?journal_id=707072018-02-27T21:27:02ZEregon (Benoit Daloze)
<ul></ul><p>This still happens on 2.2.9, should it be backported too?<br>
(On gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2))</p>