Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions clang/test/CodeGen/fat-lto-objects-cfi.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// REQUIRES: x86-registered-target

// RUN: %clangxx --target=x86_64-unknown-fuchsia -O2 -flto -ffat-lto-objects \
// RUN: -fsanitize=cfi -fvisibility=hidden -S -emit-llvm -o - %s \
// RUN: | FileCheck %s

// CHECK: llvm.embedded.object
// CHECK-SAME: section ".llvm.lto"

// CHECK-LABEL: define hidden void @foo
// CHECK: entry:
// CHECK-NEXT: %cmp14.not = icmp eq i64 %len, 0
// CHECK-NEXT: br i1 %cmp14.not, label %for.end7, label %for.cond1.preheader.preheader
// CHECK: for.cond1.preheader.preheader: ; preds = %entry
// CHECK-NEXT: %arrayidx.1 = getelementptr inbounds nuw i8, ptr %ptr, i64 4
// CHECK-NEXT: br label %for.cond1.preheader

// CHECK-NOT: @llvm.type.test

// The code below is a reduced case from https://github.com/llvm/llvm-project/issues/112053
#define __PRINTFLIKE(__fmt, __varargs) __attribute__((__format__(__printf__, __fmt, __varargs)))
typedef void func(void* arg, const char* fmt, ...) __PRINTFLIKE(2, 3);
typedef __SIZE_TYPE__ size_t;
typedef unsigned long uintptr_t;

extern "C"
void foo(const void* ptr, size_t len, long disp_addr,
func* printf_func, void* printf_arg) {
uintptr_t address = (uintptr_t)ptr;
size_t count;

for (count = 0; count < len; count += 16) {
union {
unsigned int buf[4];
unsigned char cbuf[16];
} u;
size_t s = 10;
size_t i;

for (i = 0; i < s / 4; i++) {
u.buf[i] = ((const unsigned int*)address)[i];
printf_func(printf_arg, "%08x ", static_cast<unsigned int>(u.buf[i]));
}
}
}

