[SILProfiler] Skip exprs with invalid locations

Ignore exprs with invalid locations because we can't generate correct
coverage information for them. Continue to visit their children.

rdar://54312893&55913380&51612977

Author: vedantk

lib/SIL/SILProfiler.cpp

@@ -177,6 +177,12 @@ bool visitFunctionDecl(ASTWalker &Walker, AbstractFunctionDecl *AFD, F Func) {
   return continueWalk;
 }
 
+/// Whether to skip visitation of an expression. Children of skipped exprs
+/// should still be visited.
+static bool skipExpr(Expr *E) {
+  return !E->getStartLoc().isValid() || !E->getEndLoc().isValid();
+}
+
 /// An ASTWalker that maps ASTNodes to profiling counters.
 struct MapRegionCounters : public ASTWalker {
   /// The next counter value to assign.
@@ -238,6 +244,9 @@ struct MapRegionCounters : public ASTWalker {
   }
 
   std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+    if (skipExpr(E))
+      return {true, E};
+
     // If AST visitation begins with an expression, the counter map must be
     // empty. Set up a counter for the root.
     if (Parent.isNull()) {
@@ -400,7 +409,10 @@ class SourceMappingRegion {
 
   bool hasStartLoc() const { return StartLoc.hasValue(); }
 
-  void setStartLoc(SourceLoc Loc) { StartLoc = Loc; }
+  void setStartLoc(SourceLoc Loc) {
+    assert(Loc.isValid());
+    StartLoc = Loc;
+  }
 
   const SourceLoc &getStartLoc() const {
     assert(StartLoc && "Region has no start location");
@@ -409,7 +421,10 @@ class SourceMappingRegion {
 
   bool hasEndLoc() const { return EndLoc.hasValue(); }
 
-  void setEndLoc(SourceLoc Loc) { EndLoc = Loc; }
+  void setEndLoc(SourceLoc Loc) {
+    assert(Loc.isValid());
+    EndLoc = Loc;
+  }
 
   const SourceLoc &getEndLoc() const {
     assert(EndLoc && "Region has no end location");
@@ -573,6 +588,9 @@ struct PGOMapping : public ASTWalker {
   }
 
   std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+    if (skipExpr(E))
+      return {true, E};
+
     unsigned parent = getParentCounter();
 
     if (Parent.isNull()) {
@@ -996,6 +1014,9 @@ struct CoverageMapping : public ASTWalker {
   }
 
   std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+    if (skipExpr(E))
+      return {true, E};
+
     if (!RegionStack.empty())
       extendRegion(E);
 

test/Profiler/coverage_invalid_loc.swift

@@ -0,0 +1,30 @@
+// RUN: %target-swift-frontend -disable-sil-ownership-verifier -Xllvm -sil-full-demangle -profile-generate -profile-coverage-mapping -emit-sorted-sil -emit-sil -module-name coverage_invalid_loc %s | %FileCheck %s
+
+// The implicit tuple and array exprs formed to call `dynamicallyCall`
+// happen to have invalid source locations (n.b. this may not always be true,
+// but it was true at the time this test was written).
+//
+// The coverage pass must skip exprs with invalid locations because there is
+// no better alternative: creating fake locations for exprs may make coverage
+// reporting incorrect, and not all implicit exprs have valid locations.
+//
+// Test that a) the coverage pass *can* skip exprs with invalid locations and
+// that b) this does not result in the children of implicit exprs being skipped.
+
+@dynamicCallable
+public struct Callable {
+  func dynamicallyCall(withArguments: [(Int) -> Int]) {}
+}
+
+// CHECK: sil_coverage_map {{.*}} closure #1 (Swift.Int) -> Swift.Int in coverage_invalid_loc.foo(a: coverage_invalid_loc.Callable) -> ()
+// CHECK-NEXT:   [[@LINE+9]]:5 -> {{.*}}:30 : 0
+// CHECK-NEXT: }
+
+// CHECK: sil_coverage_map {{.*}} "foo" "foo" 0 {
+// CHECK-NEXT:   [[@LINE+4]]:30 -> {{.*}}:2 : 0
+// CHECK-NEXT: }
+
+@_silgen_name("foo")
+public func foo(a: Callable) {
+  a({ (x : Int) -> Int in x })
+}