From: Michael King Date: 2009-03-05T23:04:24+09:00 Subject: [ruby-core:22683] Re: MBARI8 patch fixes bugs caused by incorrect volatile variable declarations --000e0cd56aa430a44704645fa5dd Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit A coworker of mine is working on his masters thesis using Ruby and some C libraries. He was originally having some issues with the memory leak and tried the MBARI 7 patches. He did see a nice improvement, but since he had compiled for 64bit he was seeing segfaults happening during long running processes. After I mentioned the MBARI 8 patches he recompiled his Ruby and reported to me that his script was no longer segfaulting. - Michael On Sat, Feb 28, 2009 at 12:47 AM, Brent Roman wrote: > > Much of this is excerpted from updates at: > > http://sites.google.com/site/brentsrubypatches/ > > I believe I discovered two latent MRI bugs that had been unearthed by my > factoring of rb_eval() into separate eval functions for each node type (in > MBARI 4). This was all done in the course of determining why the MBARI7 > patch set, while very solid on x86-32, was segfaulting on x86-64 CPUs. > > First: > The function evaluating a string literal node would, in some cases, pass > the > internal pointer of a newly created ruby string object into rb_reg_new(), > which would derive a new regular expression object from it. Trouble was, > gcc, when optimizing on a machine like the x86-64, would determine that the > pointer to the newly created string object need not be stacked and in fact > could be entirely "forgotten" as soon its text buffer was passed into > rb_reg_new(). Nothing wrong with that... unless a GC pass happened to be > triggered while deriving the new regular expression from that string > object's internal text buffer. In which case, that now unreferenced String > object would never be marked and, as a result, that String and its text > buffer would be prematurely freed, trashing the regular expression pattern, > resulting in very occasional regex match failures and (very rare) heap > corruption. > > The best test case I came up with for this first bug was: > > $ ruby -e "GC.stress=true; load 'test/pathtest/test_pathtest.rb'" > > Lots of test failures with MBARI 7 (only on x86-64), all passed with MBARI > 8 > > The RB_GC_GUARD() macro introduced in 1.8.7 works nicely to prevent this > sort of problem: > + RB_GC_GUARD(str); /* prevent tail call optimization here */ > return str2; > case NODE_DREGX_ONCE: /* regexp expand once */ > str2 = rb_reg_new(RSTRING(str)->ptr, RSTRING(str)->len, > node->nd_cflag); > nd_set_type(node, NODE_LIT); > + RB_GC_GUARD(str); /* ensure str is not GC'd in rb_reg_new */ > return node->nd_lit = str2; > > > Second: > eval.c is full of setjmp()s and longjmp() calls. These are error prone > constructs for a number reasons. The most insidious of which is the fact > that the 'C' spec does not require that non-volatile local variables in the > function containing a setjmp() be preserved when it returns via longjmp(). > A few of the unpatched functions containing EXEC_TAG() in eval.c missed > this > point, failing to declare as 'volatile' variables that might need to be > preserved on the stack in exceptional cases (redo clauses in some contexts, > for example). And, many variables were declared volatile that did not need > to be, adding to the confusion. This coding rule is difficult to maintain > during incremental development, even if one follows it properly in the > first > place. I added a few such errors of my own with MBARI4, because I did not > fully understand the volatile qualifier in this context and tried to follow > the "patterns" I saw in the existing code. The volatile qualifier here > is > to prevent variables from being cached in registers. So, of course, CPUs > with large register files would be most susceptible to this class of bug. > > > Two independent testers had more elaborate test cases that failed very > occasionally on x86-64 with the MBARI 7 patches on x86-64. They both > report > that their tests now work with MBARI 8A. > > Here is one of the simpler examples of volatile qualifiers being misplaced > in the unpatched eval.c: > (See if you don't agree) > > /* function to call func under the specified class/module context */ > static VALUE > exec_under(func, under, cbase, args) > VALUE (*func)(); > - VALUE under, cbase; > + VALUE under; > + volatile VALUE cbase; > void *args; > { > - VALUE val = Qnil; /* OK */ > + VALUE val; > int state; > - int mode; > + volatile int mode; > struct FRAME *f = ruby_frame; > > PUSH_CLASS(under); > PUSH_FRAME(); > ruby_frame->self = f->self; > ruby_frame->last_func = f->last_func; > ruby_frame->orig_func = f->orig_func; > ruby_frame->last_class = f->last_class; > ruby_frame->argc = f->argc; > if (cbase) { > PUSH_CREF(cbase); > } > > mode = scope_vmode; > SCOPE_SET(SCOPE_PUBLIC); > PUSH_TAG(PROT_NONE); > if ((state = EXEC_TAG()) == 0) { > val = (*func)(args); > } > POP_TAG(); > if (cbase) POP_CREF(); > SCOPE_SET(mode); > POP_FRAME(); > POP_CLASS(); > if (state) JUMP_TAG(state); > > return val; > } > > > Unfortunately, there is a lot of this sort of thing in eval.c. > MBARI8 is a big patch as a result. > > I'm guessing that we are not seeing failures in the field because the > x86-32 > has very few registers, so, in practice, variables are usually on the stack > and thus preserved through longjmps even when they are not explicitly > declared volatile. > > I'd actually be quite relieved if I were proven wrong about this :-) > Otherwise, these fixes will be in the patches I submit upstream. > > The only bright side is that, by removing the volatile qualifier from > variables where > it was unnecessary, MBARI8 on x86-64 seems to have realized a few percent > speed improvement running > the ruby test suite over both unpatched Ruby and the Ruby with the MBARI7 > patch. > > > - brent > > > -- > View this message in context: > http://www.nabble.com/MBARI8-patch-fixes-bugs-caused-by-incorrect-volatile-variable-declarations-tp22259357p22259357.html > Sent from the ruby-core mailing list archive at Nabble.com. > > > --000e0cd56aa430a44704645fa5dd Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable A coworker of mine is working on his masters thesis using Ruby and some C l= ibraries. He was originally having some issues with the memory leak and tri= ed the MBARI 7 patches. He did see a nice improvement, but since he had com= piled for 64bit he was seeing segfaults happening during long running proce= sses. After I mentioned the MBARI 8 patches he recompiled his Ruby and repo= rted to me that his script was no longer segfaulting.

