[SIL Diagnostics] Create a mandatory pass to check correct usage of yields in generalized accessors: _read and _modify #21067

ravikandhadai · 2018-12-05T23:09:28Z

This pass is based on the existing SIL verifier checks but diagnoses only those errors that can be introduced by programmers.

rdar://43578476

ravikandhadai · 2018-12-05T23:17:16Z

@swift-ci Please smoke test

gottesmm

Just a quick drive by.

gottesmm · 2018-12-05T23:19:24Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    // correctness, but it could change the errors diagnosed when there are
+    // multiple errors. Breadth-first search could diagnose errors along
+    // shorter paths.
+    llvm::DenseMap<SILBasicBlock*, BBState> visitedBBs;


As an aside. Please run this through git-clang-format. The patch is inconsistent about the location of the '*'.

ravikandhadai · 2018-12-06T02:37:30Z

@swift-ci Please clean smoke test

rjmccall

Generally looks good; just a few style quibbles.

rjmccall · 2018-12-06T05:27:37Z

include/swift/AST/DiagnosticsSIL.def

+NOTE(previous_yield, none, "previous 'yield' was here", ())
+
+ERROR(possible_return_before_yield, none, "cannot guarantee that the accessor "
+  "'yield's a value before returning", ())


This probably reads better if you just don't quote yield. It's a verb, like return, and doesn't need to be treated as super-technical jargon.

rjmccall · 2018-12-06T05:29:50Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

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


You should consider pushing this up to SILTransform, but it's not really harmful to declare it here if that's common practice for other diagnostic passes.

