Implements a new API to set a function entry hook for profiling.

Implements a new API to set a function entry hook for profiling.

Exposes a new API; V8::SetFunctionEntryHook.
If a non-NULL function entry hook is set, the code generator(s) will invoke on the entry hook at the very start of each generated function.

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

[Sigurður Ásgeirsson, 2012-06-27 16:41:20]

Hi guys, here's a start on the entry hook API along with a test for it. Note
that as-is this will test only the non-optimized codegen, I'd appreciate some
tips on how to also cover the optimized path.
I made the entry hook function an uintptr_t to emphasize that this you can't
really pass in any old void (*)() - an entry hook really needs to be implemented
in asm. If there's a better or more appropriate way to express this in V8isms,
please let me know.

[danno, 2012-06-27 23:01:54]

My main concern here is the lack of support on other platforms, specifically
that the API silently fails on x64 and ARM.

https://chromiumcodereview.appspot.com/10706002/diff/1/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10706002/diff/1/include/v8.h#newcode3189
include/v8.h:3189: static void SetFunctionEntryHook(uintptr_t entry_hook);
I am not sure exactly what the right thing to do is, but it seems a bit fragile
that the call silently fails on x64 and ARM.

https://chromiumcodereview.appspot.com/10706002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/10706002/diff/1/test/cctest/test-api.c...
test/cctest/test-api.cc:10900: "function foo(i) { return i * i; }"
To test optimized code, you can for V8 to optimize a function the next time it
gets called. You need to call OptimizeFunctionOnNextCall, take a look at the
test case in "UndetectableOptimized" for an example of usage. Make sure that you
called the unoptimized version of a function at least once before optimizing it,
otherwise we won't have type information and will likely deopt immediately after
optimizing, and you won't be testing what you think.

https://chromiumcodereview.appspot.com/10706002/diff/1/tools/test-wrapper-gyp...
File tools/test-wrapper-gypbuild.py (right):

