Thanks Ulan.

https://codereview.chromium.org/356393003/diff/60001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/356393003/diff/60001/src/arm/assembler-arm.cc#newcode1073
src/arm/assembler-arm.cc:1073: int Operand::instructions_required(const
Assembler* assembler,
On 2014/07/01 13:29:47, ulan wrote:
It is possible to accidentally misuse this function:

s = op.instructions_required(asm, instr)
// Do something assuming that instr will have size s
// Emit instructions to trigger extended constant pool
s != op.instructions_required(asm, instr)
// Actually emit instr with a different size

I don't see solution for this though.

Yes, this would theoretically be possible. I've added a comment warning
of this, and added some checks to ensure we never emit a constant pool
entry between making this call and emitting the actual instruction.

https://codereview.chromium.org/356393003/diff/60001/src/arm/assembler-arm.cc#newcode3710
src/arm/assembler-arm.cc:3710: Instruction::ImmedMovwMovtValue(instr) ==
0));
On 2014/07/01 13:29:47, ulan wrote:
Instruction::ImmedMovwMovtValue(next_instr) == 0

Good catch, thanks!

https://codereview.chromium.org/356393003/diff/60001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/356393003/diff/60001/src/arm/full-codegen-arm.cc#newcode4801
src/arm/full-codegen-arm.cc:4801: //   ldr ip, [pc/pp, <constant pool
offset>]  |   movw ip, <immed low>
On 2014/07/01 13:29:47, ulan wrote:
This comments suggests only two cases, but there are three (the third
being
movw_movt immediate)

Done.

https://codereview.chromium.org/356393003/diff/60001/src/arm/full-codegen-arm.cc#newcode4811
src/arm/full-codegen-arm.cc:4811: int branch_offset = pc -
Instruction::kPCReadOffset - branch_address +
On 2014/07/01 13:29:48, ulan wrote:
Where does Instruction::kPCReadOffset come from?


If you look at Assembler::branch_offset() it applies kPCReadOffset to a
pc offset.  This is used by Assembler::b(Label..) before calling through
to the raw Assembler::b(int..) function.

I think the code here was wrong originally, but "got away with it" due
to the fact that it didn't take the code generated by
EmitProfilingCounterReset into account in the calculation (I think it
jumped to the __str(r3, FieldMemOffset(r2, Cell::kValueOffset) at L371
rather than the instruction after as intended, but this worked because
r3 still held the old counter value.

https://codereview.chromium.org/356393003/diff/60001/src/arm/full-codegen-arm.cc#newcode4822
src/arm/full-codegen-arm.cc:4822: //
       |   movt ip, <immed high>
On 2014/07/01 13:29:47, ulan wrote:
Update the comment?

Done.

https://codereview.chromium.org/356393003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/356393003/diff/60001/src/objects.cc#newcode9677
src/objects.cc:9677: // Unfortunately the serializer relies on pointers
being visited in-order,
On 2014/07/01 13:29:48, ulan wrote:
Since we iterating first code pointers and the heap pointers, aren't
we already
out-of-order? What does 'in-order' mean?

In-order means that we visit the entries of the constant pool in order
(e.g., from top to bottom in terms of the description in
object.h:L3065-L3089), not the actual locations pointed to by these
entries.  Since the code entries come first, we are doing this in-order
here.

I've updated the comment to hopefully clarify this a bit.

https://codereview.chromium.org/356393003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to