RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Volker Simonis
Hi,

first of all I think this isn't really a high prio bug because the
assertion is wrong and there's no impact for product builds. On the
other hand I think a lot of test running on debug builds may fail
because of this issue so probably better to fix this in 9 as well (I
actually wonder that you haven't seen this issue until now :)

http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505/
https://bugs.openjdk.java.net/browse/JDK-8176505

Here are the gory details: the following assertion in
arraycopynode.cpp is wrong because src_type can be a plain Object:

const TypeAryPtr* ary_src = src_type->isa_aryptr();
assert(ary_src != NULL, "should be an array copy/clone");

This can be easily demonstrated with the following trivial test program:

public class ArraySrcType {
  public static boolean crash(Object src) {
    String[] dst = new String[1];
    System.arraycopy(src, 0, dst, 0, 1);
    return dst[0] == null;
  }
  public static void main(String[] args) {
    String[] sa = new String[1];
    for (int i = 0; i < 20_000; i++) {
      crash(sa);
    }
  }
}

Running it with a slow- or fastdebug build results in the following crash:

# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc: SuppressErrorAt=/arraycopynode.cpp:228
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error
(/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
pid=34335, tid=34410
# assert(ary_src != __null) failed: should be an array copy/clone
#

Notice that the assertion is unnecessary anyway, because some lines
later int he code we explicitely check for ary_src being NULL and bail
out if that 's the case:

    if (ary_src == NULL || ary_src->klass() == NULL ||
        ary_dest == NULL || ary_dest->klass() == NULL) {
      // We don't know if arguments are arrays
      return false;
    }

Unfortunately, I still need a sponsor :(

Thank you and best regards,
Volker
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Roland Westrelin-3

Hi Volker,

> Here are the gory details: the following assertion in
> arraycopynode.cpp is wrong because src_type can be a plain Object:
>
> const TypeAryPtr* ary_src = src_type->isa_aryptr();
> assert(ary_src != NULL, "should be an array copy/clone");

Shouldn't it be moved in the else branch where ary_src is used?

Roland.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Tobias Hartmann-2
In reply to this post by Volker Simonis
Hi Volker,

thanks for fixing this issue!

I added this assert with JDK-8156760 which was about Object.clone(). I agree with Roland that the assert should be moved into the else branch which handles clonebasic.

Could you add the regression test to your changeset?

Thanks,
Tobias

On 10.03.2017 14:58, Volker Simonis wrote:

> Hi,
>
> first of all I think this isn't really a high prio bug because the
> assertion is wrong and there's no impact for product builds. On the
> other hand I think a lot of test running on debug builds may fail
> because of this issue so probably better to fix this in 9 as well (I
> actually wonder that you haven't seen this issue until now :)
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505/
> https://bugs.openjdk.java.net/browse/JDK-8176505
>
> Here are the gory details: the following assertion in
> arraycopynode.cpp is wrong because src_type can be a plain Object:
>
> const TypeAryPtr* ary_src = src_type->isa_aryptr();
> assert(ary_src != NULL, "should be an array copy/clone");
>
> This can be easily demonstrated with the following trivial test program:
>
> public class ArraySrcType {
>   public static boolean crash(Object src) {
>     String[] dst = new String[1];
>     System.arraycopy(src, 0, dst, 0, 1);
>     return dst[0] == null;
>   }
>   public static void main(String[] args) {
>     String[] sa = new String[1];
>     for (int i = 0; i < 20_000; i++) {
>       crash(sa);
>     }
>   }
> }
>
> Running it with a slow- or fastdebug build results in the following crash:
>
> # To suppress the following error report, specify this argument
> # after -XX: or in .hotspotrc: SuppressErrorAt=/arraycopynode.cpp:228
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # Internal Error
> (/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
> pid=34335, tid=34410
> # assert(ary_src != __null) failed: should be an array copy/clone
> #
>
> Notice that the assertion is unnecessary anyway, because some lines
> later int he code we explicitely check for ary_src being NULL and bail
> out if that 's the case:
>
>     if (ary_src == NULL || ary_src->klass() == NULL ||
>         ary_dest == NULL || ary_dest->klass() == NULL) {
>       // We don't know if arguments are arrays
>       return false;
>     }
>
> Unfortunately, I still need a sponsor :(
>
> Thank you and best regards,
> Volker
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Volker Simonis
Hi Roland, Tobias,

thanks for looking at my fix! You're both right, the assertion should
be moved into the else branch instead of being removed. I've done that
and also updated the assertion message because in the else branch the
array copy node can only be a clone:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505.v1/

I've also added my small test as regression test. But I'm currently at
a loss with it. Without my fix, running it obviously crashes the VM,
but JTreg still reports the test as passed:

#section:main
----------messages:(4/206)----------
command: main compiler.arraycopy.TestObjectArrayCopy
reason: User specified action: run main/othervm
compiler.arraycopy.TestObjectArrayCopy
Mode: othervm [/othervm specified]
elapsed time (seconds): 1.731
----------configuration:(0/0)----------
----------System.out:(23/1256)----------
# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc:  SuppressErrorAt=/arraycopynode.cpp:228
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error
(/usr/work/d046063/OpenJDK/jdk9-dev/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
pid=12359, tid=12441
#  assert(ary_src != __null) failed: should be an array copy/clone
...
#
Current thread is 12441
Dumping core ...
----------System.err:(1/15)----------
STATUS:Passed.

Can you please check if you see the same behavior with your version of
JTreg? Or am I doing something fundamentally wrong? I'm pretty sure
that a crashing VM should lead to test failure and I'm also pretty
sure that it worked that way in the past.

Regards,
Volker


On Fri, Mar 10, 2017 at 3:13 PM, Tobias Hartmann
<[hidden email]> wrote:

> Hi Volker,
>
> thanks for fixing this issue!
>
> I added this assert with JDK-8156760 which was about Object.clone(). I agree with Roland that the assert should be moved into the else branch which handles clonebasic.
>
> Could you add the regression test to your changeset?
>
> Thanks,
> Tobias
>
> On 10.03.2017 14:58, Volker Simonis wrote:
>> Hi,
>>
>> first of all I think this isn't really a high prio bug because the
>> assertion is wrong and there's no impact for product builds. On the
>> other hand I think a lot of test running on debug builds may fail
>> because of this issue so probably better to fix this in 9 as well (I
>> actually wonder that you haven't seen this issue until now :)
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505/
>> https://bugs.openjdk.java.net/browse/JDK-8176505
>>
>> Here are the gory details: the following assertion in
>> arraycopynode.cpp is wrong because src_type can be a plain Object:
>>
>> const TypeAryPtr* ary_src = src_type->isa_aryptr();
>> assert(ary_src != NULL, "should be an array copy/clone");
>>
>> This can be easily demonstrated with the following trivial test program:
>>
>> public class ArraySrcType {
>>   public static boolean crash(Object src) {
>>     String[] dst = new String[1];
>>     System.arraycopy(src, 0, dst, 0, 1);
>>     return dst[0] == null;
>>   }
>>   public static void main(String[] args) {
>>     String[] sa = new String[1];
>>     for (int i = 0; i < 20_000; i++) {
>>       crash(sa);
>>     }
>>   }
>> }
>>
>> Running it with a slow- or fastdebug build results in the following crash:
>>
>> # To suppress the following error report, specify this argument
>> # after -XX: or in .hotspotrc: SuppressErrorAt=/arraycopynode.cpp:228
>> #
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> # Internal Error
>> (/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
>> pid=34335, tid=34410
>> # assert(ary_src != __null) failed: should be an array copy/clone
>> #
>>
>> Notice that the assertion is unnecessary anyway, because some lines
>> later int he code we explicitely check for ary_src being NULL and bail
>> out if that 's the case:
>>
>>     if (ary_src == NULL || ary_src->klass() == NULL ||
>>         ary_dest == NULL || ary_dest->klass() == NULL) {
>>       // We don't know if arguments are arrays
>>       return false;
>>     }
>>
>> Unfortunately, I still need a sponsor :(
>>
>> Thank you and best regards,
>> Volker
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Tobias Hartmann-2
Hi Volker,

On 10.03.2017 17:34, Volker Simonis wrote:

> Hi Roland, Tobias,
>
> thanks for looking at my fix! You're both right, the assertion should
> be moved into the else branch instead of being removed. I've done that
> and also updated the assertion message because in the else branch the
> array copy node can only be a clone:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505.v1/
>
> I've also added my small test as regression test. But I'm currently at
> a loss with it. Without my fix, running it obviously crashes the VM,
> but JTreg still reports the test as passed:
>
> #section:main
> ----------messages:(4/206)----------
> command: main compiler.arraycopy.TestObjectArrayCopy
> reason: User specified action: run main/othervm
> compiler.arraycopy.TestObjectArrayCopy
> Mode: othervm [/othervm specified]
> elapsed time (seconds): 1.731
> ----------configuration:(0/0)----------
> ----------System.out:(23/1256)----------
> # To suppress the following error report, specify this argument
> # after -XX: or in .hotspotrc:  SuppressErrorAt=/arraycopynode.cpp:228
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error
> (/usr/work/d046063/OpenJDK/jdk9-dev/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
> pid=12359, tid=12441
> #  assert(ary_src != __null) failed: should be an array copy/clone
> ...
> #
> Current thread is 12441
> Dumping core ...
> ----------System.err:(1/15)----------
> STATUS:Passed.
>
> Can you please check if you see the same behavior with your version of
> JTreg? Or am I doing something fundamentally wrong? I'm pretty sure
> that a crashing VM should lead to test failure and I'm also pretty
> sure that it worked that way in the past.

I can confirm the same behavior on my system (though it's very intermittent). I think it's because 20.000 loop iterations are just enough to trigger the C2 compilation but when the compilation fails, the test is done and jtreg is already shutting down without recognizing that the VM crashed.

I would suggest to either user -Xbatch or increase the number of loop iterations (40.000 works on my system).

Thanks,
Tobias

> On Fri, Mar 10, 2017 at 3:13 PM, Tobias Hartmann
> <[hidden email]> wrote:
>> Hi Volker,
>>
>> thanks for fixing this issue!
>>
>> I added this assert with JDK-8156760 which was about Object.clone(). I agree with Roland that the assert should be moved into the else branch which handles clonebasic.
>>
>> Could you add the regression test to your changeset?
>>
>> Thanks,
>> Tobias
>>
>> On 10.03.2017 14:58, Volker Simonis wrote:
>>> Hi,
>>>
>>> first of all I think this isn't really a high prio bug because the
>>> assertion is wrong and there's no impact for product builds. On the
>>> other hand I think a lot of test running on debug builds may fail
>>> because of this issue so probably better to fix this in 9 as well (I
>>> actually wonder that you haven't seen this issue until now :)
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505/
>>> https://bugs.openjdk.java.net/browse/JDK-8176505
>>>
>>> Here are the gory details: the following assertion in
>>> arraycopynode.cpp is wrong because src_type can be a plain Object:
>>>
>>> const TypeAryPtr* ary_src = src_type->isa_aryptr();
>>> assert(ary_src != NULL, "should be an array copy/clone");
>>>
>>> This can be easily demonstrated with the following trivial test program:
>>>
>>> public class ArraySrcType {
>>>   public static boolean crash(Object src) {
>>>     String[] dst = new String[1];
>>>     System.arraycopy(src, 0, dst, 0, 1);
>>>     return dst[0] == null;
>>>   }
>>>   public static void main(String[] args) {
>>>     String[] sa = new String[1];
>>>     for (int i = 0; i < 20_000; i++) {
>>>       crash(sa);
>>>     }
>>>   }
>>> }
>>>
>>> Running it with a slow- or fastdebug build results in the following crash:
>>>
>>> # To suppress the following error report, specify this argument
>>> # after -XX: or in .hotspotrc: SuppressErrorAt=/arraycopynode.cpp:228
>>> #
>>> # A fatal error has been detected by the Java Runtime Environment:
>>> #
>>> # Internal Error
>>> (/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
>>> pid=34335, tid=34410
>>> # assert(ary_src != __null) failed: should be an array copy/clone
>>> #
>>>
>>> Notice that the assertion is unnecessary anyway, because some lines
>>> later int he code we explicitely check for ary_src being NULL and bail
>>> out if that 's the case:
>>>
>>>     if (ary_src == NULL || ary_src->klass() == NULL ||
>>>         ary_dest == NULL || ary_dest->klass() == NULL) {
>>>       // We don't know if arguments are arrays
>>>       return false;
>>>     }
>>>
>>> Unfortunately, I still need a sponsor :(
>>>
>>> Thank you and best regards,
>>> Volker
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Volker Simonis
On Mon, Mar 13, 2017 at 9:22 AM, Tobias Hartmann
<[hidden email]> wrote:

> Hi Volker,
>
> On 10.03.2017 17:34, Volker Simonis wrote:
>> Hi Roland, Tobias,
>>
>> thanks for looking at my fix! You're both right, the assertion should
>> be moved into the else branch instead of being removed. I've done that
>> and also updated the assertion message because in the else branch the
>> array copy node can only be a clone:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505.v1/
>>
>> I've also added my small test as regression test. But I'm currently at
>> a loss with it. Without my fix, running it obviously crashes the VM,
>> but JTreg still reports the test as passed:
>>
>> #section:main
>> ----------messages:(4/206)----------
>> command: main compiler.arraycopy.TestObjectArrayCopy
>> reason: User specified action: run main/othervm
>> compiler.arraycopy.TestObjectArrayCopy
>> Mode: othervm [/othervm specified]
>> elapsed time (seconds): 1.731
>> ----------configuration:(0/0)----------
>> ----------System.out:(23/1256)----------
>> # To suppress the following error report, specify this argument
>> # after -XX: or in .hotspotrc:  SuppressErrorAt=/arraycopynode.cpp:228
>> #
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> #  Internal Error
>> (/usr/work/d046063/OpenJDK/jdk9-dev/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
>> pid=12359, tid=12441
>> #  assert(ary_src != __null) failed: should be an array copy/clone
>> ...
>> #
>> Current thread is 12441
>> Dumping core ...
>> ----------System.err:(1/15)----------
>> STATUS:Passed.
>>
>> Can you please check if you see the same behavior with your version of
>> JTreg? Or am I doing something fundamentally wrong? I'm pretty sure
>> that a crashing VM should lead to test failure and I'm also pretty
>> sure that it worked that way in the past.
>
> I can confirm the same behavior on my system (though it's very intermittent). I think it's because 20.000 loop iterations are just enough to trigger the C2 compilation but when the compilation fails, the test is done and jtreg is already shutting down without recognizing that the VM crashed.
>

Thanks!

> I would suggest to either user -Xbatch or increase the number of loop iterations (40.000 works on my system).
>

You're right. It's probably a race between the main and the compiler
thread. Just strange that I always get an error exit code when
executing the test on the command line and always a zero exit code if
executed within JTreg (altough I can see that the full hs_err file
gets written).

But that's another problem :) For this change I've just added -Xbatch
as you proposed (and also -XX:-UseOnStackRepelacement). So the test
should work reliable now:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505.v2/

Thank you and best regards,
Volker

> Thanks,
> Tobias
>
>> On Fri, Mar 10, 2017 at 3:13 PM, Tobias Hartmann
>> <[hidden email]> wrote:
>>> Hi Volker,
>>>
>>> thanks for fixing this issue!
>>>
>>> I added this assert with JDK-8156760 which was about Object.clone(). I agree with Roland that the assert should be moved into the else branch which handles clonebasic.
>>>
>>> Could you add the regression test to your changeset?
>>>
>>> Thanks,
>>> Tobias
>>>
>>> On 10.03.2017 14:58, Volker Simonis wrote:
>>>> Hi,
>>>>
>>>> first of all I think this isn't really a high prio bug because the
>>>> assertion is wrong and there's no impact for product builds. On the
>>>> other hand I think a lot of test running on debug builds may fail
>>>> because of this issue so probably better to fix this in 9 as well (I
>>>> actually wonder that you haven't seen this issue until now :)
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505/
>>>> https://bugs.openjdk.java.net/browse/JDK-8176505
>>>>
>>>> Here are the gory details: the following assertion in
>>>> arraycopynode.cpp is wrong because src_type can be a plain Object:
>>>>
>>>> const TypeAryPtr* ary_src = src_type->isa_aryptr();
>>>> assert(ary_src != NULL, "should be an array copy/clone");
>>>>
>>>> This can be easily demonstrated with the following trivial test program:
>>>>
>>>> public class ArraySrcType {
>>>>   public static boolean crash(Object src) {
>>>>     String[] dst = new String[1];
>>>>     System.arraycopy(src, 0, dst, 0, 1);
>>>>     return dst[0] == null;
>>>>   }
>>>>   public static void main(String[] args) {
>>>>     String[] sa = new String[1];
>>>>     for (int i = 0; i < 20_000; i++) {
>>>>       crash(sa);
>>>>     }
>>>>   }
>>>> }
>>>>
>>>> Running it with a slow- or fastdebug build results in the following crash:
>>>>
>>>> # To suppress the following error report, specify this argument
>>>> # after -XX: or in .hotspotrc: SuppressErrorAt=/arraycopynode.cpp:228
>>>> #
>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>> #
>>>> # Internal Error
>>>> (/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
>>>> pid=34335, tid=34410
>>>> # assert(ary_src != __null) failed: should be an array copy/clone
>>>> #
>>>>
>>>> Notice that the assertion is unnecessary anyway, because some lines
>>>> later int he code we explicitely check for ary_src being NULL and bail
>>>> out if that 's the case:
>>>>
>>>>     if (ary_src == NULL || ary_src->klass() == NULL ||
>>>>         ary_dest == NULL || ary_dest->klass() == NULL) {
>>>>       // We don't know if arguments are arrays
>>>>       return false;
>>>>     }
>>>>
>>>> Unfortunately, I still need a sponsor :(
>>>>
>>>> Thank you and best regards,
>>>> Volker
>>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Tobias Hartmann-2
Hi Volker,

On 13.03.2017 16:20, Volker Simonis wrote:
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505.v1/

Looks good to me! I can do the sponsoring.

Best regards,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

Roland Westrelin-3
In reply to this post by Volker Simonis
Loading...