[Coroutine] Remove the prologue data of `-fsanitize=function` for split functions

[ychen]

Summary

There is no proper RTTI for these split functions. So just delete the prologue data.

This is intended as a temporary fix for PR50345 for 13.0.x release. The alternative is to disable -fsanitize=function.

[rjmccall]

I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from -fsanitize=functions if it wants to sanitize the resumption calls, which I guess it currently doesn't. So something here may be a necessary fix.

However, I don't think it's a sufficient fix for PR 50345, because the way that the frontend currently creates these prologue attributes is deeply problematic for any number of function transformations, not just coroutine splitting. For example, any sort of function-cloning transformation will end up constructing an incorrect relative reference. I expect that this self-reference will also interfere with DCE. So in addition to whatever function-type fix we need for coroutines, we just need to change how we create this prologue. I recommend the design I laid out in the PR:

• Have the frontend emit a more abstract attribute, like sanitize_function_type(i8** @1)
• Either lower this abstract attribute in a codegen pass by turning it into a prologue attribute or just handle it directly in the appropriate backend.

The coroutine part of the fix would then simply be to remove the sanitize_function_type attribute from the resumption function clones; or better yet, switch the coro.switch lowering to use the "prototype" design used by coro.retcon and coro.async, and then set the appropriate attribute (if any) on the prototype so that it will be cloned into the resumption functions.

In the meantime, this sanitizer should be disabled in 13.x.

[ychen]

In D114728#3159303, @rjmccall wrote:

I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from -fsanitize=functions if it wants to sanitize the resumption calls, which I guess it currently doesn't. So something here may be a necessary fix.

However, I don't think it's a sufficient fix for PR 50345, because the way that the frontend currently creates these prologue attributes is deeply problematic for any number of function transformations, not just coroutine splitting. For example, any sort of function-cloning transformation will end up constructing an incorrect relative reference. I expect that this self-reference will also interfere with DCE. So in addition to whatever function-type fix we need for coroutines, we just need to change how we create this prologue. I recommend the design I laid out in the PR:

• Have the frontend emit a more abstract attribute, like sanitize_function_type(i8** @1)
• Either lower this abstract attribute in a codegen pass by turning it into a prologue attribute or just handle it directly in the appropriate backend.

The coroutine part of the fix would then simply be to remove the sanitize_function_type attribute from the resumption function clones; or better yet, switch the coro.switch lowering to use the "prototype" design used by coro.retcon and coro.async, and then set the appropriate attribute (if any) on the prototype so that it will be cloned into the resumption functions.

@rjmccall Yeah agreed that the attribute method is a better approach forward (I would work on a patch). I was intended to propose this as an alternative to disabling -fsanitize=function for 13.x.

In the meantime, this sanitizer should be disabled in 13.x.

@pcc does this sound good to you?

[ychen]

In D114728#3159303, @rjmccall wrote:

I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from -fsanitize=functions if it wants to sanitize the resumption calls, which I guess it currently doesn't. So something here may be a necessary fix.

However, I don't think it's a sufficient fix for PR 50345, because the way that the frontend currently creates these prologue attributes is deeply problematic for any number of function transformations, not just coroutine splitting. For example, any sort of function-cloning transformation will end up constructing an incorrect relative reference. I expect that this self-reference will also interfere with DCE. So in addition to whatever function-type fix we need for coroutines, we just need to change how we create this prologue. I recommend the design I laid out in the PR:

• Have the frontend emit a more abstract attribute, like sanitize_function_type(i8** @1)
• Either lower this abstract attribute in a codegen pass by turning it into a prologue attribute or just handle it directly in the appropriate backend.

The coroutine part of the fix would then simply be to remove the sanitize_function_type attribute from the resumption function clones; or better yet, switch the coro.switch lowering to use the "prototype" design used by coro.retcon and coro.async, and then set the appropriate attribute (if any) on the prototype so that it will be cloned into the resumption functions.

In the meantime, this sanitizer should be disabled in 13.x.

