[CFI][LowerTypeTests] Fix indirect call with alias #113987
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
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()`.
igorkudrin
added
LTO
igorkudrin
requested review from
bogner,
eugenis,
teresajohnson and
kstoimenov
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Igor Kudrin (igorkudrin) ChangesThis is a fixed version of #106185, which was reverted in #113978 due to a buildbot failure. Motivation example: 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 Full diff: https://github.com/llvm/llvm-project/pull/113987.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
index 902d1305c818ac..7c405025630c95 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
@@ -135,16 +135,20 @@ template <> struct MappingTraits<TypeIdSummary> {
}
};
-struct FunctionSummaryYaml {
+struct GlobalValueSummaryYaml {
+ // Commonly used fields
unsigned Linkage, Visibility;
bool NotEligibleToImport, Live, IsLocal, CanAutoHide;
unsigned ImportType;
- std::vector<uint64_t> Refs;
- std::vector<uint64_t> TypeTests;
- std::vector<FunctionSummary::VFuncId> TypeTestAssumeVCalls,
- TypeCheckedLoadVCalls;
- std::vector<FunctionSummary::ConstVCall> TypeTestAssumeConstVCalls,
- TypeCheckedLoadConstVCalls;
+ // 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 = {};
+ std::vector<FunctionSummary::VFuncId> TypeCheckedLoadVCalls = {};
+ std::vector<FunctionSummary::ConstVCall> TypeTestAssumeConstVCalls = {};
+ std::vector<FunctionSummary::ConstVCall> TypeCheckedLoadConstVCalls = {};
};
} // End yaml namespace
@@ -176,8 +180,8 @@ LLVM_YAML_IS_SEQUENCE_VECTOR(FunctionSummary::ConstVCall)
namespace llvm {
namespace yaml {
-template <> struct MappingTraits<FunctionSummaryYaml> {
- static void mapping(IO &io, FunctionSummaryYaml& summary) {
+template <> struct MappingTraits<GlobalValueSummaryYaml> {
+ static void mapping(IO &io, GlobalValueSummaryYaml &summary) {
io.mapOptional("Linkage", summary.Linkage);
io.mapOptional("Visibility", summary.Visibility);
io.mapOptional("NotEligibleToImport", summary.NotEligibleToImport);
@@ -185,6 +189,7 @@ template <> struct MappingTraits<FunctionSummaryYaml> {
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);
@@ -199,7 +204,7 @@ template <> struct MappingTraits<FunctionSummaryYaml> {
} // End yaml namespace
} // End llvm namespace
-LLVM_YAML_IS_SEQUENCE_VECTOR(FunctionSummaryYaml)
+LLVM_YAML_IS_SEQUENCE_VECTOR(GlobalValueSummaryYaml)
namespace llvm {
namespace yaml {
@@ -207,61 +212,99 @@ 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<FunctionSummaryYaml> FSums;
- io.mapRequired(Key.str().c_str(), FSums);
+ std::vector<GlobalValueSummaryYaml> GVSums;
+ io.mapRequired(Key.str().c_str(), GVSums);
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 &FSum : FSums) {
+ 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;
+ }
SmallVector<ValueInfo, 0> Refs;
- Refs.reserve(FSum.Refs.size());
- for (auto &RefGUID : FSum.Refs) {
+ Refs.reserve(GVSum.Refs.size());
+ for (auto &RefGUID : GVSum.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>(
- 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),
+ 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),
ArrayRef<FunctionSummary::ParamAccess>{}, ArrayRef<CallsiteInfo>{},
ArrayRef<AllocInfo>{}));
}
}
static void output(IO &io, GlobalValueSummaryMapTy &V) {
for (auto &P : V) {
- std::vector<FunctionSummaryYaml> FSums;
+ std::vector<GlobalValueSummaryYaml> GVSums;
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());
- FSums.push_back(FunctionSummaryYaml{
+ GVSums.push_back(GlobalValueSummaryYaml{
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, Refs, FSum->type_tests(),
- FSum->type_test_assume_vcalls(), FSum->type_checked_load_vcalls(),
+ FSum->flags().ImportType, /*Aliasee=*/std::nullopt, 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);
}
}
};
@@ -281,6 +324,9 @@ 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 3fcfc6a876776d..6ba371069bb230 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -2083,8 +2083,12 @@ 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
new file mode 100644
index 00000000000000..0c5324ee96c939
--- /dev/null
+++ b/llvm/test/Transforms/LowerTypeTests/cfi-icall-alias.ll
@@ -0,0 +1,54 @@
+;; 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
changed the title
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
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.
This is a fixed version of #106185, which was reverted in #113978 due to a buildbot failure.
Motivation example:
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 tofalse, and consequently the indirect call to the function was eventually optimized toubsantrap().