Revert "[CFI][LowerTypeTests] Fix indirect call with alias" #113978
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
This reverts commit 67bcce2.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: Igor Kudrin (igorkudrin) ChangesReverts llvm/llvm-project#106185 This is breaking Sanitizer bots: https://lab.llvm.org/buildbot/#/builders/66/builds/5449/steps/8/logs/stdio Full diff: https://github.com/llvm/llvm-project/pull/113978.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
index d12bc260f5cf4e..902d1305c818ac 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
@@ -135,14 +135,10 @@ template <> struct MappingTraits<TypeIdSummary> {
}
};
-struct GlobalValueSummaryYaml {
- // Commonly used fields
+struct FunctionSummaryYaml {
unsigned Linkage, Visibility;
bool NotEligibleToImport, Live, IsLocal, CanAutoHide;
unsigned ImportType;
- // Fields for AliasSummary
- std::optional<uint64_t> Aliasee;
- // Fields for FunctionSummary
std::vector<uint64_t> Refs;
std::vector<uint64_t> TypeTests;
std::vector<FunctionSummary::VFuncId> TypeTestAssumeVCalls,
@@ -180,8 +176,8 @@ LLVM_YAML_IS_SEQUENCE_VECTOR(FunctionSummary::ConstVCall)
namespace llvm {
namespace yaml {
-template <> struct MappingTraits<GlobalValueSummaryYaml> {
- static void mapping(IO &io, GlobalValueSummaryYaml &summary) {
+template <> struct MappingTraits<FunctionSummaryYaml> {
+ static void mapping(IO &io, FunctionSummaryYaml& summary) {
io.mapOptional("Linkage", summary.Linkage);
io.mapOptional("Visibility", summary.Visibility);
io.mapOptional("NotEligibleToImport", summary.NotEligibleToImport);
@@ -189,7 +185,6 @@ template <> struct MappingTraits<GlobalValueSummaryYaml> {
io.mapOptional("Local", summary.IsLocal);
io.mapOptional("CanAutoHide", summary.CanAutoHide);
io.mapOptional("ImportType", summary.ImportType);
- io.mapOptional("Aliasee", summary.Aliasee);
io.mapOptional("Refs", summary.Refs);
io.mapOptional("TypeTests", summary.TypeTests);
io.mapOptional("TypeTestAssumeVCalls", summary.TypeTestAssumeVCalls);
@@ -204,7 +199,7 @@ template <> struct MappingTraits<GlobalValueSummaryYaml> {
} // End yaml namespace
} // End llvm namespace
-LLVM_YAML_IS_SEQUENCE_VECTOR(GlobalValueSummaryYaml)
+LLVM_YAML_IS_SEQUENCE_VECTOR(FunctionSummaryYaml)
namespace llvm {
namespace yaml {
@@ -212,99 +207,61 @@ namespace yaml {
// FIXME: Add YAML mappings for the rest of the module summary.
template <> struct CustomMappingTraits<GlobalValueSummaryMapTy> {
static void inputOne(IO &io, StringRef Key, GlobalValueSummaryMapTy &V) {
- std::vector<GlobalValueSummaryYaml> GVSums;
- io.mapRequired(Key.str().c_str(), GVSums);
+ std::vector<FunctionSummaryYaml> FSums;
+ io.mapRequired(Key.str().c_str(), FSums);
uint64_t KeyInt;
if (Key.getAsInteger(0, KeyInt)) {
io.setError("key not an integer");
return;
}
auto &Elem = V.try_emplace(KeyInt, /*IsAnalysis=*/false).first->second;
- for (auto &GVSum : GVSums) {
- GlobalValueSummary::GVFlags GVFlags(
- static_cast<GlobalValue::LinkageTypes>(GVSum.Linkage),
- static_cast<GlobalValue::VisibilityTypes>(GVSum.Visibility),
- GVSum.NotEligibleToImport, GVSum.Live, GVSum.IsLocal,
- GVSum.CanAutoHide,
- static_cast<GlobalValueSummary::ImportKind>(GVSum.ImportType));
- if (GVSum.Aliasee) {
- auto ASum = std::make_unique<AliasSummary>(GVFlags);
- if (!V.count(*GVSum.Aliasee))
- V.emplace(*GVSum.Aliasee, /*IsAnalysis=*/false);
- ValueInfo AliaseeVI(/*IsAnalysis=*/false, &*V.find(*GVSum.Aliasee));
- // Note: Aliasee cannot be filled until all summaries are loaded.
- // This is done in fixAliaseeLinks() which is called in
- // MappingTraits<ModuleSummaryIndex>::mapping().
- ASum->setAliasee(AliaseeVI, /*Aliasee=*/nullptr);
- Elem.SummaryList.push_back(std::move(ASum));
- continue;
- }
+ for (auto &FSum : FSums) {
SmallVector<ValueInfo, 0> Refs;
- Refs.reserve(GVSum.Refs.size());
- for (auto &RefGUID : GVSum.Refs) {
+ Refs.reserve(FSum.Refs.size());
+ for (auto &RefGUID : FSum.Refs) {
auto It = V.try_emplace(RefGUID, /*IsAnalysis=*/false).first;
Refs.push_back(ValueInfo(/*IsAnalysis=*/false, &*It));
}
Elem.SummaryList.push_back(std::make_unique<FunctionSummary>(
- GVFlags, /*NumInsts=*/0, FunctionSummary::FFlags{}, std::move(Refs),
- SmallVector<FunctionSummary::EdgeTy, 0>{}, std::move(GVSum.TypeTests),
- std::move(GVSum.TypeTestAssumeVCalls),
- std::move(GVSum.TypeCheckedLoadVCalls),
- std::move(GVSum.TypeTestAssumeConstVCalls),
- std::move(GVSum.TypeCheckedLoadConstVCalls),
+ GlobalValueSummary::GVFlags(
+ static_cast<GlobalValue::LinkageTypes>(FSum.Linkage),
+ static_cast<GlobalValue::VisibilityTypes>(FSum.Visibility),
+ FSum.NotEligibleToImport, FSum.Live, FSum.IsLocal,
+ FSum.CanAutoHide,
+ static_cast<GlobalValueSummary::ImportKind>(FSum.ImportType)),
+ /*NumInsts=*/0, FunctionSummary::FFlags{}, std::move(Refs),
+ SmallVector<FunctionSummary::EdgeTy, 0>{}, std::move(FSum.TypeTests),
+ std::move(FSum.TypeTestAssumeVCalls),
+ std::move(FSum.TypeCheckedLoadVCalls),
+ std::move(FSum.TypeTestAssumeConstVCalls),
+ std::move(FSum.TypeCheckedLoadConstVCalls),
ArrayRef<FunctionSummary::ParamAccess>{}, ArrayRef<CallsiteInfo>{},
ArrayRef<AllocInfo>{}));
}
}
static void output(IO &io, GlobalValueSummaryMapTy &V) {
for (auto &P : V) {
- std::vector<GlobalValueSummaryYaml> GVSums;
+ std::vector<FunctionSummaryYaml> FSums;
for (auto &Sum : P.second.SummaryList) {
if (auto *FSum = dyn_cast<FunctionSummary>(Sum.get())) {
std::vector<uint64_t> Refs;
Refs.reserve(FSum->refs().size());
for (auto &VI : FSum->refs())
Refs.push_back(VI.getGUID());
- GVSums.push_back(GlobalValueSummaryYaml{
+ FSums.push_back(FunctionSummaryYaml{
FSum->flags().Linkage, FSum->flags().Visibility,
static_cast<bool>(FSum->flags().NotEligibleToImport),
static_cast<bool>(FSum->flags().Live),
static_cast<bool>(FSum->flags().DSOLocal),
static_cast<bool>(FSum->flags().CanAutoHide),
- FSum->flags().ImportType, /*Aliasee=*/std::nullopt, Refs,
- FSum->type_tests(), FSum->type_test_assume_vcalls(),
- FSum->type_checked_load_vcalls(),
+ FSum->flags().ImportType, Refs, FSum->type_tests(),
+ FSum->type_test_assume_vcalls(), FSum->type_checked_load_vcalls(),
FSum->type_test_assume_const_vcalls(),
FSum->type_checked_load_const_vcalls()});
- } else if (auto *ASum = dyn_cast<AliasSummary>(Sum.get());
- ASum && ASum->hasAliasee()) {
- GVSums.push_back(GlobalValueSummaryYaml{
- ASum->flags().Linkage, ASum->flags().Visibility,
- static_cast<bool>(ASum->flags().NotEligibleToImport),
- static_cast<bool>(ASum->flags().Live),
- static_cast<bool>(ASum->flags().DSOLocal),
- static_cast<bool>(ASum->flags().CanAutoHide),
- ASum->flags().ImportType,
- /*Aliasee=*/ASum->getAliaseeGUID()});
- }
- }
- if (!GVSums.empty())
- io.mapRequired(llvm::utostr(P.first).c_str(), GVSums);
- }
- }
- static void fixAliaseeLinks(GlobalValueSummaryMapTy &V) {
- for (auto &P : V) {
- for (auto &Sum : P.second.SummaryList) {
- if (auto *Alias = dyn_cast<AliasSummary>(Sum.get())) {
- ValueInfo AliaseeVI = Alias->getAliaseeVI();
- auto AliaseeSL = AliaseeVI.getSummaryList();
- if (AliaseeSL.empty()) {
- ValueInfo EmptyVI;
- Alias->setAliasee(EmptyVI, nullptr);
- } else
- Alias->setAliasee(AliaseeVI, AliaseeSL[0].get());
- }
+ }
}
+ if (!FSums.empty())
+ io.mapRequired(llvm::utostr(P.first).c_str(), FSums);
}
}
};
@@ -324,9 +281,6 @@ template <> struct CustomMappingTraits<TypeIdSummaryMapTy> {
template <> struct MappingTraits<ModuleSummaryIndex> {
static void mapping(IO &io, ModuleSummaryIndex& index) {
io.mapOptional("GlobalValueMap", index.GlobalValueMap);
- if (!io.outputting())
- CustomMappingTraits<GlobalValueSummaryMapTy>::fixAliaseeLinks(
- index.GlobalValueMap);
io.mapOptional("TypeIdMap", index.TypeIdMap);
io.mapOptional("WithGlobalValueDeadStripping",
index.WithGlobalValueDeadStripping);
diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index 6ba371069bb230..3fcfc6a876776d 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -2083,12 +2083,8 @@ bool LowerTypeTestsModule::lower() {
for (auto &I : *ExportSummary)
for (auto &GVS : I.second.SummaryList)
if (GVS->isLive())
- for (const auto &Ref : GVS->refs()) {
+ for (const auto &Ref : GVS->refs())
AddressTaken.insert(Ref.getGUID());
- for (auto &RefGVS : Ref.getSummaryList())
- if (auto Alias = dyn_cast<AliasSummary>(RefGVS.get()))
- AddressTaken.insert(Alias->getAliaseeGUID());
- }
NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions");
if (CfiFunctionsMD) {
diff --git a/llvm/test/Transforms/LowerTypeTests/cfi-icall-alias.ll b/llvm/test/Transforms/LowerTypeTests/cfi-icall-alias.ll
deleted file mode 100644
index 0c5324ee96c939..00000000000000
--- a/llvm/test/Transforms/LowerTypeTests/cfi-icall-alias.ll
+++ /dev/null
@@ -1,54 +0,0 @@
-;; Check that if the address of a weak function is only taken through an alias,
-;; it is still added to a list of exported functions and @llvm.type.test() is
-;; lowered to an actual check against the generated CFI jumptable.
-
-RUN: rm -rf %t.dir && split-file %s %t.dir && cd %t.dir
-RUN: opt test.ll --thinlto-bc --thinlto-split-lto-unit -o test.bc
-RUN: llvm-modextract test.bc -n 0 -o test0.bc
-RUN: llvm-modextract test.bc -n 1 -o test1.bc
-
-;; Check that a CFI jumptable is generated.
-RUN: opt test1.bc -passes=lowertypetests -lowertypetests-read-summary=in.yaml \
-RUN: -lowertypetests-summary-action=export -lowertypetests-write-summary=exported.yaml \
-RUN: -S -o - | FileCheck %s --check-prefix=REGULAR
-REGULAR: @__typeid__ZTSFvvE_global_addr = hidden alias i8, ptr @.cfi.jumptable
-REGULAR: @f = alias void (), ptr @.cfi.jumptable
-REGULAR: define private void @.cfi.jumptable()
-
-;; CHECK that @llvm.type.test() is lowered to an actual check.
-RUN: opt test0.bc -passes=lowertypetests -lowertypetests-read-summary=exported.yaml \
-RUN: -lowertypetests-summary-action=import -S -o - | FileCheck %s --check-prefix=THIN
-THIN: define i1 @test() {
-THIN-NEXT: %1 = icmp eq i64 ptrtoint (ptr @alias to i64), ptrtoint (ptr @__typeid__ZTSFvvE_global_addr to i64)
-THIN-NEXT: ret i1 %1
-THIN-NEXT: }
-
-;--- test.ll
-target triple = "x86_64-pc-linux-gnu"
-
-@alias = alias void(), ptr @f
-
-define weak void @f() !type !0 {
- ret void
-}
-
-define i1 @test() {
- %1 = call i1 @llvm.type.test(ptr nonnull @alias, metadata !"_ZTSFvvE")
- ret i1 %1
-}
-
-declare i1 @llvm.type.test(ptr, metadata)
-
-!0 = !{i64 0, !"_ZTSFvvE"}
-;--- in.yaml
----
-GlobalValueMap:
- 8346051122425466633: # guid("test")
- - Live: true
- Refs: [5833419078793185394] # guid("alias")
- TypeTests: [9080559750644022485] # guid("_ZTSFvvE")
- 5833419078793185394: # guid("alias")
- - Aliasee: 14740650423002898831 # guid("f")
- 14740650423002898831: # guid("f")
- -
-...
|
igorkudrin
added a commit
that referenced
this pull request
Oct 31, 2024
This is a fixed version of #106185, which was reverted in #113978 due to a buildbot failure. Motivation example: ``` > cat test.cpp extern "C" [[gnu::weak]] void f() {} void alias() __attribute__((alias("f"))); int main() { auto p = alias; p(); } > clang test.cpp -fsanitize=cfi-icall -flto=thin -fuse-ld=lld > ./a.out [1] 1868 illegal hardware instruction ./a.out ``` If the address of a function was only taken through its alias, the function was not considered exported and therefore was not included in the CFI jumptable. This resulted in `@llvm.type.test()` being lowered to `false`, and consequently the indirect call to the function was eventually optimized to `ubsantrap()`.
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
This is a fixed version of llvm#106185, which was reverted in llvm#113978 due to a buildbot failure. Motivation example: ``` > cat test.cpp extern "C" [[gnu::weak]] void f() {} void alias() __attribute__((alias("f"))); int main() { auto p = alias; p(); } > clang test.cpp -fsanitize=cfi-icall -flto=thin -fuse-ld=lld > ./a.out [1] 1868 illegal hardware instruction ./a.out ``` If the address of a function was only taken through its alias, the function was not considered exported and therefore was not included in the CFI jumptable. This resulted in `@llvm.type.test()` being lowered to `false`, and consequently the indirect call to the function was eventually optimized to `ubsantrap()`.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
) Reverts llvm#106185 This is breaking Sanitizer bots: https://lab.llvm.org/buildbot/#/builders/66/builds/5449/steps/8/logs/stdio
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This is a fixed version of llvm#106185, which was reverted in llvm#113978 due to a buildbot failure. Motivation example: ``` > cat test.cpp extern "C" [[gnu::weak]] void f() {} void alias() __attribute__((alias("f"))); int main() { auto p = alias; p(); } > clang test.cpp -fsanitize=cfi-icall -flto=thin -fuse-ld=lld > ./a.out [1] 1868 illegal hardware instruction ./a.out ``` If the address of a function was only taken through its alias, the function was not considered exported and therefore was not included in the CFI jumptable. This resulted in `@llvm.type.test()` being lowered to `false`, and consequently the indirect call to the function was eventually optimized to `ubsantrap()`.
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.
Reverts #106185
This is breaking Sanitizer bots: https://lab.llvm.org/buildbot/#/builders/66/builds/5449/steps/8/logs/stdio