Thanks Soren, Anton for your comments. Could you please take another look at updated patch.
http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc#newcode670 src/arm/macro-assembler-arm.cc:670: // Reserve place for the return address and align the frame preparing for On 2011/02/02 13:24:38, Søren Gjesse wrote:
Please update this comment.
Done. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc#newcode1482 src/arm/macro-assembler-arm.cc:1482: ASSERT(allow_stub_calls()); // stub calls are not allowed in some stubs On 2011/02/02 13:24:38, Søren Gjesse wrote:
Start comment with uppercase letter end and with period.
Done. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc#newcode1492 src/arm/macro-assembler-arm.cc:1492: int64_t offset = (ref0.address() - ref1.address()); On 2011/02/02 13:56:28, antonm wrote:
should have noticed that earlier, sorry. I think ARM is 32-bit
platform (or at
least we only support 32-bit ARM), so you don't need to be that
paranoid about
offsets: return ref0.address() - ref1.address() should be enough.
sorry copy/paste mistake from x64. Done. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc#newcode1520 src/arm/macro-assembler-arm.cc:1520: // return address pushed on stack (could have moved after GC). On 2011/02/02 14:23:50, Søren Gjesse wrote:
On 2011/02/02 13:56:28, antonm wrote: > On 2011/02/02 13:24:38, Søren Gjesse wrote: > > As far as I can see this relies on DirectCEntryStub itself never
moving. It
is > > the same assumption we have for the CEntryStub (and the
RegExpCEntryStub I
> > think). Please add a comment on this. > > > > For the CEntryStub we have been safe so far as it is generated
quite early
> (with > > crankshaft this is actually not the case for the variant that
saves
doubles). > > How about this will it be generated early, or can a test case
where this
> > actually moves be crafted? > > Søren, yes, that's exactly the reason we call goes indirectly via
stub.
Sure, and as discussed offline having DirectCEntryStub created in Heap::CreateFixedStubs will in practice ensure that DirectCEntryStub
itself will
not move even though there is no explicit code in the GC to ensure
that. Added comment http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc#newcode1521 src/arm/macro-assembler-arm.cc:1521: DirectCEntryStub stub; On 2011/02/02 13:24:38, Søren Gjesse wrote:
I thing the calling of this stub should be factored out, maybe to a
method on
the stub
stub.GenerateCall(ref)
as this relies on the exact code generated by the stub.
Done. passed function as a parameter instead of ref. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-arm.cc#newcode1560 src/arm/macro-assembler-arm.cc:1560: mov(ip, Operand(ExternalReference(Top::k_pending_exception_address))); On 2011/02/02 13:56:28, antonm wrote:
that should be scheduled_exception, not pending.
(scheduled exceptions are thrown by embedder's code, pending by v8 and
JS)
and if this version passes the tests, that sucks. May I ask you to
add a test
case which reveals the bug? You should use v8::ThrowException in API
callback Thanks for catching and the explanation, i wasnt aware of the difference and none of the unit tests/browsing showed up the problem. Added a test. http://codereview.chromium.org/6170001/diff/125002/src/assembler.h File src/assembler.h (right): http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode471 src/assembler.h:471: // BUILTIN_CALL - builtin/runtime call. On 2011/02/02 13:24:38, Søren Gjesse wrote:
builtin/runtime -> builtin
Done. http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode474 src/assembler.h:474: // FP_CALL - builtin/runtime call that returns floating point. On 2011/02/02 13:24:38, Søren Gjesse wrote:
Please change "builtin/runtime call that returns floating point." to
something
like "direct call to a C-function which will never cause a GC". It is
not always
a function with the signature double f(double, double), e.g. "native_compare_doubles".
if iam not mistaken, native_compare_doubles uses the BUILTIN_CALL case and not FP_RETURN case. In that sense isnt the current comment accurate? http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode483 src/assembler.h:483: FP_RETURN_CALL, On 2011/02/02 13:24:38, Søren Gjesse wrote:
FP_RETURN_CALL -> FP_CALL as in comment.
modified the comment to FP_RETURN_CALL to reflect the current naming. Let me know if FP_CALL would be more accurate http://codereview.chromium.org/6170001/diff/125002/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/6170001/diff/125002/test/cctest/test-api.cc#newcode7360 test/cctest/test-api.cc:7360: v8::Handle<v8::ObjectTemplate> global = v8::ObjectTemplate::New(); On 2011/02/02 13:56:28, antonm wrote:
looks like you shouldn't use this global any more.
Done. http://codereview.chromium.org/6170001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
