[llvm][fatlto] Drop any CFI related instrumentation after emitting bitcode

[ilovepi]

We want to support CFI instrumentation for the bitcode section, without
miscompiling the object code portion of a FatLTO object. We can reuse
the existing mechanisms in the LowerTypeTestsPass to do that, by just
adding the pass to the FatLTO pipeline after the EmbedBitcodePass with
the correct options set.

Fixes #112053

[llvmbot]

@llvm/pr-subscribers-clang

Author: Paul Kirth (ilovepi)

Changes

We want to support CFI instrumentation for the bitcode section, without
miscompiling the object code portion of a FatLTO object. We can reuse
the existing mechanisms in the LowerTypeTestsPass to do that, by just
adding the pass to the FatLTO pipeline after the EmbedBitcodePass with
the correct options set.

Fixes #112053

---

Full diff: https://github.com/llvm/llvm-project/pull/112788.diff

2 Files Affected:

• (added) clang/test/CodeGen/fat-lto-objects-cfi.cpp (+46)
• (modified) llvm/lib/Passes/PassBuilderPipelines.cpp (+4)

diff --git a/clang/test/CodeGen/fat-lto-objects-cfi.cpp b/clang/test/CodeGen/fat-lto-objects-cfi.cpp
new file mode 100644
index 00000000000000..022e74fd9b6f22
--- /dev/null
+++ b/clang/test/CodeGen/fat-lto-objects-cfi.cpp
@@ -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]));
+    }
+  }
+}
+
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index 17710eb94b6ded..81ea6ac7dcdf18 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -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));
+
   // Use the ThinLTO post-link pipeline with sample profiling
   if (ThinLTO && PGOOpt && PGOOpt->Action == PGOOptions::SampleUse)
     MPM.addPass(buildThinLTODefaultPipeline(Level, /*ImportSummary=*/nullptr));

[pcc]

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

[pcc]

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?

[petrhosek]

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

[ilovepi]

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.

[ilovepi]

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.

[pcc]

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.

[ilovepi]

Hmm, windows seems to be breaking in the premerge tests. I'll try to spell the test w/ a cc1 command line instead, and hopefully that will avoid trying to find the sanitizer ignorelist file.

[ilovepi]

The LLDB test failure in premerge is unrelated to FatLTO, so I'm going to go ahead and land this.