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

[Thomas Stuefe]

> 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 two additional commits since the last revision:

 - Lucys proposal
 - Revert first attempt

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2595/files
  - new: https://git.openjdk.java.net/jdk/pull/2595/files/07e83dfb..e096f09c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2595&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2595&range=00-01

  Stats: 84 lines in 3 files changed: 28 ins; 28 del; 28 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2595.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2595/head:pull/2595

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