preserving history with breakpoints [Was Re: [squeak-dev] Squeak 4.6 release candidate]

Eliot Miranda eliot.miranda at gmail.com
Sat Jul 4 02:23:27 UTC 2015


Hi Chris, Hi All,

On Thu, Jul 2, 2015 at 12:26 PM, Chris Muller <ma.chris.m at gmail.com> wrote:

> We have a release candidate image.
>
>   http://ftp.squeak.org/4.6/
>
> The new sources file is required.
>
> Please test your apps.  This could be the final image unless major
> issues are uncovered.
>

One issue that I'm very tempted to address before the final release is the
bug to do with inadvertent editing of a breakpointed method.

When a breakpointed method is installed, the original is kept in a class
var of BreakpointManager, and the new method, while it has full source, has
that source stored in the method's trailer, not on the changes file, and so
there is no back pointer to the source.  If one doesn't realise the method
contains a break and one redefines it, then the version history for the
method will be broken.  The new version fo the method will appear to be the
only version.

There are two things that IMO should be done:

1. a special warning should be raised when the compiler encounters a self
break in method source.  The BreakpointManager can catch and squash this
warning when it is raised, but the warning can alert the programmer and
give them a chance to abort the compilation, revert the method, and
resubmit.

2. when
ClassDescription>>logMethodSource:forMethodWithNode:inCategory:withStamp:notifying:
looks for the previous version of a method it can query the
BreakpointManager to find out if the current version is a breakpointed
method and get the real method from the BreakpointManager to preserve the
history links.

#1 is nice to have, and likely reduces the need for #2, but #2 is a
must-have because it prevents history links being broken.  My issue is
how ClassDescription>>logMethodSource:forMethodWithNode:inCategory:withStamp:notifying:
queries the BreakpointManager.  The issue is the dependency it introduces
in a key Kernel method on the SYstem package.  Right now there are probably
lots of such dependencies and unloading System is impossible anyway, but I
don't want to introduce any more dependencies. I can see two approaches
that avoid the dependency.

One is to use CompiledMethod>>#hasBreakpoint:

logMethodSource: aText forMethodWithNode: aCompiledMethodWithNode
inCategory: category withStamp: changeStamp notifying: requestor
| priorMethodOrNil newText |
priorMethodOrNil := self compiledMethodAt: aCompiledMethodWithNode selector
ifAbsent: [].
(priorMethodOrNil notNil and: [priorMethodOrNil hasBreakpoint]) ifTrue:
[priorMethodOrNil := priorMethodOrNil nonBreakpointedOriginal].
newText := (requestor notNil
and: [Preferences confirmFirstUseOfStyle])
ifTrue: [aText askIfAddStyle: priorMethodOrNil req: requestor]
ifFalse: [aText].
aCompiledMethodWithNode method putSource: newText
fromParseNode: aCompiledMethodWithNode node
class: self category: category withStamp: changeStamp
inFile: 2 priorMethod: priorMethodOrNil.

where CompiledMethod>> nonBreakpointedOriginal is a new method that gets
the original from BreakpointManager.  But for this to be a good approach
CompiledMethod>> hasBreakpoint needs to be an override.
Currently hasBreakpoint is a System method:

!CompiledMethod methodsFor: '*System-Tools-debugger support' stamp: 'emm
5/30/2002 09:22'!
hasBreakpoint
^BreakpointManager methodHasBreakpoint: self! !

SO the question is how do I introduce an original that read

!CompiledMethod methodsFor: 'debugger support' stamp: 'blah de blah'!
hasBreakpoint
^false! !

so that  System's version overrides it?  Also how do I write an update that
accomplishes that?


The second approach is merely to use Smalltalk classNamed:, but that's ugly:

logMethodSource: aText forMethodWithNode: aCompiledMethodWithNode
inCategory: category withStamp: changeStamp notifying: requestor
| priorMethodOrNil newText |
priorMethodOrNil := self compiledMethodAt: aCompiledMethodWithNode selector
ifAbsent: [].
priorMethodOrNil ifNotNil:
[(Smalltalk classNamed: #BreakpointManager) ifNotNil:
[priorMethodOrNil := priorMethodOrNil nonBreakpointedOriginalFor:
priorMethodOrNil]].
newText := (requestor notNil
and: [Preferences confirmFirstUseOfStyle])
ifTrue: [aText askIfAddStyle: priorMethodOrNil req: requestor]
ifFalse: [aText].
aCompiledMethodWithNode method putSource: newText
fromParseNode: aCompiledMethodWithNode node
class: self category: category withStamp: changeStamp
inFile: 2 priorMethod: priorMethodOrNil.

or something similar.


I wouldn't suggest this except that I've made this error several times in
the VMMaker package over the past few weeks.  it's very easy to do, and it
screws up history horribly.  i really think we should fix this.  It makes
breakpoints dangerous otherwise.

What d'y'all think?

-- 
best,
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20150703/74263766/attachment.htm


More information about the Squeak-dev mailing list