[PATCH] D132084: [Cloning] handle blockaddress array clone in the same module

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D132084#3736472 <https://reviews.llvm.org/D132084#3736472>, @ychen wrote:

> In D132084#3736000 <https://reviews.llvm.org/D132084#3736000>, @efriedma wrote:
>
>> In D132084#3735812 <https://reviews.llvm.org/D132084#3735812>, @ychen wrote:
>>
>>>>> the current situation of silently creating broken IR when cloning blockaddress is not ideal.
>>>>
>>>> I'm not sure what invariant we can enforce here.  Maybe something related to the CloneFunctionChangeType?
>>>
>>> That's hard in general and feels insufficient to just handle some cases but not the others. How about emitting a warning of sorts all the way back to the frontend if any blockaddress is written? Also, add a boolean flag for CloneFunction API to optionally call IndirectBrExpandPass's logic.
>>
>> Oh, I was just talking about the API invariant.  Clearly coroutine lowering needs to do something different here.  And probably clang should warn or error if indirect goto can't generate the expected code.
>
> Add a new boolean flag `HandleIndirectBranch` defaulted to true: if a caller can know the blockaddress is safe to clone, they could set this to false(I expect this to be false most of the time). When `HandleIndirectBranch` is true, special-casing the case like `global blockaddress array for function F where F has no blockaddress captured/escaped(https://llvm.org/docs/LangRef.html#pointer-capture)` by cloning that array; for other cases, call IndirectBrExpandPass. Regardless of `HandleIndirectBranch` value, if a function references any global value and blockaddress maybe captured, we emit the warning/error because IndirectBrExpandPass can not handle all cases correctly.

I suspect the special case for "global blockaddress array" won't really trigger in practice; most uses of indirectbr involve the value escaping somehow, because otherwise there isn't really any value over just using a switch.  (I don't think your testcase is representative.)

IndirectBrExpandPass can handle all cases correctly; it's just not efficient, like I mentioned before.  And you probably don't want to do it without the caller explicitly asking for it.

>> I forget, why do we have to clone the coroutine body?  I think I read an explanation at some point, but I'm not remembering where it was; it seems like it should be possible to avoid cloning.
>
> Since the instructions executed before the first `co_await` call need to stay at the original function(ramp function). But for patterns like the below, this could not be decided statically so cloning is needed.
>
>   task corofoo(){
>     while(...){
>       if(..) co_await();
>       else <code>;
>     }
>   }

Cloning is one way to implement that, sure.  I don't think it's the only way, though; you could move the body into a helper function, and have both coroutine entry-points call it.  Maybe at some cost to performance.  It might make sense to implement a scheme like this in case we run into other situations where we want to avoid cloning.

> PS: I think it is helpful to add bound check for indirectbr. But I'm not sure where to put it, a new clang flag?

We could add a check to -fsanitize=undefined (https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html) to check that the address passed to an indirectbr is one of the expected addresses?  Normally the undefined-behavior sanitizers insert the code early, though, in clang IR generation, so we can give good diagnostics.  Not sure it would have caught a bug in a transform, though.

Alternatively, you could implement something more like CFI (https://clang.llvm.org/docs/ControlFlowIntegrity.html), which inserts checks late.  So when the indirect-goto sanitizer is enabled, clang adds a function attribute, then the backend adds some checks just before isel, or something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132084