Diagnose Infinite Recursion #11869

CodaFi · 2017-09-12T02:40:38Z

Add a new warning that detects when a function will call itself
recursively on all code paths. Attempts to invoke functions like this
may cause unbounded stack growth at least or undefined behavior in the
worst cases.

The detection code is implemented as DFS for a reachable exit path in
a given SILFunction.

Resolves SR-626, SR-677, SR-2559, and SR-4406

Has implications for SR-3016.

CodaFi · 2017-09-12T02:41:33Z

@swift-ci please smoke test

CodaFi · 2017-09-12T03:07:23Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

+          ClassType = ClassType.getMetatypeInstanceType(M);
+
+        auto *F
+            = M.lookUpFunctionInVTable(ClassType.getClassOrBoundGenericClass(),


@slavapestov Is weeding out volatile methods/witnesses enough here?

Update: Doesn’t matter now that dynamic witness methods are always going through ObjcMethodInst

CodaFi · 2017-09-18T01:00:42Z

The results of an unscientific comparison (stuck a SharedTimer in the pass and built stdlib):

master+Timer:

===-------------------------------------------------------------------------===
                               Swift compilation
===-------------------------------------------------------------------------===
  Total Execution Time: 272.6375 seconds (272.9904 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   <snip>
   0.0690 (  0.0%)   0.0007 (  0.0%)   0.0697 (  0.0%)   0.0697 (  0.0%)  Mandatory Diagnose Unreachable Pass
   </snip>
  163.4211 (100.0%)  109.2164 (100.0%)  272.6375 (100.0%)  272.9904 (100.0%)  Total

master+PR+Timer:

===-------------------------------------------------------------------------===
                               Swift compilation
===-------------------------------------------------------------------------===
  Total Execution Time: 273.9029 seconds (274.2406 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   <snip>
   0.0709 (  0.0%)   0.0006 (  0.0%)   0.0715 (  0.0%)   0.0715 (  0.0%)  Mandatory Diagnose Unreachable Pass
   </snip>
  163.8055 (100.0%)  110.0974 (100.0%)  273.9029 (100.0%)  274.2406 (100.0%)  Total

CodaFi · 2017-09-18T01:19:46Z

@swift-ci please smoke test

CodaFi · 2017-09-25T15:38:08Z

@swiftix ping

swiftix · 2017-09-29T03:44:17Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

+      return true;
+
+    if (FullApplySite FAI = FullApplySite::isa(&I)) {
+      auto &M = FAI.getModule();


I would suggest to move the logic that tries to determine the target function of the witness_method and class_method into a dedicated utility function, which would return the target function if it was able to find it. This function could be useful even outside of this code snippet.

It may also be interesting to do some checks to see if the exact dynamic type of the object, which is the operand of a class_method/witness_method instruction, is known statically. You may want to have a look at the Devirtualize.cpp. I think it has a lot of things you could reuse here.

BasicCalleeAnalysis is definitely already doing this. I'm going to look into refactoring this into a BottomUpIPAnalysis pass.

swiftix · 2017-09-29T03:48:36Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

+  Stack.push_back(Fn.getEntryBlock());
+
+  while (!Stack.empty()) {
+    SILBasicBlock *CurBlock = Stack.pop_back_val();


I wonder how this loop handles loops. Does its visit the BBs which belong to a loop multiple times?

BTW, is it necessary to start with the entry block? Does it matter at all, which block is used as a starting point? Would using RPOT order speed-up the convergence?

I wonder how this loop handles loops. Does its visit the BBs which belong to a loop multiple times?

BasicBlocks can only migrate from the lower state (FoundRecursion) to higher states (FoundPathOutOfFunction) at which point they will be popped and ignored on the next turn. If the block doesn't migrate, then it will not be added to the work queue. Hence the analysis always converges.

For the loops I was able to come up with to convince myself of this, it wasn't visiting any of the basic blocks more than once and I wouldn't expect it to. Even if the loop header gets re-enqueued its successors will not be re-enqueued.

swiftix · 2017-09-29T03:50:40Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

@@ -785,6 +890,18 @@ void swift::performSILDiagnoseUnreachable(SILModule *M, SILModuleTransform *T) {
    // Remove unreachable blocks.
    removeUnreachableBlocks(Fn, *M, &State);

+    // Gather exit blocks to check for naive self-recursion.
+    llvm::SmallPtrSet<SILBasicBlock*, 4> ExitBlocks;


IIRC, there was a utility function for something like this somewhere. But may be I'm wrong.

There's SILFunction::findExitingBlocks, but it doesn't look like its other usage in Local.cpp depends on it being ordered. I could switch them both to using a SmallPtrSet.

Yeah, I think it should be fine to switch them to use SmallPtrSet

swiftix · 2017-09-29T03:54:46Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

+
+    // Diagnose infinitely recursive applies.
+    if (checkInfinitelyRecursiveApplies(Fn, ExitBlocks))
+      diagnose(Fn.getModule().getASTContext(),


I wonder if it would make sense to extend such an analysis to detect even a more general form of recursion, i.e. not only the self-recursion? It may turn out that it is just too expensive. But may be it is not so expensive.

You would essentially need to build a call-graph, I guess. We have a BasicCalleeAnalysis that could help here.

WDYT?

It would be nice ™ to look into mutual recursion of two functions because of this SR.

@CodaFi @swiftix I do not think that we build the call graph at -Onone ever (maybe my memory is wrong). If we are to do so, please check compile time (if it becomes an issue, I imagine you can basically add a limit on the size of the call graph being exposed perhaps).

swiftix · 2017-09-29T03:55:41Z

test/SILOptimizer/infinite_recursion.swift

+
+func tr(_ key: String) -> String { // expected-warning {{all paths through this function will call itself}}
+  return tr(key) ?? key // expected-warning {{left side of nil coalescing operator '??' has non-optional type}}
+}


Could you add some negative test-cases as well? I.e. those ones which cannot be detected with a simple analysis yet, but could be eventually detected in the future?

There’s already a negative test for mutual recursion. Can you think of any others?

May be something where class_method or witness_method cannot be devirtualized, i.e. you cannot determine the target?

Added a new regression test for dynamic methods. I can't really think of another way to have an in-module recursive function that can't be devirtualized.

gottesmm

Quick compile time comment.

gottesmm · 2017-10-01T18:00:11Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

+
+    // Diagnose infinitely recursive applies.
+    if (checkInfinitelyRecursiveApplies(Fn, ExitBlocks))
+      diagnose(Fn.getModule().getASTContext(),


@CodaFi @swiftix I do not think that we build the call graph at -Onone ever (maybe my memory is wrong). If we are to do so, please check compile time (if it becomes an issue, I imagine you can basically add a limit on the size of the call graph being exposed perhaps).

NachoSoto · 2017-12-14T17:33:53Z

test/SILOptimizer/infinite_recursion.swift

+class C {
+  lazy var a: String = {
+    let a = "test"
+    print(self.a) // Should warn - interprocedural cycle.


This is SR-5224 :)

I think I know how to fix it too! Stand by.

CodaFi · 2017-12-14T23:57:07Z

@swift-ci please smoke test

CodaFi · 2017-12-15T00:00:06Z

lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp

+        return;
+
+      SILFunction *TargetFn = Fn;
+      if (Fn->isBare() == IsNotBare) {


@swiftix Is there a better way to detect this kind of thing? I'd rather not compute RPO info for more functions than I have to.

I'll take a wild guess that large functions are almost always "not bare", so skipping this doesn't buy much. Although, it's probably helping for deserialized stdlib functions. @adrian-prantl, do you know what functions we expect to be "bare", and is that planning to change in the future?

I suppose you're looking for a more narrow way to filter on closures for lazy variable initialization. @jckarter may have some ideas.

CodaFi · 2017-12-15T00:23:09Z

@jrose-apple This crashes while trying to deserialize generic parameters for a vtable function. Any ideas?

jrose-apple · 2017-12-15T00:36:57Z

Nothing offhand. Those tests are about recovering from Clang declarations disappearing or changing names, though, and no work on the SIL deserializer has been done to support that. Doing inlining that the compiler normally wouldn't do could certainly trigger this…but I wouldn't really expect this to be doing more inlining than the compiler already does.

CodaFi · 2017-12-15T00:58:13Z

It’s using the same primitives as the speculative devirtualizer. I can probably just bail on class decls with a Clang node in the meantime.

jrose-apple · 2017-12-15T01:27:04Z

It's not specifically Clang decls. It's decls that reference Clang decls. You can't tell if they're going to do it ahead of time.

(I'm really saying "no, I don't know why this is happening, someone-probably-you will have to go find out, but here's the interesting bit about those tests".)

CodaFi · 2017-12-15T01:49:04Z

Wonderful... There's a comment from @atrick that outlines what I'm seeing here

  // FIXME: There are unfortunate inconsistencies in the treatment of
  // generic param decls. Currently the first request for context wins
  // because we don't want to change context on-the-fly.
  // Here are typical scenarios:
  // (1) AST reads decl, get's scope.
  //     Later, readSILFunction tries to force module scope.
  // (2) readSILFunction forces module scope.
  //     Later, readVTable requests an enclosing scope.
  // ...other combinations are possible, but as long as AST lookups
  // precede SIL linkage, we should be ok.

The generic parameters are being deserialized in an AbstractFunctionDecl context instead of the ambient ClassDecl context. ~~I guess I could always force deserialization with the AbstractFunctionDecl context to match the behavior of the AST's requests.~~

atrick · 2017-12-15T21:43:24Z

That comment is the culmination of my attempt to understand deserialization well enough to work around some catastrophic bug. My understanding has only regressed since then, sorry.

CodaFi · 2017-12-15T22:22:13Z

Even if I remove the assertion, there's a real lack of recovery in SIL deserialization. @jrose-apple perhaps it's enough to hack in a primitive that bails if it has to deserialize vtables at all. After all, really the only way that would be helpful is if the pass wanted to diagnose cross-module recursive calls and it can't even do in-module mutual recursion.

jrose-apple · 2017-12-15T22:45:16Z

Hm, well, the good news is it doesn't seem to depend on Clang decls after all? :-)

Let's see. @CodaFi, are you actually using any of the cross-function analysis? I mean, you have a negative check for mutually recursive functions. Maybe it's worth just subsetting that out of the first implementation.

I would say that the right answer is probably to make SIL function generic parameters just be independent from the AST all the time…unless there's a reason to have them continue pointing back to the AST? But that's not something that should get rolled into this PR; it needs its own work.

CodaFi · 2017-12-15T22:46:58Z

are you actually using any of the cross-function analysis

Nope. This thing is only deserializing vtables in that test is because it sees a function call with a class-type receiver that happens to be in the damaged module.

atrick · 2018-02-15T23:41:02Z

lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp

+          }
+
+          // Ignore non-closure callees.
+          if (!Callee->getLocation().isASTNode<AbstractClosureExpr>()) {


You might want to at least check hasLocation first. I would have to run an experiment to find out what SIL functions aren't getting a location.

CodaFi · 2018-02-16T06:11:04Z

I think the fact that this patch smashed two separate-but-related kinds of SR together is causing more friction than is worth it. I'm going to split the fix for SR-5224 off and just commit the bare-bones algorithm.

CodaFi · 2018-02-16T06:16:53Z

Beauty, eh?

@swift-ci please smoke test

atrick · 2018-02-21T03:20:04Z

Sorry for the delay. I've been off grid for a while.

This looks very nice! Just one thing I have an issue with...

If a successor is a block for which we've already found a recursive call (State==FoundRecursion), why do we put that block back on the Worklist, only to call hasRecursiveCallInPath() again?

You could solve this and simplify things considerably IMO, by doing away with States and simply maintaining a Visited set and Worklist.

  SmallPtrSet<SILBasicBlock *, 16> Visited;
  SmallVector<SILBasicBlock *, 16> WorkList;
  auto pushSuccessor = [&](SILBasicBlock *Succ){
    if (!Visited.insert(Succ).second)
      return;
    
    if (!hasRecursiveCallInPath(*Succ, TargetFn))
      WorkList.push_back(Succ);
  };
  
  pushSuccessor(Fn.getEntryBlock());

  while (!WorkList.empty()) {
    SILBasicBlock *CurBlock = WorkList.pop_back_val();
    if (ExitBlocks.count(CurBlock))
      return false;
    
    for (SILBasicBlock *Succ : CurBlock->getSuccessorBlocks())
     pushSuccessor(Succ);
  }
  return true;

CodaFi · 2018-02-23T20:37:14Z

I agree that it simplifies things, but successors that we have already detected recursion-containing paths for are never re-enqueued.

Suppose that we have detected recursion already in a block that is one of the successors of this block, and further, does not dominate it (as we would have ended the recursion analysis for that path at that dominating block). We would ask the state dictionary for information about that block and be told it has state FoundRecursion . If the current state of this block is either FoundRecursion or FoundRecursionFreePath the re-enqueue of that successor block will not happen.

I do agree that your algorithm is a nice simplification though. I'll take it!

CodaFi · 2018-02-23T20:51:52Z

@atrick One final review then?

atrick · 2018-02-23T22:20:13Z

I think you need a bool flag to record that at least one recursive call is reachable and otherwise suppress diagnosis. Alternatively, you could comment that this always needs to run immediately after DiagnoseUnreachable (presumably that removed all unreachable exiting blocks), but a flag would be more defensive.

This will still diagnose infinite recursion for a function with a reachable recursive call that does not dominate an infinite loop. I personally think that's good, especially since it's just a warning (using an infinite loop as the recursive base case can't be intentional). I don't think clang does significantly better here--it also diagnoses this case if the exit is reachable from the call, and clang gets it more wrong by failing to diagnose when a recursive call does dominate an infinite loop. (The way to get this "right" is to consider infinite loops "exits"). [In short, I don't think you should change this!]

I hate to do this to you, but I just realized that calling findExitingBlocks, which itself runs an analysis over all blocks, is fairly silly. Since you're never visiting a block more than once and, for large CFGs, likely visiting only a fraction of the blocks, you might as well just call Block.getTerminator()->isFunctionExiting().

CodaFi · 2018-02-24T21:48:02Z

This will still diagnose infinite recursion for a function with a reachable recursive call that does not dominate an infinite loop. I personally think that's good

I agree. I'll see about applying this to Clang as well.

CodaFi · 2018-02-24T21:54:55Z

@atrick Done.

atrick

Great!

Add a new warning that detects when a function will call itself recursively on all code paths. Attempts to invoke functions like this may cause unbounded stack growth at least or undefined behavior in the worst cases. The detection code is implemented as DFS for a reachable exit path in a given SILFunction.

CodaFi · 2018-02-26T21:28:02Z

@swift-ci please smoke test

CodaFi · 2018-02-26T22:12:29Z

⛵️

CodaFi requested a review from swiftix September 12, 2017 02:40

CodaFi commented Sep 12, 2017

View reviewed changes

CodaFi changed the title ~~[WIP] Diagnose Infinite Recursion~~ Diagnose Infinite Recursion Sep 18, 2017

CodaFi force-pushed the unconditional-selfie-ban branch 2 times, most recently from cc8dc7b to 0918105 Compare September 18, 2017 01:19

swiftix reviewed Sep 29, 2017

View reviewed changes

jessesquires mentioned this pull request Sep 29, 2017

[90] Issue #90 - Oct 5, 2017 SwiftWeekly/swiftweekly.github.io#318

Closed

gottesmm reviewed Oct 1, 2017

View reviewed changes

CodaFi force-pushed the unconditional-selfie-ban branch from 0918105 to fad43de Compare November 15, 2017 08:32

NachoSoto reviewed Dec 14, 2017

View reviewed changes

CodaFi force-pushed the unconditional-selfie-ban branch from fad43de to e76c0a2 Compare December 14, 2017 23:56

CodaFi commented Dec 15, 2017

View reviewed changes

atrick reviewed Feb 15, 2018

View reviewed changes

CodaFi force-pushed the unconditional-selfie-ban branch from d4c1574 to 563bb07 Compare February 16, 2018 06:16

CodaFi force-pushed the unconditional-selfie-ban branch from ad7f556 to 1deae40 Compare February 23, 2018 20:50

CodaFi force-pushed the unconditional-selfie-ban branch 3 times, most recently from 73d36a1 to c6e74fa Compare February 24, 2018 21:54

CodaFi force-pushed the unconditional-selfie-ban branch from c6e74fa to 6d6d974 Compare February 25, 2018 03:57

atrick self-requested a review February 26, 2018 18:22

atrick approved these changes Feb 26, 2018

View reviewed changes

CodaFi force-pushed the unconditional-selfie-ban branch from 6d6d974 to c9c4fe0 Compare February 26, 2018 20:19

CodaFi force-pushed the unconditional-selfie-ban branch from c9c4fe0 to 5c7b790 Compare February 26, 2018 21:27

CodaFi merged commit dcd09e8 into swiftlang:master Feb 26, 2018

CodaFi deleted the unconditional-selfie-ban branch February 26, 2018 22:12

hamishknight mentioned this pull request Mar 25, 2018

[Sema] Only directly access members within didSet if accessed on 'self' #15280

Merged

NachoSoto mentioned this pull request Feb 12, 2018

[SR-5224] Unable to detect infinite recursion defining lazy property #47799

Open

Diagnose Infinite Recursion #11869

Diagnose Infinite Recursion #11869

Uh oh!

Conversation

CodaFi commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Sep 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Sep 18, 2017

Uh oh!

CodaFi commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Dec 15, 2017

Uh oh!

jrose-apple commented Dec 15, 2017

Uh oh!

CodaFi commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Dec 15, 2017

Uh oh!

CodaFi commented Sep 12, 2017 •

edited

Loading

CodaFi commented Sep 18, 2017 •

edited

Loading

CodaFi Dec 14, 2017 •

edited

Loading

CodaFi Dec 14, 2017 •

edited

Loading

CodaFi commented Dec 15, 2017 •

edited

Loading

CodaFi commented Dec 15, 2017 •

edited

Loading

CodaFi commented Dec 15, 2017 •

edited

Loading

CodaFi commented Feb 23, 2018 •

edited

Loading