[InstallAPI] Tie lifetime of FE objects to DylibVerifier #88189
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
cyndyishida
added
the
skip-precommit-approval
|
@llvm/pr-subscribers-clang Author: Cyndy Ishida (cyndyishida) ChangesA few verification checks need to happen until all AST's have been traversed, specifically for zippered framework checking. To keep source location until that time valid, hold onto to references of FrontendRecords + SourceManager. Full diff: https://github.com/llvm/llvm-project/pull/88189.diff 6 Files Affected:
diff --git a/clang/include/clang/InstallAPI/DylibVerifier.h b/clang/include/clang/InstallAPI/DylibVerifier.h
index 07dbd3bf5f2b10..a3df25f10de4b1 100644
--- a/clang/include/clang/InstallAPI/DylibVerifier.h
+++ b/clang/include/clang/InstallAPI/DylibVerifier.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_INSTALLAPI_DYLIBVERIFIER_H
#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceManager.h"
#include "clang/InstallAPI/MachO.h"
namespace clang {
@@ -99,11 +100,7 @@ class DylibVerifier : llvm::MachO::RecordVisitor {
Result getState() const { return Ctx.FrontendState; }
/// Set different source managers to the same diagnostics engine.
- void setSourceManager(SourceManager &SourceMgr) const {
- if (!Ctx.Diag)
- return;
- Ctx.Diag->setSourceManager(&SourceMgr);
- }
+ void setSourceManager(IntrusiveRefCntPtr<SourceManager> SourceMgr);
private:
/// Determine whether to compare declaration to symbol in binary.
@@ -190,6 +187,9 @@ class DylibVerifier : llvm::MachO::RecordVisitor {
// Track DWARF provided source location for dylibs.
DWARFContext *DWARFCtx = nullptr;
+
+ // Source manager for each unique compiler instance.
+ llvm::SmallVector<IntrusiveRefCntPtr<SourceManager>, 12> SourceManagers;
};
} // namespace installapi
diff --git a/clang/include/clang/InstallAPI/Frontend.h b/clang/include/clang/InstallAPI/Frontend.h
index 5cccd891c58093..bc4e77de2b7256 100644
--- a/clang/include/clang/InstallAPI/Frontend.h
+++ b/clang/include/clang/InstallAPI/Frontend.h
@@ -36,7 +36,7 @@ class InstallAPIAction : public ASTFrontendAction {
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
StringRef InFile) override {
Ctx.Diags->getClient()->BeginSourceFile(CI.getLangOpts());
- Ctx.Verifier->setSourceManager(CI.getSourceManager());
+ Ctx.Verifier->setSourceManager(CI.getSourceManagerPtr());
return std::make_unique<InstallAPIVisitor>(
CI.getASTContext(), Ctx, CI.getSourceManager(), CI.getPreprocessor());
}
diff --git a/clang/include/clang/InstallAPI/FrontendRecords.h b/clang/include/clang/InstallAPI/FrontendRecords.h
index 59271e81e230c2..ef82398addd7ac 100644
--- a/clang/include/clang/InstallAPI/FrontendRecords.h
+++ b/clang/include/clang/InstallAPI/FrontendRecords.h
@@ -21,6 +21,7 @@ namespace installapi {
struct FrontendAttrs {
const AvailabilityInfo Avail;
const Decl *D;
+ const SourceLocation Loc;
const HeaderType Access;
};
diff --git a/clang/lib/InstallAPI/DylibVerifier.cpp b/clang/lib/InstallAPI/DylibVerifier.cpp
index 2387ee0e78ad56..4fa2d4e9292c72 100644
--- a/clang/lib/InstallAPI/DylibVerifier.cpp
+++ b/clang/lib/InstallAPI/DylibVerifier.cpp
@@ -214,16 +214,16 @@ bool DylibVerifier::compareObjCInterfaceSymbols(const Record *R,
StringRef SymName, bool PrintAsWarning = false) {
if (SymLinkage == RecordLinkage::Unknown)
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- PrintAsWarning ? diag::warn_library_missing_symbol
- : diag::err_library_missing_symbol)
+ Ctx.Diag->Report(SymCtx.FA->Loc, PrintAsWarning
+ ? diag::warn_library_missing_symbol
+ : diag::err_library_missing_symbol)
<< SymName;
});
else
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- PrintAsWarning ? diag::warn_library_hidden_symbol
- : diag::err_library_hidden_symbol)
+ Ctx.Diag->Report(SymCtx.FA->Loc, PrintAsWarning
+ ? diag::warn_library_hidden_symbol
+ : diag::err_library_hidden_symbol)
<< SymName;
});
};
@@ -270,16 +270,14 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
if (R->isExported()) {
if (!DR) {
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_library_missing_symbol)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_library_missing_symbol)
<< getAnnotatedName(R, SymCtx);
});
return Result::Invalid;
}
if (DR->isInternal()) {
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_library_hidden_symbol)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_library_hidden_symbol)
<< getAnnotatedName(R, SymCtx);
});
return Result::Invalid;
@@ -306,8 +304,7 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
Outcome = Result::Invalid;
}
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(), ID)
- << getAnnotatedName(R, SymCtx);
+ Ctx.Diag->Report(SymCtx.FA->Loc, ID) << getAnnotatedName(R, SymCtx);
});
return Outcome;
}
@@ -329,15 +326,13 @@ DylibVerifier::Result DylibVerifier::compareAvailability(const Record *R,
switch (Mode) {
case VerificationMode::ErrorsAndWarnings:
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::warn_header_availability_mismatch)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::warn_header_availability_mismatch)
<< getAnnotatedName(R, SymCtx) << IsDeclAvailable << IsDeclAvailable;
});
return Result::Ignore;
case VerificationMode::Pedantic:
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_header_availability_mismatch)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_header_availability_mismatch)
<< getAnnotatedName(R, SymCtx) << IsDeclAvailable << IsDeclAvailable;
});
return Result::Invalid;
@@ -353,16 +348,14 @@ bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
const Record *DR) {
if (DR->isThreadLocalValue() && !R->isThreadLocalValue()) {
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_dylib_symbol_flags_mismatch)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_dylib_symbol_flags_mismatch)
<< getAnnotatedName(DR, SymCtx) << DR->isThreadLocalValue();
});
return false;
}
if (!DR->isThreadLocalValue() && R->isThreadLocalValue()) {
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_header_symbol_flags_mismatch)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_header_symbol_flags_mismatch)
<< getAnnotatedName(R, SymCtx) << R->isThreadLocalValue();
});
return false;
@@ -370,16 +363,14 @@ bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
if (DR->isWeakDefined() && !R->isWeakDefined()) {
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_dylib_symbol_flags_mismatch)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_dylib_symbol_flags_mismatch)
<< getAnnotatedName(DR, SymCtx) << R->isWeakDefined();
});
return false;
}
if (!DR->isWeakDefined() && R->isWeakDefined()) {
Ctx.emitDiag([&]() {
- Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
- diag::err_header_symbol_flags_mismatch)
+ Ctx.Diag->Report(SymCtx.FA->Loc, diag::err_header_symbol_flags_mismatch)
<< getAnnotatedName(R, SymCtx) << R->isWeakDefined();
});
return false;
@@ -487,6 +478,14 @@ void DylibVerifier::setTarget(const Target &T) {
assignSlice(T);
}
+void DylibVerifier::setSourceManager(
+ IntrusiveRefCntPtr<SourceManager> SourceMgr) {
+ if (!Ctx.Diag)
+ return;
+ SourceManagers.push_back(std::move(SourceMgr));
+ Ctx.Diag->setSourceManager(SourceManagers.back().get());
+}
+
DylibVerifier::Result DylibVerifier::verify(ObjCIVarRecord *R,
const FrontendAttrs *FA,
const StringRef SuperClass) {
diff --git a/clang/lib/InstallAPI/Frontend.cpp b/clang/lib/InstallAPI/Frontend.cpp
index bd7589c13e043f..04d06f46d26520 100644
--- a/clang/lib/InstallAPI/Frontend.cpp
+++ b/clang/lib/InstallAPI/Frontend.cpp
@@ -23,7 +23,8 @@ std::pair<GlobalRecord *, FrontendAttrs *> FrontendRecordsSlice::addGlobal(
GlobalRecord *GR =
llvm::MachO::RecordsSlice::addGlobal(Name, Linkage, GV, Flags, Inlined);
- auto Result = FrontendRecords.insert({GR, FrontendAttrs{Avail, D, Access}});
+ auto Result = FrontendRecords.insert(
+ {GR, FrontendAttrs{Avail, D, D->getLocation(), Access}});
return {GR, &(Result.first->second)};
}
@@ -39,8 +40,8 @@ FrontendRecordsSlice::addObjCInterface(StringRef Name, RecordLinkage Linkage,
ObjCInterfaceRecord *ObjCR =
llvm::MachO::RecordsSlice::addObjCInterface(Name, Linkage, SymType);
- auto Result =
- FrontendRecords.insert({ObjCR, FrontendAttrs{Avail, D, Access}});
+ auto Result = FrontendRecords.insert(
+ {ObjCR, FrontendAttrs{Avail, D, D->getLocation(), Access}});
return {ObjCR, &(Result.first->second)};
}
@@ -51,8 +52,8 @@ FrontendRecordsSlice::addObjCCategory(StringRef ClassToExtend,
const Decl *D, HeaderType Access) {
ObjCCategoryRecord *ObjCR =
llvm::MachO::RecordsSlice::addObjCCategory(ClassToExtend, CategoryName);
- auto Result =
- FrontendRecords.insert({ObjCR, FrontendAttrs{Avail, D, Access}});
+ auto Result = FrontendRecords.insert(
+ {ObjCR, FrontendAttrs{Avail, D, D->getLocation(), Access}});
return {ObjCR, &(Result.first->second)};
}
@@ -67,8 +68,8 @@ std::pair<ObjCIVarRecord *, FrontendAttrs *> FrontendRecordsSlice::addObjCIVar(
Linkage = RecordLinkage::Internal;
ObjCIVarRecord *ObjCR =
llvm::MachO::RecordsSlice::addObjCIVar(Container, IvarName, Linkage);
- auto Result =
- FrontendRecords.insert({ObjCR, FrontendAttrs{Avail, D, Access}});
+ auto Result = FrontendRecords.insert(
+ {ObjCR, FrontendAttrs{Avail, D, D->getLocation(), Access}});
return {ObjCR, &(Result.first->second)};
}
diff --git a/clang/tools/clang-installapi/ClangInstallAPI.cpp b/clang/tools/clang-installapi/ClangInstallAPI.cpp
index 0c1980ba5d6280..fd71aaec59435b 100644
--- a/clang/tools/clang-installapi/ClangInstallAPI.cpp
+++ b/clang/tools/clang-installapi/ClangInstallAPI.cpp
@@ -121,6 +121,7 @@ static bool run(ArrayRef<const char *> Args, const char *ProgName) {
// Execute, verify and gather AST results.
// An invocation is ran for each unique target triple and for each header
// access level.
+ Records FrontendRecords;
for (const auto &[Targ, Trip] : Opts.DriverOpts.Targets) {
Ctx.Verifier->setTarget(Targ);
Ctx.Slice = std::make_shared<FrontendRecordsSlice>(Trip);
@@ -131,6 +132,7 @@ static bool run(ArrayRef<const char *> Args, const char *ProgName) {
InMemoryFileSystem.get(), Opts.getClangFrontendArgs()))
return EXIT_FAILURE;
}
+ FrontendRecords.emplace_back(std::move(Ctx.Slice));
}
if (Ctx.Verifier->verifyRemainingSymbols() == DylibVerifier::Result::Invalid)
|
A few verification checks need to happen until all AST's have been traversed, specifically for zippered framework checking. To keep source location until that time valid, hold onto to references of FrontendRecords + SourceManager.
cyndyishida
force-pushed
the
eng/PR-zipperPrep
branch
from
d74c47d to
a7894f9
Compare
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.
A few verification checks need to happen until all AST's have been traversed, specifically for zippered framework checking. To keep source location until that time valid, hold onto to references of FrontendRecords + SourceManager.