Hey Danno,

please take another look. There's still some wonkyness in ARM that I have to
suss out, but this is mostly done for now.

Siggi


https://codereview.chromium.org/16578008/diff/11001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/16578008/diff/11001/src/api.cc#newcode5150
src/api.cc:5150: bool v8::V8::SetFunctionEntryHook(FunctionEntryHook
entry_hook) {
On 2013/06/13 15:33:44, danno wrote:
API calls should take an Isolate parameter if you need it. To
deprecate the
other API, you might need to have both the Isolate and non-isolate
version for a
while, the non-isolate version calling the isolate version with
"i::Isolate::Current();"

Done.

https://codereview.chromium.org/16578008/diff/11001/src/api.cc#newcode5155
src/api.cc:5155: if (entry_hook != NULL &&
isolate->HasFunctionEntryHook())
On 2013/06/13 15:33:44, danno wrote:
Seems like this should be a ASSERT(entry_hook != NULL); and a if
(isolate->HasFunctionEntryHook()) {

Done.

https://codereview.chromium.org/16578008/diff/11001/src/api.cc#newcode5158
src/api.cc:5158: isolate->SetFunctionEntryHook(entry_hook);
On 2013/06/13 15:33:44, danno wrote:
It would be great to introduce some checks to make sure that this
routine is
called at a time that is acceptable. It must be invoked on an Isolate
before the
first code object is generated, otherwise you will get strange
effects. Is there
any way to police that? Or perhaps introduce a new isolate constructor
that
takes the hook to make sure that it's always set from the very
beginning?
Something like that might make this functionality harder to abuse.

Done.

https://codereview.chromium.org/16578008/diff/11001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/16578008/diff/11001/src/frames.cc#newcode46
src/frames.cc:46: ReturnAddressLocationResolver
return_address_location_resolver = NULL;
On 2013/06/13 15:33:44, danno wrote:
Shouldn't this also be isolate based now?

Sadly no. The instrumentation and profiler are per-process, and when
enabled, the profiler will rewrite all return addresses in the process.

https://codereview.chromium.org/16578008/diff/11001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/16578008/diff/11001/src/isolate.cc#newcode2171
src/isolate.cc:2171:
On 2013/06/13 15:33:44, danno wrote:
Stray whitespace change?

Done.

https://codereview.chromium.org/16578008/diff/11001/src/isolate.cc#newcode2458
src/isolate.cc:2458: if (function_entry_hook == NULL &&
function_entry_hook_ != NULL) {
On 2013/06/13 15:33:44, danno wrote:
Might it be a little clearer if you can never pass a non-null value to
function_entry_hook? Instead have an explicit DisableFunctionEntryHook
that
resets function_entry_hook_ to NullFunctionEntryHook?

I made this one-way as we'd discussed. It makes everything simpler and
slightly more efficient.

https://codereview.chromium.org/16578008/diff/11001/src/snapshot-common.cc
File src/snapshot-common.cc (right):

https://codereview.chromium.org/16578008/diff/11001/src/snapshot-common.cc#newcode84
src/snapshot-common.cc:84: bool Snapshot::Initialize(const char*
snapshot_file) {
On 2013/06/13 15:33:44, danno wrote:
If you need the isolate, you should pass it in as a parameter.

Moved this to the sole non-test user of this function, as I'm not sure
how to otherwise untangle this.

https://codereview.chromium.org/16578008/diff/11001/src/snapshot-common.cc#newcode88
src/snapshot-common.cc:88: if (isolate &&
isolate->HasFunctionEntryHook())
On 2013/06/13 15:33:44, danno wrote:
nit: isolate != NULL

Done.

https://codereview.chromium.org/16578008/

--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
--- You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to