Fix incorrect patching for OSR.

Fix incorrect patching for OSR.

If OSR happens before regular recompilation, the unoptimized function code
on the stack may not have deoptimization support.  In that case, graph
creation compiles the unoptimized code again to include support.  That
code is then installed as shared code.  When we patch code for OSR, the
function code on the stack and not the shared code is what we want.

R=titzer@chromium.org
TEST=block-conflicts.js with --always-osr --concurrent-osr

Committed: https://code.google.com/p/v8/source/detail?r=18261

[titzer, 2013-12-03 15:42:43]

https://codereview.chromium.org/99013003/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/99013003/diff/20001/src/runtime.cc#newcode8671
src/runtime.cc:8671: if (static_cast<uint32_t>(unoptimized->instruction_size())
<= pc_offset) {
I don't see how this check is strict enough. I think you want to check that
unoptimized code from the shared function info contains the passed PC. Actually,
I think you might need to rework the runtime call for that.

[Yang, 2013-12-03 17:49:16]

On 2013/12/03 15:42:43, titzer wrote:
> https://codereview.chromium.org/99013003/diff/20001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/99013003/diff/20001/src/runtime.cc#newcode8671
> src/runtime.cc:8671: if
(static_cast<uint32_t>(unoptimized->instruction_size())
> <= pc_offset) {
> I don't see how this check is strict enough. I think you want to check that
> unoptimized code from the shared function info contains the passed PC.
Actually,
> I think you might need to rework the runtime call for that.

I changed this as suggested. Now we no longer compute the offset in the builtin,
but in the runtime function. Crawling the stack for the PC should be cheap.

[titzer, 2013-12-04 17:49:16]

https://codereview.chromium.org/99013003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/99013003/diff/40001/src/runtime.cc#newcode8670
src/runtime.cc:8670: if (!unoptimized->contains(frame->pc())) {
I am not sure this is an improvement to no longer take the pc offset, then do a
stack walk just to check if it is the fast case. I think you should instead pass
the PC and then you can directly check if the unoptimized code contains that PC,
and only do a stack walk in the (unlikely) failure case.

[Yang, 2013-12-05 08:46:47]

On 2013/12/04 17:49:16, titzer wrote:
> https://codereview.chromium.org/99013003/diff/40001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/99013003/diff/40001/src/runtime.cc#newcode8670
> src/runtime.cc:8670: if (!unoptimized->contains(frame->pc())) {
> I am not sure this is an improvement to no longer take the pc offset, then do
a
> stack walk just to check if it is the fast case. I think you should instead
pass
> the PC and then you can directly check if the unoptimized code contains that
PC,
> and only do a stack walk in the (unlikely) failure case.

Like discussed offline, passing the PC from builtin is not that simple, since it
would not be GC safe if passed directly. I added a comment for that.

[titzer, 2013-12-05 09:29:02]

LGTM
Life in V8 is full of tradeoffs.

[Yang, 2013-12-05 16:17:55]

Committed patchset #4 manually as r18261.