Fix memory leaks on x64...

Fix memory leaks on x64

This change uses ZoneObject as base class for our jumptable entry. In
addition this change refactors the JumpTableEntry a bit.


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

[Kevin Millikin (Chromium), 2011-03-09 09:02:10]

Drive by: is there any reason to malloc these at all (zone or otherwise)?

Just make jump_table_ a ZoneList<JumpTableEntry> instead of
ZoneList<JumpTableEntry*>?

[Lasse Reichstein, 2011-03-09 09:09:17]

If the ZoneList grows, it will copy the contents to a new and longer backing
array. I wasn't sure that the Labels would work if copied, but after
double-checking, it seems they would. I.e., you are right, there is no good
reason not to just embed the struct directly in the list.

[Lasse Reichstein, 2011-03-09 09:16:54]

Can you explain why this changes the memory behavior of the code? I can't see it
:)

I see no advantage in making this an external class (but that might be related
to not seeing any difference).

I also think that Kevin is right, and we should make it a
ZoneList<JumpTableEntry>.

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

http://codereview.chromium.org/6647012/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:549: jump_info->SetAddress(entry);
Why?
Is there any advantage in this, relative to setting the address in the
constructor? (Because it does mean setting the same memory's value twice and
temporarily leaving the entry in an invalid state).

(On the other hand, if you do make it a list of JumpTableEntrys instead of
pointers to same, you might want a no-argument constructor and a setter that
initializes the object).

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

http://codereview.chromium.org/6647012/diff/1/src/x64/lithium-codegen-x64.h#n...
src/x64/lithium-codegen-x64.h:277: class JumpTableEntry : public ZoneObject {
Why create an external class for a purely internal data-structure? Especially
when you need to make it a friend class anyway?

[Lasse Reichstein, 2011-03-09 09:36:12]

Never mind me. I missed that the new class was a ZoneObject.

I still think that we should make it embedded in the list, but if you want to
save that for later, this LGTM (if you initialize the address in the
constructor! :)

[Rico, 2011-03-09 09:48:06]

Changed this to use Kevins suggestion.

[Kevin Millikin (Chromium), 2011-03-09 09:54:32]

LGTM if comments below are addressed.

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

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:261: JumpTableEntry* info = &jump_table_[i];
This is OK, but I'm nervous to see it.  A pointer into the backing store of a
list can be dangerous because the backing store can change and the old one
become invalid.

It's OK here because the list isn't resized, but it could be a potential problem
with various refactorings.

It's easily avoided:

...i++) {
  __ bind(&jump_table_[i].label);
  __ Jump(jump_table_[i].address_, RelocInfo::RUNTIME_ENTRY);
}

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:543: if (jump_table_.length() == 0 ||
if (jump_table_.is_empty() || jump_table_.last().address != entry) {
  ...
}

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:547: __ j(cc, &jump_table_[jump_table_.length() -
1].label_);
__ j(cc, &jump_table_.last().label);

[Rico, 2011-03-09 09:55:44]

Added usage of last() and is_empty() on the zonelist instead of using the
length.

[Lasse Reichstein, 2011-03-09 09:57:06]

LGTM.

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

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:261: JumpTableEntry* info = &jump_table_[i];
Agree.

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.h:247: Label label_;
Kevin also reminded me that struct fields don't need the "_" suffix, so while
you're at it ... :)

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

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:545: jump_table_.Add(entry);
If the JumpTableEntry constructor was explicit, this wouldn't typecheck. You'll
need .Add(JumpTableEntry(address))

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.h:244: inline JumpTableEntry(Address address)
Whoops, we forgot the "explicit" here. I'm surprised it lints.