rjmccall · 2018-12-06T05:36:06Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+          // When the successor follows a 'yield', update the successor state.
+          if (isa<YieldInst>(term)) {
+            insertResult.first->second.yieldState = YieldState::AfterYield;
+            insertResult.first->second.yieldInst = term;


You should consider encapsulating BBState: add functions to do the various state transitions (these can assert that (1) the current state is appropriate for the transition and (2) they've been provided the expected information (e.g. a yield instruction)) and functions to get the state-specific information (these can assert that the state is as expected). This sort of approach encourages you to write better, stronger assertions and can catch problems a lot faster if the code needs to evolve.

ravikandhadai · 2018-12-06T22:28:56Z

@gottesmm @rjmccall Thanks for the comments. I am trying to debug a test failure in the file: generalized_accessor.sil I created in this PR. The test fails with the error "expected value ownership kind in SIL code". But there is an ownership annotation: @trivial at the point where it shows the error. Also, I couldn't reproduce this error locally. Just wondering if this sounds familiar to you and if there is something I am missing.

rjmccall · 2018-12-06T23:14:53Z

It's possible that the bots run sil-opt tests with -enable-sil-ownership enabled but your local configuration doesn't. I don't know why that would be true.

ravikandhadai · 2018-12-06T23:53:09Z

@rjmccall Thanks for the inputs. I think I know the problem. I guess @trivial is removed by @gottesmm recently and I wasn't on recent master.

ravikandhadai · 2018-12-07T00:25:34Z

@swift-ci Please smoke test

ravikandhadai · 2018-12-07T00:29:25Z

@swift-ci Please smoke test

ravikandhadai · 2018-12-07T00:33:55Z

@swift-ci Please smoke test

ravikandhadai · 2018-12-07T02:01:08Z

@swift-ci Please smoke test OS X platform

ravikandhadai · 2018-12-07T18:06:36Z

@rjmccall I have encapsulated the state transitions of BBState in this commit based on your suggestions. Let me know if this is what you had in mind.

rjmccall

Looks great, thanks!

ravikandhadai · 2018-12-07T19:25:25Z

@swift-ci Please Test Source Compatibility

ravikandhadai · 2018-12-07T19:27:16Z

@swift-ci please benchmark

swift-ci · 2018-12-07T20:17:55Z

Build comment file:

No performance and code size changes

devincoughlin

This looks good to me. There are some suggestions for diagnostic improvements inline and a suggestion to use an RPO traversal.

devincoughlin · 2018-12-10T18:41:54Z

include/swift/AST/DiagnosticsSIL.def

@@ -358,6 +358,21 @@ WARNING(warning_int_to_fp_inexact, none,
  "'%1' is not exactly representable as %0; it becomes '%2'",
  (Type, StringRef, StringRef))

+
+// Yield usage errors
+ERROR(return_before_yield, none, "accessor doesn't yield before returning",())


Can this be more prescriptive? Perhaps "accessor must yield before returning"?

devincoughlin · 2018-12-10T18:48:22Z

include/swift/AST/DiagnosticsSIL.def

+// Yield usage errors
+ERROR(return_before_yield, none, "accessor doesn't yield before returning",())
+
+ERROR(multiple_yields, none, "encountered multiple yields along "


Using "encountered" makes this diagnostic seem like it comes from the compiler's point of view. Can we rephrase this to be more from the user's point of view? Perhaps "accessor must not yield more than once"?

devincoughlin · 2018-12-10T19:26:55Z

include/swift/AST/DiagnosticsSIL.def

+
+NOTE(previous_yield, none, "previous yield was here", ())
+
+ERROR(possible_return_before_yield, none, "cannot guarantee that the accessor "


"guarantee" is also from the compiler's point of view. Could we use similar diagnostic text to the definite initialization pass? Perhaps: "'accessor must yield on all paths before returning"?

devincoughlin · 2018-12-10T19:35:57Z

include/swift/AST/DiagnosticsSIL.def

+  "yields a value before returning", ())
+
+NOTE(conflicting_join, none,
+    "some paths reaching here have a yield and some don't", ())


As a QoI improvement, I feel like this would be more helpful if this referred to control construct when we could figure it out. For example: "'then' branch yields but 'else' branch does not." You would still need a fallback case for when the compiler couldn't determine or understand the control construct.

Okay. Let me see how much we can recover here.

devincoughlin · 2018-12-10T19:39:11Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+  public:
+    void updateToAfterYield(SILInstruction *yldInst) {
+      assert(yieldState == BeforeYield);
+      assert(yldInst != nullptr);


I think it would good to have call 'yldInst' 'yieldInst' so it is consistent with 'yieldState'.

@devincoughlin yldInst is a parameter, and yieldState is the field of the struct: BBState. There is also a field calledyieldInst in the BBState struct. So one assertion is checking a property on a field and another a property on the parameter. Perhaps I can make this parameter: yieldInstParam

devincoughlin · 2018-12-10T19:47:04Z

test/SILOptimizer/generalized_accessors.swift

+  var computed: Int {
+    mutating _read {
+      if stored > 0 {
+        return


What happens when the only return is an explicit return and it hasn't yielded yet? Does the diagnostic go on the return statement itself?

Yes. In the following example, the diagnostic will go on the return statement

var loc = 1 if flag { loc = 2 } return // expected-error {{accessor doesn't yield before returning}} }

There can be only one return instruction in SIL. If the function can exit at multiple places the error will appear on the closing brace of the function definition. This is because at SIL level we only see one return instruction whose Loc is the closing brace of the function definition.

devincoughlin · 2018-12-10T20:19:39Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    llvm::Optional<SILInstruction *> returnInst = None;
+    ASTContext &astCtx = fun.getModule().getASTContext();
+
+    while (!worklist.empty()) {


Since we're not iterating to a fixed point, instead of using a worklist can you just use PostOrderFunctionInfo::getReversePostOrder() to do the traversal?

There are a couple of issues here: (a) the analysis does visit a node more than one once, if there is a conflict i.e, in the case of error. It pushes an already visited successor on to the worklist, if the successor is seen in a state that is different from the state it was seen with before. In the worst case, it processes each node twice, but that can happen only when there is an error. (b) The use of breadth-first search prioritizes shorter error paths over longer ones when there are multiple error paths. (However, an exception to this rule is that the analysis prioritizes "multiple-yield" error over a "return-without-yield" error by deferring the reporting of the latter error until the iteration completes.) I wonder whether RPO will preserve this behavior of shortest paths? E.g. if we have a tree of the form: (where D is a successor of C)
A
B ------ C
-------D

Then post-order is BDCA and I suppose reverse post-order is ACDB. So we see B at the end, whereas BFS will see it as the first successor of A?

Is the goal of visiting conflict blocks multiple times just to improve diagnostics in some way? It's not clear to me what it gets you at the moment.

A block seen in the conflict state won't be visited twice, but a block seen in a non-conflict state could be processed again if it is seen in the conflict state. This is just to determine if we have a multiple yield error or a missing yield error. E.g.

if (flag) { yield value } <-- we see conflict here. And the error is that we don't yield on all paths. return

OTOH,

if (flag) { yield value } <-- we see conflict here as before, but the error is that there are multiple yields along a path yield value return

It is just done to distinguish between the above two scenarios. Multiple yield errors seem easier to report by pointing to the two yields.

That seems fair.

rjmccall · 2019-02-06T16:57:07Z

Is this ready for review? There's a lot of commented-out code.

ravikandhadai · 2019-02-06T17:40:02Z

No not yet. I am working on recovering good diagnostics information for if-then-elses where one branch has a yield but other doesn't. I will ping you once it is ready for review.

ravikandhadai · 2019-02-14T02:23:17Z

@eeckstein @atrick This PR adds a mandatory pass for checking that generalized accessors (_read and _modify) yield exactly once. It would be great if you can provide your feedback on this.

@rjmccall @devincoughlin I have planned to bring in the improvements to the diagnostics in two patches. In this commit, I have rewritten the original yield-once analysis in a data-flow analysis style by making the transfer functions and merge operations explicit. This refactoring makes adding improvements to the diagnostics easier. I have also added more tests in this commit. Let me know you feedback on this commit.

Once this commit is merged, in a subsequent PR I will bring in the improvements for better diagnostics. The improvements will mainly target "conflict" cases where one branch yields but the other doesn't.

ravikandhadai · 2019-02-14T02:36:00Z

@swift-ci Please smoke test

ravikandhadai · 2019-02-15T03:31:39Z

Fixing merge conflicts.

ravikandhadai · 2019-02-15T03:31:47Z

@swift-ci please smoke test

rjmccall · 2019-02-15T03:10:27Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    /// (BeforeYield) or after seeing a yield (AfterYield), or in both states
+    /// (Conflict). This enum is a semi-lattice where Conflict is the top and
+    /// the merge of BeforeYield and AfterYield states is Conflict.
+    enum { BeforeYield, AfterYield, Conflict } yieldState = BeforeYield;


Please give BBState a constructor with two arguments, remove the defaulting, and clean up the factories. It's fine to make it a private constructor, and you may need to add a getInitialState factory.

Sure. I like that but the only thing that makes default initialization necessary is the DenseMap bbToStateMap used in the worklist algorithm. It has the type: llvm::DenseMap<SILBasicBlock *, BBState>. I can make the BBState optional and remove any default initialization/no-argument constructors. WDYT?

I don't think DenseMap unconditionally requires its value type to be default-initializable; it's only required if you use methods that need default-initialization, like where you use try_emplace, or if you use operator[] anywhere instead of find. I could be mistaken, of course.

Yes your are right, without those operations, DenseMap doesn't need default constructors. I have pushed a commit to fix these.

Looks good, thanks!

rjmccall · 2019-02-15T03:12:17Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    llvm::DenseMap<SILBasicBlock *, BBState> bbToStateMap;
+    SmallVector<SILBasicBlock *, 16> worklist;
+
+    auto *entryBB = &(*fun.begin());


fun.getEntryBlock()

rjmccall · 2019-02-15T03:13:32Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    SmallVector<SILBasicBlock *, 16> worklist;
+
+    auto *entryBB = &(*fun.begin());
+    bbToStateMap.try_emplace(entryBB);


Having a BBState::getInitialState() will make this much clearer.

rjmccall · 2019-02-15T03:16:02Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    llvm::Optional<SILInstruction *> returnInst = None;
+    ASTContext &astCtx = fun.getModule().getASTContext();
+
+    while (!worklist.empty()) {


Is the goal of visiting conflict blocks multiple times just to improve diagnostics in some way? It's not clear to me what it gets you at the moment.

gottesmm

A quick drive by. Also thanks for adding a .sil and a .swift test! = ).

gottesmm · 2019-02-15T20:13:22Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+  private:
+    // The following state is maintained for emitting diagnostics.
+
+    // For AfterYield and Conflict states, this field records the yield


This should be a doxygen comment. (doxygen comment == '///'). We really need to get some sort of doxygen producing job up. It will make it easier to make sure this works well.

gottesmm · 2019-02-15T20:14:08Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+    // instruction that was seen while propagating the state.
+    SILInstruction *yieldInst;
+
+    BBState(YieldState yState, SILInstruction *yieldI):


Can you make sure to run this through git-clang-format before committing? (I am guessing this needs to be run through b/c I don't think git-clang-format puts the ':' there.

gottesmm · 2019-02-15T20:14:37Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+  /// A structure that records an error found during the analysis along with
+  /// some context information that will be used by diagnostics.
+  struct YieldError {
+    // The kind of error.


This should be a doxygen comment.

gottesmm · 2019-02-15T20:15:28Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+  /// an error that is detected.
+  /// \return the state at the exit of the basic block if it can be computed
+  /// and None otherwise.
+  llvm::Optional<BBState>


We have imported Optional into the swift namespace, so I don't think you need the llvm:: any more for it. On another note, can you also make this helper function have static on it?

gottesmm · 2019-02-15T20:19:32Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+
+      auto nextState = resultState.getValue();
+
+      for (auto &succ : bb->getSuccessors()) {


Do you ever actually need access to the successor? If not you can use:

for (auto *succBB : bb->getSuccessorBlocks()) {

Sure. Will do.

ravikandhadai · 2019-02-18T18:44:38Z

@swift-ci Please smoke test

ravikandhadai · 2019-02-19T17:37:51Z

@swift-ci Please smoke test

ravikandhadai · 2019-02-19T18:16:53Z

@swift-ci Please smoke test

ravikandhadai · 2019-02-19T18:17:37Z

@swift-ci Please smoke test linux platform

shahmishal · 2019-02-19T18:18:55Z

Github is having issues, which is causing CI to fail.

https://www.githubstatus.com

devincoughlin

LGTM. Some minor comments inline.

devincoughlin · 2019-02-19T19:59:37Z

include/swift/AST/DiagnosticsSIL.def

+ERROR(possible_return_before_yield, none,
+      "accessor must yield on all paths before returning", ())
+
+NOTE(one_yield, none, "found yield along one path", ())


Perhaps "yield along one path is here"?

devincoughlin · 2019-02-19T20:04:03Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+


Could you put a brief description here of the high-level property that the pass is checking for here and a brief overview of high-level approach it takes. Not too much detail, but enough that someone new to the project could read it and know what the purpose of this pass is.

devincoughlin · 2019-02-19T20:08:42Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+  transferStateThroughBasicBlock(SILBasicBlock *bb, BBState inState,
+                                 Optional<YieldError> &error) {
+    auto *term = bb->getTerminator();
+


Should this initialize 'error' to None? That way if a caller reuses the passed in error it will get correctly set to None when there is no error.

devincoughlin · 2019-02-19T20:12:40Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

+      return oldState;
+    }
+
+    // Here, one state is AfterYield and the other one is BeforeYield.


Can you add an assertion for this? That way if the lattice changes but the merge function doesn't we'll get an assertion failure.

shahmishal · 2019-02-19T20:43:08Z

@swift-ci Please smoke test macOS platform

ravikandhadai · 2019-02-19T22:30:25Z

@devincoughlin Thanks for the suggestions. I pushed a commit that fixes them.

eeckstein

lgtm

ravikandhadai · 2019-02-20T00:00:52Z

@swift-ci Please test and merge

ravikandhadai · 2019-02-20T00:05:53Z

@swift-ci Please test and merge

yields in generalized accessors: _read and _modify, which are yield-once corountines. This pass is based on the existing SIL verifier checks but diagnoses only those errors that can be introduced by programmers when using yields. <rdar://43578476>

ravikandhadai · 2019-02-20T02:26:31Z

@swift-ci Please test and merge

ravikandhadai · 2019-02-20T02:46:02Z

@swift-ci Please test and merge

ravikandhadai · 2019-02-20T19:03:57Z

@swift-ci Please test and merge

ravikandhadai · 2019-02-20T22:06:01Z

@swift-ci Please test and merge

ravikandhadai force-pushed the yieldcheck branch 2 times, most recently from 9bca614 to 772bb8d Compare December 5, 2018 23:16

ravikandhadai requested review from rjmccall and devincoughlin December 5, 2018 23:17

gottesmm reviewed Dec 5, 2018

View reviewed changes

rjmccall reviewed Dec 6, 2018

View reviewed changes

ravikandhadai force-pushed the yieldcheck branch from 772bb8d to e07362b Compare December 7, 2018 00:25

rjmccall approved these changes Dec 7, 2018

View reviewed changes

devincoughlin reviewed Dec 10, 2018

View reviewed changes

ravikandhadai force-pushed the yieldcheck branch from 1dfb9be to fdc7656 Compare February 14, 2019 02:00

ravikandhadai assigned atrick and eeckstein and unassigned atrick and eeckstein Feb 14, 2019

ravikandhadai requested a review from eeckstein February 14, 2019 02:03

ravikandhadai force-pushed the yieldcheck branch from eea5446 to 6501719 Compare February 15, 2019 03:30

rjmccall reviewed Feb 15, 2019

View reviewed changes

ravikandhadai force-pushed the yieldcheck branch from 6501719 to 5c01b4a Compare February 15, 2019 19:50

rjmccall approved these changes Feb 15, 2019

View reviewed changes

gottesmm reviewed Feb 15, 2019

View reviewed changes

ravikandhadai force-pushed the yieldcheck branch from 5c01b4a to 9ad1fdd Compare February 15, 2019 22:40

devincoughlin approved these changes Feb 19, 2019

View reviewed changes

ravikandhadai force-pushed the yieldcheck branch from 9ad1fdd to 7cc7809 Compare February 19, 2019 22:28

ravikandhadai force-pushed the yieldcheck branch from 7cc7809 to 5cb9f68 Compare February 19, 2019 22:39

eeckstein approved these changes Feb 19, 2019

View reviewed changes

ravikandhadai force-pushed the yieldcheck branch from 5cb9f68 to a9b0ebe Compare February 20, 2019 02:26

swift-ci merged commit 1a66c77 into swiftlang:master Feb 21, 2019


		NOTE(previous_yield, none, "previous yield was here", ())

		ERROR(possible_return_before_yield, none, "cannot guarantee that the accessor "


		auto nextState = resultState.getValue();

		for (auto &succ : bb->getSuccessors()) {

[SIL Diagnostics] Create a mandatory pass to check correct usage of yields in generalized accessors: _read and _modify #21067

[SIL Diagnostics] Create a mandatory pass to check correct usage of yields in generalized accessors: _read and _modify #21067

Uh oh!

Conversation

ravikandhadai commented Dec 5, 2018

Uh oh!

ravikandhadai commented Dec 5, 2018

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai commented Dec 6, 2018

Uh oh!

rjmccall 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!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai commented Dec 6, 2018

Uh oh!

rjmccall commented Dec 6, 2018

Uh oh!

ravikandhadai commented Dec 6, 2018

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

ravikandhadai commented Dec 7, 2018

Uh oh!

swift-ci commented Dec 7, 2018

Build comment file:

No performance and code size changes

Uh oh!

devincoughlin 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!

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!

ravikandhadai Dec 10, 2018 • 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!

ravikandhadai Dec 10, 2018 •

edited

Loading

ravikandhadai Dec 10, 2018 •

edited

Loading

ravikandhadai Feb 15, 2019 •

edited

Loading

ravikandhadai commented Feb 14, 2019 •

edited

Loading

rjmccall Feb 15, 2019 •

edited

Loading