[ClangImporter] Block Re-Entrancy In Lookups #28679
Conversation
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
lib/ClangImporter/ClangImporter.cpp
Outdated
| // Simply importing the categories adds them to the list of extensions. | ||
| for (auto I = objcClass->visible_categories_begin(), | ||
| E = objcClass->visible_categories_end(); | ||
| I != E; ++I) { | ||
| // Delay installing categories that don't have an owning module. | ||
| if (!I->getOwningModule()) { |
There was a problem hiding this comment.
Are all categories defined outside of the module "by us"? So this would catch only extensions exported to Objective-C or do you expect that it will affect other categories?
There was a problem hiding this comment.
Categories with no Clang Module are synthesized by us - usually in textual headers like bridging headers. The worst I expect to happen is somebody has such a setup and somehow managed to not get the re-entrant behavior and so saw the decls in the textual header stomp the ones in the module.
There was a problem hiding this comment.
BTW, there should also be a hasOwningModule that may be faster.
There was a problem hiding this comment.
Ah, true. I'll get that in a follow-up.
|
@swift-ci please test source compatibility Release |
|
Build failed |
|
Swift Syntax needs to be updated. @swift-ci please smoke test macOS platform |
|
The failures tell me that I've broken override handling for certain properties. |
The general problem with this approach is that the clang importer is part of the cache-fill for name lookup. This means that qualified lookups it runs will be re-entrant and result in validation-order-dependent behaviors. The next commit will restore the expected ordering behavior here.
Bridging headers are weird. A general principle in the Clang Importer is that when it comes to the notion of "overrides" or "redeclarations", it's always better to prefer a declaration in the same translation unit, module, header, etc. over a declaration outside of that thing. This behavior, unfortunately, occurred as an emergent property of the re-entrant lookup that was being performed when importing properties. The behavior of the visible categories list is in "deserialization order" - that is, a category defined in a header that imports a module can potentially appear before the module has had its declarations fully deserialized and installed. We were assuming this order was instead "source order", in which modules would be recursively expanded and their declarations installed sequentially. If this assumption were to hold, then the visible categories list would respect the principle above, and we would see categories in defining modules and headers before we saw categories in extant translation units. This is not the case. Delayed deserialization of modules means the first category that gets parsed actually wins. We were not noticing this, however, because the few places where it actually mattered were performing re-entrant lookup and were accidentally deserializing modules and installing decls in this fictitious "source order". Since we've untangled this behavior, we now have to emulate the order the Importer expects. Introduce the notion of a "delayed category" - a category that is defined outside of a module, usually by us. As the name implies, we'll try to load the categories defined in modules first, then go after the delayed categories defined in extant translation units. This gives us a stable "source order", preserves the Principle of Closest Definition, and undoes the behavior change for overriden property declarations in the previous commit.
CodaFi
force-pushed
the
latent-semantic-mapping
branch
from
079732e to
4ddbb5b
Compare
|
Let's run the gauntlet. |
CodaFi
force-pushed
the
latent-semantic-mapping
branch
from
4ddbb5b to
d00b595
Compare
An unfortunate pessimization that is needed in order to properly build the member table to run override checking. Considering this was happening anyways thanks to re-entrant lookups, there shouldn't actually be a perf hit here.
CodaFi
force-pushed
the
latent-semantic-mapping
branch
from
d00b595 to
4a3bb30
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
|
Hit the workspace error @swift-ci please clean smoke test macOS platform |
|
⛵️ |
The first time we go to import a property, the lookup we run for the override check would run re-entrantly and accidentally wind up doing something desirable in the majority of cases. This patch untangles 3 bugs that were supporting each other:
Blocking re-entrancy in name lookups by installing a table of imported members and using that for the override check. Note that we aren't concerned about no longer being able to find Swift members, since it's unlikely the re-entrant check would have seen them in the first place.
Extension import ordering has to be changed to delay categories in semantically "later" translation units. The Clang Importer accidentally got the order it wanted here because a re-entrant lookup would simply skip categories in, say, imported textual headers which should appear later and walk into modules closer to the defining class which actually appear later.
Re-entrant member loading accidentally deserialized members all the way up the class hierarchy. This meant that override checking always saw an at least partially correct view of the world. Under the new scheme we now have to force this ourselves.