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

[Igor Veresov]

On 2/24/11 5:27 PM, John Rose wrote:
> 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.

I can see your point but I just didn't want to introduce additional 
clutter. For example generate_block_arraycopy() never requires barrier 
suppression, so why do we need to add an argument there? From the 
debugging standpoint it's way better to emit a barrier than not to. 
Missing barriers will be a nightmare to find. So, I'd rather have a 
barrier erroneously emitted in some path by default, which would make 
the marking crash almost immediately.

As for using !must_clear_dest as a gate, I thought it would more clearly 
convey to the reader the reason why the barrier is suppressed - because 
the dest array is not cleared and contains garbage, so the reader won't 
have to go back to see in which cases use_pre_barrier is set. Perhaps I 
should add more comments.

>
> 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."
>

That's pretty much equivalent. I don't have strong feelings about this 
and will change it if you think it's more readable.

igor

> -- John