- Michael

On Sat, Feb 28, 2009 at 12:= 47 AM, Brent Roman <brent@mbari.org> wrote:

Much of this is excerpted from updates at:

http://sites.google.com/site/brentsrubypatches/

I believe I discovered two latent MRI bugs that had been unearthed by my factoring of rb_eval() into separate eval functions for each node type (in<= br> MBARI 4). =A0This was all done in the course of determining why the MBARI7<= br> patch set, while very solid on x86-32, was segfaulting on x86-64 CPUs.

First:
The function evaluating a string literal node would, in some cases, pass th= e
internal pointer of a newly created ruby string object into rb_reg_new(), which would derive a new regular expression object from it. =A0Trouble was,=
gcc, when optimizing on a machine like the x86-64, would determine that the=
pointer to the newly created string object need not be stacked and in fact<= br> could be entirely "forgotten" as soon its text buffer was passed = into
rb_reg_new(). =A0Nothing wrong with that... unless a GC pass happened to be=
triggered while deriving the new regular expression from that string
object's internal text buffer. =A0In which case, that now unreferenced = String
object would never be marked and, as a result, that String and its text
buffer would be prematurely freed, trashing the regular expression pattern,=
resulting in very occasional regex match failures and (very rare) heap
corruption.

The best test case I came up with for this first bug was:

=A0 $ ruby -e "GC.stress=3Dtrue; load 'test/pathtest/test_pathtes= t.rb'"

Lots of test failures with MBARI 7 (only on x86-64), =A0all passed with MBA= RI
8

The RB_GC_GUARD() macro introduced in 1.8.7 works nicely to prevent this sort of problem:
+ =A0 =A0 =A0 RB_GC_GUARD(str); =A0/* prevent tail call optimization here *= /
=A0 =A0 =A0 return str2;
=A0 =A0 case NODE_DREGX_ONCE: =A0 =A0 =A0/* regexp expand once */
=A0 =A0 =A0 str2 =3D rb_reg_new(RSTRING(str)->ptr, RSTRING(str)->len= ,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0node->nd_cflag);
=A0 =A0 =A0 nd_set_type(node, NODE_LIT);
+ =A0 =A0 =A0RB_GC_GUARD(str); =A0/* ensure str is not GC'd in rb_reg_n= ew */
=A0 =A0 =A0 return node->nd_lit =3D str2;


