Bug 117373 - REGRESSION: ASSERT failures in stack walking code on FTL branch
Summary: REGRESSION: ASSERT failures in stack walking code on FTL branch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-06-08 13:20 PDT by Mark Lam
Modified: 2013-06-08 13:40 PDT (History)
5 users (show)

See Also:


Attachments
the patch. (1.44 KB, patch)
2013-06-08 13:23 PDT, Mark Lam
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.