pharo
pharo copied to clipboard
[P12][P13] Preventing debug point removal when target method has been modified from reinstalling the method as it was before installation + adding tests
Fixes #16743
Description: When modifying in the debugger a method with a debug point in it, it would reinstall the method correctly in the debugger (which is correct) while reinstalling the version of the method before installation of the debug point (which is not correct, the installed method should be the modified one).
Explanation:
When recompiling a method in the debugger, it install the method by the OpalCompiler.
As the method already exists is recompiled, it creates a MethodModified
announcement.
This announcement is received by the DebugPointManager
, class-side.
DebugPointManager class >> #registerInterestToSystemAnnouncement
<systemEventRegistration>
...
self codeChangeAnnouncer weak when: MethodModified send: #handleMethodModified: to: self
...
DebugPointManager class >> #handleMethodModified: anAnnouncement
self removeFromMethod: anAnnouncement oldMethod
At this stage, the modified method is installed in the system.
When the announcement is received, it finally calls the method #removeFromMethod:
on each debug point targeting the modified method, which calls the method #removeFromMethod:for:
on their target.
In the case of a node, the method simply removes the debug point:
DebugPointNodeTarget>>#removeFromMethod: aMethod for: aDebugPoint
aDebugPoint remove
However, the method remove
from debug points does several things including uninstalling its metalink:
DebugPoint>>#remove
| nodes |
nodes := self link nodes copy.
self behaviors do: [ :bh | bh remove ].
self class remove: self.
self link ifNotNil: [ self link uninstall ].
DebugPointManager notifyDebugPointRemoved: self fromNodes: nodes
Uninstalling the metalink uninstalls the associated reflective method. However, this metalink has the compiledOnLinkInstallation
option.
In this case, uninstalling the reflective method actually reinstalls the compiled method as it was before the metalink was installed:
ReflectiveMethod> #removeLink: aMetaLink
(aMetaLink optionCompileOnLinkInstallation or: [
compiledMethod isRealPrimitive ])
ifTrue: [ self compileAndInstallCompiledMethod ]
ifFalse: [ compiledMethod invalidate ].
...
CompiledMethod>>#invalidate
| reflectiveMethod |
self reflectivityDisabled ifTrue: [ ^self ].
reflectiveMethod := self reflectiveMethod.
reflectiveMethod ifNil: [^self "do nothing"].
(self isRealPrimitive or: (reflectiveMethod ast metaLinkOptionsFromClassAndMethod includes: #optionCompileOnLinkInstallation))
ifTrue: [reflectiveMethod compileAndInstallCompiledMethod ]
ifFalse: [reflectiveMethod installReflectiveMethod]
So that's why the modified method in the debugger is overriden by the previous version of the method. The problem didn't appear in the previous breakpoint system, because breakpoints didn't uninstall their metalink when their method was modified or remove. So I fixed the problem by doing the same thing: for debug points: debug points do not uninstll their metalink if they are removed because their target method has been modified or removed. To me, this solution is not very clean, but it is okay as the metalink should be garbage collected anyway and their target method is not installed anymore, so it should not have any effect.
Anyway, the bug must be in Pharo12 too so the fix should be backported later