Second:
eval.c is full of setjmp()s and longjmp() calls. =A0These are error prone constructs for a number reasons. =A0The most insidious of which is the fact=
that the 'C' spec does not require that non-volatile local variable= s in the
function containing a setjmp() be preserved when it returns via longjmp().<= br> A few of the unpatched functions containing EXEC_TAG() in eval.c missed thi= s
point, failing to declare as 'volatile' variables that might need t= o be
preserved on the stack in exceptional cases (redo clauses in some contexts,=
for example). =A0And, many variables were declared volatile that did not ne= ed
to be, adding to the confusion. =A0This coding rule is difficult to maintai= n
during incremental development, even if one follows it properly in the firs= t
place. =A0I added a few such errors of my own with MBARI4, because I did no= t
fully understand the volatile qualifier in this context and tried to follow=
the "patterns" I saw in the existing code. =A0 =A0The volatile qu= alifier here is
to prevent variables from being cached in registers. =A0So, of course, CPUs=
with large register files would be most susceptible to this class of bug.

Two independent testers had more elaborate test cases that failed very
occasionally on x86-64 with the MBARI 7 patches on x86-64. =A0They both rep= ort
that their tests now work with MBARI 8A.

Here is one of the simpler examples of volatile qualifiers being misplaced<= br> in the unpatched eval.c:
(See if you don't agree)

/* function to call func under the specified class/module context */
static VALUE
exec_under(func, under, cbase, args)
=A0 =A0VALUE (*func)();
- =A0 =A0VALUE under, cbase;
+ =A0 =A0VALUE under;
+ =A0 =A0volatile VALUE cbase;
=A0 =A0void *args;
{
- =A0 =A0VALUE val =3D Qnil; =A0 =A0 =A0 =A0 =A0/* OK */
+ =A0 =A0VALUE val;
=A0 =A0int state;
- =A0 =A0int mode;
+ =A0 =A0volatile int mode;
=A0 =A0struct FRAME *f =3D ruby_frame;

=A0 =A0PUSH_CLASS(under);
=A0 =A0PUSH_FRAME();
=A0 =A0ruby_frame->self =3D f->self;
=A0 =A0ruby_frame->last_func =3D f->last_func;
=A0 =A0ruby_frame->orig_func =3D f->orig_func;
=A0 =A0ruby_frame->last_class =3D f->last_class;
=A0 =A0ruby_frame->argc =3D f->argc;
=A0 =A0if (cbase) {
=A0 =A0 =A0 =A0PUSH_CREF(cbase);
=A0 =A0}

=A0 =A0mode =3D scope_vmode;
=A0 =A0SCOPE_SET(SCOPE_PUBLIC);
=A0 =A0PUSH_TAG(PROT_NONE);
=A0 =A0if ((state =3D EXEC_TAG()) =3D=3D 0) {
=A0 =A0 =A0 =A0val =3D (*func)(args);
=A0 =A0}
=A0 =A0POP_TAG();
=A0 =A0if (cbase) POP_CREF();
=A0 =A0SCOPE_SET(mode);
=A0 =A0POP_FRAME();
=A0 =A0POP_CLASS();
=A0 =A0if (state) JUMP_TAG(state);

=A0 =A0return val;
}


Unfortunately, there is a lot of this sort of thing in eval.c.
MBARI8 is a big patch as a result.

I'm guessing that we are not seeing failures in the field because the x= 86-32
has very few registers, so, in practice, variables are usually on the stack=
and thus preserved through longjmps even when they are not explicitly
declared volatile.

I'd actually be quite relieved if I were proven wrong about this :-) Otherwise, these fixes will be in the patches I submit upstream.

The only bright side is that, by removing the volatile qualifier from
variables where
it was unnecessary, MBARI8 on x86-64 seems to have realized a few percent speed improvement running
the ruby test suite over both unpatched Ruby and the Ruby with the MBARI7 patch.


- brent


--
View this message in context: http://www.nabble.com/MBARI8-patch-fixes-bu= gs-caused-by-incorrect-volatile-variable-declarations-tp22259357p22259357.h= tml
Sent from the ruby-core mailing list archive at Nabble.com.



--000e0cd56aa430a44704645fa5dd--