SILOptimizer: make vtable pruning less aggressive #33770

compnerd · 2020-09-02T21:57:16Z

A class which is marked as internal or public can be visible outside
of the current file, where the use of the VWT indirectly is possible.
If the VWT is modified and inlined, it is possible that the offsets will
no longer match resulting in an invalid dispatch. Although this change
drastically reduces the efficacy of this pass, it ensures correctness as
a temporary measure.

Thanks to Alexander Smarus for the basis for the test case!

https://bugs.swift.org/browse/SR-13449
rdar://problem/68170038

compnerd · 2020-09-02T21:57:24Z

CC: @jckarter

compnerd · 2020-09-02T21:57:28Z

@swift-ci please test

gottesmm · 2020-09-02T22:02:28Z

Before this goes in I would like to understand more what is happening here.

gottesmm · 2020-09-02T22:03:49Z

Also note that @jckarter is on vacation right now as I understand it.

gottesmm · 2020-09-02T22:05:22Z

lib/SILOptimizer/Transforms/PruneVTables.cpp

-  void runOnVTable(SILModule *M,
-                   SILVTable *vtable) {
+  void runOnVTable(SILModule *M, SILVTable *vtable) {
+    if (!vtable->getClass()->isOutermostPrivateOrFilePrivateScope()) {


Turning this off for everything is not appropriate, we at least should be using the tools that we already have that handle this sort of thing. There are ways of checking this visibility while handling wmo/non-wmo that we should be using rather than referring to the ast.

Reading SILVTable, it seems that there is a query that tells you if the vtable will be serialized or was serialized. Given that we seem to not have SILLinkage here, I imagine that is what one would want to use.

@eeckstein what are your thoughts?

Can you point me to what you have in mind for checking here? I think that the WMO doesn't guarantee that every single reference is going to get fully monomorphized right? But, sure WMO with internal visibility should allow additional cases.

This problem can only occur in non-WMO mode or for "open" classes in WMO mode, right?
Can we disable this optimization just for "open" classes and for non-WMO mode (which is not so important)?

slavapestov · 2020-09-02T22:39:41Z

The class vtable is stored in the metadata, not the VWT.

I think a better fix would be to disable vtable pruning entirely in non-WMO mode. A more precise check would be to use vtable pruning only for fileprivate classes in non-WMO mode.

texasmichelle · 2020-09-02T22:49:46Z

Before this goes in I would like to understand more what is happening here.

Background for this PR is SR-13449.

compnerd · 2020-09-02T23:06:05Z

The class vtable is stored in the metadata, not the VWT.

Correct. The effective VWT in the metadata is what is being pruned.

I think a better fix would be to disable vtable pruning entirely in non-WMO mode. A more precise check would be to use vtable pruning only for fileprivate classes in non-WMO mode.

Yes, I agree with you, and that is what I would prefer to do:

fileprivate/private only classes always
in WMO, add internal classes as well (assuming that is fully safe)

I suppose that we could disable vtable pruning entirely in non-WMO mode, but if we can do it safely, why not?

swift-ci · 2020-09-02T23:16:26Z

Build failed
Swift Test Linux Platform
Git Sha - 3400f86d753014156698189b02cd3c70dcd95a02

gottesmm · 2020-09-02T23:48:36Z

@slavapestov question. I imagine the issue here in the design space is that we never have given vtables a SILLinkage and the infrastructure around isExternallyUsedSymbol. Your thoughts?

My thought is given that this causes a real crash, I think it makes sense to just disable this in this way for now. Really we should just fix the vtable to have VTables.

test/SILOptimizer/prune-vtables-cross-file.swift

swift-ci · 2020-09-02T23:52:53Z

Build failed
Swift Test OS X Platform
Git Sha - 3400f86d753014156698189b02cd3c70dcd95a02

compnerd · 2020-09-03T18:52:55Z

@slavapestov, @eeckstein - okay, the pass always runs under WMO now and only scans private classes on non-WMO mode.

compnerd · 2020-09-03T18:53:56Z

@swift-ci please test

swift-ci · 2020-09-03T19:16:26Z

Build failed
Swift Test OS X Platform
Git Sha - 3400f86d753014156698189b02cd3c70dcd95a02

lib/SILOptimizer/Transforms/PruneVTables.cpp

A class which is marked as internal or public can be visible outside of the current file, where the use of the VWT indirectly is possible. If the VWT is modified and inlined, it is possible that the offsets will no longer match resulting in an invalid dispatch. Limit the pass to when WMO is enabled or the type is private in non-WMO cases.

compnerd · 2020-09-03T23:19:48Z

@swift-ci please test

swift-ci · 2020-09-03T23:27:39Z

Build failed
Swift Test OS X Platform
Git Sha - e5770ab964fe8411739de7d101c3dab6f4a60345

eeckstein

lgtm!

slavapestov · 2020-09-04T19:57:49Z

lib/SILOptimizer/Transforms/PruneVTables.cpp

    LLVM_DEBUG(llvm::dbgs() << "PruneVTables inspecting table:\n";
               vtable->print(llvm::dbgs()));
+    if (!M->isWholeModule() &&
+        vtable->getClass()->getEffectiveAccess() >= AccessLevel::FilePrivate) {


Shouldn't it be > not >= then?

Oops, I thought Id fixed that. #33808

I think this change is unnecessarily pessimistic. The check for calleesAreStaticallyKnowable below should already prevent us from pruning nonfinal internal vtable entries. We should still be able to prune entries for final methods as long as the final method is not itself an override, because it should never need a vtable entry for originally-final methods to begin with, and we can reliably check whether a method is final across modules without an optimization pass. I suppose that means we could set the nonoverridden bit in SILGen for final methods.

It looks like if a method is originally final we already don't give it a vtable entry at all. So, we should be able to remove the if (!methodDecl->isFinal()) precondition below and get the same effect. I think that's better than reimplementing calleesAreStaticallyKnowable here.

gottesmm requested a review from jckarter September 2, 2020 22:02

gottesmm reviewed Sep 2, 2020

View reviewed changes

compnerd commented Sep 2, 2020

View reviewed changes

test/SILOptimizer/prune-vtables-cross-file.swift Outdated Show resolved Hide resolved

compnerd force-pushed the pruned-pruning-date branch from 3400f86 to e5770ab Compare September 3, 2020 18:52

slavapestov approved these changes Sep 3, 2020

View reviewed changes

lib/SILOptimizer/Transforms/PruneVTables.cpp Outdated Show resolved Hide resolved

compnerd mentioned this pull request Sep 3, 2020

utils: add a timeout to the integration tests #33774

Merged

compnerd force-pushed the pruned-pruning-date branch from e5770ab to a92e9b3 Compare September 3, 2020 23:18

eeckstein approved these changes Sep 4, 2020

View reviewed changes

compnerd merged commit 3cd4258 into swiftlang:master Sep 4, 2020

compnerd deleted the pruned-pruning-date branch September 4, 2020 15:52

slavapestov reviewed Sep 4, 2020

View reviewed changes

lxbndr mentioned this pull request Sep 7, 2020

[SR-13449] Wrong method call when binary is built with optimizations #55891

Closed

SILOptimizer: make vtable pruning less aggressive #33770

SILOptimizer: make vtable pruning less aggressive #33770

Uh oh!

Conversation

compnerd commented Sep 2, 2020 • edited by eeckstein Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Sep 2, 2020

Uh oh!

compnerd commented Sep 2, 2020

Uh oh!

gottesmm commented Sep 2, 2020

Uh oh!

gottesmm commented Sep 2, 2020

Uh oh!

gottesmm Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

compnerd Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

eeckstein Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Sep 2, 2020

Uh oh!

texasmichelle commented Sep 2, 2020

Uh oh!

compnerd commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Sep 2, 2020

Uh oh!

gottesmm commented Sep 2, 2020

Uh oh!

Uh oh!

swift-ci commented Sep 2, 2020

Uh oh!

compnerd commented Sep 3, 2020

Uh oh!

compnerd commented Sep 3, 2020

Uh oh!

swift-ci commented Sep 3, 2020

Uh oh!

Uh oh!

compnerd commented Sep 3, 2020

Uh oh!

swift-ci commented Sep 3, 2020

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

slavapestov Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

compnerd Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

jckarter Sep 8, 2020 • 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!

jckarter Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

compnerd commented Sep 2, 2020 •

edited by eeckstein

Loading

compnerd commented Sep 2, 2020 •

edited

Loading

jckarter Sep 8, 2020 •

edited

Loading