JDK-4742833 : RetData::fixup_ret keeps oop live across RetData_lock which may GC
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 1.4.0,1.4.0_01,1.4.2
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic,windows_nt,windows_2000
  • CPU: generic,x86
  • Submitted: 2002-09-06
  • Updated: 2002-09-25
  • Resolved: 2002-09-25
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other Other
1.4.1_01 01Fixed 1.4.2Fixed
Related Reports
Duplicate :  
JDK-4509816 - RAS: Vtest, Vmark client intermittent Fail in merlin_b81 c2 on win2ksp1
Description
Holding an oop in a local variable across a GC point will break when 
GC moves the object.

Found by Cliff and Coleen during investigation of 4726812
Comments
CONVERTED DATA BugTraq+ Release Management Values COMMIT TO FIX: 1.4.1_01 mantis FIXED IN: 1.4.1_01 mantis INTEGRATED IN: 1.4.1_01 mantis mantis-b03
14-06-2004
EVALUATION GC can invalidate an oop stored in a local variable, switch to using a handle. Previous crashes in MethodDataOopDesc::bci_to_dp(int bci) identified in 4729122 and 4509816 are believed to be the result of this bug.
11-06-2004
SUGGESTED FIX The problem in interp_masm_i486.cpp was found while investigating the lock/GC issue in the rest of the diffs. Aside from interp_masm_i486.cpp, the problem is platform independent. Diffs for Build Dated 20020906-152828 src/cpu/i486/vm/interp_masm_i486.cpp [1.124 vs. 1.125] src/share/vm/interpreter/interpreterRuntime.cpp [1.414 vs. 1.416] src/share/vm/oops/methodDataOop.cpp [1.22 vs. 1.25] src/share/vm/oops/methodDataOop.hpp [1.26 vs. 1.27] src/cpu/i486/vm/interp_masm_i486.cpp [1.124 vs. 1.125] @@ -708,7 +708,9 @@ void InterpreterMacroAssembler::update_m void InterpreterMacroAssembler::update_mdp_for_ret(Register return_bci) { assert(ProfileInterpreter, "must be profiling interpreter"); + pushl(return_bci); // save/restore across call_VM call_VM(noreg, CAST_FROM_FN_PTR(address, InterpreterRuntime::update_mdp_for_ret), return_bci); + popl(return_bci); } #endif // !CORE src/share/vm/interpreter/interpreterRuntime.cpp [1.414 vs. 1.416] @@ -693,12 +693,20 @@ IRT_END IRT_ENTRY(void, InterpreterRuntime::update_mdp_for_ret(JavaThread* thread, int return_bci)) assert(ProfileInterpreter, "must be profiling interpreter"); ResourceMark rm(thread); + HandleMark hm(thread); frame fr = thread->last_frame(); assert(fr.is_interpreted_frame(), "must come from interpreter"); - methodDataOop mdo = fr.interpreter_frame_method()->method_data(); - ProfileData* data = mdo->data_at(mdo->dp_to_di(fr.interpreter_frame_mdp())); + methodDataHandle h_mdo(thread, fr.interpreter_frame_method()->method_data()); + + // Grab a lock to ensure atomic access to setting the return bci and + // the displacement. This can block and GC, invalidating all naked oops. + MutexLocker ml(RetData_lock); + + // ProfileData is essentially a wrapper around a derived oop, so we + // need to take the lock before making any ProfileData structures. + ProfileData* data = h_mdo->data_at(h_mdo->dp_to_di(fr.interpreter_frame_mdp())); RetData* rdata = data->as_RetData(); - address new_mdp = rdata->fixup_ret(return_bci, mdo); + address new_mdp = rdata->fixup_ret(return_bci, h_mdo); fr.interpreter_frame_set_mdp(new_mdp); IRT_END src/share/vm/oops/methodDataOop.cpp [1.22 vs. 1.25] @@ -191,13 +191,17 @@ void RetData::post_initialize(BytecodeSt } } - address RetData::fixup_ret(int return_bci, methodDataOop mdo) { - // First find the mdp which corresponds to the return bci. - address mdp = mdo->bci_to_dp(return_bci); + // This routine needs to atomically update the RetData structure, so the + // caller needs to hold the RetData_lock before it gets here. Since taking + // the lock can block (and allow GC) and since RetData is a ProfileData is a + // wrapper around a derived oop, taking the lock in _this_ method will + // basically cause the 'this' pointer's _data field to contain junk after the + // lock. We require the caller to take the lock before making the ProfileData + // structure. Currently the only caller is InterpreterRuntime::update_mdp_for_ret + address RetData::fixup_ret(int return_bci, methodDataHandle h_mdo) { - // Grab a lock to ensure atomic access to setting the return bci and the - // displacement. - MutexLocker ml(RetData_lock); + // First find the mdp which corresponds to the return bci. + address mdp = h_mdo->bci_to_dp(return_bci); // Now check to see if any of the cache slots are open. for (uint row = 0; row < row_limit(); row++) { src/share/vm/oops/methodDataOop.hpp [1.26 vs. 1.27] @@ -592,7 +592,7 @@ public: } // Interpreter Runtime support - address fixup_ret(int return_bci, methodDataOop mdo); + address fixup_ret(int return_bci, methodDataHandle mdo); // Code generation support static ByteSize bci_offset(uint row) {
11-06-2004