Merge pull request swiftlang#35636 from guitard0g/oslog_opt_animation_begin

[os_log][SIL Optimizer] Modify OSLogOptimization pass to skip folding on evaluation failure of non-strings

Author: Josh Learn

include/swift/SIL/SILConstants.h

@@ -604,6 +604,11 @@ class SymbolicValue {
   /// version. This only works for valid constants.
   SymbolicValue cloneInto(SymbolicValueAllocator &allocator) const;
 
+  /// Check that all nested SymbolicValues are constant. Symbolic values such as arrays,
+  /// aggregates and pointers can contain non-constant symbolic values, when instructions
+  /// are skipped during evaluation.
+  bool containsOnlyConstants() const;
+
   void print(llvm::raw_ostream &os, unsigned indent = 0) const;
   void dump() const;
 };

lib/SIL/IR/SILConstants.cpp

@@ -260,6 +260,52 @@ SymbolicValue::cloneInto(SymbolicValueAllocator &allocator) const {
   llvm_unreachable("covered switch");
 }
 
+bool SymbolicValue::containsOnlyConstants() const {
+  if (!isConstant())
+    return false;
+
+  auto thisRK = representationKind;
+  switch (thisRK) {
+  case RK_UninitMemory:
+  case RK_Unknown:
+  case RK_Metatype:
+  case RK_Function:
+  case RK_Enum:
+  case RK_IntegerInline:
+  case RK_Integer:
+  case RK_String:
+  case RK_Closure:
+    return true;
+  case RK_Aggregate: {
+    auto elts = getAggregateMembers();
+    for (auto elt : elts)
+      if (!elt.containsOnlyConstants())
+        return false;
+    return true;
+  }
+  case RK_EnumWithPayload: {
+    return getEnumPayloadValue().containsOnlyConstants();
+  }
+  case RK_DirectAddress:
+  case RK_DerivedAddress: {
+    auto *memObject = getAddressValueMemoryObject();
+    return memObject->getValue().containsOnlyConstants();
+  }
+  case RK_ArrayStorage: {
+    CanType elementType;
+    ArrayRef<SymbolicValue> elts = getStoredElements(elementType);
+    for (auto elt : elts)
+      if (!elt.containsOnlyConstants())
+        return false;
+    return true;
+  }
+  case RK_Array: {
+    return getStorageOfArray().containsOnlyConstants();
+  }
+  }
+  llvm_unreachable("covered switch");
+}
+
 //===----------------------------------------------------------------------===//
 // SymbolicValueMemoryObject implementation
 //===----------------------------------------------------------------------===//

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -323,8 +323,9 @@ static bool isFoldableArray(SILValue value, ASTContext &astContext) {
     return true;
   SILFunction *callee = cast<ApplyInst>(constructorInst)->getCalleeFunction();
   return !callee ||
-         (!callee->hasSemanticsAttr("array.init.empty") &&
-          !callee->hasSemanticsAttr("array.uninitialized_intrinsic"));
+         (!callee->hasSemanticsAttr(semantics::ARRAY_INIT_EMPTY) &&
+          !callee->hasSemanticsAttr(semantics::ARRAY_UNINITIALIZED_INTRINSIC) &&
+          !callee->hasSemanticsAttr(semantics::ARRAY_FINALIZE_INTRINSIC));
 }
 
 /// 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) {
   for (SILValue constantSILValue : foldState.getConstantSILValues()) {
     SymbolicValue constantSymbolicVal =
         evaluator.lookupConstValue(constantSILValue).getValue();
+    // Make sure that the symbolic value tracked in the foldState is a constant.
+    // In the case of ArraySymbolicValue, the array storage could be a non-constant
+    // if some instruction in the array initialization sequence was not evaluated
+    // and skipped.
+    if (!constantSymbolicVal.containsOnlyConstants()) {
+      assert(constantSymbolicVal.getKind() != SymbolicValue::String && "encountered non-constant string symbolic value");
+      continue;
+    }
 
     SILInstruction *definingInst = constantSILValue->getDefiningInstruction();
     assert(definingInst);

stdlib/private/OSLog/OSLogTestHelper.swift

@@ -113,3 +113,55 @@ internal func _os_log_impl_test(
       start: UnsafePointer(bufferMemory),
       count: Int(bufferSize)))
 }
+
+
+
+/// A function that pretends to be os_signpost(.animationBegin, ...). The purpose
+/// of this function is to test whether the OSLogOptimization pass works properly
+/// on the special case of animation begin signposts.
+@_transparent
+public func _osSignpostAnimationBeginTestHelper(
+  _ format: AnimationFormatString.OSLogMessage,
+  _ arguments: CVarArg...
+) {
+  _animationBeginSignpostHelper(formatStringPointer: format.formatStringPointer,
+                                arguments: arguments)
+}
+
+@usableFromInline
+internal func _animationBeginSignpostHelper(
+  formatStringPointer: UnsafePointer<CChar>,
+  arguments: [CVarArg]
+) {}
+
+// A namespace for utilities specific to os_signpost animation tests.
+public enum AnimationFormatString {
+  @inlinable
+  @_optimize(none)
+  @_semantics("constant_evaluable")
+  internal static func constructOSLogInterpolation(
+    _ formatString: String
+  ) -> OSLogInterpolation {
+    var s = OSLogInterpolation(literalCapacity: 1, interpolationCount: 0)
+    s.formatString += formatString
+    s.formatString += " isAnimation=YES"
+    return s
+  }
+
+  @frozen
+  public struct OSLogMessage : ExpressibleByStringLiteral {
+    @usableFromInline
+    var formatStringPointer: UnsafePointer<CChar>
+
+    @_transparent
+    public init(stringLiteral value: String) {
+      let message =
+        OSLogTestHelper.OSLogMessage(
+          stringInterpolation:
+            constructOSLogInterpolation(
+              value))
+      let formatString = message.interpolation.formatString
+      formatStringPointer = _getGlobalStringTablePointer(formatString)
+    }
+  }
+}

