Closed Bug 470176 Opened 13 years ago Closed 13 years ago

TM: let-fun can modify consts

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: jruderman, Assigned: graydon)

References

Details

(Keywords: testcase, verified1.9.1)

Attachments

(1 file)

const e = 8; let (f = function() { for (var h=0;h<6;++h) ++e; }) { f(); } print(e);

interpreter: 8
jit: 12
Fixed by patch for bug 465443, I bet.

/be
(In reply to comment #1)
> Fixed by patch for bug 465443, I bet.

No, this is a path that does not involve the property cache -- plain old global variable pre-increment, recorded in the loop in the anonymous function by TraceRecorder::record_JSOP_INCNAME. Graydon, you game to kill this too?

/be
Sure. Though I'm perplexed: the code path for trace-recording incname very much
*does* go through TraceRecorder::name, and thereby funnel into the property
cache path in the trace recorder. And JSOP_INCNAME has JOF_INC, so should have
been caught by the last patch. I'll have to poke around and see what's going
on.
Assignee: general → graydon
Attached patch Fix the bugSplinter Review
Ah. So the case here involved my not having understood the activeCallOrGlobalSlot function before, and in fact not understanding the "importing" of properties in general. Now I'm a bit closer to understanding. It doesn't consult the property cache and it would be largely pointless for it to do so, accelerating trace-recording at best and having no effect on trace-execution. Plus the things it does with the result of lookup are entirely different (importing as an unboxed stack slot vs. emitting shape-guard and property-access code). Fine then! 

This patch adds tests to bail from the trace when writing to a const in this case as well, and a comment to explain to myself or any future travelers why this lookup helper differs from the test_property_cache helper, and why they probably ought not to be merged.

This also fixes the case in bug 470173.
Attachment #353740 - Flags: review?(brendan)
Attachment #353740 - Flags: review?(brendan) → review+
Comment on attachment 353740 [details] [diff] [review]
Fix the bug

Cool.

/be
This should block for sure, so the patch won't need a?.

/be
Status: NEW → ASSIGNED
Flags: blocking1.9.1?
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b3
Pushed to tracemonkey in revision 0ceae28c4c55.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 470173
Flags: blocking1.9.1? → blocking1.9.1+
Checking in js1_7/extensions/regress-470176.js
http://hg.mozilla.org/mozilla-central/rev/c16d5c53a362
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.