RFR: 8133646: Internal Error: x86/vm/macroAssembler_x86.cpp:886 DEBUG MESSAGE: StubRoutines::call_stub: threads must correspond

[David Holmes]

On 26/08/2015 11:04 PM, Daniel D. Daugherty wrote:
>
> On 8/25/15 8:17 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> Thanks for the review.
>>
>> On 26/08/2015 2:28 AM, Coleen Phillimore wrote:
>>>
>>> David,
>>> Can you add this duplicate code into a verify_thread() function? This
>>> would help if we are ever able to combine common code in
>>> stubRoutines_x86_32/64.cpp
>>
>> I'm so grateful you posed that as a question not a request. :) Answer:
>> no.
>>
>> The duplicated structure is pre-existing, and the error messages
>> differ so the context would have to be passed in. I'm totally
>> unfamiliar with playing in the macroassembler so would be unsure how
>> to factor this out into a separate function without messing up
>> register usage, or using the correct "variables" from the "calling
>> context".
>>
>> Also note that for some reason these sanity checks don't exist on
>> x86_32, nor sparc (only aarch64 also has it presumably based on the
>> x86_64 version). I think the time to factor this out would be if/when
>> we do the code combining.
>
> We do have a similar check on x86_32:
>
> src/cpu/x86/vm/stubGenerator_x86_32.cpp:
>
>     343    address generate_catch_exception() {
> <snip>
>     351  #ifdef ASSERT
>     352      // verify that threads correspond
>     353      { Label L;
>     354        __ get_thread(rbx);
>     355        __ cmpptr(rbx, rcx);
>     356        __ jcc(Assembler::equal, L);
>     357        __ stop("StubRoutines::catch_exception: threads must
> correspond");
>     358        __ bind(L);
>     359      }
>     360  #endif
>
> The check is much simpler because we don't have enough registers
> on x86_32 to set aside one for the thread.

Thanks Dan. Yes I misspoke. I had seen this but as it only has one check 
there was no need for any additional diagnostics.

> On SPARC we have this:
>
> src/cpu/sparc/vm/stubGenerator_sparc.cpp:
>
>     322    address generate_catch_exception() {
> <snip>
>     326      // verify that thread corresponds
>     327      __ verify_thread();
>
> There are lots of other calls to verify_thread() sprinkled
> through the SPARC code...
>
> src/cpu/sparc/vm/macroAssembler_sparc.cpp:
>
>     398  void MacroAssembler::verify_thread() {
>     399    if (VerifyThread) {
>     400      // NOTE: this chops off the heads of the 64-bit O registers.
>     401  #ifdef CC_INTERP
>     402      save_frame(0);
>     403  #else
>     404      // make sure G2_thread contains the right value
>     405      save_frame_and_mov(0, Lmethod, Lmethod);   // to avoid
> clobbering O0
>   (and propagate Lmethod for -Xprof)
>     406      mov(G1, L1);                // avoid clobbering G1
>     407      // G2 saved below
>     408      mov(G3, L3);                // avoid clobbering G3
>     409      mov(G4, L4);                // avoid clobbering G4
>     410      mov(G5_method, L5);         // avoid clobbering G5_method
>     411  #endif /* CC_INTERP */
>     412  #if defined(COMPILER2) && !defined(_LP64)
>     413      // Save & restore possible 64-bit Long arguments in G-regs
>     414      srlx(G1,32,L0);
>     415      srlx(G4,32,L6);
>     416  #endif
>     417 call(CAST_FROM_FN_PTR(address,verify_thread_subroutine),
> relocInfo::
> runtime_call_type);
>     418      delayed()->mov(G2_thread, O0);
>     419
>     420      mov(L1, G1);                // Restore G1
>     421      // G2 restored below
>     422      mov(L3, G3);                // restore G3
>     423      mov(L4, G4);                // restore G4
>     424      mov(L5, G5_method);         // restore G5_method
>     425  #if defined(COMPILER2) && !defined(_LP64)
>     426      // Save & restore possible 64-bit Long arguments in G-regs
>     427      sllx(L0,32,G2);             // Move old high G1 bits high
> in G2
>     428      srl(G1, 0,G1);              // Clear current high G1 bits
>     429      or3 (G1,G2,G1);             // Recover 64-bit G1
>     430      sllx(L6,32,G2);             // Move old high G4 bits high
> in G2
>     431      srl(G4, 0,G4);              // Clear current high G4 bits
>     432      or3 (G4,G2,G4);             // Recover 64-bit G4
>     433  #endif
>     434      restore(O0, 0, G2_thread);
>     435    }
>     436  }
>
> Does anyone else have a headache after reading verify_thread()? :-)

I hadn't caught this - so thanks very much. Despite the apparent 
complexity the crux of the above is the call to:

static Thread* verify_thread_subroutine(Thread* gthread_value) {
   Thread* correct_value = ThreadLocalStorage::thread();
   guarantee(gthread_value == correct_value, "G2_thread value must be 
the thread");
   return correct_value;
}

which only does the same single-check that the x32 version does.

So bottom line: I'll push the x64 additions.

Thanks again,
David

> Dan
>
>
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>>> On 8/25/15 2:07 AM, David Holmes wrote:
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8133646
>>>>
>>>> webrev: http://cr.openjdk.java.net/~dholmes/8133646/webrev/
>>>>
>>>> The error raised in this bug was not reproducible - it was only seen
>>>> on one machine which was subsequently re-imaged to run Windows. A
>>>> similar Solaris system did not reproduce the bug.
>>>>
>>>> Rather than just close this as not-reproducible I want to push the
>>>> enhanced diagnostics that Vladimir provided here:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-August/019667.html
>>>>
>>>>
>>>>
>>>> which I've added to the two chunks of code that perform this thread
>>>> sanity check.
>>>>
>>>> Thanks,
>>>> David
>>>
>