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#newcode83 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#newcode749 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#newcode7522 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#newcode7528 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#newcode7531 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#newcode7534 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#newcode7480 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#newcode7491 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#newcode7496 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#newcode6425 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#newcode6462 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. http://codereview.chromium.org/10706002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