Hi @rjmccall, I gave this some thought, this sanitize_function_type attribute would be a prefix/prologue thing instead of a function attribute since it needs to take a constant value (https://github.com/llvm/llvm-project/blob/a32c2c380863d02eb0fd5e8757a62d96114b9519/llvm/lib/IR/Function.cpp#L1854) for the RTTI global variable. Then it needs some corresponding change in the bitcode representation. It seems easier just represent it as a metadata node attached to a function. This aligns with the intention

In D114728#3159303, @rjmccall wrote:

I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from -fsanitize=functions if it wants to sanitize the resumption calls, which I guess it currently doesn't. So something here may be a necessary fix.

However, I don't think it's a sufficient fix for PR 50345, because the way that the frontend currently creates these prologue attributes is deeply problematic for any number of function transformations, not just coroutine splitting. For example, any sort of function-cloning transformation will end up constructing an incorrect relative reference. I expect that this self-reference will also interfere with DCE. So in addition to whatever function-type fix we need for coroutines, we just need to change how we create this prologue. I recommend the design I laid out in the PR:

• Have the frontend emit a more abstract attribute, like sanitize_function_type(i8** @1)
• Either lower this abstract attribute in a codegen pass by turning it into a prologue attribute or just handle it directly in the appropriate backend.

The coroutine part of the fix would then simply be to remove the sanitize_function_type attribute from the resumption function clones; or better yet, switch the coro.switch lowering to use the "prototype" design used by coro.retcon and coro.async, and then set the appropriate attribute (if any) on the prototype so that it will be cloned into the resumption functions.

In the meantime, this sanitizer should be disabled in 13.x.

Hi @rjmccall , I'm working on a patch for this with the sanitize_function_type attribute idea. However, I'm wondering if it makes sense to you to use a metadata node on the function instead. A function attribute may not work because it can not point to the RTTI global variable. Something equivalent to the "function prologue"(https://llvm.org/docs/LangRef.html#prologue-data, it is basically a hidden operand of a function) is possible but that requires bitcode & IR text/parsing changes, which I'm trying to avoid (unless I have to). WDYT?

[rjmccall]

I don't have a strong opinion about attribute vs. metadata; if metadata are the best technical path forward, that's fine with me. I don't think function metadata can be "lost" the same way that metadata internal to a function can, right?

[ychen]

In D114728#3196591, @rjmccall wrote:

I don't have a strong opinion about attribute vs. metadata; if metadata are the best technical path forward, that's fine with me. I don't think function metadata can be "lost" the same way that metadata internal to a function can, right?

That's right. There is a metadata kind attached to it. Users have to be very explicit (by intention) to drop it.

[ychen]

D115844 and D116130 supersede this.

clang/test/CodeGenCXX/catch-undef-behavior.cpp

void downcast_reference(B &b) {
   // CHECK:      [[C_INT:%.+]] = ptrtoint %class.C* [[C]] to i64
   // CHECK-NEXT: [[MASKED:%.+]] = and i64 [[C_INT]], 15
   // CHECK-NEXT: [[TEST:%.+]] = icmp eq i64 [[MASKED]], 0
   // AND the alignment test with the objectsize test.
   // CHECK:      [[AND:%.+]] = and i1 {{.*}}, [[TEST]]
   // CHECK-NEXT: br i1 [[AND]]
 }
 
-//
+// FIXME: If the function signature value is changed, make the same change in
+//        CoroSplit. Making them share a single API to avoid this is better.
+//        However, function prefix/prologue data mechansim do not work well with
+//        coroutine split in general, we may want to redesign that or
+//        deprecate some features (see intended usages in
+//        https://reviews.llvm.org/D6454) in the future.
 // CHECK-LABEL: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 846595819, i32 trunc (i64 sub (i64 ptrtoint (i8** {{.*}} to i64), i64 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i64)) to i32) }>
 // CHECK-X32: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 846595819, i32 sub (i32 ptrtoint (i8** [[IndirectRTTI_ZTIFvPFviEE]] to i32), i32 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i32)) }>
 // CHECK-X86: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 846595819, i32 sub (i32 ptrtoint (i8** [[IndirectRTTI_ZTIFvPFviEE]] to i32), i32 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i32)) }>
 void indirect_function_call(void (*p)(int)) {
   // CHECK: [[PTR:%.+]] = bitcast void (i32)* {{.*}} to <{ i32, i32 }>*
 
   // Signature check
   // CHECK-NEXT: [[SIGPTR:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, i32 }>* [[PTR]], i32 0, i32 0

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

if (!SP->getDeclaration() && SP->getUnit() &&
       SP->replaceLinkageName(MDString::get(Context, NewF->getName()));
   }
 
   NewF->setLinkage(savedLinkage);
   NewF->setVisibility(savedVisibility);
   NewF->setUnnamedAddr(savedUnnamedAddr);
   NewF->setDLLStorageClass(savedDLLStorageClass);
 
+  if (Shape.ABI == coro::ABI::Switch && NewF->hasPrologueData()) {
+    if (auto *CS = dyn_cast<ConstantStruct>(NewF->getPrologueData())) {
+      if (auto *CI = dyn_cast<ConstantInt>(CS->getOperand(0))) {
+        // This value should match the value returned by
+        // TargetCodeGenInfo::getUBSanFunctionSignature().
+        unsigned Sig = (0xeb << 0) | // jmp rel8
+                       (0x06 << 8) | //           .+0x08
+                       ('v' << 16) | ('2' << 24);
+        if (CI->getZExtValue() == Sig)
+          NewF->setPrologueData(nullptr);
+      }
+    }
+  }
+
   // Replace the attributes of the new function:
   auto OrigAttrs = NewF->getAttributes();
   auto NewAttrs = AttributeList();
 
   switch (Shape.ABI) {
   case coro::ABI::Switch:
     // Bootstrap attributes by copying function attributes from the
     // original function.  This should include optimization settings and so on.

llvm/test/Transforms/Coroutines/coro-split-00.ll

 ; Tests that coro-split pass splits the coroutine into f, f.resume and f.destroy
 ; RUN: opt < %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s
 
-define i8* @f() "coroutine.presplit"="1" {
+define i8* @f() "coroutine.presplit"="1" prologue <{ i32, i32 }> <{ i32 846595819, i32 1 }> {
 entry:
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
   %need.alloc = call i1 @llvm.coro.alloc(token %id)
   br i1 %need.alloc, label %dyn.alloc, label %begin
 
 dyn.alloc:  
   %size = call i32 @llvm.coro.size.i32()
   %alloc = call i8* @malloc(i32 %size)
cleanup:
   %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
   call void @free(i8* %mem)
   br label %suspend
 suspend:
   call i1 @llvm.coro.end(i8* %hdl, i1 0)  
   ret i8* %hdl
 }
 
-; CHECK-LABEL: @f(
+; CHECK-LABEL: @f() prologue <{ i32, i32 }> <{ i32 846595819, i32 1 }> {
 ; CHECK: call i8* @malloc
 ; CHECK: @llvm.coro.begin(token %id, i8* %phi)
 ; CHECK: store void (%f.Frame*)* @f.resume, void (%f.Frame*)** %resume.addr
 ; CHECK: %[[SEL:.+]] = select i1 %need.alloc, void (%f.Frame*)* @f.destroy, void (%f.Frame*)* @f.cleanup
 ; CHECK: store void (%f.Frame*)* %[[SEL]], void (%f.Frame*)** %destroy.addr
 ; CHECK: call void @print(i32 0)
 ; CHECK-NOT: call void @print(i32 1)
 ; CHECK-NOT: call void @free(
 ; CHECK: ret i8* %hdl
 
-; CHECK-LABEL: @f.resume(
+; CHECK-LABEL: @f.resume({{.*}}) {
 ; CHECK-NOT: call i8* @malloc
 ; CHECK-NOT: call void @print(i32 0)
 ; CHECK: call void @print(i32 1)
 ; CHECK-NOT: call void @print(i32 0)
 ; CHECK: call void @free(
 ; CHECK: ret void
 
-; CHECK-LABEL: @f.destroy(
+; CHECK-LABEL: @f.destroy({{.*}}) {
 ; CHECK-NOT: call i8* @malloc
 ; CHECK-NOT: call void @print(
 ; CHECK: call void @free(
 ; CHECK: ret void
 
-; CHECK-LABEL: @f.cleanup(
+; CHECK-LABEL: @f.cleanup({{.*}}) {
 ; CHECK-NOT: call i8* @malloc
 ; CHECK-NOT: call void @print(
 ; CHECK-NOT: call void @free(
 ; CHECK: ret void
 
 declare i8* @llvm.coro.free(token, i8*)
 declare i32 @llvm.coro.size.i32()
 declare i8  @llvm.coro.suspend(token, i1)