https://chromiumcodereview.appspot.com/10706002/diff/1/tools/test-wrapper-gyp...
tools/test-wrapper-gypbuild.py:220: returncodes +=
subprocess.call([sys.executable,
Why is this change needed?

[Sigurður Ásgeirsson, 2012-06-28 09:41:41]

Thanks, a new patch coming up, unless you object to the API suggestion.

https://chromiumcodereview.appspot.com/10706002/diff/1/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10706002/diff/1/include/v8.h#newcode3189
include/v8.h:3189: static void SetFunctionEntryHook(uintptr_t entry_hook);
On 2012/06/27 23:01:54, danno wrote:
> I am not sure exactly what the right thing to do is, but it seems a bit
fragile
> that the call silently fails on x64 and ARM.

Good point. Another thing that occurred to me is that there's no way to "stack"
this API, e.g. it doesn't make sense to have multiple profilers active at the
same time.
How about having this function return bool, return true on success, and fail it
on unsupported platforms as well as on setting any transition except to and from
a NULL hook?

https://chromiumcodereview.appspot.com/10706002/diff/1/tools/test-wrapper-gyp...
File tools/test-wrapper-gypbuild.py (right):

https://chromiumcodereview.appspot.com/10706002/diff/1/tools/test-wrapper-gyp...
tools/test-wrapper-gypbuild.py:220: returncodes +=
subprocess.call([sys.executable,
On 2012/06/27 23:01:54, danno wrote:
> Why is this change needed?

Ah, sorry, this is an unrelated change. This is needed for the script to run on
my machine, presumably because I don't have a python interpreter installed, and
so a file with a .py suffix will not execute directly.
Will move to a separate change.

[Sigurður Ásgeirsson, 2012-06-28 10:35:34]

Thanks, please take another look.

https://chromiumcodereview.appspot.com/10706002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/10706002/diff/1/test/cctest/test-api.c...
test/cctest/test-api.cc:10900: "function foo(i) { return i * i; }"
On 2012/06/27 23:01:54, danno wrote:
> To test optimized code, you can for V8 to optimize a function the next time it
> gets called. You need to call OptimizeFunctionOnNextCall, take a look at the
> test case in "UndetectableOptimized" for an example of usage. Make sure that
you
> called the unoptimized version of a function at least once before optimizing
it,
> otherwise we won't have type information and will likely deopt immediately
after
> optimizing, and you won't be testing what you think.

Done.

[Sigurður Ásgeirsson, 2012-06-29 15:43:45]

PTAL - I moved to using a code stub to bridge the entry hook to a function with
native calling conventions. I figure this can easily be supported and tested on
all platforms/architectures?
Does ARM have a high-resolution on-chip timer?

Note that I don't think it makes sense to enable the profiling in absence of the
rdtsc instruction on ia32/x64, where it has been fixed in late CPUs to make an
excellent wall-clock timer. I'm not sure how I'd go about that, however.
Also, I'm unsure about using the GetEntryHookLocation method to test for whether
profiling is enabled. Maybe it would be better to expose another method
IsProfilingEnabled or some such?

[Sigurður Ásgeirsson, 2012-07-03 14:08:06]

PTAL, I wrote the support for x64 and sketched the ARM stub. Right now ARM is
missing the timer, and note that because ARM doesn't have an indirect call, that
stub indirects through a forwarding function.
There's something I'm not getting about stack alignment on ARM, as-is, I'm
getting stack alignment errors on the third call.
Maybe I'm misunderstanding the calling convention? Are the register arguments
perhaps supposed to be stack-backed as on x64?

[danno, 2012-07-10 09:37:13]

Generally, the approach looks sound, but there is a bunch of details that need
to be ironed out.

http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:83: unsigned standard_features =
static_cast<unsigned>(OS::CpuFeaturesImpliedByPlatform() |
80 col

http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:749: if (fits_shifter(-static_cast<int>(imm32),
rotate_imm, immed_8, NULL)) {
these seem to be unrelated changes, probably to make some version of GCC work?
Perhaps more these to a separate CL?

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7522: __ Push(lr, r1, r0);
In general, you can't make guarantees about stack alignment within generated
code. We align on the way into C code in the CEntryStub, but since you don't use
that, you will actually have to handle both cases, the aligned and unaligned,
here if you expect the callee to have an aligned stack.

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7528: __ sub(r0, lr, Operand(0xC));
Define constants for this.

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7531: __ add(r1, sp, Operand(0x0C));
Is it possible to use an existing constant in frames-arm.h for this?

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7534:
reinterpret_cast<Address>(V8::GetFunctionEntryHookDispatcher()));
If you really need the dispatcher for ARM only, then it should just be part of
the ARM implementation and not exposed through the API. I am not sure why you
need the indirect dispatch, I think you can just use:

mov(pc, target_reg, LeaveCC, al);?

http://codereview.chromium.org/10706002/diff/6005/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/10706002/diff/6005/src/code-stubs.h#newcode1148
src/code-stubs.h:1148: 
Two empty lines between classes

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7480: __ db(0x0F);  // rdtsc();
Any reason to not add this to assembler-ia32.h?

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7491: __ lea(eax, Operand(esp, 0xC));
Constants? (maybe from frame-ia32.h, also applies to x64)

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7496: __ sub(eax, Immediate(5));
constant?

http://codereview.chromium.org/10706002/diff/6005/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/v8.cc#newcode185
src/v8.cc:185: profiler_entry_hook(function, stack_pointer, entry_time);
80 col?

http://codereview.chromium.org/10706002/diff/6005/src/v8.h
File src/v8.h (right):

http://codereview.chromium.org/10706002/diff/6005/src/v8.h#newcode107
src/v8.h:107: static FunctionEntryHook GetFunctionEntryHookDispatcher();
Why does the dispatch need to be exposed in the API? I don't think this is
something clients need to be able to call. In any case, there seem to be no
tests of this.

http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:6425: __ db(0x0F);  // rdtsc();
Add to the assembler?

http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:6462: code->set_is_pregenerated(true);
Why do you need to pre-generate on x64 and ia32 but not on ARM?

[Sigurður Ásgeirsson, 2012-07-11 14:50:34]

PTAL.
I'd like to discuss the pregenerate/no pregenerate issue with you, I don't
understand what the concerns are.

http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:83: unsigned standard_features =
static_cast<unsigned>(OS::CpuFeaturesImpliedByPlatform() |
On 2012/07/10 09:37:13, danno wrote:
> 80 col
Done.

http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:749: if (fits_shifter(-static_cast<int>(imm32),
rotate_imm, immed_8, NULL)) {
On 2012/07/10 09:37:13, danno wrote:
> these seem to be unrelated changes, probably to make some version of GCC work?
> Perhaps more these to a separate CL?

Ah true, this was to compile under VC 2008.
Will move to a separate cl.

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7522: __ Push(lr, r1, r0);
On 2012/07/10 09:37:13, danno wrote:
> In general, you can't make guarantees about stack alignment within generated
> code. We align on the way into C code in the CEntryStub, but since you don't
use
> that, you will actually have to handle both cases, the aligned and unaligned,
> here if you expect the callee to have an aligned stack. 

Done.

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7528: __ sub(r0, lr, Operand(0xC));
On 2012/07/10 09:37:13, danno wrote:
> Define constants for this.

Done.

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7531: __ add(r1, sp, Operand(0x0C));
On 2012/07/10 09:37:13, danno wrote:
> Is it possible to use an existing constant in frames-arm.h for this?

This stub is called pre-frame generation, and doesn't itself create a regular
stack frame - added a constant.

http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:7534:
reinterpret_cast<Address>(V8::GetFunctionEntryHookDispatcher()));
On 2012/07/10 09:37:13, danno wrote:
> If you really need the dispatcher for ARM only, then it should just be part of
> the ARM implementation and not exposed through the API. I am not sure why you
> need the indirect dispatch, I think you can just use:
> 
> mov(pc, target_reg, LeaveCC, al);?

This is for the simulator.
The simulator only forwards to functions that are tagged as an external
reference, and I couldn't figure a clean way to hook into that to dispatch
straight to the entry hook function.

The dispatcher function is in internal::V8 - I gather this is the
interface/implementation layer between the API and the internals?

I didn't notice any other data members in that namespace/class, but it would be
simpler and perhaps cleaner to declare the function pointer in that namespace
and to manipulate and access it directly?

http://codereview.chromium.org/10706002/diff/6005/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/10706002/diff/6005/src/code-stubs.h#newcode1148
src/code-stubs.h:1148: 
On 2012/07/10 09:37:13, danno wrote:
> Two empty lines between classes

Done.

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7480: __ db(0x0F);  // rdtsc();
On 2012/07/10 09:37:13, danno wrote:
> Any reason to not add this to assembler-ia32.h?

This is already in the assembler, but under a condition on a flag there. I don't
understand the reason for the condition & flag, so I just did the equivalent of
emit().
Removed now that the hook signature doesn't include a timer.

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7491: __ lea(eax, Operand(esp, 0xC));
On 2012/07/10 09:37:13, danno wrote:
> Constants? (maybe from frame-ia32.h, also applies to x64)

Done.

http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7496: __ sub(eax, Immediate(5));
On 2012/07/10 09:37:13, danno wrote:
> constant?

Done.

http://codereview.chromium.org/10706002/diff/6005/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/v8.cc#newcode185
src/v8.cc:185: profiler_entry_hook(function, stack_pointer, entry_time);
On 2012/07/10 09:37:13, danno wrote:
> 80 col?

Done.

http://codereview.chromium.org/10706002/diff/6005/src/v8.h
File src/v8.h (right):

http://codereview.chromium.org/10706002/diff/6005/src/v8.h#newcode107
src/v8.h:107: static FunctionEntryHook GetFunctionEntryHookDispatcher();
On 2012/07/10 09:37:13, danno wrote:
> Why does the dispatch need to be exposed in the API? I don't think this is
> something clients need to be able to call. In any case, there seem to be no
> tests of this.

This is src/v8.h, in namespace internal::V8, which I had read to be the
API<->implementation interface layer.
The API I had read to be defined in include/v8.h, which only contains the single
API function addition I intended to make.
This is tested through the unittest, as these are only used by internal
implementation.

http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:6425: __ db(0x0F);  // rdtsc();
On 2012/07/10 09:37:13, danno wrote:
> Add to the assembler?

Gone, as in the ia32 case.

http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:6462: code->set_is_pregenerated(true);
On 2012/07/10 09:37:13, danno wrote:
> Why do you need to pre-generate on x64 and ia32 but not on ARM?

I need to discuss this with you, or someone else knowledgeable on the issues.
I'm not sure what the concerns are, but I know that this code stub cannot be
serialized to a snapshot.

[Sigurður Ásgeirsson, 2012-07-12 11:58:58]

I believe this is ready for prime time, please take another look.

[danno, 2012-07-12 15:23:11]

LGTM. If you fix the nits and double check the TODO, I can land this for you.

https://chromiumcodereview.appspot.com/10706002/diff/20008/src/arm/code-stubs...
File src/arm/code-stubs-arm.cc (right):

https://chromiumcodereview.appspot.com/10706002/diff/20008/src/arm/code-stubs...
src/arm/code-stubs-arm.cc:7565: if (frame_alignment > kPointerSize)
nit: in V8 we always surround if code that can't requires a line break with {}

https://chromiumcodereview.appspot.com/10706002/diff/20008/test/cctest/test-a...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/10706002/diff/20008/test/cctest/test-a...
test/cctest/test-api.cc:10895: // TODO(siggi): Verify return_addr_location.
Do you want to do something about the TODO?

https://chromiumcodereview.appspot.com/10706002/diff/20008/test/cctest/test-a...
test/cctest/test-api.cc:10947: // Nulling it should alwasy succeed.
nit: always

[Sigurður Ásgeirsson, 2012-07-12 15:38:54]

Comments addressed - thanks!

https://chromiumcodereview.appspot.com/10706002/diff/20008/src/arm/code-stubs...
File src/arm/code-stubs-arm.cc (right):

https://chromiumcodereview.appspot.com/10706002/diff/20008/src/arm/code-stubs...
src/arm/code-stubs-arm.cc:7565: if (frame_alignment > kPointerSize)
On 2012/07/12 15:23:11, danno wrote:
> nit: in V8 we always surround if code that can't requires a line break with {}


Done.

https://chromiumcodereview.appspot.com/10706002/diff/20008/test/cctest/test-a...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/10706002/diff/20008/test/cctest/test-a...
test/cctest/test-api.cc:10895: // TODO(siggi): Verify return_addr_location.
On 2012/07/12 15:23:11, danno wrote:
> Do you want to do something about the TODO?

I'd prefer to leave it in for now, if that's all right?
I figure that once I have entry hooks for code stubs, I should be able to verify
that the return address seen on foo/bar invocations refers to code that I've
seen before.

https://chromiumcodereview.appspot.com/10706002/diff/20008/test/cctest/test-a...
test/cctest/test-api.cc:10947: // Nulling it should alwasy succeed.
On 2012/07/12 15:23:11, danno wrote:
> nit: always

Done.

[danno, 2012-07-12 15:39:59]

Landing

[Sigurður Ásgeirsson, 2012-07-12 16:43:19]

It seems some compilers will only cast function pointers to integral types or
void*, I believe I've seen precedent for intptr_t in V8 - please try submitting
again?

[Sigurður Ásgeirsson, 2012-07-13 15:58:30]

Fixed the linux/X64 fallout. I hadn't realized Windows stands alone in it's
calling convention :(.

[danno, 2012-07-13 16:16:16]

LGTM agian, landing

[Sigurður Ásgeirsson, 2012-07-16 14:55:51]

Please take another look. This is now passing tests on X64 for me.

[danno, 2012-07-17 15:13:10]

LGTM, landing