418889 - Instances of builtin types only referenced from the local variables can get prematurely collected

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Tommy Reilly]

(Note: Found in FP 9.0.115.0 aka MovieStar)

Steps to reproduce:
1. Launch Photoshop Express on FP 9.0.115.0
2. Launch Editor
3. Select crop/rotate
4. Pick up one of the crop handles and drag crop rectangle around
 
 Actual Results: Intermittent flashing of rectangle while cropping. This is indicative of this particular sort of failure:

private function f()
{
  var p = new Point(1,1);
  var x:Number = p.x;
  var y:Number = p.y;
  for(var i:int=0;i<10000;i++) { new Point(0,0); }
  x = p.x;
  y = p.y;
  trace(x+y)
}

Intermittently, the traced output will be 0 because foo will have been incorrectly collected when the other allocations take place.
  
Expected Results: No flashing.
  
Workaround (if any): 

If you subclass the built in type with a user defined type and use those types instead the JIT behaves differently and this problem doesn't arise.
 
 
 
Transferred Comments:

treilly - Thu Feb 21 14:58:55 CST 2008
I updated the bug to make it clearer

Comment 1

[Tommy Reilly]

Attachment: Hacky fix

Comment 2

[Edwin Smith]

Comment on attachment 304811 [details] [diff] [review]
Hacky fix

equivalent but less hacky/risky fix would be to add a new MIR opcode, MIR_addptr, identical to MIR_add, except without the MIR_cse flag.

if we find any other places we're doing pointer math, its easy to change LIR_add to LIR_addptr

Comment 3

[Tommy Reilly]

If we're gonna go that far why not just make cseMatch call extendLast use on the MIR_addptr's oprnd2?   would extendLastUse(ins, ip, ip) work?

Comment 4

[Tommy Reilly]

I scanned all MIR_add/sub's and this is the only instance of pointer math.

Comment 5

[Tommy Reilly]

Ed says:
 the extra 
> add's in x86 cost less than the extra store/loads from higher 
> register pressure.
>  

so I'll roll the MIR_addptr idea and forget about trying to make cse work.

Comment 6

[Tommy Reilly]

Attachment: Less hacky but essentially the same fix

Comment 7

[Rick Reitmaier]

Comment on attachment 304821 [details] [diff] [review]
Less hacky but essentially the same fix

ok,so teh ptr, is no longer getting cse'd but still, the exterior pointer may not survive past the wb call; no?  Did I miss some code?

Comment 8

[Tommy Reilly]

The real pointer will have to still be around for additional set/get slot operations b/c we aren't letting CSE happen on the derived pointer the object will be visible.

Using the unoffset pointer for the WB calls is really just fixing another bug (that Apollo ran into a couple weeks ago).

Comment 9

[Edwin Smith]

Comment on attachment 304821 [details] [diff] [review]
Less hacky but essentially the same fix

looks good to me, clearly x-platform testing is a must.

Comment 10

[Tommy Reilly]

Attachment: better fix, no changes in platform specific bits

Comment 11

[Tommy Reilly]

pushed:

http://hg.mozilla.org/tamarin-central/?rev/f114248654e0

Comment 12

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.