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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
