[squeak-dev] renaming a class causes invalid super pointer CompiledMethod..!

Nicolas Cellier nicolas.cellier.aka.nice at gmail.com
Wed Jan 21 00:05:37 UTC 2015


2015-01-20 23:56 GMT+01:00 Chris Muller <asqueaker at gmail.com>:

> Hi Eliot, there were TWO problems, Bert and Tobias have fixed one of
> them.  The second one is still unfixed in trunk.  When you run this
> script and arrive at the debugger, step INTO:
>
> Object compile: 'test  ^''Hello from Object'''.
> (Object subclass: #MyClass instanceVariableNames: ''
> classVariableNames: '' poolDictionaries: '' category: 'Test') compile:
> 'test  ^ super halt test, ''my super call lands on itself!'''.
> (Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.
> (Smalltalk classNamed: #MyNewAbstraction) new test
>
>
But it behaves as it should...
You first send halt to super.
If you resume it just answer self.

Then you send test to self (it's not a super send and never been)



> then be sure to step INTO the super call to #test.  See how it lands
> you in teh same method?  THAT bug exists back at least as far back as
> 4.2, so I don't think its Environments related.
>
> I took a look at it and presented my findings in the other email
> thread with subject "The Inbox: Environments-cmm.52.mcz", -- that
> screenshot showing that the bytecodes are teh same and the literal
> binding looks correct.
>
> So I have no idea why its not calling super correctly...
>
>
> On Tue, Jan 20, 2015 at 2:38 PM, Eliot Miranda <eliot.miranda at gmail.com>
> wrote:
> >
> > Ugh, I'm behind the times, Tobias you've already fixed it; thanks!
> Tobias, I like your use of becomeForward:; it is more robust.  But I
> *think* the undeclared removal is misplaced.  Don't you think that that
> code belongs in the BindingPolicy?
> >
> > On Tue, Jan 20, 2015 at 12:33 PM, Eliot Miranda <eliot.miranda at gmail.com>
> wrote:
> >>
> >>
> >>
> >> On Tue, Jan 20, 2015 at 12:26 PM, Eliot Miranda <
> eliot.miranda at gmail.com> wrote:
> >>>
> >>> Hi Chris, Hi Colin,
> >>>
> >>>
> >>> On Thu, Jan 15, 2015 at 2:48 PM, Chris Muller <ma.chris.m at gmail.com>
> wrote:
> >>>>
> >>>> I did a minor class-hierarchy refactoring today which resulted in a an
> >>>> invalid super pointer, resulting in a confusing image lock up due to
> >>>> tight recursion.  The script, below, demonstrates the issue in trunk:
> >>>>
> >>>> Object compile: 'test  ^''Hello from Object'''.
> >>>> (Object subclass: #MyClass instanceVariableNames: ''
> >>>> classVariableNames: '' poolDictionaries: '' category: 'Test') compile:
> >>>> 'test  ^ super halt test, ''my super call lands on myself!'''.
> >>>> (Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.
> >>>> ((Smalltalk classNamed: #MyNewAbstraction) subclass: #MyClass
> >>>> instanceVariableNames: '' classVariableNames: '' poolDictionaries: ''
> >>>> category: 'Test').
> >>>> (Smalltalk classNamed: #MyNewAbstraction) new test
> >>>>
> >>>> But an existing method in the original MyClass had a call to super
> >>>> which now lands in itself, resulting in a tight recursion.
> >>>>
> >>>> The same script without creating the new subclass results in an
> >>>> instant VM crash (both interpreter 2357 and cog 3205):
> >>>>
> >>>> Object compile: 'test  ^''Hello from Object'''.
> >>>> (Object subclass: #MyClass instanceVariableNames: ''
> >>>> classVariableNames: '' poolDictionaries: '' category: 'Test') compile:
> >>>> 'test  ^ super halt test, ''my super call lands on itself!'''.
> >>>> (Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.
> >>>> (Smalltalk classNamed: #MyNewAbstraction) new test
> >>>>
> >>>> I even tried recompiling my entire class hierarchy which was renamed,
> >>>> but it didn't help.
> >>>>
> >>>> How can this be?  I _KNOW_ I've used rename class many times in the
> >>>> past...  A regression?
> >>>
> >>>
> >>> This is indeed an Environments regression.  The issue is where to fix
> the problem.  One way to look at it is that
> Environment>>renameClass:from:to: does not update bindings in the renamed
> class's methods.
> >>>
> >>> Here's what the old code did:
> >>>
> >>> SystemDictionary>>renameClass: aClass from: oldName to: newName
> >>> "Rename the class, aClass, to have the title newName."
> >>>
> >>> | oldref category |
> >>> category := SystemOrganization categoryOfElement: oldName.
> >>> self organization classify: newName under: category suppressIfDefault:
> true.
> >>> self organization removeElement: oldName.
> >>> oldref := self associationAt: oldName.
> >>> self removeKey: oldName.
> >>> ** oldref key: newName.
> >>> self add: oldref.  "Old association preserves old refs"
> >>> Smalltalk renamedClass: aClass from: oldName to: newName.
> >>> self flushClassNameCache.
> >>> SystemChangeNotifier uniqueInstance classRenamed: aClass from: oldName
> to: newName inCategory: category
> >>>
> >>> The starred line changes the key on the old class's binding, meaning
> that all the methods whose methodClassAssociation is that binding get
> automatically updated (note that class methods have a
> methodClassAssociation that has no key and just points to the class object).
> >>>
> >>> The new code simply forgets to do this:
> >>>
> >>> Environment>>renameClass: aClass from: oldName to: newName
> >>> "Rename the class, aClass, to have the title newName."
> >>>
> >>> | binding category |
> >>> category := self organization categoryOfElement: oldName.
> >>> self organization classify: newName under: category suppressIfDefault:
> true.
> >>> self organization removeElement: oldName.
> >>> binding := self declarationOf: oldName.
> >>> declarations removeKey: oldName.
> >>> self binding: binding removedFrom: self.
> >>> binding := newName => aClass.
> >>> declarations add: binding.
> >>> self binding: binding addedTo: self.
> >>> Smalltalk renamedClass: aClass from: oldName to: newName.
> >>> SystemChangeNotifier uniqueInstance
> >>> classRenamed: aClass
> >>> from: oldName
> >>> to: newName
> >>> inCategory: category
> >>>
> >>>
> >>> But should the change go in Environment>>renameClass:from:to: or in
> the BindingPolicy?  i.e. it could be implemented either in
> Environment>>renameClass:from:to:, or in Environment>>showBinding:.  I
> guess the former, but I'm cc'ing Colin, because know it?, he wrote it!
> >>
> >>
> >> and if the fix should indeed go in Environment>>renameClass:from:to: ,
> it is as simple as
> >>
> >> Environment>>renameClass: aClass from: oldName to: newName
> >> "Rename the class, aClass, to have the title newName."
> >>
> >> | binding category |
> >> category := self organization categoryOfElement: oldName.
> >> self organization classify: newName under: category suppressIfDefault:
> true.
> >> self organization removeElement: oldName.
> >> binding := self declarationOf: oldName.
> >> declarations removeKey: oldName.
> >> self binding: binding removedFrom: self.
> >> binding := newName => aClass.
> >> declarations add: binding.
> >> self binding: binding addedTo: self.
> >>
> >> ** aClass updateMethodBindingsTo: binding.
> >> Smalltalk renamedClass: aClass from: oldName to: newName.
> >> SystemChangeNotifier uniqueInstance
> >> classRenamed: aClass
> >> from: oldName
> >> to: newName
> >> inCategory: category
> >>
> >> I'll make this change and we can alter later if Colin thinks it's in
> the wrong place.
> >>
> >>>
> >>>
> >>> In any case there needs to be a test that checks that a method's
> methodClassAssociation is correctly updated after a rename.  I bet Colin
> would have not made this slip had there been a test.
> >>>
> >>>
> >>> Here's the code I used to test this:
> >>>
> >>> | mca1 mca2 |
> >>> Object compile: 'test  ^''Hello from Object'''.
> >>> (Object
> >>> subclass: #MyClass instanceVariableNames: ''
> >>> classVariableNames: ''
> >>> poolDictionaries: ''
> >>> category: 'Test')
> >>> compile: 'test  ^ super halt test, ''my super call lands on itself!'''.
> >>> mca1 := ((Smalltalk classNamed: #MyClass)>>#test)
> methodClassAssociation.
> >>> (Smalltalk classNamed: #MyClass)
> >>> rename: 'MyNewAbstraction'.
> >>> mca2 := ((Smalltalk classNamed: #MyNewAbstraction)>>#test)
> methodClassAssociation.
> >>> { mca1. mca2. mca1 == mca2. mca1 == (Smalltalk bindingOf:
> #MyNewAbstraction). mca2 == (Smalltalk bindingOf: #MyNewAbstraction)}
> >>>
> >>> It answers { #MyClass=>nil . #MyClass=>nil . true . false . false}
> >>> i.e. the binding isn't updated, but changes from #MyClass=>MyClass to
> #MyClass=>nil in binding:removedFrom:.
> >>>
> >>>
> >>> --
> >>> HTH,
> >>> Eliot
> >>
> >>
> >>
> >>
> >> --
> >> best,
> >> Eliot
> >
> >
> >
> >
> > --
> > best,
> > Eliot
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20150121/4864fc2f/attachment.htm


More information about the Squeak-dev mailing list