That code hit the debug assert that we had before, but I've removed the assert with this change (because it isn't true). A test based on the code in issue 220 passed in release builds before this change and would have passed in debug builds without the assert.
We should still be able to construct a regression test, I think. What would happen before the fix is that incorrectly dealing with the non-zero-sized reference would leave the topmost word of the reference (receiver for named references and key for keyed ones) in place of the word just below it (which would be the topmost frame-allocated local variable if there are any, or else the function). On Mon, Feb 2, 2009 at 7:54 AM, Kasper Lund <kasp...@chromium.org> wrote: > LGTM, but I do think we need a regression test case. What's wrong with > the code in issue 220 -- can't that be used a a regression test case? > You could even extend it a bit to also cover the const case. > > On Sun, Feb 1, 2009 at 9:53 AM, <kmilli...@chromium.org> wrote: > > Reviewers: Kasper Lund, Mads Ager, iposva, > > > > Message: > > This is a fix for issue 220. Discarding the value of SetValue on a > > reference and discarding the reference itself (which preserves > > top-of-stack) only commute when the reference is zero-sized. > > > > We should be able to construct a regression test for this, but it's > > tricky. > > > > We should also double check that our behavior in this case is what we > > want---it is only triggered by constant declarations and function > > declarations in statement position, neither of which is ECMA-262. > > > > Description: > > Fix for off-by-one when initializing a constant or function > > declaration that was not a slot. > > > > Please review this at http://codereview.chromium.org/19745 > > > > SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ > > > > Affected files: > > M src/codegen-arm.cc > > M src/codegen-ia32.cc > > > > > > Index: src/codegen-arm.cc > > =================================================================== > > --- src/codegen-arm.cc (revision 1198) > > +++ src/codegen-arm.cc (working copy) > > @@ -1144,15 +1144,13 @@ > > } > > > > if (val != NULL) { > > - // Set initial value. > > - Reference target(this, node->proxy()); > > - ASSERT(target.is_slot()); > > - Load(val); > > - target.SetValue(NOT_CONST_INIT); > > - // Get rid of the assigned value (declarations are statements). > It's > > - // safe to pop the value lying on top of the reference before > unloading > > - // the reference itself (which preserves the top of stack) because > we > > - // know it is a zero-sized reference. > > + { > > + // Set initial value. > > + Reference target(this, node->proxy()); > > + Load(val); > > + target.SetValue(NOT_CONST_INIT); > > + } > > + // Get rid of the assigned value (declarations are statements). > > frame_->Pop(); > > } > > } > > Index: src/codegen-ia32.cc > > =================================================================== > > --- src/codegen-ia32.cc (revision 1198) > > +++ src/codegen-ia32.cc (working copy) > > @@ -1431,15 +1431,13 @@ > > } > > > > if (val != NULL) { > > - // Set initial value. > > - Reference target(this, node->proxy()); > > - ASSERT(target.is_slot()); > > - Load(val); > > - target.SetValue(NOT_CONST_INIT); > > - // Get rid of the assigned value (declarations are statements). > It's > > - // safe to pop the value lying on top of the reference before > unloading > > - // the reference itself (which preserves the top of stack) because > we > > - // know that it is a zero-sized reference. > > + { > > + // Set initial value. > > + Reference target(this, node->proxy()); > > + Load(val); > > + target.SetValue(NOT_CONST_INIT); > > + } > > + // Get rid of the assigned value (declarations are statements). > > frame_->Pop(); > > } > > } > > > > > > > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
