[9] RFR(S): 8156760: VM crashes if -XX:-ReduceInitialCardMarks is set

[Tobias Hartmann]

Hi Yumin,

On 23.05.2016 19:43, yumin qi wrote:
> Hi, Tobias
> 
> src/share/vm/opto/graphKit.cpp:
> 
> 4210   // If we are writing a NULL then we need no post barrier
> 4211   if (val != NULL && val->is_Con() && val->bottom_type() == TypePtr::NULL_PTR) {
> 
> val is valid now so val != NULL can be removed I think.

Thanks, you are right. I updated the webrev in-place.

Best regards,
Tobias

> 
> I'm not a Reviewer
> 
> 
> Thanks
> 
> Yumin
> 
> 
> 
> On Mon, May 23, 2016 at 3:29 AM, Tobias Hartmann <tobias.hartmann at oracle.com <mailto:tobias.hartmann at oracle.com>> wrote:
> 
>     Hi Roland,
> 
>     thanks for the review! Please see comments below.
> 
>     On 23.05.2016 11:34, Roland Westrelin wrote:
>     >
>     > Hi Tobias,
>     >
>     >> Problem 3: C2 crashes with SIGSEGV in
>     >> ArrayCopyNode::prepare_array_copy() because we expect an array
>     >> clone/copy and dereference 'src_type->isa_aryptr()' but actually have
>     >> a non-array Object.clone() [3]. This is because with
>     >> !ReduceInitialCardMarks, ArrayCopyNode::try_clone_instance() does not
>     >> capture the Object.clone() intrinsic because we emit card marking
>     >> code (we bail out in 'ArrayCopyNode::finish_transform()'). We
>     >> continue assuming that the array copy is a non-instance copy. I added
>     >> an additional check to bail out in this case.
>     >
>     > One problem I noticed in this code is that
>     > ArrayCopyNode::try_clone_instance() returns NULL to mean both this is
>     > not a basic clone:
>     >
>     >   if (!is_clonebasic()) {
>     >     return NULL;
>     >   }
>     >
>     > and the clone failed:
>     >
>     >   if (!finish_transform(phase, can_reshape, ctl, mem)) {
>     >     return NULL;
>     >   }
>     >
>     > ArrayCopyNode::finish_transform() would fail with
>     > !ReduceInitialCardMarks. The way I fixed this locally is to return
>     > NodeSentinel when the clone fails so the caller can distinguish not a
>     > clone from a failure. And then ArrayCopyNode::finish_transform():
>     >
>     >   Node* mem = try_clone_instance(phase, can_reshape, count);
>     >   if (mem != NULL) {
>     >     return mem == NodeSentinel ? NULL: mem;
>     >   }
>     >
>     > Does that solve the same problem you're seeing?
> 
>     Yes, that's a better solution and solves the problem as well.
> 
>     Here is the updated webrev:
>     http://cr.openjdk.java.net/~thartmann/8156760/webrev.01/
> 
>     Thanks,
>     Tobias
> 
>     >
>     > Roland.
>     >
> 
>