RFR(8u): 8038636, 8055008, 8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links ...and 8057570.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(8u): 8038636, 8055008, 8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links ...and 8057570.

KEVIN WALLS
Hi,

I'd like to get a hotspot review of these backports from 9 to 8u.  This
is mainly runtime territory but some of the history is from the compiler
side.

webrev: http://cr.openjdk.java.net/~kevinw/8055008.8156137/webrev.00/

The one we need is:

8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links
jbs: https://bugs.openjdk.java.net/browse/JDK-8156137
9 changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/882e8cda60b3

The 9 changeset is short, just changing klass.cpp, but not possible in 8
without additional work, so...

1.

8038636: speculative traps break when classes are redefined
https://bugs.openjdk.java.net/browse/JDK-8038636

9 changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/a7784ddacbef

9 review thread:
http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-April/013948.html 
(in 9 since April 2014)

That one imports cleanly into 8u.


2.

With that imported, we need:

8055008:Clean up code that saves the previous versions of redefined classes
https://bugs.openjdk.java.net/browse/JDK-8055008

9 changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/e3fb51ae8d7d

This is the change to stop using PreviousVersionNode and get an
InstanceKlass* from previous_versions().


My webrev: http://cr.openjdk.java.net/~kevinw/8055008.8156137/webrev.00/

...is 8055008 and 8156137 in 8u. The klass.cpp change here is 8156137. 
The rest is 8055008, so I can commit them separately.

8156137 doesn't have its own test, but I have found if you get this area
wrong, the test runtime/RedefineObject crashes.


Notes:
For 8055008 there was a change in
src/share/vm/classfile/classLoaderData.cpp which is already in 8u.

src/share/vm/classfile/metadataOnStackMark.cpp:
+MetadataOnStackMark::MetadataOnStackMark(bool has_redefined_a_class) {

The change is to stop using JvmtiExport::has_redefined_a_class() here
which is already in 8u at this point, but the param is already there
with a different name, so renamed as per 8055008.

universe.cpp: had a change in 8055008 but following that there was:

8057570: RedefineClasses() tests fail
assert(((Metadata*)obj)->is_valid()) failed: obj is valid
https://bugs.openjdk.java.net/browse/JDK-8057570
9 changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/479ed4234a9d

..this puts back a few lines in nmethod.cpp which 8055008 removed:

+ // Call function Method*, not embedded in these other places.
+ if (_method != NULL) f(_method);

..and in universe.cpp:

+ // Make the dependent methods not entrant (in VM_Deoptimize they are
made zombies)
+ CodeCache::make_marked_nmethods_not_entrant();

..and removes: CodeCache::make_marked_nmethods_zombies();

My webrev takes these into account.


Then there are 8038636's two follow-on bugs which I'd like to follow-up
in a separate thread.
8039960 is a test change
8040237: nsk/jvmti/RetransformClasses/retransform001 crashed the VM on
all platforms when run with with -server -Xcomp
https://bugs.openjdk.java.net/browse/JDK-8040237


Thanks!
Kevin



Reply | Threaded
Open this post in threaded view
|

Re: RFR(8u): 8038636, 8055008, 8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links ...and 8057570.

coleen.phillimore

Hi Kevin, These changes look good.  It's nice to have the simpler
version of the InstanceKlass::_previous_versions in jdk8.  There was a
bug tail so there may be more changes after 8040237.  It's worth
following through all the bugs to get something close to what we have in
jdk9/10.  But this change looks good so far.

Thanks,
Coleen

On 11/13/17 1:08 PM, Kevin Walls wrote:

> Hi,
>
> I'd like to get a hotspot review of these backports from 9 to 8u. This
> is mainly runtime territory but some of the history is from the
> compiler side.
>
> webrev: http://cr.openjdk.java.net/~kevinw/8055008.8156137/webrev.00/
>
> The one we need is:
>
> 8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links
> jbs: https://bugs.openjdk.java.net/browse/JDK-8156137
> 9 changeset:
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/882e8cda60b3
>
> The 9 changeset is short, just changing klass.cpp, but not possible in
> 8 without additional work, so...
>
> 1.
>
> 8038636: speculative traps break when classes are redefined
> https://bugs.openjdk.java.net/browse/JDK-8038636
>
> 9 changeset:
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/a7784ddacbef
>
> 9 review thread:
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-April/013948.html 
> (in 9 since April 2014)
>
> That one imports cleanly into 8u.
>
>
> 2.
>
> With that imported, we need:
>
> 8055008:Clean up code that saves the previous versions of redefined
> classes
> https://bugs.openjdk.java.net/browse/JDK-8055008
>
> 9 changeset:
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/e3fb51ae8d7d
>
> This is the change to stop using PreviousVersionNode and get an
> InstanceKlass* from previous_versions().
>
>
> My webrev: http://cr.openjdk.java.net/~kevinw/8055008.8156137/webrev.00/
>
> ...is 8055008 and 8156137 in 8u. The klass.cpp change here is
> 8156137.  The rest is 8055008, so I can commit them separately.
>
> 8156137 doesn't have its own test, but I have found if you get this
> area wrong, the test runtime/RedefineObject crashes.
>
>
> Notes:
> For 8055008 there was a change in
> src/share/vm/classfile/classLoaderData.cpp which is already in 8u.
>
> src/share/vm/classfile/metadataOnStackMark.cpp:
> +MetadataOnStackMark::MetadataOnStackMark(bool has_redefined_a_class) {
>
> The change is to stop using JvmtiExport::has_redefined_a_class() here
> which is already in 8u at this point, but the param is already there
> with a different name, so renamed as per 8055008.
>
> universe.cpp: had a change in 8055008 but following that there was:
>
> 8057570: RedefineClasses() tests fail
> assert(((Metadata*)obj)->is_valid()) failed: obj is valid
> https://bugs.openjdk.java.net/browse/JDK-8057570
> 9 changeset:
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/479ed4234a9d
>
> ..this puts back a few lines in nmethod.cpp which 8055008 removed:
>
> + // Call function Method*, not embedded in these other places.
> + if (_method != NULL) f(_method);
>
> ..and in universe.cpp:
>
> + // Make the dependent methods not entrant (in VM_Deoptimize they are
> made zombies)
> + CodeCache::make_marked_nmethods_not_entrant();
>
> ..and removes: CodeCache::make_marked_nmethods_zombies();
>
> My webrev takes these into account.
>
>
> Then there are 8038636's two follow-on bugs which I'd like to
> follow-up in a separate thread.
> 8039960 is a test change
> 8040237: nsk/jvmti/RetransformClasses/retransform001 crashed the VM on
> all platforms when run with with -server -Xcomp
> https://bugs.openjdk.java.net/browse/JDK-8040237
>
>
> Thanks!
> Kevin
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(8u): 8038636, 8055008, 8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links ...and 8057570.

