Request for review(M): 6627983: G1: Bad oop deference during marking

[John Rose]

On Feb 24, 2011, at 4:49 PM, Igor Veresov wrote:

> Just by code analysis. Did I miss something?

I felt a slight doubt because there are tricky paths through the various subroutine layers in library_call.  I see you converted "must_clear_dest" to "suppress_pre_barrier" along two paths.  There's a path through generate_block_arraycopy which *almost* needs the flag also, but it allows the flag to default.

To make the code more obviously correct, I suggest making the flag non-optional in library_call.  And when the role of the flag changes, a line or two of code could mark the transition like this:
   bool use_pre_barrier = (bt == T_OBJECT);  // usual case
   if (must_clear_dest)  use_pre_barrier = false;  // 6627983 suppress pre_barrier, if any
   ...
   foo(..., use_pre_barrier);

If it is simply foo(..., !must_clear_dest), it becomes hard for maintainers to see what the argument means.

In the stub generator the optional boolean is OK as is, although I generally prefer optional booleans to default to 'false'.  Given the way the bug is structured, I keep wanting to call it 'suppress_pre_barrier', meaning "just this once, trust me when I tell you not to emit the pre-barrier, if there is one."

-- John