Skip to content

Commit 31b86f1

Browse files
eecksteineeckstein
committed
ConstantFolding: remove a wrong peephole optimization for signed "< 0" and ">= 0" comparisons
The wrong optimization was: fold x < 0 into false, if x is known to be a result of an unsigned operation with overflow checks enabled. It was done under the wrong assumption that the result of an overflow-checked unsigned operation fits into a signed integer and is positive. This is wrong, because the result of an unsigned operation can be larger than Int.max and therefore, when used in a signed integer operation, be re-interpreted as a negative signed value. Fixes a miscompile which resulted in a missing abort on arithmetic overflow. rdar://73596890
1 parent 21421b4 commit 31b86f1

File tree

4 files changed

+18
-82
lines changed

4 files changed

+18
-82
lines changed

‎lib/SILOptimizer/Utils/ConstantFolding.cpp‎

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -476,48 +476,6 @@ static SILValue constantFoldCompare(BuiltinInst *BI, BuiltinValueKind ID) {
476476
return B.createIntegerLiteral(BI->getLoc(), BI->getType(), APInt());
477477
}
478478
}
479-
480-
// Fold x < 0 into false, if x is known to be a result of an unsigned
481-
// operation with overflow checks enabled.
482-
BuiltinInst *BIOp;
483-
if (match(BI, m_BuiltinInst(BuiltinValueKind::ICMP_SLT,
484-
m_TupleExtractOperation(m_BuiltinInst(BIOp), 0),
485-
m_Zero()))) {
486-
// Check if Other is a result of an unsigned operation with overflow.
487-
switch (BIOp->getBuiltinInfo().ID) {
488-
default:
489-
break;
490-
case BuiltinValueKind::UAddOver:
491-
case BuiltinValueKind::USubOver:
492-
case BuiltinValueKind::UMulOver:
493-
// Was it an operation with an overflow check?
494-
if (match(BIOp->getOperand(2), m_One())) {
495-
SILBuilderWithScope B(BI);
496-
return B.createIntegerLiteral(BI->getLoc(), BI->getType(), APInt());
497-
}
498-
}
499-
}
500-
501-
// Fold x >= 0 into true, if x is known to be a result of an unsigned
502-
// operation with overflow checks enabled.
503-
if (match(BI, m_BuiltinInst(BuiltinValueKind::ICMP_SGE,
504-
m_TupleExtractOperation(m_BuiltinInst(BIOp), 0),
505-
m_Zero()))) {
506-
// Check if Other is a result of an unsigned operation with overflow.
507-
switch (BIOp->getBuiltinInfo().ID) {
508-
default:
509-
break;
510-
case BuiltinValueKind::UAddOver:
511-
case BuiltinValueKind::USubOver:
512-
case BuiltinValueKind::UMulOver:
513-
// Was it an operation with an overflow check?
514-
if (match(BIOp->getOperand(2), m_One())) {
515-
SILBuilderWithScope B(BI);
516-
return B.createIntegerLiteral(BI->getLoc(), BI->getType(), APInt(1, 1));
517-
}
518-
}
519-
}
520-
521479
return nullptr;
522480
}
523481

‎test/SILOptimizer/constant_propagation.sil‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -470,10 +470,7 @@ bb0:
470470
// CHECK-NEXT: return %4 : $Builtin.Int64
471471
}
472472

473-
// Fold x < 0 into false, if x is known to be a result of an unsigned
474-
// operation with overflow checks enabled.
475-
// At the same time x >= 0 is always true under the same conditions.
476-
sil @fold_unsigned_op_with_overflow_lt_zero : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> Builtin.Int64 {
473+
sil @dont_fold_unsigned_op_with_overflow_lt_zero : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> Builtin.Int64 {
477474
bb0(%0 : $Builtin.Int64, %1 : $Builtin.Int64):
478475
%zero = integer_literal $Builtin.Int64, 0
479476
%2 = integer_literal $Builtin.Int1, -1
@@ -503,17 +500,10 @@ bb0(%0 : $Builtin.Int64, %1 : $Builtin.Int64):
503500

504501
return %uadd_with_overflow_result : $Builtin.Int64
505502

506-
// CHECK-LABEL: sil @fold_unsigned_op_with_overflow_lt_zero
503+
// CHECK-LABEL: sil @dont_fold_unsigned_op_with_overflow_lt_zero
507504
// CHECK: builtin "uadd_with_overflow_Int64"
508-
// CHECK: integer_literal $Builtin.Int1, 0
509-
// CHECK: integer_literal $Builtin.Int1, -1
510-
// CHECK: builtin "usub_with_overflow_Int64"
511-
// CHECK: integer_literal $Builtin.Int1, 0
512-
// CHECK: integer_literal $Builtin.Int1, -1
513-
// CHECK: builtin "umul_with_overflow_Int64"
514-
// CHECK: integer_literal $Builtin.Int1, 0
515-
// CHECK: integer_literal $Builtin.Int1, -1
516-
// CHECK-NEXT: return {{.*}}$Builtin.Int64
505+
// CHECK-NOT: integer_literal
506+
// CHECK: } // end sil function 'dont_fold_unsigned_op_with_overflow_lt_zero'
517507
}
518508

519509

‎test/SILOptimizer/constant_propagation_ownership.sil‎

Lines changed: 8 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -539,22 +539,11 @@ bb0:
539539
// CHECK-NEXT: return %4 : $Builtin.Int64
540540
}
541541

