Addressed comments and added runtime support for recognizing target
platforms
where movw/movt is a win. Please take another look.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode367
src/arm/assembler-arm-inl.h:367: ASSERT(IsMovT(Memory::int32_at(pc +
4)));
On 2012/10/10 13:56:52, jfb wrote:
+ kInstrSize
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode369
src/arm/assembler-arm-inl.h:369: Instruction* next_instr =
Instruction::At(pc + 4);
On 2012/10/10 13:56:52, jfb wrote:
+ kInstrSize
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode372
src/arm/assembler-arm-inl.h:372: instr->ImmedMovwMovtValue());
On 2012/10/10 13:56:52, jfb wrote:
The bottom two bits should be zero, which might be an easy
JIT-hardening. If the
bottom bit isn't zero and we pass the address to BX or BLX then we're
switching
to Thumb, which is a nice exploit vector.
I'd just & ~3
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode387
src/arm/assembler-arm-inl.h:387: // Or pre v8:
On 2012/10/10 13:56:52, jfb wrote:
Pre v8?
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode398
src/arm/assembler-arm-inl.h:398: IsMovT(Memory::int32_at(candidate +
4)));
On 2012/10/10 13:56:52, jfb wrote:
+ kInstrSize
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode443
src/arm/assembler-arm-inl.h:443: void
Assembler::set_target_address_at(Address pc, Address target) {
On 2012/10/10 13:56:52, jfb wrote:
As noted above:
target &= ~3;
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode445
src/arm/assembler-arm-inl.h:445: ASSERT(IsMovT(Memory::int32_at(pc +
4)));
On 2012/10/10 13:56:52, jfb wrote:
+ kInstrSize
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode457
src/arm/assembler-arm-inl.h:457: ASSERT(IsMovT(Memory::int32_at(pc +
4)));
On 2012/10/10 13:56:52, jfb wrote:
+ kInstrSize
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm-inl.h#newcode467
src/arm/assembler-arm-inl.h:467: // accessing this address in the
constant pool remains unchanged.
On 2012/10/10 13:56:52, jfb wrote:
Is this actually true? I can't find a reference to that effect.
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.cc#newcode819
src/arm/assembler-arm.cc:819: return true;
On 2012/10/10 14:19:29, Michael Starzinger wrote:
Indentation is off.
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.cc#newcode819
src/arm/assembler-arm.cc:819: return true;
On 2012/10/10 14:19:29, Michael Starzinger wrote:
Indentation is off.
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.cc#newcode842
src/arm/assembler-arm.cc:842: s == LeaveCC) {
I solved this a little differently. Embedded objects that are likely to
be patched (ICs) don't use the two-instruction sequence.
On 2012/10/10 13:56:52, jfb wrote:
In this case we're emitting 2 instructions, and when doing GC we need
to flush
the I$ for those two instructions. Would it be worth it to prevent
MOVW/MOVT
from straddling a cacheline? If at this point it would straddle then
we can just
emit a NOP, and then MOVW/MOVT together on the next cacheline. I'm
assuming JIT
function's code always starts at a cacheline boundary.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.cc#newcode844
src/arm/assembler-arm.cc:844: RecordRelocInfo(x.rmode_, x.imm32_,
DONT_USE_CONSTANT_POOL);
For object pointers, the address is not aligned, so it's not correct to
mask here. I've added masks elsewhere where the code target is known to
be aligned.
On 2012/10/10 14:34:29, Michael Starzinger wrote:
On 2012/10/10 13:56:52, jfb wrote:
> As before, if there's relocinfo then it's code (right?) so & ~3
I am not sure about this. We also have RelocInfos for embedded object
pointers.
Those will still be aligned to word boundaries, but the general
assumption that
relocation infos are just for code targets doesn't hold.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.h#newcode696
src/arm/assembler-arm.h:696: INLINE(static Address
target_address_from_return_address(Address pc));
On 2012/10/10 14:19:29, Michael Starzinger wrote:
Can we have a one-liner comment of what this method does (also on
other
architectures).
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-arm.h#newcode1491
src/arm/assembler-arm.h:1491: class ScopedPredictableCodeSize {
On 2012/10/10 14:19:29, Michael Starzinger wrote:
I would call this PredictableCodeSizeScope instead.
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/macro-assembler-arm.cc#newcode300
src/arm/macro-assembler-arm.cc:300: IsPowerOf2(src2.immediate() + 1)) {
Possible, but that's for a separate CL.
On 2012/10/10 13:56:52, jfb wrote:
It would be kind of silly, but if src2 == 0xffffffff then this is
undefined. In
that case it should just be a MVN Rd, 0, which can be handled before
this
else_if.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/assembler.h
File src/assembler.h (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/assembler.h#newcode375
src/assembler.h:375: // location provides a place for these pointers to
exist natually
On 2012/10/10 14:19:29, Michael Starzinger wrote:
s/natually/naturally/
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (left):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler-ia32.h#oldcode1
src/ia32/assembler-ia32.h:1: // Copyright (c) 1994-2006 Sun Microsystems
Inc.
On 2012/10/10 14:19:29, Michael Starzinger wrote:
Looks like a typo?
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler-ia32.h#newcode604
src/ia32/assembler-ia32.h:604: inline static Address
target_address_from_return_address(Address pc);
On 2012/10/10 14:19:29, Michael Starzinger wrote:
See comment in ARM assembler.
Done.
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):
https://chromiumcodereview.appspot.com/11037023/diff/25001/src/x64/assembler-x64.h#newcode584
src/x64/assembler-x64.h:584: static inline Address
target_address_from_return_address(Address pc);
On 2012/10/10 14:19:29, Michael Starzinger wrote:
See comment in ARM assembler.
Done.
https://chromiumcodereview.appspot.com/11037023/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
