Smalltalk › Squeak › Squeak - Dev

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

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

5 messages Options

Eliot Miranda-2

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

Hi Chris, Hi All,

On Thu, Jul 2, 2015 at 12:26 PM, Chris Muller <[hidden email]> 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

David T. Lewis

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

On Fri, Jul 03, 2015 at 07:23:27PM -0700, Eliot Miranda wrote:

> Hi Chris, Hi All,
>
> On Thu, Jul 2, 2015 at 12:26 PM, Chris Muller <[hidden email]> 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.

Squeak 4.6 needs to be frozen. This sounds like something that can be
addressed in a next release after 4.6, so lets do that please.

Dave

David T. Lewis

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

On Sat, Jul 04, 2015 at 12:10:41AM -0400, David T. Lewis wrote:

> On Fri, Jul 03, 2015 at 07:23:27PM -0700, Eliot Miranda wrote:
> > Hi Chris, Hi All,
> >
> > On Thu, Jul 2, 2015 at 12:26 PM, Chris Muller <[hidden email]> 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.
>
> Squeak 4.6 needs to be frozen. This sounds like something that can be
> addressed in a next release after 4.6, so lets do that please.
>

I guess that is entirely the wrong thing for me to say, since I don't
even know what would be involved in the fix.

What I should have said is that it is great that we have the release
candidate prepared in time for ECOOP (thanks!), so I think it would be
good to avoid any late changes to that release package unless really
necessary.

Dave

Eliot Miranda-2

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

Hi Dave,

On Jul 4, 2015, at 7:15 AM, "David T. Lewis" <[hidden email]> wrote:

> On Sat, Jul 04, 2015 at 12:10:41AM -0400, David T. Lewis wrote:
>> On Fri, Jul 03, 2015 at 07:23:27PM -0700, Eliot Miranda wrote:
>>> Hi Chris, Hi All,
>>>
>>> On Thu, Jul 2, 2015 at 12:26 PM, Chris Muller <[hidden email]> 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.
>>
>> Squeak 4.6 needs to be frozen. This sounds like something that can be
>> addressed in a next release after 4.6, so lets do that please.
>
> I guess that is entirely the wrong thing for me to say, since I don't
> even know what would be involved in the fix.
>
> What I should have said is that it is great that we have the release
> candidate prepared in time for ECOOP (thanks!), so I think it would be
> good to avoid any late changes to that release package unless really
> necessary.

I was about to say that I agree with your first reply even though, having slept on it, I woke up realizing the #2 fix above is easy. So given yiur second message I'll implement #2 and leave #1 for after the release.

To get the override in place, create a config for the status quo, commit a version of Kernel containing the default CompiledMethod>>#hasBreakpoint, create a config, then commit a version of System which restores the override. I guess I was tired last night cuz this is obvious. Ok time to get up, have a cup of tea and do this :)

> Dave
>
>

Eliot Miranda-2

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

Hi David,

On Sat, Jul 4, 2015 at 7:25 AM, Eliot Miranda <[hidden email]> wrote:

Hi Dave,

On Jul 4, 2015, at 7:15 AM, "David T. Lewis" <[hidden email]> wrote:

> On Sat, Jul 04, 2015 at 12:10:41AM -0400, David T. Lewis wrote:
>> On Fri, Jul 03, 2015 at 07:23:27PM -0700, Eliot Miranda wrote:
>>> Hi Chris, Hi All,
>>>
>>> On Thu, Jul 2, 2015 at 12:26 PM, Chris Muller <[hidden email]> 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.
>>
>> Squeak 4.6 needs to be frozen. This sounds like something that can be
>> addressed in a next release after 4.6, so lets do that please.
>
> I guess that is entirely the wrong thing for me to say, since I don't
> even know what would be involved in the fix.
>
> What I should have said is that it is great that we have the release
> candidate prepared in time for ECOOP (thanks!), so I think it would be
> good to avoid any late changes to that release package unless really
> necessary.

I was about to say that I agree with your first reply even though, having slept on it, I woke up realizing the #2 fix above is easy. So given yiur second message I'll implement #2 and leave #1 for after the release.

To get the override in place, create a config for the status quo, commit a version of Kernel containing the default CompiledMethod>>#hasBreakpoint, create a config, then commit a version of System which restores the override. I guess I was tired last night cuz this is obvious. Ok time to get up, have a cup of tea and do this :)

OK, #2 is implemented and committed. #1, the warning on defining a method containing a break, can wait until after the release. I think it's important to fix #2, though, because destroying method history is a bad bug.

Cheers!

> Dave
>
>

best,

Eliot