Skip to content

Commit 0ca0852

Browse files
eme64eme64
committed
8370459: C2: CompressBitsNode::Value produces wrong result on Windows (1UL vs 1ULL), found by ExpressionFuzzer
Reviewed-by: dlong, jbhateja, thartmann
1 parent 4cfabd6 commit 0ca0852

File tree

2 files changed

+111
-15
lines changed

2 files changed

+111
-15
lines changed

‎src/hotspot/share/opto/intrinsicnode.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ static const Type* bitshuffle_value(const TypeInteger* src_type, const TypeInteg
273273
// result.lo = 0
274274
if (maskcon != -1L) {
275275
int bitcount = population_count(static_cast<julong>(bt == T_INT ? maskcon & 0xFFFFFFFFL : maskcon));
276-
hi = (1UL << bitcount) - 1;
276+
hi = right_n_bits_typed<jlong>(bitcount);
277277
lo = 0L;
278278
} else {
279279
// preserve originally assigned hi (MAX_INT/LONG) and lo (MIN_INT/LONG) values
@@ -376,7 +376,7 @@ static const Type* bitshuffle_value(const TypeInteger* src_type, const TypeInteg
376376
// Rule 3:
377377
// We can further constrain the upper bound of bit compression if the number of bits
378378
// which can be set(one) is less than the maximum number of bits of integral type.
379-
hi = MIN2((jlong)((1UL << result_bit_width) - 1L), hi);
379+
hi = MIN2(right_n_bits_typed<jlong>(result_bit_width), hi);
380380
}
381381
} else {
382382
assert(opc == Op_ExpandBits, "");

‎test/hotspot/jtreg/compiler/c2/gvn/TestBitCompressValueTransform.java‎

Lines changed: 109 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
/*
2525
* @test
26-
* @bug 8350896
26+
* @bug 8350896 8370459
2727
* @library /test/lib /
2828
* @summary C2: wrong result: Integer/Long.compress gets wrong type from CompressBitsNode::Value.
2929
* @run driver compiler.c2.gvn.TestBitCompressValueTransform
@@ -62,11 +62,15 @@ public class TestBitCompressValueTransform {
6262
public final long LIMIT_L7 = GEN_L.next();
6363
public final long LIMIT_L8 = GEN_L.next();
6464

65-
public final int BOUND_LO_I = GEN_I.next();
66-
public final int BOUND_HI_I = GEN_I.next();
65+
public final int BOUND1_LO_I = GEN_I.next();
66+
public final int BOUND2_LO_I = GEN_I.next();
67+
public final int BOUND1_HI_I = GEN_I.next();
68+
public final int BOUND2_HI_I = GEN_I.next();
6769

68-
public final long BOUND_LO_L = GEN_L.next();
69-
public final long BOUND_HI_L = GEN_L.next();
70+
public final long BOUND1_LO_L = GEN_L.next();
71+
public final long BOUND2_LO_L = GEN_L.next();
72+
public final long BOUND1_HI_L = GEN_L.next();
73+
public final long BOUND2_HI_L = GEN_L.next();
7074

7175
@Test
7276
@IR (counts = { IRNode.COMPRESS_BITS, " >0 " }, applyIfCPUFeature = { "bmi2", "true" })
@@ -327,7 +331,8 @@ public void run15() {
327331

328332
@DontCompile
329333
public int test16_interpreted(int src, int mask) {
330-
src = Math.max(BOUND_LO_I, Math.min(src, BOUND_HI_I));
334+
src = Math.max(BOUND1_LO_I, Math.min(src, BOUND1_HI_I));
335+
mask = Math.max(BOUND2_LO_I, Math.min(mask, BOUND2_HI_I));
331336
int res = Integer.compress(src, mask);
332337

333338
if (res > LIMIT_I1) {
@@ -360,7 +365,8 @@ public int test16_interpreted(int src, int mask) {
360365
@Test
361366
@IR (counts = { IRNode.COMPRESS_BITS, " >0 " }, applyIfCPUFeature = {"bmi2" , "true"})
362367
public int test16(int src, int mask) {
363-
src = Math.max(BOUND_LO_I, Math.min(src, BOUND_HI_I));
368+
src = Math.max(BOUND1_LO_I, Math.min(src, BOUND1_HI_I));
369+
mask = Math.max(BOUND2_LO_I, Math.min(mask, BOUND2_HI_I));
364370
int res = Integer.compress(src, mask);
365371

366372
// Check the result with some random value ranges, if any of the
@@ -411,7 +417,8 @@ public void run16() {
411417

412418
@DontCompile
413419
public int test17_interpreted(int src, int mask) {
414-
src = Math.max(BOUND_LO_I, Math.min(src, BOUND_HI_I));
420+
src = Math.max(BOUND1_LO_I, Math.min(src, BOUND1_HI_I));
421+
mask = Math.max(BOUND2_LO_I, Math.min(mask, BOUND2_HI_I));
415422
int res = Integer.expand(src, mask);
416423

417424
if (res > LIMIT_I1) {
@@ -444,7 +451,8 @@ public int test17_interpreted(int src, int mask) {
444451
@Test
445452
@IR (counts = { IRNode.EXPAND_BITS, " >0 " }, applyIfCPUFeature = {"bmi2" , "true"})
446453
public int test17(int src, int mask) {
447-
src = Math.max(BOUND_LO_I, Math.min(src, BOUND_HI_I));
454+
src = Math.max(BOUND1_LO_I, Math.min(src, BOUND1_HI_I));
455+
mask = Math.max(BOUND2_LO_I, Math.min(mask, BOUND2_HI_I));
448456
int res = Integer.expand(src, mask);
449457

450458
// Check the result with some random value ranges, if any of the
@@ -495,7 +503,8 @@ public void run17() {
495503

496504
@DontCompile
497505
public long test18_interpreted(long src, long mask) {
498-
src = Math.max(BOUND_LO_L, Math.min(src, BOUND_HI_L));
506+
src = Math.max(BOUND1_LO_L, Math.min(src, BOUND1_HI_L));
507+
mask = Math.max(BOUND2_LO_L, Math.min(mask, BOUND2_HI_L));
499508
long res = Long.compress(src, mask);
500509

501510
if (res > LIMIT_L1) {
@@ -528,7 +537,8 @@ public long test18_interpreted(long src, long mask) {
528537
@Test
529538
@IR (counts = { IRNode.COMPRESS_BITS, " >0 " }, applyIfCPUFeature = {"bmi2" , "true"})
530539
public long test18(long src, long mask) {
531-
src = Math.max(BOUND_LO_L, Math.min(src, BOUND_HI_L));
540+
src = Math.max(BOUND1_LO_L, Math.min(src, BOUND1_HI_L));
541+
mask = Math.max(BOUND2_LO_L, Math.min(mask, BOUND2_HI_L));
532542
long res = Long.compress(src, mask);
533543

534544
// Check the result with some random value ranges, if any of the
@@ -579,7 +589,8 @@ public void run18() {
579589

580590
@DontCompile
581591
public long test19_interpreted(long src, long mask) {
582-
src = Math.max(BOUND_LO_L, Math.min(src, BOUND_HI_L));
592+
src = Math.max(BOUND1_LO_L, Math.min(src, BOUND1_HI_L));
593+
mask = Math.max(BOUND2_LO_L, Math.min(mask, BOUND2_HI_L));
583594
long res = Long.expand(src, mask);
584595

585596
if (res > LIMIT_L1) {
@@ -612,7 +623,8 @@ public long test19_interpreted(long src, long mask) {
612623
@Test
613624
@IR (counts = { IRNode.EXPAND_BITS, " >0 " }, applyIfCPUFeature = {"bmi2" , "true"})
614625
public long test19(long src, long mask) {
615-
src = Math.max(BOUND_LO_L, Math.min(src, BOUND_HI_L));
626+
src = Math.max(BOUND1_LO_L, Math.min(src, BOUND1_HI_L));
627+
mask = Math.max(BOUND2_LO_L, Math.min(mask, BOUND2_HI_L));
616628
long res = Long.expand(src, mask);
617629

618630
// Check the result with some random value ranges, if any of the
@@ -661,6 +673,90 @@ public void run19() {
661673
Asserts.assertEQ(actual, expected);
662674
}
663675

676+
@Test
677+
@IR (counts = { IRNode.COMPRESS_BITS, " >0 " }, applyIfCPUFeature = { "bmi2", "true" })
678+
public static long test20(int x) {
679+
// Analysis of when this is used to produce wrong results on Windows:
680+
//
681+
// src = -2683206580L = ffff_ffff_6011_844c
682+
// mask = 0..maxuint, at runtime: 4294950911 = 0xffff_bfff
683+
//
684+
// Hence we go to the B) case of CompressBits in bitshuffle_value
685+
//
686+
// mask_bit_width = 64
687+
// clz = 32
688+
// result_bit_width = 32
689+
//
690+
// So we have result_bit_width < mask_bit_width
691+
//
692+
// And we do:
693+
// lo = result_bit_width == mask_bit_width ? lo : 0L;
694+
// -> lo = 0
695+
//
696+
// And we do:
697+
// hi = MIN2((jlong)((1UL << result_bit_width) - 1L), hi);
698+
//
699+
// But watch out: on windows 1UL is only a 32 bit value. Intended was probably 1ULL.
700+
// So when we calculate "1UL << 32", we just get 1. And so then hi would be 0 now.
701+
// If we instead did "1ULL << 32", we would get 0x1_0000_0000, and hi = 0xffff_ffff.
702+
//
703+
// We create type [lo, hi]:
704+
// Windows: [0, 0] -> constant zero
705+
// correct: [0, 0xffff_ffff] -> does not constant fold. At runtime: 0x3008_c44c
706+
return Long.compress(-2683206580L, Integer.toUnsignedLong(x));
707+
}
708+
709+
@DontCompile
710+
public static long test20_interpreted(int x) {
711+
return Long.compress(-2683206580L, Integer.toUnsignedLong(x));
712+
}
713+
714+
@Run (test = "test20")
715+
public void run20() {
716+
for (int i = 0; i < 100; i++) {
717+
int arg = GEN_I.next();
718+
719+
long actual = test20(arg);
720+
long expected = test20_interpreted(arg);
721+
Asserts.assertEQ(actual, expected);
722+
}
723+
}
724+
725+
@Test
726+
@IR (counts = { IRNode.COMPRESS_BITS, " >0 " }, applyIfCPUFeature = { "bmi2", "true" })
727+
public static long test21(long x) {
728+
// Analysis of when this is used to produce wrong results on Windows:
729+
//
730+
// Very similar to case in test20, but this time we go into the A) case.
731+
//
732+
// maskcon = 0xffff_ffff
733+
// bitcount = 32
734+
//
735+
// And now the problematic part:
736+
// hi = (1UL << bitcount) - 1;
737+
//
738+
// On Windows, this becomes 0 (but it should be 0xffff_ffff).
739+
// Hence, the range wrongly collapses to [0, 0], and the CompressBits node
740+
// is wrongly replaced with a zero constant.
741+
return Long.compress(x, 0xffff_ffffL);
742+
}
743+
744+
@DontCompile
745+
public static long test21_interpreted(long x) {
746+
return Long.compress(x, 0xffff_ffffL);
747+
}
748+
749+
@Run (test = "test21")
750+
public void run21() {
751+
for (int i = 0; i < 100; i++) {
752+
int arg = GEN_I.next();
753+
754+
long actual = test21(arg);
755+
long expected = test21_interpreted(arg);
756+
Asserts.assertEQ(actual, expected);
757+
}
758+
}
759+
664760
public static void main(String[] args) {
665761
TestFramework.run(TestBitCompressValueTransform.class);
666762
}

0 commit comments

Comments
 (0)