Skip to content

Diagnose Infinite Recursion #11869

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

Merged
merged 1 commit into from
Feb 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/AST/DiagnosticsSIL.def
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ WARNING(unreachable_case,none,
WARNING(switch_on_a_constant,none,
"switch condition evaluates to a constant", ())
NOTE(unreachable_code_note,none, "will never be executed", ())
WARNING(warn_infinite_recursive_function,none,
"all paths through this function will call itself", ())

// 'transparent' diagnostics
ERROR(circular_transparent,none,
Expand Down
2 changes: 2 additions & 0 deletions include/swift/SILOptimizer/PassManager/Passes.def
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ PASS(DefiniteInitialization, "definite-init",
"Definite Initialization for Diagnostics")
PASS(Devirtualizer, "devirtualizer",
"Indirect Call Devirtualization")
PASS(DiagnoseInfiniteRecursion, "diagnose-infinite-recursion",
"Diagnose Infinitely-Recursive Code")
PASS(DiagnoseStaticExclusivity, "diagnose-static-exclusivity",
"Static Enforcement of Law of Exclusivity")
PASS(DiagnoseUnreachable, "diagnose-unreachable",
Expand Down
1 change: 1 addition & 0 deletions lib/SILOptimizer/Mandatory/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ set(MANDATORY_SOURCES
Mandatory/DIMemoryUseCollector.cpp
Mandatory/DIMemoryUseCollectorOwnership.cpp
Mandatory/DataflowDiagnostics.cpp
Mandatory/DiagnoseInfiniteRecursion.cpp
Mandatory/DiagnoseStaticExclusivity.cpp
Mandatory/DiagnoseUnreachable.cpp
Mandatory/GuaranteedARCOpts.cpp
Expand Down
163 changes: 163 additions & 0 deletions lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
//==-- DiagnoseInfiniteRecursion.cpp - Find infinitely-recursive applies --==//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//
//
// This file implements a diagnostic pass that detects deleterious forms of
// recursive functions.
//
//===----------------------------------------------------------------------===//

#define DEBUG_TYPE "infinite-recursion"
#include "swift/AST/DiagnosticsSIL.h"
#include "swift/AST/Expr.h"
#include "swift/Parse/Lexer.h"
#include "swift/SIL/CFG.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILInstruction.h"
#include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
#include "swift/SILOptimizer/PassManager/Passes.h"
#include "swift/SILOptimizer/PassManager/Transforms.h"
#include "swift/SILOptimizer/Utils/Devirtualize.h"
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/iterator_range.h"
#include "llvm/Support/Debug.h"

using namespace swift;

template<typename...T, typename...U>
static void diagnose(ASTContext &Context, SourceLoc loc, Diag<T...> diag,
U &&...args) {
Context.Diags.diagnose(loc,
diag, std::forward<U>(args)...);
}

static bool hasRecursiveCallInPath(SILBasicBlock &Block,
SILFunction *Target,
ModuleDecl *TargetModule) {
// Process all instructions in the block to find applies that reference
// the parent function. Also looks through vtables for statically
// dispatched (witness) methods.
for (auto &I : Block) {
auto *AI = dyn_cast<ApplyInst>(&I);
if (AI && AI->getCalleeFunction() && AI->getCalleeFunction() == Target)
return true;

if (FullApplySite FAI = FullApplySite::isa(&I)) {
// Don't touch dynamic dispatch.
if (isa<ObjCMethodInst>(FAI.getCallee()))
continue;

auto &M = FAI.getModule();
if (auto *CMI = dyn_cast<ClassMethodInst>(FAI.getCallee())) {
auto ClassType = CMI->getOperand()->getType();

// FIXME: If we're not inside the module context of the method,
// we may have to deserialize vtables. If the serialized tables
// are damaged, the pass will crash.
//
// Though, this has the added bonus of not looking into vtables
// outside the current module. Because we're not doing IPA, let
// alone cross-module IPA, this is all well and good.
auto *BGC = ClassType.getNominalOrBoundGenericNominal();
if (BGC && BGC->getModuleContext() != TargetModule) {
continue;
}

auto *F = getTargetClassMethod(M, ClassType, CMI);
if (F == Target)
return true;

continue;
}

if (auto *WMI = dyn_cast<WitnessMethodInst>(FAI.getCallee())) {
SILFunction *F;
SILWitnessTable *WT;

std::tie(F, WT) = M.lookUpFunctionInWitnessTable(
WMI->getConformance(), WMI->getMember());
if (F == Target)
return true;

continue;
}
}
}
return false;
}

static bool hasInfinitelyRecursiveApply(SILFunction &Fn,
SILFunction *TargetFn) {
SmallPtrSet<SILBasicBlock *, 16> Visited;
SmallVector<SILBasicBlock *, 16> WorkList;
// Keep track of whether we found at least one recursive path.
bool foundRecursion = false;

auto *TargetModule = TargetFn->getModule().getSwiftModule();
auto analyzeSuccessor = [&](SILBasicBlock *Succ) -> bool {
if (!Visited.insert(Succ).second)
return false;

// If the successor block contains a recursive call, end analysis there.
if (!hasRecursiveCallInPath(*Succ, TargetFn, TargetModule)) {
WorkList.push_back(Succ);
return false;
}
return true;
};

// Seed the work list with the entry block.
foundRecursion |= analyzeSuccessor(Fn.getEntryBlock());

while (!WorkList.empty()) {
SILBasicBlock *CurBlock = WorkList.pop_back_val();
// Found a path to the exit node without a recursive call.
if (CurBlock->getTerminator()->isFunctionExiting())
return false;

for (SILBasicBlock *Succ : CurBlock->getSuccessorBlocks())
foundRecursion |= analyzeSuccessor(Succ);
}
return foundRecursion;
}

namespace {
class DiagnoseInfiniteRecursion : public SILFunctionTransform {
public:
DiagnoseInfiniteRecursion() {}

private:
void run() override {
SILFunction *Fn = getFunction();
// Don't rerun diagnostics on deserialized functions.
if (Fn->wasDeserializedCanonical())
return;

// Ignore empty functions and straight-line thunks.
if (Fn->empty() || Fn->isThunk() != IsNotThunk)
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 comment here on why you want this to run on IsReabstractionThunk but not other thunks? Is this to save work? Would we misdiagnose something? Do reabstraction thunks have a different shape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not be run on thunks. If the thunk IsRebastractionThunk it will fail this condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that condition makes sense. I guess the answer here is that it saves some work to ignore these.

return;

// If we can't diagnose it, there's no sense analyzing it.
if (!Fn->hasLocation() || Fn->getLocation().getSourceLoc().isInvalid())
return;

if (hasInfinitelyRecursiveApply(*Fn, Fn)) {
diagnose(Fn->getModule().getASTContext(),
Fn->getLocation().getSourceLoc(),
diag::warn_infinite_recursive_function);
}
}
};
} // end anonymous namespace

SILTransform *swift::createDiagnoseInfiniteRecursion() {
return new DiagnoseInfiniteRecursion();
}
1 change: 1 addition & 0 deletions lib/SILOptimizer/PassManager/PassPipeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ static void addMandatoryOptPipeline(SILPassPipelinePlan &P,
P.addDiagnosticConstantPropagation();
P.addGuaranteedARCOpts();
P.addDiagnoseUnreachable();
P.addDiagnoseInfiniteRecursion();
P.addEmitDFDiagnostics();
// Canonical swift requires all non cond_br critical edges to be split.
P.addSplitNonCondBrCriticalEdges();
Expand Down
110 changes: 110 additions & 0 deletions test/SILOptimizer/infinite_recursion.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// RUN: %target-swift-frontend -emit-sil -primary-file %s -o /dev/null -verify

func a() { // expected-warning {{all paths through this function will call itself}}
a()
}

func b(_ x : Int) { // expected-warning {{all paths through this function will call itself}}
if x != 0 {
b(x)
} else {
b(x+1)
}
}

func c(_ x : Int) {
if x != 0 {
c(5)
}
}

func d(_ x : Int) { // expected-warning {{all paths through this function will call itself}}
var x = x
if x != 0 {
x += 1
}
return d(x)
}

// Doesn't warn on mutually recursive functions

func e() { f() }
func f() { e() }

func g() { // expected-warning {{all paths through this function will call itself}}
while true { // expected-note {{condition always evaluates to true}}
g()
}

g() // expected-warning {{will never be executed}}
}

func h(_ x : Int) {
while (x < 5) {
h(x+1)
}
}

func i(_ x : Int) { // expected-warning {{all paths through this function will call itself}}
var x = x
while (x < 5) {
x -= 1
}
i(0)
}

func j() -> Int { // expected-warning {{all paths through this function will call itself}}
return 5 + j()
}

func k() -> Any { // expected-warning {{all paths through this function will call itself}}
return type(of: k())
}

class S {
convenience init(a: Int) { // expected-warning {{all paths through this function will call itself}}
self.init(a: a)
}
init(a: Int?) {}

static func a() { // expected-warning {{all paths through this function will call itself}}
return a()
}

func b() { // expected-warning {{all paths through this function will call itself}}
var i = 0
repeat {
i += 1
b()
} while (i > 5)
}
}

class T: S {
// No warning, calls super
override func b() {
var i = 0
repeat {
i += 1
super.b()
} while (i > 5)
}
}

func == (l: S?, r: S?) -> Bool { // expected-warning {{all paths through this function will call itself}}
if l == nil && r == nil { return true }
guard let l = l, let r = r else { return false }
return l === r
}

public func == <Element>(lhs: Array<Element>, rhs: Array<Element>) -> Bool { // expected-warning {{all paths through this function will call itself}}
return lhs == rhs
}

func factorial(_ n : UInt) -> UInt { // expected-warning {{all paths through this function will call itself}}
return (n != 0) ? factorial(n - 1) * n : factorial(1)
}

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

20 changes: 20 additions & 0 deletions test/SILOptimizer/infinite_recursion_objc.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// RUN: %target-swift-frontend -emit-sil -primary-file %s -o /dev/null -verify

// REQUIRES: objc_interop

// A negative test that the infinite recursion pass doesn't diagnose dynamic
// dispatch.

import Foundation

class MyRecursiveClass {
required init() {}
@objc dynamic func foo() {
return type(of: self).foo(self)()
}

@objc dynamic func foo2() {
return self.foo()
}
}

2 changes: 1 addition & 1 deletion test/SILOptimizer/unreachable_code.swift
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ class r20097963MyClass {
}
}

func die() -> Never { die() }
func die() -> Never { die() } // expected-warning {{all paths through this function will call itself}}

func testGuard(_ a : Int) {
guard case 4 = a else { } // expected-error {{'guard' body must not fall through, consider using a 'return' or 'throw'}}
Expand Down