Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diagnose Infinite Recursion #11869

Merged
merged 1 commit into from Feb 26, 2018
Merged

Conversation

CodaFi
Copy link
Member

@CodaFi CodaFi commented Sep 12, 2017

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 CodaFi requested a review from swiftix September 12, 2017 02:40
@CodaFi
Copy link
Member Author

CodaFi commented Sep 12, 2017

@swift-ci please smoke test

ClassType = ClassType.getMetatypeInstanceType(M);

auto *F
= M.lookUpFunctionInVTable(ClassType.getClassOrBoundGenericClass(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@CodaFi
Copy link
Member Author

CodaFi commented Sep 18, 2017

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 CodaFi changed the title [WIP] Diagnose Infinite Recursion Diagnose Infinite Recursion Sep 18, 2017
@CodaFi CodaFi force-pushed the unconditional-selfie-ban branch 2 times, most recently from cc8dc7b to 0918105 Compare September 18, 2017 01:19
@CodaFi
Copy link
Member Author

CodaFi commented Sep 18, 2017

@swift-ci please smoke test

@CodaFi
Copy link
Member Author

CodaFi commented Sep 25, 2017

@swiftix ping

return true;

if (FullApplySite FAI = FullApplySite::isa(&I)) {
auto &M = FAI.getModule();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Stack.push_back(Fn.getEntryBlock());

while (!Stack.empty()) {
SILBasicBlock *CurBlock = Stack.pop_back_val();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@CodaFi CodaFi Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).


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}}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick compile time comment.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is SR-5224 :)

Copy link
Member Author

@CodaFi CodaFi Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

@CodaFi
Copy link
Member Author

CodaFi commented Dec 14, 2017

@swift-ci please smoke test

return;

SILFunction *TargetFn = Fn;
if (Fn->isBare() == IsNotBare) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

CodaFi commented Dec 15, 2017

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

@jrose-apple
Copy link
Contributor

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
Copy link
Member Author

CodaFi commented Dec 15, 2017

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
Copy link
Contributor

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
Copy link
Member Author

CodaFi commented Dec 15, 2017

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
Copy link
Member

atrick commented Dec 15, 2017

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
Copy link
Member Author

CodaFi commented Dec 15, 2017

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
Copy link
Contributor

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
Copy link
Member Author

CodaFi commented Dec 15, 2017

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.

}

// Ignore non-closure callees.
if (!Callee->getLocation().isASTNode<AbstractClosureExpr>()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

CodaFi commented Feb 16, 2018

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
Copy link
Member Author

CodaFi commented Feb 16, 2018

Beauty, eh?

@swift-ci please smoke test

@atrick
Copy link
Member

atrick commented Feb 21, 2018

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
Copy link
Member Author

CodaFi commented Feb 23, 2018

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
Copy link
Member Author

CodaFi commented Feb 23, 2018

@atrick One final review then?

@atrick
Copy link
Member

atrick commented Feb 23, 2018

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
Copy link
Member Author

CodaFi commented Feb 24, 2018

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 CodaFi force-pushed the unconditional-selfie-ban branch 3 times, most recently from 73d36a1 to c6e74fa Compare February 24, 2018 21:54
@CodaFi
Copy link
Member Author

CodaFi commented Feb 24, 2018

@atrick Done.

Copy link
Member

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

CodaFi commented Feb 26, 2018

@swift-ci please smoke test

@CodaFi
Copy link
Member Author

CodaFi commented Feb 26, 2018

⛵️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants