[5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for `discard` #65449

kavon · 2023-04-26T22:13:29Z

Cherry-pick of

• Description: Adds a missing piece of SE-390 where exits from a method that uses discard self that did not consume self explicitly are now flagged as errors. The purpose is to prevent mistakenly invoking the deinit in such contexts.
• Risk: Medium. Will introduce a source break in functions that are using discard self and do not consume self on all possible exits of the function (see test/SILOptimizer/discard_checking.swift for examples). The set of programs using discard is probably quite small currently and we'd want to introduce this change early, though the error could be downgraded into a warning for 5.9 upon request.
• Original PR: #66190 + dependency #65060
• Reviewed By: pending
• Testing: regression tests included
• Resolves: rdar://106099027

kavon · 2023-04-26T22:13:40Z

@swift-ci please test

kavon · 2023-04-27T19:27:19Z

NOTE: to land this correctly you'll have to revert a commit in #65425 if that one is merged first.

kavon · 2023-06-05T05:59:47Z

@swift-ci please test

gottesmm

Some small nits. Can you fix then LGTM.

gottesmm · 2023-06-05T07:36:38Z

include/swift/SIL/SILCloner.h

+  if (!getBuilder().hasOwnership()) {
+    return recordFoldedValue(Inst, getOpValue(Inst->getOperand()));
+  }
+  auto *MVI = getBuilder().createDropDeinit(getOpLocation(Inst->getLoc()),


Did you use git-clang-format here? I am pretty sure this isn't formatted correctly.

What in particular about the formatting is bad? It's even identical to visitMoveValueInst just above, so I don't see a problem here.

Can you always please use git-clang-format. That way we all use consistent formatting.

It doesn't look like everyone uses git-clang-format, since Erik wrote this bit of code that I'm cherry-picking.

Ok. I am going to let this go for now since you are cherry-picking. But I think as a team/project we need to have a larger conversation around this. Generally it is considered to be a good citizen thing to do: https://forums.swift.org/t/using-git-clang-format-in-the-swift-compiler-code-base/4996.

gottesmm · 2023-06-05T07:38:44Z

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

+    //         d   d      .   .
+    //
+    BasicBlockSet visited(destroyPoint->getFunction());
+    std::deque<SILBasicBlock *> bfsWorklist = {destroyPoint->getParent()};


We generally do not use std::deque in the code base. Does this really need to be a BFS? Couldn't it be a DFS and you use a BasicBlockWorklist? If you really need a queue, my suggestion is to use two std::vectors and swap them.

Reading this, I am pretty sure that this doesn't need a DFS. Can you use a BasicBlockWorklist here? It should be more efficient and we use it throughout the optimizer in this situation.

I did see uses of std::deque for BFS in the code base. As the comment mentions, I'm looking for the nearest exit of the function so a DFS will not work. I don't see a reason to overcomplicate this queue in an effort to optimize it, when it cannot be a performance issue: this code is for emitting an error diagnostic.

Does it really matter if we emit the closest one in terms of a CFG walk? The notion of closeness in terms of the CFG doesn't necessarily correspond to closeness in the source code (which is what I believe you are trying to approximate, no?). For example consider a situation where one has a split CFG like the followings:

bb0 / \ v v bb1 bb2 | v bb3

where bb1 is a huge basic block containing 5 pages of code and bb2/bb3 are two small blocks due to an early exit. In this case a BFS would give an apparent location far away from where-ever we are emitting errors from. I have definitely seen cases like this in real world code. If one accepts that CFG closeness correspond to closeness in source code, whether or not one uses a DFS or a BFS becomes immaterial.

In terms of using deque, this is about using standardized patterns throughout the optimizer/code base. In general, we have a set of standardized ways of doing things in the code base. One of them is when iterating over blocks to use BasicBlockWorklist if possible. It isn't just about performance (even though that is nice)... it is also about standardizing our code into standard patterns/utilities.

That's a good point. Neither approach is perfect and some other strategy is probably better. I settled on BFS because in practice it gives better locations, for example, from one of the regression tests:

struct Basics: ~Copyable { borrowing func thrower() throws {} consuming func test12(_ c: Color) throws { // with DFS the error ends up here guard case .red = c else { discard self return } try thrower() // with BFS the error is nicely located here print("hi") _ = consume self }

This is an important situation, since it's not always obvious to people that a try is a possible function exit.

I am sure I can come up with a similar test case where one could get the other behavior. That being said, it seems like you feel very strongly about this. I am fine with this.

test/SILGen/non_loadable_move_only.swift

gottesmm · 2023-06-05T07:43:28Z

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

-  // First remove all destroy_addr that have not been claimed.
+
+  /// Whether the marked value appeared in a discard statement.
+  const bool isDiscardingContext = !addressUseState.dropDeinitInsts.empty();


IIRC there really isn't a point in doing a const bool. We (if you look through the code base) generally do not use const with value types like bool.

Whether it's a discarding context should not change, so expressing that via const is only beneficial to prevent mistakes.

In general, in our code base we do not put constants on types like this. It was definitely against LLVM style for a long time.

That being said, given that we need to get stuff in, I am fine letting this through for now.

Using const is fine.

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

gottesmm · 2023-06-05T07:50:06Z

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

+        continue;
+
+      auto *dropDeinit = addressUseState.dropDeinitInsts.front();
+      diagnosticEmitter.emitMissingConsumeInDiscardingContext(destroyPair.first,


This is another example of a place where it seems like you didn't git-clang-format.

What in particular about the way I've formatted this is bad?

I am pretty sure that git-clang-format would put the arguments on the next line. We should all just be using git-clang-format.

jckarter

I think this is in a good enough state to land.

gottesmm

I am also happy with this.

kavon · 2023-06-05T20:23:01Z

@swift-ci please test

kavon · 2023-06-05T22:35:29Z

@swift-ci smoke test linux platform

kavon · 2023-06-06T00:42:35Z

@swift-ci please test

his instruction is a marker for a following destroy instruction to suppress the call of the move-only type's deinitializer.

…eeded by a drop_deinit rdar://105798769

…ry-picked yet." This reverts commit 22a9c77.

As part of SE-390, you're required to write either: - `consume self` - pass self as a `consuming` parameter to a function - `discard self` before the function ends in a context that contains a `discard self` somewhere. This prevents people from accidentally invoking the deinit due to implicit destruction of `self` before exiting the function. rdar://106099027 (cherry picked from commit 88d35a0)

kavon · 2023-06-06T02:39:54Z

@swift-ci please test

kavon · 2023-06-06T03:57:44Z

@swift-ci clean test macOS platform

kavon requested a review from a team as a code owner April 26, 2023 22:13

kavon requested a review from gottesmm April 26, 2023 22:14

kavon mentioned this pull request Apr 26, 2023

[5.9][move-only] A set of batched changes #65425

Merged

AnthonyLatsis added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9 labels May 3, 2023

kavon force-pushed the 5.9-drop-deinit branch from c8b016d to b8c7540 Compare June 5, 2023 05:17

kavon changed the title ~~[5.9🍒] Add the drop_deinit instruction~~ [5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for discard Jun 5, 2023

kavon requested a review from jckarter June 5, 2023 05:59

gottesmm reviewed Jun 5, 2023

View reviewed changes

jckarter approved these changes Jun 5, 2023

View reviewed changes

gottesmm approved these changes Jun 5, 2023

View reviewed changes

tbkka approved these changes Jun 5, 2023

View reviewed changes

kavon force-pushed the 5.9-drop-deinit branch from f44a90c to 070f348 Compare June 5, 2023 22:35

eeckstein and others added 7 commits June 5, 2023 19:39

SIL: add the drop_deinit instruction

c16f5a0

his instruction is a marker for a following destroy instruction to suppress the call of the move-only type's deinitializer.

SILGen: emit drop_deinit in move-only destructors

df245a6

MoveOnlyDeinitInsertion: don't call the deinit if the destroy is prec…

6200129

…eeded by a drop_deinit rdar://105798769

tests: re-enable simplification passes in SILGen/moveonly_deinits.swift

98352d7

Revert "Remove reference to DropDeinitInst since it has not been cher…

a710276

…ry-picked yet." This reverts commit 22a9c77.

fix test differences between main and release/5.9

a515cbd

kavon force-pushed the 5.9-drop-deinit branch from 070f348 to a515cbd Compare June 6, 2023 02:39

kavon mentioned this pull request Jun 6, 2023

[5.9🍒] prevent reinitialization of self after discard #66352

Merged

kavon merged commit a515cbd into swiftlang:release/5.9 Jun 6, 2023

kavon deleted the 5.9-drop-deinit branch June 6, 2023 15:38

[5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for discard #65449

[5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for discard #65449

Uh oh!

Conversation

kavon commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kavon commented Apr 26, 2023

Uh oh!

kavon commented Apr 27, 2023

Uh oh!

kavon commented Jun 5, 2023

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kavon Jun 5, 2023 • 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!

gottesmm Jun 5, 2023 • 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!

gottesmm Jun 5, 2023 • 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!

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!

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!

jckarter left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

kavon commented Jun 5, 2023

Uh oh!

kavon commented Jun 5, 2023

Uh oh!

kavon commented Jun 6, 2023

Uh oh!

kavon commented Jun 6, 2023

Uh oh!

kavon commented Jun 6, 2023

Uh oh!

Uh oh!

[5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for `discard` #65449

[5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for `discard` #65449

kavon commented Apr 26, 2023 •

edited

Loading

kavon Jun 5, 2023 •

edited

Loading

gottesmm Jun 5, 2023 •

edited

Loading

gottesmm Jun 5, 2023 •

edited

Loading