Skip to content
This repository has been archived by the owner. It is now read-only.
Permalink
Browse files
8261552: s390: MacroAssembler::encode_klass_not_null() may produce wr…
…ong results for non-zero values of narrow klass base

Co-authored-by: Lutz Schmidt <lucy@openjdk.org>
Reviewed-by: mdoerr, lucy
  • Loading branch information
tstuefe and RealLucy committed Mar 2, 2021
1 parent f5ab7f6 commit fdd109327ce920585e44c550e15c7937a8bed8da
Showing with 66 additions and 17 deletions.
  1. +66 −17 src/hotspot/cpu/s390/macroAssembler_s390.cpp
@@ -3603,6 +3603,7 @@ void MacroAssembler::encode_klass_not_null(Register dst, Register src) {
Register current = (src != noreg) ? src : dst; // Klass is in dst if no src provided. (dst == src) also possible.
address base = CompressedKlassPointers::base();
int shift = CompressedKlassPointers::shift();
bool need_zero_extend = base != 0;
assert(UseCompressedClassPointers, "only for compressed klass ptrs");

BLOCK_COMMENT("cKlass encoder {");
@@ -3619,28 +3620,76 @@ void MacroAssembler::encode_klass_not_null(Register dst, Register src) {
bind(ok);
#endif

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)) {
lgr_if_needed(dst, current);
z_agfi(dst, -(int)base_l);
} else {
load_const(Z_R0, base);
lgr_if_needed(dst, current);
z_sgr(dst, Z_R0);
}
current = dst;
}
// Scale down the incoming klass pointer first.
// We then can be sure we calculate an offset that fits into 32 bit.
// More generally speaking: all subsequent calculations are purely 32-bit.
if (shift != 0) {
assert (LogKlassAlignmentInBytes == shift, "decode alg wrong");
z_srlg(dst, current, shift);
current = dst;
}
lgr_if_needed(dst, current); // Move may be required (if neither base nor shift != 0).

if (base != NULL) {
// Use scaled-down base address parts to match scaled-down klass pointer.
unsigned int base_h = ((unsigned long)base)>>(32+shift);
unsigned int base_l = (unsigned int)(((unsigned long)base)>>shift);

// General considerations:
// - when calculating (current_h - base_h), all digits must cancel (become 0).
// Otherwise, we would end up with a compressed klass pointer which doesn't
// fit into 32-bit.
// - Only bit#33 of the difference could potentially be non-zero. For that
// to happen, (current_l < base_l) must hold. In this case, the subtraction
// will create a borrow out of bit#32, nicely killing bit#33.
// - With the above, we only need to consider current_l and base_l to
// calculate the result.
// - Both values are treated as unsigned. The unsigned subtraction is
// replaced by adding (unsigned) the 2's complement of the subtrahend.

if (base_l == 0) {
// - By theory, the calculation to be performed here (current_h - base_h) MUST
// cancel all high-word bits. Otherwise, we would end up with an offset
// (i.e. compressed klass pointer) that does not fit into 32 bit.
// - current_l remains unchanged.
// - Therefore, we can replace all calculation with just a
// zero-extending load 32 to 64 bit.
// - Even that can be replaced with a conditional load if dst != current.
// (this is a local view. The shift step may have requested zero-extension).
} else {
if ((base_h == 0) && is_uimm(base_l, 31)) {
// If we happen to find that (base_h == 0), and that base_l is within the range
// which can be represented by a signed int, then we can use 64bit signed add with
// (-base_l) as 32bit signed immediate operand. The add will take care of the
// upper 32 bits of the result, saving us the need of an extra zero extension.
// For base_l to be in the required range, it must not have the most significant
// bit (aka sign bit) set.
lgr_if_needed(dst, current); // no zero/sign extension in this case!
z_agfi(dst, -(int)base_l); // base_l must be passed as signed.
need_zero_extend = false;
current = dst;
} else {
// To begin with, we may need to copy and/or zero-extend the register operand.
// We have to calculate (current_l - base_l). Because there is no unsigend
// subtract instruction with immediate operand, we add the 2's complement of base_l.
if (need_zero_extend) {
z_llgfr(dst, current);
need_zero_extend = false;
} else {
llgfr_if_needed(dst, current);
}
current = dst;
z_alfi(dst, -base_l);
}
}
}

if (need_zero_extend) {
// We must zero-extend the calculated result. It may have some leftover bits in
// the hi-word because we only did optimized calculations.
z_llgfr(dst, current);
} else {
llgfr_if_needed(dst, current); // zero-extension while copying comes at no extra cost.
}

BLOCK_COMMENT("} cKlass encoder");
}

0 comments on commit fdd1093

Please sign in to comment.