Bug 117373

Summary: REGRESSION: ASSERT failures in stack walking code on FTL branch
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, msaboff, oliver
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch. oliver: review+

Description Mark Lam 2013-06-08 13:20:44 PDT
The first assertion, in CallFrame::setLocationAsBytecodeOffset(), wrongly assumes that the incoming offset argument is in units of bytes. This is not true. It is in units of Instruction*. Hence, the assertion which checks for the low 2 bits to be clear can fail.  The fix is to remove the assertion.
Comment 1 Mark Lam 2013-06-08 13:23:42 PDT
Created attachment 204095 [details]
the patch.
Comment 2 Oliver Hunt 2013-06-08 13:27:50 PDT
Comment on attachment 204095 [details]
the patch.

Can we have an enum type so distinguish whether an offset should be bytes or instructions?

e.g. enum InstructionOffset {};  enum ByteOffset {}; ByteOffset byteOffsetFromInstruction(unsigned i) { return i * sizeof Instruction } , etc
Comment 3 Mark Lam 2013-06-08 13:31:15 PDT
<rdar://problem/14100876>
Comment 4 Mark Lam 2013-06-08 13:37:10 PDT
(In reply to comment #2)
> (From update of attachment 204095 [details])
> Can we have an enum type so distinguish whether an offset should be bytes or instructions?
> 
> e.g. enum InstructionOffset {};  enum ByteOffset {}; ByteOffset byteOffsetFromInstruction(unsigned i) { return i * sizeof Instruction } , etc

I'm not sure that will work.  My gut feeling is that it will result in a lot of casting, which would defeat the type safety checks (which is why I think you want the enums, right?).

Thanks for the review.  Patch landed in r151347: <http://trac.webkit.org>.
Comment 5 Mark Lam 2013-06-08 13:40:15 PDT
(In reply to comment #4)
> Patch landed in r151347: <http://trac.webkit.org>.

Landed in r151347: <http://trac.webkit.org/changeset/151347>.