[PATCH][stackmap] Stackmaps don't modify the heap, but they do read all memory and they may not return normally

[Filip Pizlo]

Heh, you'll be glad I didn't just commit it.  It would have broken TableGen.

Here's the patch that makes stackmap not-nounwind, fixes TableGen so that an un-attributed intrinsic still emits valid code, and adds a unit test for Darwin.  The test would have failed before my patch, because the presence of nounwind on stackmap would have meant that the compact_unwind section would have been elided.  It's sort of implicitly true that anyone using stackmap for deoptimization will have to rely on unwind data.  On Darwin, that implies compact_unwind.  I wish I could make the test itself more portable, but an unportable test is better than no test.

Can you look at this? :-)

Note that the biggest part of this is the test.  I'm expanding MCJITCAPITest to check that we can get the compact_unwind section even when running the MCJIT with opt -O2.  That required linking in libIPO because that's where you get the PassManagerBuilder.  I think this goes a long way towards expanding test coverage on MCJIT, stackmaps, and how they are actually meant to be used.

-Filip

-------------- next part --------------
A non-text attachment was scrubbed...
Name: stackmap-can-throw.patch
Type: application/octet-stream
Size: 6937 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140220/79824fc1/attachment.obj>
-------------- next part --------------



On Feb 20, 2014, at 2:41 PM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Feb 20, 2014, at 2:33 PM, Andrew Trick <atrick at apple.com> wrote:
> 
>> 
>> On Feb 19, 2014, at 7:47 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>> 
>>> Stackmaps, as opposed to patchpoints, are intended for OSR exits, aka deoptimization.  The outcome of executing a stackmap should be one of two things:
>>> 
>>> - Nothing.  This is the usual outcome, since OSR exits are very rare.
>>> - Exit (i.e. "unwind") via the client runtime's OSR exit machinery, while recovering the values of the variables passed to the stackmap, and then running arbitrary code that may read/write the entire heap - but only after the caller is no longer on the stack.
>>> 
>>> The best way to model this is to say that stackmaps are "readonly" but not "nounwind".  I attach a patch that does this.
>>> 
>>> Prior to this patch, stackmaps are "nounwind" but not "readonly" - i.e. we assume that they may read or write the world, but that they always return normally.  They definitely don't write anything - at least not in a way that is observable to the caller - so "readonly" is appropriate.  However, "readonly" and "nounwind" means, among other things, that if you don't use the return value of the function then the call can be DCE'd.  Stackmap returns void, so it's definitely true that nobody will ever use its return value.  I believe that marking stackmap as "nounwind" is inaccurate even without the "readonly" attribute, since its original customer used it precisely for a custom kind of unwinding.  So, "readonly" but not "nounwind" feels like it most accurately models the way that stackmaps are used.
>>> 
>>> FWIW, I tested this against the test suite we use for the WebKit FTL JIT, and this doesn't break anything.  I don't have performance numbers, yet - though I'd advocate for this change even if wasn't yet a win since I think it's just good form to accurately model effects.
>>> 
>>> <stackmap-read-only-throws.patch>
>> 
>> Thanks Filip,
>> 
>> Would you mind attaching your patch to llvm.org/PR18912?
>> 
>> Unfortunately, readonly may-unwind calls are untested. I don’t think the limited patterns that we currently generate with stackmap are sufficient to expose the bugs in this area. I think we should at least audit the optimizer first and maybe find some better way to test it before claiming that it works.
> 
> Please go ahead and commit the change to make stackmap a “Throw”. The bug only refers to the readonly-ness.
> 
> -Andy