Skip to content

Commit a56bb11

Browse files
author
Josh Learn
committed
[5.4] Fix OSLogOptimization pass to mark array.finalize_intrinsic as not foldable
(This is a cherry-pick of PR swiftlang#35636 to the release/5.4 branch.) This PR fixes a compiler crash caused due to an assertion failure in the OSLogOptimization pass that happens when optimizing the `os_signpost(.animationBegin, ...)` API introduced in iOS 14 for measuring performance of animations. A combination of recent changes to the compiler in Swift 5.4, including introduction of a new _finalizeUninitializedArray intrinsic, caused the optimization pass to analyze more code than necessary for that API, which caused an invariant assumed by the pass to fail. This change relaxes the assumption and makes the pass take an early exit when the invariants do not hold, which prevents it from crashing in such cases. Resolves rdar://72634574
1 parent 927a2b3 commit a56bb11

File tree

6 files changed

+184
-2
lines changed

6 files changed

+184
-2
lines changed

include/swift/SIL/SILConstants.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -604,6 +604,11 @@ class SymbolicValue {
604604
/// version. This only works for valid constants.
605605
SymbolicValue cloneInto(SymbolicValueAllocator &allocator) const;
606606

607+
/// Check that all nested SymbolicValues are constant. Symbolic values such as arrays,
608+
/// aggregates and pointers can contain non-constant symbolic values, when instructions
609+
/// are skipped during evaluation.
610+
bool containsOnlyConstants() const;
611+
607612
void print(llvm::raw_ostream &os, unsigned indent = 0) const;
608613
void dump() const;
609614
};

lib/SIL/IR/SILConstants.cpp

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,52 @@ SymbolicValue::cloneInto(SymbolicValueAllocator &allocator) const {
260260
llvm_unreachable("covered switch");
261261
}
262262

263+
bool SymbolicValue::containsOnlyConstants() const {
264+
if (!isConstant())
265+
return false;
266+
267+
auto thisRK = representationKind;
268+
switch (thisRK) {
269+
case RK_UninitMemory:
270+
case RK_Unknown:
271+
case RK_Metatype:
272+
case RK_Function:
273+
case RK_Enum:
274+
case RK_IntegerInline:
275+
case RK_Integer:
276+
case RK_String:
277+
case RK_Closure:
278+
return true;
279+
case RK_Aggregate: {
280+
auto elts = getAggregateMembers();
281+
for (auto elt : elts)
282+
if (!elt.containsOnlyConstants())
283+
return false;
284+
return true;
285+
}
286+
case RK_EnumWithPayload: {
287+
return getEnumPayloadValue().containsOnlyConstants();
288+
}
289+
case RK_DirectAddress:
290+
case RK_DerivedAddress: {
291+
auto *memObject = getAddressValueMemoryObject();
292+
return memObject->getValue().containsOnlyConstants();
293+
}
294+
case RK_ArrayStorage: {
295+
CanType elementType;
296+
ArrayRef<SymbolicValue> elts = getStoredElements(elementType);
297+
for (auto elt : elts)
298+
if (!elt.containsOnlyConstants())
299+
return false;
300+
return true;
301+
}
302+
case RK_Array: {
303+
return getStorageOfArray().containsOnlyConstants();
304+
}
305+
}
306+
llvm_unreachable("covered switch");
307+
}
308+
263309
//===----------------------------------------------------------------------===//
264310
// SymbolicValueMemoryObject implementation
265311
//===----------------------------------------------------------------------===//

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,9 @@ static bool isFoldableArray(SILValue value, ASTContext &astContext) {
322322
return true;
323323
SILFunction *callee = cast<ApplyInst>(constructorInst)->getCalleeFunction();
324324
return !callee ||
325-
(!callee->hasSemanticsAttr("array.init.empty") &&
326-
!callee->hasSemanticsAttr("array.uninitialized_intrinsic"));
325+
(!callee->hasSemanticsAttr(semantics::ARRAY_INIT_EMPTY) &&
326+
!callee->hasSemanticsAttr(semantics::ARRAY_UNINITIALIZED_INTRINSIC) &&
327+
!callee->hasSemanticsAttr(semantics::ARRAY_FINALIZE_INTRINSIC));
327328
}
328329

329330
/// Return true iff the given value is a closure but is not a creation of a
@@ -997,6 +998,14 @@ static void substituteConstants(FoldState &foldState) {
997998
for (SILValue constantSILValue : foldState.getConstantSILValues()) {
998999
SymbolicValue constantSymbolicVal =
9991000
evaluator.lookupConstValue(constantSILValue).getValue();
1001+
// Make sure that the symbolic value tracked in the foldState is a constant.
1002+
// In the case of ArraySymbolicValue, the array storage could be a non-constant
1003+
// if some instruction in the array initialization sequence was not evaluated
1004+
// and skipped.
1005+
if (!constantSymbolicVal.containsOnlyConstants()) {
1006+
assert(constantSymbolicVal.getKind() != SymbolicValue::String && "encountered non-constant string symbolic value");
1007+
continue;
1008+
}
10001009

10011010
SILInstruction *definingInst = constantSILValue->getDefiningInstruction();
10021011
assert(definingInst);

stdlib/private/OSLog/OSLogTestHelper.swift

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,55 @@ internal func _os_log_impl_test(
113113
start: UnsafePointer(bufferMemory),
114114
count: Int(bufferSize)))
115115
}
116+
117+
118+
119+
/// A function that pretends to be os_signpost(.animationBegin, ...). The purpose
120+
/// of this function is to test whether the OSLogOptimization pass works properly
121+
/// on the special case of animation begin signposts.
122+
@_transparent
123+
public func _osSignpostAnimationBeginTestHelper(
124+
_ format: AnimationFormatString.OSLogMessage,
125+
_ arguments: CVarArg...
126+
) {
127+
_animationBeginSignpostHelper(formatStringPointer: format.formatStringPointer,
128+
arguments: arguments)
129+
}
130+
131+
@usableFromInline
132+
internal func _animationBeginSignpostHelper(
133+
formatStringPointer: UnsafePointer<CChar>,
134+
arguments: [CVarArg]
135+
) {}
136+
137+
// A namespace for utilities specific to os_signpost animation tests.
138+
public enum AnimationFormatString {
139+
@inlinable
140+
@_optimize(none)
141+
@_semantics("constant_evaluable")
142+
internal static func constructOSLogInterpolation(
143+
_ formatString: String
144+
) -> OSLogInterpolation {
145+
var s = OSLogInterpolation(literalCapacity: 1, interpolationCount: 0)
146+
s.formatString += formatString
147+
s.formatString += " isAnimation=YES"
148+
return s
149+
}
150+
151+
@frozen
152+
public struct OSLogMessage : ExpressibleByStringLiteral {
153+
@usableFromInline
154+
var formatStringPointer: UnsafePointer<CChar>
155+
156+
@_transparent
157+
public init(stringLiteral value: String) {
158+
let message =
159+
OSLogTestHelper.OSLogMessage(
160+
stringInterpolation:
161+
constructOSLogInterpolation(
162+
value))
163+
let formatString = message.interpolation.formatString
164+
formatStringPointer = _getGlobalStringTablePointer(formatString)
165+
}
166+
}
167+
}

test/SILOptimizer/OSLogMandatoryOptTest.sil

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,3 +1055,59 @@ bb0:
10551055
// CHECK: bb0
10561056
// CHECK: alloc_stack $OSLogInterpolationDCEStub
10571057
}
1058+
1059+
// The following tests are for checking that constant folding doesn't
1060+
// cause a crash when evaulating the instructions of an animation signpost.
1061+
1062+
// Protocol stub for CVarArgStub
1063+
protocol CVarArgStub {}
1064+
1065+
// Ensure that Int conforms to our protocol stub
1066+
extension Int64 : CVarArgStub {}
1067+
1068+
// _finalizeUninitializedArray<A>(_:)
1069+
sil shared_external [serialized] [_semantics "array.finalize_intrinsic"] @$ss27_finalizeUninitializedArrayySayxGABnlF : $@convention(thin) <Element> (@owned Array<Element>) -> @owned Array<Element>
1070+
1071+
// Test that the OSLogOptimization does not fail while attempting to
1072+
// fold CVarArgStubs in animation signposts.
1073+
// CHECK-LABEL: @testFoldingOfCVarArgStubConstruction
1074+
sil [ossa] @testFoldingOfCVarArgStubConstruction : $@convention(thin) () -> () {
1075+
bb0:
1076+
// Construct an OSLogMessageStub instance.
1077+
%0 = string_literal utf8 "animation begins here %d"
1078+
%1 = integer_literal $Builtin.Word, 24
1079+
%2 = integer_literal $Builtin.Int1, -1
1080+
%3 = metatype $@thin String.Type
1081+
// function_ref String.init(_builtinStringLiteral:utf8CodeUnitCount:isASCII:)
1082+
%4 = function_ref @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
1083+
%5 = apply %4(%0, %1, %2, %3) : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String // users: %60, %47
1084+
%6 = function_ref @oslogMessageInit : $@convention(thin) (@owned String) -> @owned OSLogMessageStub
1085+
%7 = apply %6(%5) : $@convention(thin) (@owned String) -> @owned OSLogMessageStub
1086+
1087+
// Begin chain of evaluated instructions
1088+
%8 = begin_borrow %7 : $OSLogMessageStub
1089+
1090+
// Construct CVarArgStub
1091+
%11 = integer_literal $Builtin.Word, 1
1092+
// function_ref _allocateUninitializedArray<A>(_:)
1093+
%12 = function_ref @$ss27_allocateUninitializedArrayySayxG_BptBwlF : $@convention(thin) <τ_0_0> (Builtin.Word) -> (@owned Array<τ_0_0>, Builtin.RawPointer)
1094+
%13 = apply %12<CVarArgStub>(%11) : $@convention(thin) <τ_0_0> (Builtin.Word) -> (@owned Array<τ_0_0>, Builtin.RawPointer)
1095+
(%14, %15) = destructure_tuple %13 : $(Array<CVarArgStub>, Builtin.RawPointer)
1096+
%16 = pointer_to_address %15 : $Builtin.RawPointer to [strict] $*CVarArgStub
1097+
%17 = integer_literal $Builtin.IntLiteral, 42
1098+
%18 = builtin "s_to_s_checked_trunc_IntLiteral_Int64"(%17 : $Builtin.IntLiteral) : $(Builtin.Int64, Builtin.Int1)
1099+
(%19, %20) = destructure_tuple %18 : $(Builtin.Int64, Builtin.Int1)
1100+
%21 = struct $Int64 (%19 : $Builtin.Int64)
1101+
%22 = init_existential_addr %16 : $*CVarArgStub, $Int64
1102+
store %21 to [trivial] %22 : $*Int64
1103+
// function_ref _finalizeUninitializedArray<A>(_:)
1104+
%23 = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF : $@convention(thin) <τ_0_0> (@owned Array<τ_0_0>) -> @owned Array<τ_0_0>
1105+
%24 = apply %23<CVarArgStub>(%14) : $@convention(thin) <τ_0_0> (@owned Array<τ_0_0>) -> @owned Array<τ_0_0>
1106+
destroy_value %24 : $Array<CVarArgStub>
1107+
1108+
// End chain of evaluated instructions
1109+
end_borrow %8 : $OSLogMessageStub
1110+
destroy_value %7 : $OSLogMessageStub
1111+
%25 = tuple ()
1112+
return %25 : $()
1113+
}

