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

Thu Dec 9 20:52:32 PST 2021

ychen added a comment.

In D114728#3159303 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114728/new/

https://reviews.llvm.org/D114728