Merge pull request #30338 from ravikandhadai/oslog-crash-in-unreachable-code

Author: swift-ci

include/swift/AST/DiagnosticsSIL.def

@@ -536,6 +536,9 @@ ERROR(oslog_property_not_constant, none, "'OSLogInterpolation.%0' is not a "
 ERROR(oslog_message_alive_after_opts, none, "OSLogMessage instance must not "
       "be explicitly created and must be deletable", ())
 
+WARNING(oslog_call_in_unreachable_code, none, "os log call will never be "
+        "executed and may have undiagnosed errors", ())
+
 ERROR(global_string_pointer_on_non_constant, none, "globalStringTablePointer "
       "builtin must used only on string literals", ())
 

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -1329,6 +1329,17 @@ static SILInstruction *beginOfInterpolation(ApplyInst *oslogInit) {
       worklist.push_back(storeInst);
       candidateStartInstructions.insert(storeInst);
     }
+    // Skip other uses of alloc_stack including function calls on the
+    // alloc_stack and data dependenceis through them. This is done because
+    // all functions using the alloc_stack are expected to be constant evaluated
+    // and therefore should only be passed constants or auto closures. These
+    // constants must be constructed immediately before the call and would only
+    // appear in the SIL after the alloc_stack instruction. This invariant is
+    // relied upon here so as to restrict the backward dependency search, which
+    // in turn keeps the code that is constant evaluated small.
+    // Note that if the client code violates this assumption, it will be
+    // diagnosed by this pass (in function detectAndDiagnoseErrors) as it will
+    // result in non-constant values for OSLogMessage instance.
   }
 
   // Find the first basic block in the control-flow order. Typically, if
@@ -1342,14 +1353,27 @@ static SILInstruction *beginOfInterpolation(ApplyInst *oslogInit) {
   }
 
   SILBasicBlock *firstBB = nullptr;
-  SILBasicBlock *entryBB = oslogInit->getFunction()->getEntryBlock();
-  for (SILBasicBlock *bb: llvm::breadth_first<SILBasicBlock *>(entryBB)) {
-    if (candidateBBs.count(bb)) {
-      firstBB = bb;
-      break;
+  if (candidateBBs.size() == 1) {
+    firstBB = *candidateBBs.begin();
+  } else {
+    SILBasicBlock *entryBB = oslogInit->getFunction()->getEntryBlock();
+    for (SILBasicBlock *bb : llvm::breadth_first<SILBasicBlock *>(entryBB)) {
+      if (candidateBBs.count(bb)) {
+        firstBB = bb;
+        break;
+      }
+    }
+    if (!firstBB) {
+      // This case will be reached only if the log call appears in unreachable
+      // code and, for some reason, its data depedencies extend beyond a basic
+      // block. This case should generally not happen unless the library
+      // implementation of the os log APIs change. It is better to warn in this
+      // case, rather than skipping the call silently.
+      diagnose(callee->getASTContext(), oslogInit->getLoc().getSourceLoc(),
+               diag::oslog_call_in_unreachable_code);
+      return nullptr;
     }
   }
-  assert(firstBB);
 
   // Iterate over the instructions in the firstBB and find the instruction that
   // starts the interpolation.
@@ -1360,7 +1384,7 @@ static SILInstruction *beginOfInterpolation(ApplyInst *oslogInit) {
       break;
     }
   }
-  assert(startInst);
+  assert(startInst && "could not find beginning of interpolation");
   return startInst;
 }
 
@@ -1450,7 +1474,10 @@ class OSLogOptimization : public SILFunctionTransform {
     // iteration.
     for (auto *oslogInit : oslogMessageInits) {
       SILInstruction *interpolationStart = beginOfInterpolation(oslogInit);
-      assert(interpolationStart);
+      if (!interpolationStart) {
+        // The log call is in unreachable code here.
+        continue;
+      }
       madeChange |= constantFold(interpolationStart, oslogInit, assertConfig);
     }
 

test/SILOptimizer/OSLogPrototypeCompileDiagnostics.swift

@@ -5,7 +5,8 @@
 // performs compile-time analysis and optimization of the new os log prototype
 // APIs. The tests here check whether bad user inputs are diagnosed correctly.
 // The tests here model the possible invalid inputs to the os log methods.
-// TODO: diagnostics will be improved.
+// TODO: diagnostics will be improved. globalStringTablePointer builtin error
+// must be suppressed.
 
 import OSLogPrototype
 
@@ -43,10 +44,83 @@ if #available(OSX 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) {
     let logMessage: OSLogMessage = "Maximum integer value: \(Int.max)"
       // expected-error @-1 {{OSLogMessage instance must not be explicitly created and must be deletable}}
     if !b {
-      return;
+      return
     }
     h.log(level: .debug, logMessage)
       // expected-error @-1 {{globalStringTablePointer builtin must used only on string literals}}
   }
+
+  func testNoninlinedFormatOptions(h: Logger) {
+    let formatOption: OSLogIntegerFormatting = .hex(includePrefix: true)
+    h.debug("Minimum integer value: \(Int.min, format: formatOption)")
+      // expected-error @-1 {{interpolation arguments like format and privacy options must be constants}}
+      // expected-error @-2 {{globalStringTablePointer builtin must used only on string literals}}
+  }
+
+  func testNoninlinedFormatOptionsComplex(h: Logger, b: Bool) {
+    let formatOption: OSLogIntegerFormatting = .hex(includePrefix: true)
+    if !b {
+      return
+    }
+    h.debug("Minimum integer value: \(Int.min, format: formatOption)")
+      // expected-error @-1 {{interpolation arguments like format and privacy options must be constants}}
+      // expected-error @-2 {{globalStringTablePointer builtin must used only on string literals}}
+  }
 }
 
+internal enum Color {
+  case red
+  case blue
+}
+
+if #available(OSX 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) {
+
+  // No error is expected here.
+  func testUnreachableLogCall(h: Logger, c: Color)  {
+    let arg = 10
+    switch c {
+    case .red:
+      return
+    case .blue:
+      return
+    default: // expected-warning {{default will never be executed}}
+      h.debug("Unreachable log call")
+      h.info("Unreachable log call with argument \(arg)")
+      h.log(
+        """
+        Unreachable log call with argument and formatting \
+        \(arg, align: .right(columns: 10))
+        """)
+    }
+  }
+}
+
+// This is an extension used only for testing a diagnostic that doesn't arise
+// normally but may be triggered by changes to the library.
+extension OSLogInterpolation {
+  @_transparent
+  mutating func appendInterpolation(_ c: Color) {
+    switch c {
+    case .red:
+      appendInterpolation(1)
+    case .blue:
+      appendInterpolation(0)
+    }
+  }
+}
+
+if #available(OSX 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) {
+
+  func testUnreachableLogCallComplex(h: Logger, c: Color)  {
+    switch c {
+    case .red:
+      return
+    case .blue:
+      return
+    default: // expected-warning {{default will never be executed}}
+      h.info("Some call \(c)")
+        // expected-warning@-1 {{os log call will never be executed and may have undiagnosed errors}}
+        // expected-error@-2 {{globalStringTablePointer builtin must used only on string literals}}
+    }
+  }
+}