[PATCH] D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

> @efriedma think that coroutine suspend wouldn't make alloca really 'may alias'. We are just trying to make a lie to the MemCpyOpt Pass.

Yes.  I suspect the bug is actually in the implementation of corosplit.

> So it looks natural to mark arguments may alias after coro.suspend to me. How do you think about it?

For a normal pointer argument, that reasoning makes sense.  But the normal alias analysis rules should make that happen automatically. For a byval argument, it does not make sense.

> there may be any instruction executed between foo.ramp and foo.resume

In general, calling a function with a byval argument makes a copy of the memory in question, into memory owned by the callee.  That memory is local to the coroutine function.   So the code outside the function can't access it unless a pointer to the memory is explicitly escaped inside the callee.

In the context of a coroutine, that means instructions between foo.ramp and foo.resume can't modify the byval memory unless the address escapes.

---

I dug a bit more, and I think I see where the confusion is coming from.  If you look at the clang output for the testcase from the bug before any optimizations run, there's an alloca named "%a11", which is specific to coroutines.  Coroutines have to do some extra work to support arguments that are passed indirectly: we have to move-construct the argument from caller-owned memory into the coroutine frame.  The "extra" alloca is coming out of this code.

If an argument is getting passed indirectly, so it's actually just a plain pointer at the LLVM IR level, this makes perfect sense.  For arguments that are getting passed directly, or byval, it's nonsense: the argument was never in caller-owned memory in the first place, so copying it is extra work for nothing.

If you fix the clang code so it isn't generating these extra useless allocas, the problem with byval should become obvious: even without any optimizations, we end up with a use-after-free.  corosplit isn't preserving memory that's supposed to be part of the callee frame.

The only reason byval arguments currently work at all is that the extra allocas cover up the bug in corosplit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104007