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

integrating Debug Points model into Pharo #16177

Merged
merged 13 commits into from
Feb 23, 2024

Conversation

adri09070
Copy link
Contributor

@adri09070 adri09070 commented Feb 15, 2024

This PR integrates a new tool to Pharo (at least the model) that will replace the existing breakpoints and watchpoints model.
The original repository is here: https://github.com/adri09070/PharoDebugPoints (which is a fork of another repository: https://github.com/Max-Zurbriggen/PharoDebugPoints from @Max-Zurbriggen ).

This tool allows to install debug points on different type of targets:

  • an AST node, which can be targeted by selecting source code in the Calypso browser
  • a variable, which can be targeted by selecting a variable in the variable view in the Calypso browser
  • an object, in order to install object-centric debug points. In this case, an object target wraps another target (AST or variable)

Debug points are different types of instrumentation that are used to debug. For now, there are two concret types of debug points: breakpoints and watchpoints.

A debug point can have different types of behaviors that will execute before the debug point is actually hit.

Among these behaviors, there are:

  • Once behavior: the debug point becomes disabled after it is hit
  • Count behavior: counts the number of times the debug point is hit
  • Condition behavior: sets a condition that must evaluate to true to hit the debug point
  • Chain Link behavior: combines debug points into a chain. In that case, when a debug point is hit in the chain, it disables itself and enables the next debug point in the chain
  • Script behavior: executes a script each time a debug point is hit
  • Transcript behavior: logs a string to the Transcript each time a debug point is hit

All these behaviors can be set when creating a breakpoint from the Calypso browser in the method editor.

To have more flexibility to configure existing debug points or to create different types of debug points, it is possible to use an API on DebugPoint objects or via the DebugPointManager.

It will be possible to configure debug points via a UI tool, the Debug Point Browser, which will be the object of another PR in NewTools

@adri09070
Copy link
Contributor Author

There is a bootstrap error, I need to check

@Ducasse
Copy link
Member

Ducasse commented Feb 15, 2024

This is strange. Let us know if you need help.

@hernanmd
Copy link
Contributor

There is a bootstrap error, I need to check

Could you point us to the error?
Because in the CI I only see Instance of MetacelloMCBaselineOfProjectSpec did not understand #requires:

@Ducasse
Copy link
Member

Ducasse commented Feb 15, 2024

I learned something :) tx!

Copy link
Collaborator

@StevenCostiou StevenCostiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did only a partial pass, I will continue later.
In this pass, there are typos that needs to be fixed and comments for future work (i.e., not to be treated in this PR).

Comment on lines +17 to +21
DebugPoint all copy do: [ :debugPoint |
debugPoint link methods
detect: [ :m | m methodClass = anAnnouncement classRemoved ]
ifFound: [ debugPoint remove ] ]
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For later: because the DebugPoint model the breakpoints at a higher level than metalinks, it would simplify things (like that method) to know more of the underlying model, for instance here it would make sense that each debug points knows the structural elements on which it is installed/active.

In this case the debug point would know the methods it targets.

src/DebugPoints/DebugPointManager.class.st Outdated Show resolved Hide resolved
src/DebugPoints/DebugPointManager.class.st Outdated Show resolved Hide resolved
src/DebugPoints/DebugPointManager.class.st Outdated Show resolved Hide resolved
src/DebugPoints/DebugPointManager.class.st Outdated Show resolved Hide resolved
forObject: anObject
onVariableNamed: aSlotNameSymbol
accessStrategy: #read
withBehaviors: { }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark about empty behaviors.

forObject: anObject
onVariableNamed: aSlotNameSymbol
accessStrategy: #write
withBehaviors: { }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark on empty behaviors.

inClass: aClass
onVariableNamed: aSlotNameSymbol
accessStrategy: #all
withBehaviors: { }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark about empty behaviors.

| announcement |
announcement := DebugPointAdded
on: aDebugPoint
nodes: aDebugPoint link nodes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the question of high/low level concernes, I wonder if the nodes should be known by the model or only by the underlying implementation? @MarcusDenker do you have an opinion on that?

Comment on lines +370 to +374
DebugPoint all copy do: [ :debugPoint |
debugPoint link methods
detect: [ :m | m == aMethod ]
ifFound: [ debugPoint removeFromMethod: aMethod ] ]
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark, for future work: the debug point model should probably know the methods it affects without having to ask them to the metalink.

@Ducasse Ducasse merged commit a91c2ed into pharo-project:Pharo12 Feb 23, 2024
1 check passed
@MarcusDenker
Copy link
Member

This was merged with failing tests
(the CI was down, all PRs where green because no tests where run)

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.

None yet

6 participants