5 changes: 3 additions & 2 deletions llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,15 @@ class LowerTypeTestsPass : public PassInfoMixin<LowerTypeTestsPass> {
ModuleSummaryIndex *ExportSummary = nullptr;
const ModuleSummaryIndex *ImportSummary = nullptr;
bool DropTypeTests = true;
bool AlwaysDropTests = false;

public:
LowerTypeTestsPass() : UseCommandLine(true) {}
LowerTypeTestsPass(ModuleSummaryIndex *ExportSummary,
const ModuleSummaryIndex *ImportSummary,
bool DropTypeTests = false)
bool DropTypeTests = false, bool AlwaysDropTests = false)
: ExportSummary(ExportSummary), ImportSummary(ImportSummary),
DropTypeTests(DropTypeTests) {}
DropTypeTests(DropTypeTests), AlwaysDropTests(AlwaysDropTests) {}
PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
};

Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Passes/PassBuilderPipelines.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1629,6 +1629,10 @@ PassBuilder::buildFatLTODefaultPipeline(OptimizationLevel Level, bool ThinLTO,
MPM.addPass(buildLTOPreLinkDefaultPipeline(Level));
MPM.addPass(EmbedBitcodePass(ThinLTO, EmitSummary));

// If we're doing FatLTO w/ CFI enabled, we don't want the type tests in the
// object file.
MPM.addPass(LowerTypeTestsPass(nullptr, nullptr, true, true));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this build? The ctor only takes three arguments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, you have a dependent change that adds the argument.

68a064b

I'm not really a big fan of adding another bool argument like this because it makes the callers less clear, can you do something else like make the existing argument into an enum?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR depends on #112787 which adds the fourth argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'm not super happy w/ the boolean either. The only reason its required is to avoid asserting for the new use case, but that also implies that assertion doesn't always hold, so I'm not sure if its really a good idea to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And apologies for not adding you on both reviews. I had thought I added you and Nikita to #112787, but it looks like I hadn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion is to check that the code is only removing type test assumes (possibly involving a phi). So I wouldn't call the functionality that you're adding "always drop type tests". What you're adding is more like "drop type tests" and what's there already is "drop type test assumes". So maybe you can name the enumerators based on that.


// Use the ThinLTO post-link pipeline with sample profiling
if (ThinLTO && PGOOpt && PGOOpt->Action == PGOOptions::SampleUse)
MPM.addPass(buildThinLTODefaultPipeline(Level, /*ImportSummary=*/nullptr));
Expand Down
32 changes: 20 additions & 12 deletions llvm/lib/Transforms/IPO/LowerTypeTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ static cl::opt<bool>
ClDropTypeTests("lowertypetests-drop-type-tests",
cl::desc("Simply drop type test assume sequences"),
cl::Hidden, cl::init(false));
static cl::opt<bool>
ClForceDropTypeTests("lowertypetests-force-drop-type-tests",
cl::desc("Always drop all type test sequences"),
cl::Hidden, cl::init(false));

bool BitSetInfo::containsGlobalOffset(uint64_t Offset) const {
if (Offset < ByteOffset)
Expand Down Expand Up @@ -400,6 +404,7 @@ class LowerTypeTestsModule {
// Set when the client has invoked this to simply drop all type test assume
// sequences.
bool DropTypeTests;
bool AlwaysDropTests;

Triple::ArchType Arch;
Triple::OSType OS;
Expand Down Expand Up @@ -542,7 +547,7 @@ class LowerTypeTestsModule {
LowerTypeTestsModule(Module &M, ModuleAnalysisManager &AM,
ModuleSummaryIndex *ExportSummary,
const ModuleSummaryIndex *ImportSummary,
bool DropTypeTests);
bool DropTypeTests, bool AlwaysDropTests);

bool lower();

Expand Down Expand Up @@ -1828,9 +1833,11 @@ void LowerTypeTestsModule::buildBitSetsFromDisjointSet(
/// Lower all type tests in this module.
LowerTypeTestsModule::LowerTypeTestsModule(
Module &M, ModuleAnalysisManager &AM, ModuleSummaryIndex *ExportSummary,
const ModuleSummaryIndex *ImportSummary, bool DropTypeTests)
const ModuleSummaryIndex *ImportSummary, bool DropTypeTests,
bool AlwaysDropTests)
: M(M), ExportSummary(ExportSummary), ImportSummary(ImportSummary),
DropTypeTests(DropTypeTests || ClDropTypeTests) {
DropTypeTests(DropTypeTests || ClDropTypeTests),
AlwaysDropTests(AlwaysDropTests || ClForceDropTypeTests) {
assert(!(ExportSummary && ImportSummary));
Triple TargetTriple(M.getTargetTriple());
Arch = TargetTriple.getArch();
Expand Down Expand Up @@ -1882,7 +1889,7 @@ bool LowerTypeTestsModule::runForTesting(Module &M, ModuleAnalysisManager &AM) {
M, AM,
ClSummaryAction == PassSummaryAction::Export ? &Summary : nullptr,
ClSummaryAction == PassSummaryAction::Import ? &Summary : nullptr,
/*DropTypeTests*/ false)
/*DropTypeTests*/ false, /*AlwaysDropTests=*/false)
.lower();

if (!ClWriteSummary.empty()) {
Expand Down Expand Up @@ -1949,7 +1956,7 @@ void LowerTypeTestsModule::replaceDirectCalls(Value *Old, Value *New) {
Old->replaceUsesWithIf(New, isDirectCall);
}

static void dropTypeTests(Module &M, Function &TypeTestFunc) {
static void dropTypeTests(Module &M, Function &TypeTestFunc, bool AlwaysDrop) {
for (Use &U : llvm::make_early_inc_range(TypeTestFunc.uses())) {
auto *CI = cast<CallInst>(U.getUser());
// Find and erase llvm.assume intrinsics for this llvm.type.test call.
Expand All @@ -1960,8 +1967,9 @@ static void dropTypeTests(Module &M, Function &TypeTestFunc) {
// phi (which will feed the assume). Simply replace the use on the phi
// with "true" and leave the merged assume.
if (!CI->use_empty()) {
assert(
all_of(CI->users(), [](User *U) -> bool { return isa<PHINode>(U); }));
assert(AlwaysDrop || all_of(CI->users(), [](User *U) -> bool {
return isa<PHINode>(U);
}));
CI->replaceAllUsesWith(ConstantInt::getTrue(M.getContext()));
}
CI->eraseFromParent();
Expand All @@ -1974,14 +1982,14 @@ bool LowerTypeTestsModule::lower() {

if (DropTypeTests) {
if (TypeTestFunc)
dropTypeTests(M, *TypeTestFunc);
dropTypeTests(M, *TypeTestFunc, AlwaysDropTests);
// Normally we'd have already removed all @llvm.public.type.test calls,
// except for in the case where we originally were performing ThinLTO but
// decided not to in the backend.
Function *PublicTypeTestFunc =
M.getFunction(Intrinsic::getName(Intrinsic::public_type_test));
if (PublicTypeTestFunc)
dropTypeTests(M, *PublicTypeTestFunc);
dropTypeTests(M, *PublicTypeTestFunc, AlwaysDropTests);
if (TypeTestFunc || PublicTypeTestFunc) {
// We have deleted the type intrinsics, so we no longer have enough
// information to reason about the liveness of virtual function pointers
Expand Down Expand Up @@ -2449,9 +2457,9 @@ PreservedAnalyses LowerTypeTestsPass::run(Module &M,
if (UseCommandLine)
Changed = LowerTypeTestsModule::runForTesting(M, AM);
else
Changed =
LowerTypeTestsModule(M, AM, ExportSummary, ImportSummary, DropTypeTests)
.lower();
Changed = LowerTypeTestsModule(M, AM, ExportSummary, ImportSummary,
DropTypeTests, AlwaysDropTests)
.lower();
if (!Changed)
return PreservedAnalyses::all();
return PreservedAnalyses::none();
Expand Down
19 changes: 19 additions & 0 deletions llvm/test/Transforms/LowerTypeTests/drop_type_test.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
; RUN: opt -S -passes=lowertypetests -lowertypetests-force-drop-type-tests -lowertypetests-drop-type-tests < %s | FileCheck %s

define void @func() {
entry:
%0 = tail call i1 @llvm.type.test(ptr null, metadata !"foo")
br i1 %0, label %exit, label %trap
; CHECK: entry:
; CHECK-NEXT: br i1 true, label %exit, label %trap
; CHECK-NOT: @llvm.type.test

trap:
unreachable

exit:
ret void
}

declare i1 @llvm.type.test(ptr, metadata) #0
attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
Loading