[CFI][LowerTypeTests] Fix indirect call with alias #106185
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
igorkudrin
added
LTO
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-lto Author: Igor Kudrin (igorkudrin) ChangesMotivation example: If the address of a function is only taken through its alias, the function was not considered as exported and therefore was not included in the CFI jumptable. This resulted in Full diff: https://github.com/llvm/llvm-project/pull/106185.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
index b2747d24c5396d..0adafd60fdc8fd 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
@@ -135,10 +135,14 @@ template <> struct MappingTraits<TypeIdSummary> {
}
};
-struct FunctionSummaryYaml {
+struct GlobalValueSummaryYaml {
+ // Commonly used fields
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,
@@ -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,8 +212,8 @@ 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");
@@ -217,52 +222,90 @@ template <> struct CustomMappingTraits<GlobalValueSummaryMapTy> {
if (!V.count(KeyInt))
V.emplace(KeyInt, /*IsAnalysis=*/false);
auto &Elem = V.find(KeyInt)->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;
+ }
std::vector<ValueInfo> Refs;
- for (auto &RefGUID : FSum.Refs) {
+ for (auto &RefGUID : GVSum.Refs) {
if (!V.count(RefGUID))
V.emplace(RefGUID, /*IsAnalysis=*/false);
Refs.push_back(ValueInfo(/*IsAnalysis=*/false, &*V.find(RefGUID)));
}
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{}, /*EntryCount=*/0, Refs,
- ArrayRef<FunctionSummary::EdgeTy>{}, 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{}, /*EntryCount=*/0,
+ Refs, ArrayRef<FunctionSummary::EdgeTy>{}, 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;
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);
}
}
};
@@ -282,6 +325,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 0742b259c489c0..81ac4bd811c41f 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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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 is only taken through its alias, the
function was not considered as 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 `unsantrap()`.
igorkudrin
force-pushed
the
cfi-icall-alias
branch
from
041c9bf to
e2e41a8
Compare
eugenis
reviewed
Sep 4, 2024
|
Ping |
1 similar comment
|
Ping |
teresajohnson
approved these changes
Oct 26, 2024
|
Seeing some new warnings from clang after this change: These are visible on the pre-checkin job as well if you download the log: https://buildkite.com/llvm-project/github-pull-requests/builds/106415#01924ffe-ee6f-469e-98c3-e00581b5cd73 This looks benign but it might be worth explicitly defaulting the function summary members' initializations or giving |
|
This is breaking Sanitizer bots: https://lab.llvm.org/buildbot/#/builders/66/builds/5449/steps/8/logs/stdio Please revert or fix. |
igorkudrin
added a commit
that referenced
this pull request
Oct 28, 2024
This reverts commit 67bcce2.
igorkudrin
added a commit
that referenced
this pull request
Oct 28, 2024
Reverts #106185 This is breaking Sanitizer bots: https://lab.llvm.org/buildbot/#/builders/66/builds/5449/steps/8/logs/stdio
I added the initialization of function summary members in #113987, please take a look. |
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
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.
Motivation example:
If the address of a function is only taken through its alias, the function was not considered as 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().