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

Andrew Trick atrick at apple.com
Thu Feb 20 14:41:54 PST 2014


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



More information about the llvm-commits mailing list