test/SILOptimizer/OSLogMandatoryOptTest.sil

@@ -1055,3 +1055,59 @@ bb0:
     // CHECK: bb0
     // CHECK: alloc_stack $OSLogInterpolationDCEStub
 }
+
+// The following tests are for checking that constant folding doesn't
+// cause a crash when evaulating the instructions of an animation signpost.
+
+// Protocol stub for CVarArgStub
+protocol CVarArgStub {}
+
+// Ensure that Int conforms to our protocol stub
+extension Int64 : CVarArgStub {}
+
+// _finalizeUninitializedArray<A>(_:)
+sil shared_external [serialized] [_semantics "array.finalize_intrinsic"] @$ss27_finalizeUninitializedArrayySayxGABnlF : $@convention(thin) <Element> (@owned Array<Element>) -> @owned Array<Element>
+
+// Test that the OSLogOptimization does not fail while attempting to
+// fold CVarArgStubs in animation signposts.
+// CHECK-LABEL: @testFoldingOfCVarArgStubConstruction
+sil [ossa] @testFoldingOfCVarArgStubConstruction : $@convention(thin) () -> () {
+bb0:
+  // Construct an OSLogMessageStub instance.
+  %0 = string_literal utf8 "animation begins here %d"
+  %1 = integer_literal $Builtin.Word, 24
+  %2 = integer_literal $Builtin.Int1, -1
+  %3 = metatype $@thin String.Type
+  // function_ref String.init(_builtinStringLiteral:utf8CodeUnitCount:isASCII:)
+  %4 = function_ref @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
+  %5 = apply %4(%0, %1, %2, %3) : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String // users: %60, %47
+  %6 = function_ref @oslogMessageInit : $@convention(thin) (@owned String) -> @owned OSLogMessageStub
+  %7 = apply %6(%5) : $@convention(thin) (@owned String) -> @owned OSLogMessageStub
+
+  // Begin chain of evaluated instructions
+  %8 = begin_borrow %7 : $OSLogMessageStub
+
+  // Construct CVarArgStub
+  %11 = integer_literal $Builtin.Word, 1
+  // function_ref _allocateUninitializedArray<A>(_:)
+  %12 = function_ref @$ss27_allocateUninitializedArrayySayxG_BptBwlF : $@convention(thin) <τ_0_0> (Builtin.Word) -> (@owned Array<τ_0_0>, Builtin.RawPointer)
+  %13 = apply %12<CVarArgStub>(%11) : $@convention(thin) <τ_0_0> (Builtin.Word) -> (@owned Array<τ_0_0>, Builtin.RawPointer)
+  (%14, %15) = destructure_tuple %13 : $(Array<CVarArgStub>, Builtin.RawPointer)
+  %16 = pointer_to_address %15 : $Builtin.RawPointer to [strict] $*CVarArgStub
+  %17 = integer_literal $Builtin.IntLiteral, 42
+  %18 = builtin "s_to_s_checked_trunc_IntLiteral_Int64"(%17 : $Builtin.IntLiteral) : $(Builtin.Int64, Builtin.Int1)
+  (%19, %20) = destructure_tuple %18 : $(Builtin.Int64, Builtin.Int1)
+  %21 = struct $Int64 (%19 : $Builtin.Int64)
+  %22 = init_existential_addr %16 : $*CVarArgStub, $Int64
+  store %21 to [trivial] %22 : $*Int64
+  // function_ref _finalizeUninitializedArray<A>(_:)
+  %23 = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF : $@convention(thin) <τ_0_0> (@owned Array<τ_0_0>) -> @owned Array<τ_0_0>
+  %24 = apply %23<CVarArgStub>(%14) : $@convention(thin) <τ_0_0> (@owned Array<τ_0_0>) -> @owned Array<τ_0_0>
+  destroy_value %24 : $Array<CVarArgStub>
+
+  // End chain of evaluated instructions
+  end_borrow %8 : $OSLogMessageStub
+  destroy_value %7 : $OSLogMessageStub
+  %25 = tuple ()
+  return %25 : $()
+}

test/SILOptimizer/OSLogMandatoryOptTest.swift

@@ -623,3 +623,17 @@ func testWrappingWithinClosures(x: Int) {
       // CHECK-LABEL: end sil function '${{.*}}testWrappingWithinClosures1xySi_tFyyXEfU_
   }
 }
+
+func testAnimationSignpost(cond: Bool, x: Int, y: Float) {
+  _osSignpostAnimationBeginTestHelper("animation begins here %d", Int.min)
+  _osSignpostAnimationBeginTestHelper("a message without arguments")
+  _osSignpostAnimationBeginTestHelper("animation begins here %ld", x)
+  _osSignpostAnimationBeginTestHelper("animation begins here %ld", cond ? x : y)
+  _osSignpostAnimationBeginTestHelper("animation begins here %ld %f", x, y)
+  // CHECK-LABEL: @${{.*}}testAnimationSignpost4cond1x1yySb_SiSftF
+  // CHECK: string_literal utf8 "animation begins here %d isAnimation=YES"
+  // CHECK: string_literal utf8 "a message without arguments isAnimation=YES"
+  // CHECK: string_literal utf8 "animation begins here %ld isAnimation=YES"
+  // CHECK: string_literal utf8 "animation begins here %ld isAnimation=YES"
+  // CHECK: string_literal utf8 "animation begins here %ld %f isAnimation=YES"
+}