RFR: JDK-8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base [v3]

[Martin Doerr]

On Mon, 1 Mar 2021 11:09:03 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> If Compressed class pointer base has a non-zero value it may cause MacroAssembler::encode_klass_not_null() to encode a Klass pointer to a wrong narrow pointer.
>> 
>> This can be reproduced by starting the VM with
>> -Xshare:dump -XX:HeapBaseMinAddress=2g -Xmx128m
>> but CDS is not involved. It is only relevant insofar as this is the only way to get the following combination:
>> - heap is allocated at 0x800_0000. It is small and ends at 0x8800_0000.
>> - class space follows at 0x8800_0000
>> - the narrow klass pointer base points to the start of the class space at 0x8800_0000.
>> 
>> In MacroAssembler::encode_klass_not_null(), there is the following section:
>> 
>>   if (base != NULL) {
>>     unsigned int base_h = ((unsigned long)base)>>32;
>>     unsigned int base_l = (unsigned int)((unsigned long)base);
>>     if ((base_h != 0) && (base_l == 0) && VM_Version::has_HighWordInstr()) {
>>       lgr_if_needed(dst, current);
>>       z_aih(dst, -((int)base_h));     // Base has no set bits in lower half.
>>     } else if ((base_h == 0) && (base_l != 0)) {   (A)
>>       lgr_if_needed(dst, current);                
>>       z_agfi(dst, -(int)base_l);                   (B)
>>     } else {
>>       load_const(Z_R0, base);
>>       lgr_if_needed(dst, current);
>>       z_sgr(dst, Z_R0);
>>     }
>>     current = dst;
>>   }
>> 
>> We enter the condition at (A) if the narrow klass pointer base is non-zero but fits into 32bit. At (B), we want to substract the base from the Klass pointer; we do this by calculating the 32bit twos-complement of the base and add it with AGFI. AGFI adds a 32bit immediate to a 64bit register. In this case, it produces the wrong result if the base is >0x800_0000:
>> 
>> In the case of the crash, we have:
>> base: 				  8800_0000
>> klass pointer: 			  8804_1040
>> 32bit two's complement of base:   7800_0000
>> added to the klass pointer: 	1_0004_1040
>> 
>> So the result of the "substraction" is 1_0004_1040, it should be 4_1040, which would be the correct offset of the Klass* pointer within the ccs.
>> 
>> This bug has been dormant; was activated by JDK-8250989 which changed the way class space reservation happens at CDS dump time. It surfaced first as crash in a CDS-specific jtreg test (JDK-8261552).
>> 
>> ================
>> 
>> Fix:
>> 
>> I changed the AGFI instruction to a pure 32bit add (AFI). That works as long as the Klass pointer also fits into 32bit. So I narrowed the condition at (A) to only fire if it can be ensured that both narrow base and Klass* pointers fit into 32bit.
>> 
>> I also added a runtime verification in that case that any Klass pointer passed down is indeed a 32bit pointer. However, I am not really sure this is useful, or that this is the best way to do this (using TMHH and TMHL). I was looking for something like TMH or TML to check whole 32bit words but could not find any.
>> 
>> ----
>> 
>> Tests:
>> 
>> I manually tested that the crash disappears, which it does. I stepped through the encoding code and the values now look right.
>> 
>> I also did build a VM with the ability to override both class space start address and the narrow klass pointer base to exact values (see https://github.com/openjdk/jdk/compare/master...tstuefe:override-ccs-start-and-base). 
>> 
>> I used this method to test various combinations: 
>> - narrow klass pointer base > 0 < 4g + ccs end < 4g  (we hit our branch doing AFI)
>> - narrow klass pointer base > 0 < 4g + ccs end > 4g  (we hit the fallback doing SGR with r0)
>> - narrow klass pointer base = 0                      (we dont do anything)
>> 
>> (would this override-feature be useful? We could do better testing).
>> 
>> Thanks, Thomas
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update

Marked as reviewed by mdoerr (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2595