Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1990803005:
[runtime] set AsyncFunctionNext/Throw to adapt arguments (Closed)
|
Created:
4 years, 7 months ago by caitp (gmail) Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[runtime] set AsyncFunctionNext/Throw to adapt arguments
Prevent crash/UB during stack frame iteration through functions, which occurs
when debugging, when building stacktraces, etc.
Also prevents these functions from appearing in stacktraces, by unsetting the "native" flag.
BUG=v8:4483, v8:5025
R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org
Committed: https://crrev.com/f6865cb14220df1a6cc909eceb4f674978dd141f
Cr-Commit-Position: refs/heads/master@{#36339}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rework to support all functions without arguments adapter frames #
Total comments: 4
Patch Set 3 : cleanup #Patch Set 4 : Also, don't expose AsyncFunctionNext/AsyncFunctionThrow in stack trace #
Total comments: 3
Messages
Total messages: 26 (8 generated)
Hey, this fixes the debugger; crash reported in v8:4483. PTAL.
Description was changed from ========== [runtime] set internal_formal_parameter_count for AsyncFunctionNext/Throw Set the internal formal parameter count for the runtime function, to prevent issues crash/UB during stack frame iteration. BUG=v8:4483 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [runtime] set internal_formal_parameter_count for AsyncFunctionNext/Throw Set the internal formal parameter count for the runtime function, to prevent crash/UB during stack frame iteration. BUG=v8:4483 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
https://codereview.chromium.org/1990803005/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:2474: async_function_next->shared()->set_internal_formal_parameter_count(1); question: why do we not set internal_formal_parameter_count when we don't adapt arguments? Isn't it necessary regardless? I think I can avoid creating the adapter frame, so I assume I want `false` here. But maybe someone can give me some background on this
https://codereview.chromium.org/1990803005/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:2474: async_function_next->shared()->set_internal_formal_parameter_count(1); On 2016/05/18 14:38:23, caitp wrote: > question: why do we not set internal_formal_parameter_count when we don't adapt > arguments? Isn't it necessary regardless? I think I can avoid creating the > adapter frame, so I assume I want `false` here. But maybe someone can give me > some background on this It may be a more general / better solution to handle SharedFunctionInfo::kDontAdaptArgumentsSentinel in JavaScriptFrame::GetNumberOfIncomingArguments() (replacing it with SharedFunctionInfo::length() instead? or 0?), but I don't know if that would break anything.
Description was changed from ========== [runtime] set internal_formal_parameter_count for AsyncFunctionNext/Throw Set the internal formal parameter count for the runtime function, to prevent crash/UB during stack frame iteration. BUG=v8:4483 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [runtime] support kDontAdaptArgumentsSentinel in GetNumberOfIncomingArguments() Prevent crash/UB during stack frame iteration through functions which do not support arguments adapter frames. BUG=v8:4483 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
Nothing revolutionary is happening here, so I'd like to separate out a rework of frames for a separate patch. https://codereview.chromium.org/1990803005/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:2469: Builtins::kGeneratorPrototypeNext, 1, false); 1 looks like the right value here, matching the other registration of this builtin function; ditto Throw. Why was it 2 before? https://codereview.chromium.org/1990803005/diff/20001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1990803005/diff/20001/src/frames.cc#newcode841 src/frames.cc:841: return param_count; Why is this change needed?
https://codereview.chromium.org/1990803005/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:2469: Builtins::kGeneratorPrototypeNext, 1, false); On 2016/05/18 18:58:02, Dan Ehrenberg wrote: > 1 looks like the right value here, matching the other registration of this > builtin function; ditto Throw. Why was it 2 before? The original implementation used a parameter for the generator object rather than the receiver, I guess I never updated the formals count. https://codereview.chromium.org/1990803005/diff/20001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1990803005/diff/20001/src/frames.cc#newcode841 src/frames.cc:841: return param_count; On 2016/05/18 18:58:02, Dan Ehrenberg wrote: > Why is this change needed? for anything with DontAdaptArguments() called on the shared function info, it has this sentinel value (which is -1) --- so this breaks stuff. AsyncFunctionNext/Throw don't (and shouldn't) need to adapt arguments, though. An alternative fix would be to just make these functions adapt arguments (which would add a few extra test/branches each time they're called --- not too worried about that), but this is technically broken for every case that doesn't apply arguments.
On 2016/05/18 19:03:43, caitp wrote: > https://codereview.chromium.org/1990803005/diff/20001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/1990803005/diff/20001/src/bootstrapper.cc#new... > src/bootstrapper.cc:2469: Builtins::kGeneratorPrototypeNext, 1, false); > On 2016/05/18 18:58:02, Dan Ehrenberg wrote: > > 1 looks like the right value here, matching the other registration of this > > builtin function; ditto Throw. Why was it 2 before? > > The original implementation used a parameter for the generator object rather > than the receiver, I guess I never updated the formals count. > > https://codereview.chromium.org/1990803005/diff/20001/src/frames.cc > File src/frames.cc (right): > > https://codereview.chromium.org/1990803005/diff/20001/src/frames.cc#newcode841 > src/frames.cc:841: return param_count; > On 2016/05/18 18:58:02, Dan Ehrenberg wrote: > > Why is this change needed? > > for anything with DontAdaptArguments() called on the shared function info, it > has this sentinel value (which is -1) --- so this breaks stuff. > > AsyncFunctionNext/Throw don't (and shouldn't) need to adapt arguments, though. > > An alternative fix would be to just make these functions adapt arguments (which > would add a few extra test/branches each time they're called --- not too worried > about that), but this is technically broken for every case that doesn't apply > arguments. er, for every function that DontAdaptArguments()
Description was changed from ========== [runtime] support kDontAdaptArgumentsSentinel in GetNumberOfIncomingArguments() Prevent crash/UB during stack frame iteration through functions which do not support arguments adapter frames. BUG=v8:4483 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
Description was changed from ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. Also prevents these functions from appearing in stacktraces, by unsetting the "native" flag. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
lgtm
On 2016/05/18 at 19:59:48, Dan Ehrenberg wrote: > lgtm I'm not sure what implications set_native(false) has exactly. This is for the stack trace? Does it change anything else?
On 2016/05/18 20:00:29, Dan Ehrenberg wrote: > On 2016/05/18 at 19:59:48, Dan Ehrenberg wrote: > > lgtm > > I'm not sure what implications set_native(false) has exactly. This is for the > stack trace? Does it change anything else? When the inline flag is set, functions are not allowed to be inlined. I'm not sure if there are any other affects of it
On 2016/05/18 20:02:12, caitp wrote: > On 2016/05/18 20:00:29, Dan Ehrenberg wrote: > > On 2016/05/18 at 19:59:48, Dan Ehrenberg wrote: > > > lgtm > > > > I'm not sure what implications set_native(false) has exactly. This is for the > > stack trace? Does it change anything else? > > When the inline flag is set, functions are not allowed to be inlined. I'm not > sure if there are any other affects of it Looks like it also affects the behaviour of caller/arguments accessors, but those shouldn't matter since the functions shouldn't be user visible
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1990803005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990803005/60001
Message was sent while issue was closed.
Description was changed from ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. Also prevents these functions from appearing in stacktraces, by unsetting the "native" flag. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. Also prevents these functions from appearing in stacktraces, by unsetting the "native" flag. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. Also prevents these functions from appearing in stacktraces, by unsetting the "native" flag. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [runtime] set AsyncFunctionNext/Throw to adapt arguments Prevent crash/UB during stack frame iteration through functions, which occurs when debugging, when building stacktraces, etc. Also prevents these functions from appearing in stacktraces, by unsetting the "native" flag. BUG=v8:4483, v8:5025 R=yangguo@chromium.org, littledan@chromium.org, adamk@chromium.org Committed: https://crrev.com/f6865cb14220df1a6cc909eceb4f674978dd141f Cr-Commit-Position: refs/heads/master@{#36339} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f6865cb14220df1a6cc909eceb4f674978dd141f Cr-Commit-Position: refs/heads/master@{#36339}
Message was sent while issue was closed.
Sorry for the long-past-closed question, but I happened to see this code today. https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:2473: async_function_next->shared()->set_native(false); I found it surprising that this was done to _keep_ the function from appearing in stack traces. Can you explain how that works?
Message was sent while issue was closed.
caitp@igalia.com changed reviewers: + caitp@igalia.com
Message was sent while issue was closed.
https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:2473: async_function_next->shared()->set_native(false); On 2016/09/27 23:57:11, adamk wrote: > I found it surprising that this was done to _keep_ the function from appearing > in stack traces. Can you explain how that works? See IsNotInNativeScript() and IsVisibleInStackTrace() in isolate.cc
Message was sent while issue was closed.
On 2016/09/28 00:01:18, caitp wrote: > https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc#new... > src/bootstrapper.cc:2473: async_function_next->shared()->set_native(false); > On 2016/09/27 23:57:11, adamk wrote: > > I found it surprising that this was done to _keep_ the function from appearing > > in stack traces. Can you explain how that works? > > See IsNotInNativeScript() and IsVisibleInStackTrace() in isolate.cc Although, it is a bit confusing due to the wording, but that was the original reason, and it seemed to work
Message was sent while issue was closed.
https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:2473: async_function_next->shared()->set_native(false); On 2016/09/28 00:01:18, caitp wrote: > On 2016/09/27 23:57:11, adamk wrote: > > I found it surprising that this was done to _keep_ the function from appearing > > in stack traces. Can you explain how that works? > > See IsNotInNativeScript() and IsVisibleInStackTrace() in isolate.cc Ah, so "native" means "native but exposed to author script". Fascinating...
Message was sent while issue was closed.
On 2016/09/28 00:03:15, adamk wrote: > https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/1990803005/diff/60001/src/bootstrapper.cc#new... > src/bootstrapper.cc:2473: async_function_next->shared()->set_native(false); > On 2016/09/28 00:01:18, caitp wrote: > > On 2016/09/27 23:57:11, adamk wrote: > > > I found it surprising that this was done to _keep_ the function from > appearing > > > in stack traces. Can you explain how that works? > > > > See IsNotInNativeScript() and IsVisibleInStackTrace() in isolate.cc > > Ah, so "native" means "native but exposed to author script". Fascinating... apparently not. its probably a coincidence that set_native(false) appeared to do what was intended |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