KEVIN WALLS
Thanks Coleen!


On 16/11/2017 23:08, [hidden email] wrote:

>
> Hi Kevin, These changes look good.  It's nice to have the simpler
> version of the InstanceKlass::_previous_versions in jdk8.  There was a
> bug tail so there may be more changes after 8040237.  It's worth
> following through all the bugs to get something close to what we have
> in jdk9/10.  But this change looks good so far.
>
> Thanks,
> Coleen
>
> On 11/13/17 1:08 PM, Kevin Walls wrote:
>> Hi,
>>
>> I'd like to get a hotspot review of these backports from 9 to 8u.
>> This is mainly runtime territory but some of the history is from the
>> compiler side.
>>
>> webrev: http://cr.openjdk.java.net/~kevinw/8055008.8156137/webrev.00/
>>
>> The one we need is:
>>
>> 8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links
>> jbs: https://bugs.openjdk.java.net/browse/JDK-8156137
>> 9 changeset:
>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/882e8cda60b3
>>
>> The 9 changeset is short, just changing klass.cpp, but not possible
>> in 8 without additional work, so...
>>
>> 1.
>>
>> 8038636: speculative traps break when classes are redefined
>> https://bugs.openjdk.java.net/browse/JDK-8038636
>>
>> 9 changeset:
>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/a7784ddacbef
>>
>> 9 review thread:
>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-April/013948.html 
>> (in 9 since April 2014)
>>
>> That one imports cleanly into 8u.
>>
>>
>> 2.
>>
>> With that imported, we need:
>>
>> 8055008:Clean up code that saves the previous versions of redefined
>> classes
>> https://bugs.openjdk.java.net/browse/JDK-8055008
>>
>> 9 changeset:
>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/e3fb51ae8d7d
>>
>> This is the change to stop using PreviousVersionNode and get an
>> InstanceKlass* from previous_versions().
>>
>>
>> My webrev: http://cr.openjdk.java.net/~kevinw/8055008.8156137/webrev.00/
>>
>> ...is 8055008 and 8156137 in 8u. The klass.cpp change here is
>> 8156137.  The rest is 8055008, so I can commit them separately.
>>
>> 8156137 doesn't have its own test, but I have found if you get this
>> area wrong, the test runtime/RedefineObject crashes.
>>
>>
>> Notes:
>> For 8055008 there was a change in
>> src/share/vm/classfile/classLoaderData.cpp which is already in 8u.
>>
>> src/share/vm/classfile/metadataOnStackMark.cpp:
>> +MetadataOnStackMark::MetadataOnStackMark(bool has_redefined_a_class) {
>>
>> The change is to stop using JvmtiExport::has_redefined_a_class() here
>> which is already in 8u at this point, but the param is already there
>> with a different name, so renamed as per 8055008.
>>
>> universe.cpp: had a change in 8055008 but following that there was:
>>
>> 8057570: RedefineClasses() tests fail
>> assert(((Metadata*)obj)->is_valid()) failed: obj is valid
>> https://bugs.openjdk.java.net/browse/JDK-8057570
>> 9 changeset:
>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/479ed4234a9d
>>
>> ..this puts back a few lines in nmethod.cpp which 8055008 removed:
>>
>> + // Call function Method*, not embedded in these other places.
>> + if (_method != NULL) f(_method);
>>
>> ..and in universe.cpp:
>>
>> + // Make the dependent methods not entrant (in VM_Deoptimize they
>> are made zombies)
>> + CodeCache::make_marked_nmethods_not_entrant();
>>
>> ..and removes: CodeCache::make_marked_nmethods_zombies();
>>
>> My webrev takes these into account.
>>
>>
>> Then there are 8038636's two follow-on bugs which I'd like to
>> follow-up in a separate thread.
>> 8039960 is a test change
>> 8040237: nsk/jvmti/RetransformClasses/retransform001 crashed the VM
>> on all platforms when run with with -server -Xcomp
>> https://bugs.openjdk.java.net/browse/JDK-8040237
>>
>>
>> Thanks!
>> Kevin
>>
>>
>>
>