Fix potential overwriting of debug jumps of following code.

Fix potential overwriting of debug jumps of following code.

Add JSArrayLength, CallKnownFunction, and InstanceType operations.
Remove LadGlobal and StoreGlobal again (they fail).

Committed: http://code.google.com/p/v8/source/detail?r=6645

[Rico, 2011-02-02 11:35:37]

LGTM, but I am wondering if we could avoid this jump table. This will be added
(even though we will very rarely use it) to all code objects. I added the
compiler guys as reviewer as well, maybe they have input.

http://codereview.chromium.org/6347067/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/assembler-x64.cc#newcod...
src/x64/assembler-x64.cc:417: void Assembler::Pad(int n) {
Not used

[fschneider, 2011-02-02 12:54:16]

drive-by comments:

Do you have a regression test for this?

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode225
src/safepoint-table.cc:225: int res = 0;
small nit: result instead of res

http://codereview.chromium.org/6347067/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:196: // jump instruction.
The comment should say that this is used for lazy (forced) deoptimization and
that the jumps/calls are to a deoptimization routine.

[Kevin Millikin (Chromium), 2011-02-02 13:05:33]

I really only looked at the deoptimizer stuff.  It's too complicated.

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode225
src/safepoint-table.cc:225: int res = 0;
No reason not to spell out 'result'.

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode226
src/safepoint-table.cc:226: if (deoptimization_info_.length() > 0) {
if (!deoptimization_info_.is_empty()) ...

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode227
src/safepoint-table.cc:227: unsigned last_gap_end =
deoptimization_info_[0].pc_after_gap;
It's not necessarily the last one, it's the previous one.

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode230
src/safepoint-table.cc:230: if (static_cast<int>(info.deoptimization_index) ==
It's probably clearer without the continue:

if (deopt_index != kNoDeopt...) {
  // stuff
}

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc
File src/x64/deoptimizer-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:72: while (instructions > 0) {
This whole code hunk in #ifdef DEBUG is repeated below.  It needs to be moved
into a separate function.

Isn't it an idiom to write "while (instructions-- > 0) ..."?  You could do the
same below, as well (if you don't factor into a separate function).

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:79: int call_length =
MacroAssembler::kCallInstructionLength;
I find all the code below confusing.

You only really care if call_length is >= kCallInstructionLength or not.  Please
use a boolean:

bool fits = true;

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:83: pc_offset + gap_code_size +
MacroAssembler::kCallInstructionLength;
All of the code in this if (deoptimization_index !=
Safepoint::kNoDeoptimizationIndex) is more understandable if you skip the gap as
soon as you know you will, rather than at the very end.

if (...) {
  last_pc_offset += gap_code_size;
  bool fits = true;
  int end_of_long_call = last_pc_offset +
MacroAssembler::kCallInstructionLength;

I also think something like target_offset is a better name for end_of_long_call.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:86: int next_pc = table.GetPcOffset(j);
Call this next_pc_offset of something so it's clear it's an offset and not an
address.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:91: call_length = next_pc - (pc_offset +
gap_code_size);
fits = false;
break;

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:95: if (call_length >=
MacroAssembler::kCallInstructionLength) {
if (fits) ...

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:96: CodePatcher patcher(
CodePatcher patcher(code->instruction_start() + last_pc_offset,
                    Assembler::kCallInstructionLength);

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:101: last_pc_offset += gap_code_size +
Assembler::kCallInstructionLength;
last_pc_offset += Assembler::kCallInstructionLength;

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:104: CodePatcher
jump_patcher(code->instruction_start() + jump_table,
It probably bears naming

Address jump_address = code->instruction_start() + jump_table;

Or the whole thing would be simpler if you used the address of the next jump
throughout rather than the offset.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:110: CodePatcher call_patcher(
CodePatcher call_patcher(code->instruction_start + last_pc_offset,
                         Assembler::kShortCallLength);

The code patcher requires the length to be exact, so you can't really pass the
variable call_length unless you ensure that it is always equal to the length of
this particular call (which is why I say you only actually care about whether
call_length >= kCallInstructionLength).

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:114: last_pc_offset += gap_code_size +
call_patcher.masm()->pc_offset();
last_pc_offset += Assembler::kShortCallLength;

[Lasse Reichstein, 2011-02-03 14:14:12]

Please see if it makes more sense now.

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode225
src/safepoint-table.cc:225: int res = 0;
Done.

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode226
src/safepoint-table.cc:226: if (deoptimization_info_.length() > 0) {
Done.

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode227
src/safepoint-table.cc:227: unsigned last_gap_end =
deoptimization_info_[0].pc_after_gap;
Ack. Pardon my bad Danglish.
(Not that it's even good Danish).

http://codereview.chromium.org/6347067/diff/1/src/safepoint-table.cc#newcode230
src/safepoint-table.cc:230: if (static_cast<int>(info.deoptimization_index) ==
Done.

http://codereview.chromium.org/6347067/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/assembler-x64.cc#newcod...
src/x64/assembler-x64.cc:417: void Assembler::Pad(int n) {
Removed.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc
File src/x64/deoptimizer-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:72: while (instructions > 0) {
Moved int3-writing to function called ZapInstructions.

Decrementing in the condition probably is an idiom (or perhaps it's "while
(--instructions >= 0)" for the case where you use the index in the loop - and
for the microoptimizers who prefer prefix decrement to postfix). I usually try
to avoid conditions with side effects, idiomatic or not, but I guess it's easy
enough to read here.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:79: int call_length =
MacroAssembler::kCallInstructionLength;
The code is generally confusing. I have refactored it completely, and moved the
iteration over deoptimization entries to a separate class.
This allows the loop to concentrate on just checking for room and writing one of
two code pieces.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:86: int next_pc = table.GetPcOffset(j);
Done.

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:110: CodePatcher call_patcher(
Fixed.

http://codereview.chromium.org/6347067/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:196: // jump instruction.
done.

[Kevin Millikin (Chromium), 2011-02-04 10:27:29]

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc
File src/x64/deoptimizer-x64.cc (right):

http://codereview.chromium.org/6347067/diff/1/src/x64/deoptimizer-x64.cc#newc...
src/x64/deoptimizer-x64.cc:72: while (instructions > 0) {
You can (but I'm not suggesting you do) write it as:

while (instructions --> 0) { ... }

and pronounce the "-->" operator as "goes to".  :)

[Kevin Millikin (Chromium), 2011-02-04 11:34:54]

The jump table patching looks nice.  I still have comments.

I'm not concerned about the size of the table.  In fact I suspect that it will
nearly always be zero.

http://codereview.chromium.org/6347067/diff/4012/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/safepoint-table.cc#newco...
src/safepoint-table.cc:235: if (deoptimization_info_.length() > 0) {
You can write "!deoptimization_info_.is_empty()".

http://codereview.chromium.org/6347067/diff/4012/src/safepoint-table.cc#newco...
src/safepoint-table.cc:241: continue;
I'm still not thrilled about continue here.  I would rather read

if (static_cast<int>(info.deoptimization_index) !=
    Safepoint::kNoDeoptimizationIndex) {
  if (previous_gap_end + limit > info.pc) {
    ++result;
  }
  previous_gap_end = info.pc_after_gap;
}

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc#new...
src/x64/assembler-x64.cc:919: void Assembler::call(Address target) {
Since this function is not entirely safe, the comment from the header should be
repeated here (to guard against someone checking the .cc file, seeing it's
implemented, and using it).

I wonder if we just give it a RelocInfo::Mode so it would generally work?

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc
File src/x64/deoptimizer-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:51: static void ZapInstructions(Code* code, unsigned
from_offset, unsigned length) {
A similar function is called ZapCodeRange on IA32.  We should probably align
them (I don't really care which name you choose, ZapInstructions seems a bit
better).

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:90: index_ = -1;
This is also the initial state.  It seems like you could instead use
table_->length() as the limit, then this loop becomes:

ASSERT(index_ < limit_);
while (++index_ < limit_) {
  if (table_->GetEntry(index_)...) return;
}

And the check in Next becomes

if (index_ == limit_) ...

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:130: #ifdef DEBUG
We usually write #ifdef aligned at the left margin, and do not indent the code
inside it relative to the code outside.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:146: if (!next_entry.is_valid() || next_pc_offset >=
call_end_offset) {
If the next entry is not valid, I think you should assert that you are not
writing into your jump table.  That is, if the function ends with a call we
should properly pad it before the jump table so we can patch the return site.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:151: RelocInfo::NONE);
Align with the other argument.

http://codereview.chromium.org/6347067/diff/4012/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:201: int byte_count =
MacroAssembler::kCallInstructionLength +
There is already padding in SafepointTable::Emit in case the function ends in a
call.  You are doubly padding on x64.

It's probably simplest to move the padding into this platform-specific function
on all platforms.

[Lasse Reichstein, 2011-02-04 12:32:13]

Thanks for the comments.

http://codereview.chromium.org/6347067/diff/4012/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/safepoint-table.cc#newco...
src/safepoint-table.cc:235: if (deoptimization_info_.length() > 0) {
On 2011/02/04 11:34:54, Kevin Millikin wrote:
> You can write "!deoptimization_info_.is_empty()".

Done.

http://codereview.chromium.org/6347067/diff/4012/src/safepoint-table.cc#newco...
src/safepoint-table.cc:241: continue;
On 2011/02/04 11:34:54, Kevin Millikin wrote:
> I'm still not thrilled about continue here.  I would rather read
> 
> if (static_cast<int>(info.deoptimization_index) !=
>     Safepoint::kNoDeoptimizationIndex) {
>   if (previous_gap_end + limit > info.pc) {
>     ++result;
>   }
>   previous_gap_end = info.pc_after_gap;
> }

Done.

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc#new...
src/x64/assembler-x64.cc:919: void Assembler::call(Address target) {
Comment moved.

The RelocInfo::Mode is a little tricky, since we (so far only) use the
instruction during patching, where relocinfo doesn't work anyway.
Suggestions welcome.
Using RelocInfo::INTERNAL_REFERENCE seems the correct thing, but it's used for
absolute references, not relative.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc
File src/x64/deoptimizer-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:51: static void ZapInstructions(Code* code, unsigned
from_offset, unsigned length) {
Renamed.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:90: index_ = -1;
On 2011/02/04 11:34:54, Kevin Millikin wrote:
> This is also the initial state.  It seems like you could instead use
> table_->length() as the limit, then this loop becomes:
> 
> ASSERT(index_ < limit_);
> while (++index_ < limit_) {
>   if (table_->GetEntry(index_)...) return;
> }
> 
> And the check in Next becomes
> 
> if (index_ == limit_) ...

Done.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:130: #ifdef DEBUG
Ack. I don't know how this got indented. Probably a human mistake.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:146: if (!next_entry.is_valid() || next_pc_offset >=
call_end_offset) {
I pad with space enough for one final call in all cases, so that should never be
a problem.
The problem is that where I count the number of necessary short calls, I don't
know the size of the code, so I always pad the code.

So we won't override the jump table, and if we do anyway, it will be caught by
the check in line 182.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:151: RelocInfo::NONE);
On 2011/02/04 11:34:54, Kevin Millikin wrote:
> Align with the other argument.

Done.

http://codereview.chromium.org/6347067/diff/4012/src/x64/deoptimizer-x64.cc#n...
src/x64/deoptimizer-x64.cc:173: }
As discussed offline, I also rewrite to use addresses instead of offsets.

http://codereview.chromium.org/6347067/diff/4012/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:201: int byte_count =
MacroAssembler::kCallInstructionLength +
I'll remove the extra padding here for now.

[Kevin Millikin (Chromium), 2011-02-04 12:58:24]

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc#new...
src/x64/assembler-x64.cc:919: void Assembler::call(Address target) {
On 2011/02/04 12:32:13, Lasse Reichstein wrote:
> Comment moved.
> 
> The RelocInfo::Mode is a little tricky, since we (so far only) use the
> instruction during patching, where relocinfo doesn't work anyway.
> Suggestions welcome.
> Using RelocInfo::INTERNAL_REFERENCE seems the correct thing, but it's used for
> absolute references, not relative.

To be clear, I thought the comment should be duplicated so that it occurs in
both places.

[Lasse Reichstein, 2011-02-04 13:04:11]

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6347067/diff/4012/src/x64/assembler-x64.cc#new...
src/x64/assembler-x64.cc:919: void Assembler::call(Address target) {
My bad. That was indeed what I did, but not what I wrote that I had done.