ClangImporter: Take ownership of clang::TargetOptions used for code generation
#82912
Conversation
AnthonyLatsis
requested review from
zoecarver,
egorzhdan,
beccadax,
ian-twilightcoder,
Xazax-hun,
j-hui,
fahadnayyar and
susmonteiro
as code owners
|
@swift-ci please smoke test |
|
preset=buildbot_incremental,tools=RA,stdlib=RA @swift-ci please test with preset macOS |
| clang::MacroBuilder builder(predefines); | ||
| targetInfo->getTargetDefines(Instance->getLangOpts(), builder); | ||
| } else { | ||
| targetInfo = |
There was a problem hiding this comment.
Is my understanding right that TargetOpts remains nullptr when we use the existing invocation? I wonder if this can get a bit confusing that we don't have one unique way to refer to TargetOpts regardless of which path we took.
There was a problem hiding this comment.
Yes, correct. That’s pretty much why I didn’t add a getter for TargetOpts. It’s just there to keep the options alive for as long as CodeGenTargetInfo when they differ from the importer invocation’s. You’re still supposed to access them through the invocation or getCodeGenTargetInfo, depending on which you want.
There was a problem hiding this comment.
Added a doc comment to TargetOpts.
generation Since llvm/llvm-project#106271, `clang::TargetInfo` does not co-own target options, so there is no longer anything to keep them alive after we discard the throwaway Clang invocation they originate from. Make the importer take ownership of a copy of code generation target options to avoid a use after free.
|
Targeted test failures gone; merging. |
Since llvm/llvm-project#106271,
clang::TargetInfodoes not co-own target options, so there is no longer anything to keep them alive after we discard the throwaway Clang invocation they originate from. Make the importer take ownership of a copy of code generation target options to avoid a use after free.