On 4/13/16 09:48, Coleen Phillimore wrote:
Dan, Thank you for reviewing this.
On 4/13/16 12:07 PM, Daniel D. Daugherty wrote:
On 4/11/16 2:06 PM, Coleen Phillimore wrote:
Summary: Constant pool merging is not thread safe for source_file_name.
This change includes the change for the following bug because they
are tested together.
8148772: VM crash in nsk/jvmti/RedefineClasses/StressRedefine:
assert failed: Corrupted constant pool
Summary: ConstantPool::resolve_constant_at_impl() isn't thread safe
for MethodHandleInError and MethodTypeInError.
The parallel constant pool merges are mostly harmless because the
old methods constant pool pointers aren't updated. The only case I
found where it isn't harmless is that we rely on finding the
source_file_name_index from the final merged constant pool, which
could be any of the parallel merged constant pools. The code to
attempt to dig out the name from redefined classes is removed.
open webrev at http://cr.openjdk.java.net/~coleenp/8151546.01/webrev
src/share/vm/classfile/javaClasses.inline.hpp
Perhaps instead of this comment:
L226: // Because constant pools can be merged in parallel, the
source file name index
L227: // may be merged over with something else in a previous
version.
please consider this one:
// RedefineClasses() currently permits redefine operations to
// happen in parallel using a "last one wins" philosophy. That
// spec laxness allows the constant pool entry associated with
// the source_file_name_index for any older constant pool version
// to be unstable so we shouldn't try to use it.
Okay, your comment is more complete. I'll use your comment.
src/share/vm/oops/constantPool.hpp
I think this file is all changes from 8148772 which I've already
reviewed so no comments.
src/share/vm/oops/constantPool.cpp
I think this file is all changes from 8148772 which I've already
reviewed so no comments.
src/share/vm/prims/jvmtiRedefineClasses.cpp
The version number in the constant pool is new to my brain since
I last dove into this code in detail...
Right now you do the version increment here:
L1445: // Update the version number of the constant pools (may
keep scratch_cp)
L1446: merge_cp->increment_and_save_version(old_cp->version());
L1447: scratch_cp->increment_and_save_version(old_cp->version());
You could choose to only do it when you know that you're going
to keep the scratch_cp, but maybe that's being too picky.
I'd have to set it in two places if I did that, so I picked just
once. If we use the merged_cp, the scratch_cp is discarded so the
version doesn't matter.
Serguei's find of this very old bug is good:
JDK-6227506 JVMTI Spec: Atomicity of RedefineClasses should be specified
https://bugs.openjdk.java.net/browse/JDK-6227506
There's another bug out there will all the notes that Tim Bell
took when we did the monster RedefineClasses code walk through.
I believe in that bug, the lack of locking/atomicity was also
called out. I'll see if I can find that bug...
Yes, that would be good. There are a lot of statics in
VM_RedefineClasses.
I don't know why these don't cause bugs!
I think, they do but very rarely and intermittently.
This is rare corner case when two agents or two threads of one agent do
concurrent redefinitions.
Thanks,
Serguei
Coleen
Dan
bug link https://bugs.openjdk.java.net/browse/JDK-8151546
Tested with rbt, java/lang/instrument tests, com/sun/jdi tests. I
tried to write a test with all the conditions of the failure but
couldn't make it fail (so noreg-hard).
Thanks,
Coleen