test/SILOptimizer/OSLogMandatoryOptTest.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,3 +623,17 @@ func testWrappingWithinClosures(x: Int) {
623623
// CHECK-LABEL: end sil function '${{.*}}testWrappingWithinClosures1xySi_tFyyXEfU_
624624
}
625625
}
626+
627+
func testAnimationSignpost(cond: Bool, x: Int, y: Float) {
628+
_osSignpostAnimationBeginTestHelper("animation begins here %d", Int.min)
629+
_osSignpostAnimationBeginTestHelper("a message without arguments")
630+
_osSignpostAnimationBeginTestHelper("animation begins here %ld", x)
631+
_osSignpostAnimationBeginTestHelper("animation begins here %ld", cond ? x : y)
632+
_osSignpostAnimationBeginTestHelper("animation begins here %ld %f", x, y)
633+
// CHECK-LABEL: @${{.*}}testAnimationSignpost4cond1x1yySb_SiSftF
634+
// CHECK: string_literal utf8 "animation begins here %d isAnimation=YES"
635+
// CHECK: string_literal utf8 "a message without arguments isAnimation=YES"
636+
// CHECK: string_literal utf8 "animation begins here %ld isAnimation=YES"
637+
// CHECK: string_literal utf8 "animation begins here %ld isAnimation=YES"
638+
// CHECK: string_literal utf8 "animation begins here %ld %f isAnimation=YES"
639+
}

0 commit comments

Comments
 (0)