Improved function entry hook coverage

Improved function entry hook coverage 

Adds more coverage for function entry hook, sufficient to capture profiles that are contiguous from C++, through JS and back out to C++.

R=danno@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=15361

Committed: http://code.google.com/p/v8/source/detail?r=15384

[danno, 2013-06-11 11:33:14]

Your changes look straight forward, I think your stub additions are fine. See my
other comment about one particular concern. You of course will also have to
address the serialization hacks, too.

https://codereview.chromium.org/16578008/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16578008/diff/1/src/ic.cc#newcode162
src/ic.cc:162: pc_address_ = ResolveReturnAddressLocation(pc_address);
The comment above about the performance sensitivity of this code true. You'd
have to run on golem to know for use, but I would expect a small regression in
our GameBoy emulator Octane test due this change.

[Sigurður Ásgeirsson, 2013-06-12 18:47:06]

Hey Danno,

I still need to get more testing in there and cover the non-ia32 platforms, but
I think it makes sense to keep this state per-isolate?

Siggi

[danno, 2013-06-13 15:33:44]

some comments

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)
{
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();"

https://codereview.chromium.org/16578008/diff/11001/src/api.cc#newcode5155
src/api.cc:5155: if (entry_hook != NULL && isolate->HasFunctionEntryHook())
Seems like this should be a ASSERT(entry_hook != NULL); and a if
(isolate->HasFunctionEntryHook()) {

https://codereview.chromium.org/16578008/diff/11001/src/api.cc#newcode5158
src/api.cc:5158: isolate->SetFunctionEntryHook(entry_hook);
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.

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;
Shouldn't this also be isolate based now?

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: 
Stray whitespace change?

https://codereview.chromium.org/16578008/diff/11001/src/isolate.cc#newcode2458
src/isolate.cc:2458: if (function_entry_hook == NULL && function_entry_hook_ !=
NULL) {
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?

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#ne...
src/snapshot-common.cc:84: bool Snapshot::Initialize(const char* snapshot_file)
{
If you need the isolate, you should pass it in as a parameter.

https://codereview.chromium.org/16578008/diff/11001/src/snapshot-common.cc#ne...
src/snapshot-common.cc:88: if (isolate && isolate->HasFunctionEntryHook())
nit: isolate != NULL

[Sigurður Ásgeirsson, 2013-06-19 20:51:51]

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#ne...
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#ne...
src/snapshot-common.cc:88: if (isolate && isolate->HasFunctionEntryHook())
On 2013/06/13 15:33:44, danno wrote:
> nit: isolate != NULL

Done.

[Sigurður Ásgeirsson, 2013-06-20 14:28:04]

Oki, I fixed the ARM crash. Turns out the Contains check is only valid for
V8-allocated memory, as It rounds the memory to a "page" boundary and
dereferences it. This is no good for arbitrary addresses that may point to
executables, V8 heap or nowhere at all (as in the ARM simulator's case).

[danno, 2013-06-25 18:40:35]

will finish tomorrow... nothing big yet

https://codereview.chromium.org/16578008/diff/50001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/16578008/diff/50001/src/bootstrapper.cc#newco...
src/bootstrapper.cc:2579: if (isolate->initialized_from_snapshot())
V8 style nit: then/else blocks wrapped in {}

https://codereview.chromium.org/16578008/diff/50001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16578008/diff/50001/src/ic.cc#newcode42
src/ic.cc:42: 
Stray whitespace change?

[danno, 2013-06-27 08:26:36]

Did you mean to add of src/snapshot-common.cc? It is generated and should not be
in the tree at all.

[Sigurður Ásgeirsson, 2013-06-27 13:06:40]

Thanks - addressed your nits and restored 644 file mode for the files I've
touched. Seems my editor is somehow messing this up.
On a tangent, the file mode for *.h and *.cc files is inconsistent in the github
clone of V8 at least.

https://codereview.chromium.org/16578008/diff/50001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/16578008/diff/50001/src/bootstrapper.cc#newco...
src/bootstrapper.cc:2579: if (isolate->initialized_from_snapshot())
On 2013/06/25 18:40:35, danno wrote:
> V8 style nit: then/else blocks wrapped in {}

Done.

https://codereview.chromium.org/16578008/diff/50001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16578008/diff/50001/src/ic.cc#newcode42
src/ic.cc:42: 
On 2013/06/25 18:40:35, danno wrote:
> Stray whitespace change?

Oops, had some code there earlier, fixed.

[danno, 2013-06-27 13:34:42]

lgtm, I'll land for you

[danno, 2013-06-27 13:37:24]

It doesn't look like this patch applies to tip of tree. Can you please rebase?

[Sigurður Ásgeirsson, 2013-06-27 14:33:37]

Done - seems at ToT someone snuck in a lint violation:

/usr/local/google/home/siggi/src/v8/src/hydrogen.h:431:  More than one command
on the same line  [whitespace/newline] [4]

I didn't do it, nobody saw me do it, you can't prove anything!

[danno, 2013-06-27 14:39:52]

Committed patchset #9 manually as r15361 (presubmit successful).

[ulan, 2013-06-27 15:08:36]

This CL broke arm build:

../src/arm/code-stubs-arm.cc: In member function 'virtual void
v8::internal::ProfileEntryHookStub::Generate(v8::internal::MacroAssembler*)':
../src/arm/code-stubs-arm.cc:7127: error: 'NONE' is not a member of
'v8::internal::RelocInfo'
../src/arm/code-stubs-arm.cc:7127: error: expected ';' before ')' token

[Sigurður Ásgeirsson, 2013-06-27 16:12:27]

Hey Danno,

I've fixed the respective compilation errors, am in progress of running X64
tests on Windows, but I can't run the native compiled ARM tests.
Can you retry the commit?

Siggi

[danno, 2013-06-28 13:40:57]

Committed patchset #11 manually as r15384 (presubmit successful).