542-
// Fold x < 0 into false, if x is known to be a result of an unsigned
543-
// operation with overflow checks enabled.
544-
// At the same time x >= 0 is always true under the same conditions.
545-
//
546-
// CHECK-LABEL: sil [ossa] @fold_unsigned_op_with_overflow_lt_zero :
542+
// CHECK-LABEL: sil [ossa] @dont_fold_unsigned_op_with_overflow_lt_zero :
547543
// CHECK: builtin "uadd_with_overflow_Int64"
548-
// CHECK: integer_literal $Builtin.Int1, 0
549-
// CHECK: integer_literal $Builtin.Int1, -1
550-
// CHECK: builtin "usub_with_overflow_Int64"
551-
// CHECK: integer_literal $Builtin.Int1, 0
552-
// CHECK: integer_literal $Builtin.Int1, -1
553-
// CHECK: builtin "umul_with_overflow_Int64"
554-
// CHECK: integer_literal $Builtin.Int1, 0
555-
// CHECK: integer_literal $Builtin.Int1, -1
556-
// CHECK-NEXT: return {{.*}}$Builtin.Int64
557-
sil [ossa] @fold_unsigned_op_with_overflow_lt_zero : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> Builtin.Int64 {
544+
// CHECK-NOT: integer_literal
545+
// CHECK: } // end sil function 'dont_fold_unsigned_op_with_overflow_lt_zero'
546+
sil [ossa] @dont_fold_unsigned_op_with_overflow_lt_zero : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> Builtin.Int64 {
558547
bb0(%0 : $Builtin.Int64, %1 : $Builtin.Int64):
559548
%zero = integer_literal $Builtin.Int64, 0
560549
%2 = integer_literal $Builtin.Int1, -1
@@ -585,18 +574,11 @@ bb0(%0 : $Builtin.Int64, %1 : $Builtin.Int64):
585574
return %uadd_with_overflow_result : $Builtin.Int64
586575
}
587576

588-
// CHECK-LABEL: sil [ossa] @fold_unsigned_op_with_overflow_lt_zero_destructure :
577+
// CHECK-LABEL: sil [ossa] @dont_fold_unsigned_op_with_overflow_lt_zero_destructure :
589578
// CHECK: builtin "uadd_with_overflow_Int64"
590-
// CHECK: integer_literal $Builtin.Int1, 0
591-
// CHECK: integer_literal $Builtin.Int1, -1
592-
// CHECK: builtin "usub_with_overflow_Int64"
593-
// CHECK: integer_literal $Builtin.Int1, 0
594-
// CHECK: integer_literal $Builtin.Int1, -1
595-
// CHECK: builtin "umul_with_overflow_Int64"
596-
// CHECK: integer_literal $Builtin.Int1, 0
597-
// CHECK: integer_literal $Builtin.Int1, -1
598-
// CHECK-NEXT: return {{.*}}$Builtin.Int64
599-
sil [ossa] @fold_unsigned_op_with_overflow_lt_zero_destructure : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> Builtin.Int64 {
579+
// CHECK-NOT: integer_literal
580+
// CHECK: } // end sil function 'dont_fold_unsigned_op_with_overflow_lt_zero_destructure'
581+
sil [ossa] @dont_fold_unsigned_op_with_overflow_lt_zero_destructure : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> Builtin.Int64 {
600582
bb0(%0 : $Builtin.Int64, %1 : $Builtin.Int64):
601583
%zero = integer_literal $Builtin.Int64, 0
602584
%2 = integer_literal $Builtin.Int1, -1

‎test/stdlib/RangeTraps.swift‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ RangeTraps.test("throughNaN")
116116
_ = ...Double.nan
117117
}
118118

119+
RangeTraps.test("UIntOverflow")
120+
.code {
121+
expectCrashLater()
122+
_blackHole((0 ..< UInt.max).count)
123+
}
124+
119125
if #available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *) {
120126
// Debug check was introduced in https://github.com/apple/swift/pull/34961
121127
RangeTraps.test("UncheckedHalfOpen")

0 commit comments

Comments
 (0)