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

Chris Muller asqueaker at gmail.com
Tue Jan 20 22:56:46 UTC 2015


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

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
>
>
>


More information about the Squeak-dev mailing list