-
Notifications
You must be signed in to change notification settings - Fork 15.4k
release/21.x: ThinLTOBitcodeWriter: Emit __cfi_check to full LTO part of bitcode file. #154859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@efriedma-quic What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-llvm-transforms Author: None (llvmbot) ChangesBackport ff85dbd Requested by: @efriedma-quic Full diff: https://github.com/llvm/llvm-project/pull/154859.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
index e276376f21583..0d631734ca968 100644
--- a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -350,12 +350,20 @@ void splitAndWriteThinLTOBitcode(
});
}
+ auto MustEmitToMergedModule = [](const GlobalValue *GV) {
+ // The __cfi_check definition is filled in by the CrossDSOCFI pass which
+ // runs only in the merged module.
+ return GV->getName() == "__cfi_check";
+ };
+
ValueToValueMapTy VMap;
std::unique_ptr<Module> MergedM(
CloneModule(M, VMap, [&](const GlobalValue *GV) -> bool {
if (const auto *C = GV->getComdat())
if (MergedMComdats.count(C))
return true;
+ if (MustEmitToMergedModule(GV))
+ return true;
if (auto *F = dyn_cast<Function>(GV))
return EligibleVirtualFns.count(F);
if (auto *GVar =
@@ -372,7 +380,7 @@ void splitAndWriteThinLTOBitcode(
cloneUsedGlobalVariables(M, *MergedM, /*CompilerUsed*/ true);
for (Function &F : *MergedM)
- if (!F.isDeclaration()) {
+ if (!F.isDeclaration() && !MustEmitToMergedModule(&F)) {
// Reset the linkage of all functions eligible for virtual constant
// propagation. The canonical definitions live in the thin LTO module so
// that they can be imported.
@@ -394,6 +402,8 @@ void splitAndWriteThinLTOBitcode(
if (const auto *C = GV->getComdat())
if (MergedMComdats.count(C))
return false;
+ if (MustEmitToMergedModule(GV))
+ return false;
return true;
});
diff --git a/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-check.ll b/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-check.ll
new file mode 100644
index 0000000000000..b927af6b92f7c
--- /dev/null
+++ b/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-check.ll
@@ -0,0 +1,19 @@
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
+; RUN: llvm-modextract -b -n 0 -o - %t | llvm-dis | FileCheck --check-prefix=M0 %s
+; RUN: llvm-modextract -b -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=M1 %s
+
+; Check that __cfi_check is emitted on the full LTO side with
+; attributes preserved.
+
+; M0: define void @f()
+define void @f() !type !{!"f1", i32 0} {
+ ret void
+}
+
+; M1: define void @__cfi_check() #0
+define void @__cfi_check() #0 {
+ ret void
+}
+
+; M1: attributes #0 = { "branch-target-enforcement" }
+attributes #0 = { "branch-target-enforcement" }
|
efriedma-quic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The change only affects CFI, and it should be a safe fix.
|
@efriedma-quic (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
The CrossDSOCFI pass runs on the full LTO module and fills in the body of __cfi_check. This function must have the correct attributes in order to be compatible with the rest of the program. For example, when building with -mbranch-protection=standard, the function must have the branch-target-enforcement attribute, which is normally added by Clang. When __cfi_check is missing, CrossDSOCFI will give it the default set of attributes, which are likely incorrect. Therefore, emit __cfi_check to the full LTO part, where CrossDSOCFI will see it. Reviewers: efriedma-quic, vitalybuka, fmayer Reviewed By: efriedma-quic Pull Request: llvm#154833 (cherry picked from commit ff85dbd)
Backport ff85dbd
Requested by: @efriedma-quic