[Frontend] Clean Up Usage of UnifiedStatsReporter #30109
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please test |
|
And because Windows is pretty good at surfacing use-after-moves: @swift-ci please test Windows platform |
davidungar
approved these changes
Feb 28, 2020
The lifetime of the UnifiedStatsReporter was not entirely clear from context. Stick it in the CompilerInstance instead so it can live as long as the compile job. It is critical that its lifetime be extended beyond that of the ASTContext, as the context may be torn down by the time code generation happens, but additional statistics are recorded during LLVM codegen.
The only non-trivial bit is making the DiagnosticEngine parameter to swift::performLLVM required. No callers were taking advantage of this parameter allowing NULL.
Now that the CompilerInstance owns the stats reporter, use that instead.
Centralize part of the routine that selects which resources to free. Then, add an additional condition for -dump-api-path. Before, if this option were specified along with -emit-llvm or -c, the compiler would try to rebuild the torn-down ModuleDecl and crash trying to access the torn-down ASTContext.
CodaFi
force-pushed
the
lies-more-lies-and-statistics
branch
from
05fb938 to
a6651a9
Compare
|
@swift-ci smoke test |
|
@swift-ci test Windows platform |
|
Build failed |
|
Build failed |
|
Glad my thoughts made sense. Thanks for using them. |
|
@swift-ci please test Windows platform |
|
@swift-ci smoke test Linux platform |
1 similar comment
|
@swift-ci smoke test Linux platform |
|
Windows seems out for maintenance. ⛵️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch does a bunch of NFC things, and one particularly not NFC thing to fix a gigantic use-after-free in frontend jobs that pass
-dump-api-path.NFC Stuff:
Not-so-NFC Stuff:
-dump-api-pathand codegen with single primaries.The routine that frees up some memory by releasing the ASTContext and SILModules conditionally could potentially be run which would destroy the resources needed by that frontend action. See the last commit.