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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added a comment.

In D132084#3736529 <https://reviews.llvm.org/D132084#3736529>, @efriedma wrote:

> 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.)

Could you share a test case that escapes or some example URL, please?  I could not find many by googling `computed gogo`. Probably there are better keywords.

> 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.

One test case I was thinking of is

  bool doInterpret(Instruction*& nextIntruction, int& out, int x) {
      static void* dispatch_table[] = {&&inc, &&suspend, &&stop};
      #define DISPATCH() goto *dispatch_table[*nextIntruction++]
  
      if(x%2){ // x==7: store &&suspend, x==9: store &&inc
         dispatch_table[0] = dispatch_table[x%3];
         return false;
      } else {
         DISPATCH();
      }
  
  inc:
      ++out;
      DISPATCH();
  suspend:
      return false;
  stop:
      return true;
  }

I don't think and am not sure if this is representative, but I guess it is valid?

If this is cloned even after IndirectBrExpandPass (not necessarily for coroutine, but in general), it may or may not do the right thing depending on how the two instances are called.

> 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.

Sounds good, I'll look into it.

>> 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.

Sounds great. CFI approach seems more promising to me too. I'll look into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132084