Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[P12][P13] Preventing debug point removal when target method has been modified from reinstalling the method as it was before installation + adding tests #16782

Open
wants to merge 1 commit into
base: Pharo13
Choose a base branch
from

Conversation

adri09070
Copy link
Contributor

@adri09070 adri09070 commented Jun 17, 2024

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

…rom reinstalling the method as it was before installation + adding tests
@adri09070
Copy link
Contributor Author

@StevenCostiou

@adri09070
Copy link
Contributor Author

Unrelated failing tests

@adri09070 adri09070 changed the title Preventing debug point removal when target method has been modified from reinstalling the method as it was before installation + adding tests [P12][P13] Preventing debug point removal when target method has been modified from reinstalling the method as it was before installation + adding tests Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugger does not save code in method which has a breakpoint
1 participant