pharo icon indicating copy to clipboard operation
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

Open adri09070 opened this issue 8 months ago • 2 comments

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

adri09070 avatar Jun 17 '24 12:06 adri09070