[SILOptimizer] Don't diagnose infinite recursion if a branch terminates the program #19724

harlanhaskins · 2018-10-05T00:30:18Z

This patch augments the infinite recursion checker to not warn if a
branch terminates, but still warns if a branch calls into something with
@_semantics("arc.programtermination_point"). This way, calling fatalError
doesn't disqualify you for the diagnostic, but calling exit does.

This also removes the warning workaround in the standard library, and
annotates the internal _assertionFailure functions as
programtermination_points, so they get this treatment too.

gottesmm · 2018-10-05T00:51:12Z

lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp

+    // Note that this check happens _before_ the recursion check. This is
+    // deliberate, as we want to ignore recursive calls inside a program
+    // termination point.
+    if (isARCInertTrapBB(curBlock))


Can you rename this in this commit to is program termination point? Since we are expanding the use of this I would suggest that we also rename the semantics string to something more descriptive since this is no longer used just for ARC.

Might as well move it out of ARCAnalysis and into SILBasicBlock, eh?

Also, I think we should include something about the lack of side effects in the name, moreso than isProgramTerminationPoint would...

gottesmm · 2018-10-05T00:51:57Z

lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp

-    if (curBlock->getTerminator()->isFunctionExiting())
+    TermInst *terminator = curBlock->getTerminator();
+    if (terminator->isFunctionExiting() ||
+        terminator->getTermKind() == TermKind::UnreachableInst)


If you are going to go this way can you move this into a separate query function and exhaustive switch over term kind so that if people add a new weird terminator they know to update this?

airspeedswift

lgtm for the std lib part

harlanhaskins · 2018-10-05T20:12:53Z

@gottesmm I've renamed isARCInertTrap to SILBasicBlock::isProgramTerminationPoint but I think that name isn't descriptive enough. Thoughts? I've also renamed arc.programtermination_point to just programtermination_pointbut I need to go through and rearrange it in the docs to move it out of the 'strictly ARC' section.

gottesmm

3 nits and then LGTM.

gottesmm · 2018-10-05T20:36:12Z

lib/SIL/SILBasicBlock.cpp

@@ -336,6 +337,79 @@ bool SILBasicBlock::isEntry() const {
  return this == &*getParent()->begin();
 }

+//===----------------------------------------------------------------------===//
+//                            Leaking BB Analysis


Can you change all of the references here to ARC/Leaking BBs/etc to refer to trapping block analysis/etc. I'll show you the ones that I see.

gottesmm · 2018-10-05T20:42:03Z

lib/SIL/SILBasicBlock.cpp

+  }
+
+  // Otherwise, we have an unreachable and every instruction is inert from an
+  // ARC perspective in an unreachable BB.


gottesmm · 2018-10-05T20:42:21Z

lib/SIL/SILBasicBlock.cpp

+  return false;
+}
+
+/// Match a call to a trap BB with no ARC relevant side effects.


Here. Also this shouldn't be a doxygen comment (so 2 slashes).

This patch augments the infinite recursion checker to not warn if a branch terminates, but still warns if a branch calls into something with `@_semantics("arc.programtermination_point")`. This way, calling `fatalError` doesn't disqualify you for the diagnostic, but calling `exit` does. This also removes the warning workaround in the standard library, and annotates the internal _assertionFailure functions as `programtermination_point`s, so they get this treatment too.

harlanhaskins · 2018-10-05T21:03:17Z

@swift-ci please test

harlanhaskins · 2018-10-05T21:03:51Z

(I also want to make sure this doesn't introduce any new warnings)

@swift-ci please test source compatibility

swift-ci · 2018-10-05T21:03:58Z

Build failed
Swift Test OS X Platform
Git Sha - f94910babf6ca3afc78e9bd11d14a6247a8442f2

swift-ci · 2018-10-05T21:05:09Z

Build failed
Swift Test Linux Platform
Git Sha - f94910babf6ca3afc78e9bd11d14a6247a8442f2

harlanhaskins · 2018-10-05T23:16:18Z

So, interestingly, this warning happens in the source compatibility suite, and has been happening for as far back as there are still build logs (before #19693).

/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/ReactiveSwift/Sources/ValidatingProperty.swift:193:21: warning: all paths through this function will call itself
    public convenience init<U, E>(

And I think this is another false positive related to dynamic dispatch, similar to SR-7925

harlanhaskins · 2018-10-06T01:35:26Z

Release source compat run failed while cloning Vapor, but I'm satisfied with the results from the debug run. @gottesmm Good to merge?

gottesmm · 2018-10-06T01:41:34Z

Let err rip.

graydon · 2018-10-08T06:53:26Z

This is causing failures. See rdar://45080912. Backing out.

…terminates (#19724)" This reverts commit e94450e. rdar://45080912

…es (swiftlang#19724) This patch augments the infinite recursion checker to not warn if a branch terminates, but still warns if a branch calls into something with `@_semantics("arc.programtermination_point")`. This way, calling `fatalError` doesn't disqualify you for the diagnostic, but calling `exit` does. This also removes the warning workaround in the standard library, and annotates the internal _assertionFailure functions as `programtermination_point`s, so they get this treatment too.

…terminates (swiftlang#19724)" This reverts commit e94450e. rdar://45080912

harlanhaskins requested review from airspeedswift, CodaFi and gottesmm October 5, 2018 00:30

harlanhaskins changed the title ~~[SILOptimizer] Don't diagnose infinite recursion if a branch terminates~~ [SILOptimizer] Don't diagnose infinite recursion if a branch terminates the program Oct 5, 2018

gottesmm reviewed Oct 5, 2018

View reviewed changes

airspeedswift approved these changes Oct 5, 2018

View reviewed changes

gottesmm approved these changes Oct 5, 2018

View reviewed changes

harlanhaskins force-pushed the past-the-point-of-no-return branch from f94910b to c1bee86 Compare October 5, 2018 21:02

harlanhaskins merged commit e94450e into swiftlang:master Oct 6, 2018

graydon mentioned this pull request Oct 8, 2018

[SILOptimizer] Clean up infinite recursion diagnostic pass #19710

Merged

graydon added a commit that referenced this pull request Oct 8, 2018

Revert "[SILOptimizer] Don't diagnose infinite recursion if a branch …

cc16ddf

…terminates (#19724)" This reverts commit e94450e. rdar://45080912

modelorganism pushed a commit to modelorganism/swift that referenced this pull request Oct 11, 2018

Revert "[SILOptimizer] Don't diagnose infinite recursion if a branch …

ad92a01

…terminates (swiftlang#19724)" This reverts commit e94450e. rdar://45080912

[SILOptimizer] Don't diagnose infinite recursion if a branch terminates the program #19724

[SILOptimizer] Don't diagnose infinite recursion if a branch terminates the program #19724

Uh oh!

Conversation

harlanhaskins commented Oct 5, 2018

Uh oh!

gottesmm Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

harlanhaskins Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

harlanhaskins Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

airspeedswift left a comment

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Oct 5, 2018

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Oct 5, 2018

Uh oh!

harlanhaskins commented Oct 5, 2018

Uh oh!

swift-ci commented Oct 5, 2018

Uh oh!

swift-ci commented Oct 5, 2018

Uh oh!

harlanhaskins commented Oct 5, 2018

Uh oh!

harlanhaskins commented Oct 6, 2018

Uh oh!

gottesmm commented Oct 6, 2018

Uh oh!

graydon commented Oct 8, 2018

Uh oh!

Uh oh!