I think for the debugger, we all live with the bugs. I mean, we (I) unfortunately made the situation worse with the introduction of the closures around 1.0/1.1 and it has never been fixed. It is hard to fix too, this stuff is not simple. I contacted a few people quietly maybe a year ago to see if we all lived with it, because a few times i posted about the debugger and there was not a lot of response. I wondered if no one *actually* programmed in pharo. but no, we become masters of interpreting the debugger highlight... What can we do- 1) I have contacted Eliot (again). Historically he said it was fixed (or better) in teleplace images but I need him to send us any patches. He has also been helpful and highlighted the areas that need work like the decompiler and how we could write some tests to protect against unintended change 2) I have checked a recent Squeak, and it is bust there too. 3) I am writing some test cases. So that we can regression test the debugger highlighting, and building an analyser class (@esug) to make it easier to dig into the debugger. 4) carefully check Squeak class versions for merging changes into Pharo. this is not easy. 5) We have to rebuild the debugger model based on the new compiler / decompiler machinery. (This is a long term Pharo goal) I am just chipping away at the analysis, and i will share my code when i have something on the tracker. Also would be great in a separate thread if someone could explain to me the simulation guard primitive. i.e you can not step over process creation, but you can halt after it e.g. cheers, Mike

Hi Michael, I did in recent past and can renew help with 4) which is an immediate cheap solution compared to 5), but as you have seen in 2) the solution is not universal. On thing to remember with Decompiler, is that it sounds like a visitor pattern, but some very important features are sometimes implemented on the node side! This does not help understanding the too much complicated code (simple browsing is void, you often need debugging). Nicolas 2011/8/23 Michael Roberts <mike@mjr104.co.uk>:

I think for the debugger, we all live with the bugs. I mean, we (I) unfortunately made the situation worse with the introduction of the closures around 1.0/1.1 and it has never been fixed. It is hard to fix too, this stuff is not simple.  I contacted a few people quietly maybe a year ago to see if we all lived with it, because a few times i posted about the debugger and there was not a lot of response.  I wondered if no one *actually* programmed in pharo. but no, we become masters of interpreting the debugger highlight...

What can we do- 1) I have contacted Eliot (again). Historically he said it was fixed (or better) in teleplace images but I need him to send us any patches. He has also been helpful and highlighted the areas that need work like the decompiler and how we could write some tests to protect against unintended change

2) I have checked a recent Squeak, and it is bust there too.

3) I am writing some test cases. So that we can regression test the debugger highlighting, and building an analyser class (@esug) to make it easier to dig into the debugger.

4) carefully check Squeak class versions for merging changes into Pharo. this is not easy.

5) We have to rebuild the debugger model based on the new compiler / decompiler machinery. (This is a long term Pharo goal)

I am just chipping away at the analysis, and i will share my code when i have something on the tracker.

Also would be great in a separate thread if someone could explain to me the simulation guard primitive. i.e you can not step over process creation, but you can halt after it e.g.

cheers, Mike

2011/8/23 Stéphane Ducasse <stephane.ducasse@inria.fr>:

On Aug 23, 2011, at 10:01 PM, Nicolas Cellier wrote:

...

Hi Michael, I did in recent past and can renew help with 4) which is an immediate cheap solution compared to 5), but as you have seen in 2) the solution is not universal.

But Squeak has exactly the same problem that pharo to this respect I tried in 43-11481

Yes, not universal just means that...

...

On thing to remember with Decompiler, is that it sounds like a visitor pattern, but some very important features are sometimes implemented on the node side! This does not help understanding the too much complicated code (simple browsing is void, you often need debugging).

Nicolas

2011/8/23 Michael Roberts <mike@mjr104.co.uk>:

...

I think for the debugger, we all live with the bugs. I mean, we (I) unfortunately made the situation worse with the introduction of the closures around 1.0/1.1 and it has never been fixed. It is hard to fix too, this stuff is not simple.  I contacted a few people quietly maybe a year ago to see if we all lived with it, because a few times i posted about the debugger and there was not a lot of response.  I wondered if no one *actually* programmed in pharo. but no, we become masters of interpreting the debugger highlight...

What can we do- 1) I have contacted Eliot (again). Historically he said it was fixed (or better) in teleplace images but I need him to send us any patches. He has also been helpful and highlighted the areas that need work like the decompiler and how we could write some tests to protect against unintended change

2) I have checked a recent Squeak, and it is bust there too.

3) I am writing some test cases. So that we can regression test the debugger highlighting, and building an analyser class (@esug) to make it easier to dig into the debugger.

4) carefully check Squeak class versions for merging changes into Pharo. this is not easy.

...

...

5) We have to rebuild the debugger model based on the new compiler / decompiler machinery. (This is a long term Pharo goal)

I am just chipping away at the analysis, and i will share my code when i have something on the tracker.

Also would be great in a separate thread if someone could explain to me the simulation guard primitive. i.e you can not step over process creation, but you can halt after it e.g.

cheers, Mike

On Aug 24, 2011, at 9:09 AM, Michael Roberts wrote:

Nicolas thanks! super you have even a few minutes.

Even though Squeak has the problem, it is subtly different. I will post my little scaffolding class maybe the end of this week which just helps to slowly step a debugger through. And do this in different images.

Ok I'm curious to know then.

What would be great, on a wiki page perhaps, is the list of important classes that are involved in debugging. Then it makes it easier to track changes (Debugger, InstructionStream and so on). It is also somewhat educational, for me at least.

The other thing, which is more tricky, is to know what are the highlights we want? In VW i take it for granted it is correct, and possibly in old Squeak images too. Can we document this? Perhaps a start could be png files of "correct" highlights? Then we can build a test framework that asserts these intervals we can work out (1 to: 3) (34 to: 45) etc for given simulations of #send and #doStep. Any thoughts on that appreciated. Perhaps this is not needed when we finally figure out what is wrong.

Also as mentioned in the original bug list, there are other bugs with the debugger not just highlighting. Just wanted to say I wasn't ignoring those... ;-)

cheers, Mike

So the entries of interest for highlighting are Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext: Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes. This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap. By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting. The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range. Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now. So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this (ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ] and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications. Cheers Nicolas 2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how they slightly diverge. Left Pharo  Right  Squeak

50, 73     71, 73       diff 71, 73     71, 73 50, 73     50, 73 108, 115   79, 121      diff 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146     (diff negative size means no highlight) 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 etc... For example the first difference is because Pharo shows the whole assignment of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

;-) thanks! yes i looked at it a little with Marcus and got lost when the InstructionStream was invoked. Your analysis is really useful. i have a 4 hr train journey soon! Mike On Wed, Aug 24, 2011 at 10:08 PM, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:

2011/8/24 Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com>:

...

So the entries of interest for highlighting are

Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext:

Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes.

This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap.

By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting.

The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range.

Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now.

So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this

(ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ]

and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications.

Cheers

Nicolas

Oh, I couldn't resist and just tried, and it's indeed getting harder, because it involves a zoo of Compiler internals, at least the Encoder, the BlockAnalyzer... Funny, the method node is once more generated in this phase :). All this stuff is cached at upper level, but we have at least clues for further optimizations ;)

Nicolas

...

2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how they slightly diverge. Left Pharo Right Squeak

50, 73 71, 73 diff 71, 73 71, 73 50, 73 50, 73 108, 115 79, 121 diff 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 (diff negative size means no highlight) 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 etc... For example the first difference is because Pharo shows the whole

assignment

...

of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

tx nicolas this is a cool way to help :) Stef On Aug 24, 2011, at 9:54 PM, Nicolas Cellier wrote:

So the entries of interest for highlighting are

Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext:

Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes.

This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap.

By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting.

The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range.

Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now.

So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this

(ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ]

and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications.

Cheers

Nicolas

2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how they slightly diverge. Left Pharo Right Squeak

50, 73 71, 73 diff 71, 73 71, 73 50, 73 50, 73 108, 115 79, 121 diff 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 (diff negative size means no highlight) 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 etc... For example the first difference is because Pharo shows the whole assignment of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

And by changing last line of this piece of code in #transformToDo: test := MessageNode new receiver: blockVar selector: (increment key > 0 ifTrue: [#<=] ifFalse: [#>=]) arguments: (Array with: limit) precedence: precedence from: encoder sourceRange: (myRange first to: blockRange last). I get a "correct" selection... Nicolas 2011/8/25 Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com>:

A few more notes:

- Encoder>>rawSourceRanges answer a mapping AST Node -> sourceRange  Gnerally, I would expect sourceRange to be an Interval, but this has to be confirmed.  Theses ranges are constructed at source code Parse time (see senders of #noteSourceRange:forNode:) - The program counters are assigned to AST nodes at byte code #generate time - for inlined macros, like #to:do: in our case, this pc is after the initialize and test statements.

in CompiledMethod>>#rawSourceRangesAndMethodDo: I just evaluated this:

methNode encoder rawSourceRanges collect: [:ival |        sourceText copyFrom: ival first to: ival last]

And what I see is that the original to:do: message (before macros inlining) has the right range {1        to: 5        do: [:index |                temp := index.                collection                        add: [temp]]}->a Text for 'to: 5 do: [ :index |                temp := index.                collection add: [ temp ] ]' This node is the original #to:do: and has pc=42

There is also {a LeafNode}->a Text for '[ :index |                temp := index.                collection add: [ temp ] ]' which has a nil pc so it won't be taken into account in the source map.

But one of the messages produced by the inlining has this curious range: {index <= 5}->a Text for 'to: 5 do: [ :index |                temp := index.                c' This node has pc = 40, so it will be selected before the correct one above has a chance to be. {index <= 5} is the test statement produced by inlining, and this seems to be the incorrect highlighting we see. Thus we know we have to concentrate on macro inlining. This happens in MessageNode>>transformToDo: We'll see this.

In the interim, I played with eliminating the nodes having unitilialized pc Let us try to evaluate this snippet in debugger's inspector: (methNode encoder rawSourceRanges keys select: [:e | e pc > 0]) collect: [:node |        | ival |        ival := methNode encoder rawSourceRanges at: node.        node -> (sourceText copyFrom: ival first to: ival last)]

Oh god, a bug in the debugger: DoItIn: homeContext        ^ ((homeContext namedTempAt: 2) encoder rawSourceRanges keys                select: [:e | e pc > 0])                collect: [:node |                        | ival |                        ival := (node namedTempAt: 2) encoder rawSourceRanges at: node.                        node                                -> (sourceText copyFrom: ival first to: ival last)] (node namedTempAt: 2) does not mean a thing... A confusion occurred between the homeContext (DoItIn: method argument) and node (the block argument) Nevermind... forget about it.

Now let's just concentrate on MessageNode>>transformToDo: We see this code:        test := MessageNode new                                receiver: blockVar                                selector: (increment key > 0 ifTrue: [#<=] ifFalse: [#>=])                                arguments: (Array with: limit)                                precedence: precedence from: encoder                                sourceRange: (myRange first to: blockRange first).

So the intention seems to select 'to: 5 do: '

But we see this: BlockNode>>noteSourceRangeStart:end:encoder:        "Note two source ranges for this node.  One is for the debugger         and is of the last expression, the result of the block.  One is for         source analysis and is for the entire block."        encoder                noteSourceRange: (start to: end)                forNode: self closureCreationNode.        startOfLastStatement                ifNil:                        [encoder                                noteSourceRange: (start to: end)                                forNode: self]                ifNotNil:                        [encoder                                noteSourceRange: (startOfLastStatement to: end - 1)                                forNode: self]

So it seems intentional to select only the last instruction of the block for the debugger. We'd better not change this without prior asking Eliot. But obviously, this is not what is expected by the #transformToDo:

Nicolas

2011/8/25 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

tx nicolas this is a cool way to help :)

Stef

On Aug 24, 2011, at 9:54 PM, Nicolas Cellier wrote:

...

So the entries of interest for highlighting are

Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext:

Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes.

This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap.

By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting.

The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range.

Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now.

So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this

(ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ]

and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications.

Cheers

Nicolas

2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how they slightly diverge. Left Pharo  Right  Squeak

50, 73     71, 73       diff 71, 73     71, 73 50, 73     50, 73 108, 115   79, 121      diff 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146     (diff negative size means no highlight) 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 etc... For example the first difference is because Pharo shows the whole assignment of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

I will try it in my image, thanks! I got a little further with Andres. The thing about the loop example is that the block is copying values outside its scope. You get get extra bytecodes at the start of the method (Array new: ...) and extra bytecodes in the loop to do the copying of values. We suspect that the mapping is getting confused by this. This is because the encoder hands out the ranges of the original method correctly, but the mapping has to align the pc to ignore these bytecodes because they are not in the debugger source. Anyway that was our impression. I agree we need Eliot to review. cheers, Mike On Thu, Aug 25, 2011 at 9:43 PM, Stéphane Ducasse <stephane.ducasse@inria.fr

wrote:

thanks a lot nicolas It seems to work on my image too. I adding it to the bug entry. What is great is that tomorrow I will be able to follow your path in the train.

Stef On Aug 25, 2011, at 9:12 PM, Nicolas Cellier wrote:

...

And by changing last line of this piece of code in #transformToDo: test := MessageNode new receiver: blockVar selector: (increment key > 0 ifTrue: [#<=] ifFalse: [#>=]) arguments: (Array with: limit) precedence: precedence from: encoder sourceRange: (myRange first to: blockRange last). I get a "correct" selection...

Nicolas

2011/8/25 Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com>:

...

A few more notes:

- Encoder>>rawSourceRanges answer a mapping AST Node -> sourceRange Gnerally, I would expect sourceRange to be an Interval, but this has to be confirmed. Theses ranges are constructed at source code Parse time (see senders of #noteSourceRange:forNode:) - The program counters are assigned to AST nodes at byte code #generate time - for inlined macros, like #to:do: in our case, this pc is after the initialize and test statements.

in CompiledMethod>>#rawSourceRangesAndMethodDo: I just evaluated this:

methNode encoder rawSourceRanges collect: [:ival | sourceText copyFrom: ival first to: ival last]

And what I see is that the original to:do: message (before macros inlining) has the right range {1 to: 5 do: [:index | temp := index. collection add: [temp]]}->a Text for 'to: 5 do: [ :index | temp := index. collection add: [ temp ] ]' This node is the original #to:do: and has pc=42

There is also {a LeafNode}->a Text for '[ :index | temp := index. collection add: [ temp ] ]' which has a nil pc so it won't be taken into account in the source map.

But one of the messages produced by the inlining has this curious range: {index <= 5}->a Text for 'to: 5 do: [ :index | temp := index. c' This node has pc = 40, so it will be selected before the correct one above has a chance to be. {index <= 5} is the test statement produced by inlining, and this seems to be the incorrect highlighting we see. Thus we know we have to concentrate on macro inlining. This happens in MessageNode>>transformToDo: We'll see this.

In the interim, I played with eliminating the nodes having unitilialized pc Let us try to evaluate this snippet in debugger's inspector: (methNode encoder rawSourceRanges keys select: [:e | e pc > 0]) collect: [:node | | ival | ival := methNode encoder rawSourceRanges at: node. node -> (sourceText copyFrom: ival first to: ival last)]

Oh god, a bug in the debugger: DoItIn: homeContext ^ ((homeContext namedTempAt: 2) encoder rawSourceRanges keys select: [:e | e pc > 0]) collect: [:node | | ival | ival := (node namedTempAt: 2) encoder rawSourceRanges at: node. node -> (sourceText copyFrom: ival first to: ival last)] (node namedTempAt: 2) does not mean a thing... A confusion occurred between the homeContext (DoItIn: method argument) and node (the block argument) Nevermind... forget about it.

Now let's just concentrate on MessageNode>>transformToDo: We see this code: test := MessageNode new receiver: blockVar selector: (increment key > 0 ifTrue: [#<=] ifFalse: [#>=]) arguments: (Array with: limit) precedence: precedence from: encoder sourceRange: (myRange first to: blockRange first).

So the intention seems to select 'to: 5 do: '

But we see this: BlockNode>>noteSourceRangeStart:end:encoder: "Note two source ranges for this node. One is for the debugger and is of the last expression, the result of the block. One is for source analysis and is for the entire block." encoder noteSourceRange: (start to: end) forNode: self closureCreationNode. startOfLastStatement ifNil: [encoder noteSourceRange: (start to: end) forNode: self] ifNotNil: [encoder noteSourceRange: (startOfLastStatement to: end - 1) forNode: self]

So it seems intentional to select only the last instruction of the block for the debugger. We'd better not change this without prior asking Eliot. But obviously, this is not what is expected by the #transformToDo:

Nicolas

2011/8/25 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

tx nicolas this is a cool way to help :)

Stef

On Aug 24, 2011, at 9:54 PM, Nicolas Cellier wrote:

...

So the entries of interest for highlighting are

Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext:

Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes.

This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap.

By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting.

The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range.

Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now.

So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this

(ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ]

and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications.

Cheers

Nicolas

2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how they slightly diverge. Left Pharo Right Squeak

50, 73 71, 73 diff 71, 73 71, 73 50, 73 50, 73 108, 115 79, 121 diff 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 (diff negative size means no highlight) 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 etc... For example the first difference is because Pharo shows the whole

assignment

...

of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

While working on a chapter on blocks I got the following problems foo | a| a := 0. ^ {[a :=2] .[a]} | res | res := ZnClientTests new foo. res second value crLog. res first value. res second value crLog. stepping into new foo first positioned me only on the [a :=2 ] skipping the first assignment second I get debuggers when I want to try to get information. Stef

I read the mail of nicolas in the train (amazing no cooling system so it was more like a sona and I never realize before that second class in uk was o bad for tall people) and yes this code is complex. Having an AST based debugger is definitively an interesting alternative. Stef On Aug 27, 2011, at 10:58 AM, Michael Roberts wrote:

ok i'll take a look and try add some test cases to the analyser....

Mike

On Fri, Aug 26, 2011 at 5:16 PM, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote: While working on a chapter on blocks I got the following problems

foo

| a| a := 0. ^ {[a :=2] .[a]}

| res | res := ZnClientTests new foo. res second value crLog. res first value. res second value crLog.

stepping into new foo first positioned me only on the [a :=2 ] skipping the first assignment second I get debuggers when I want to try to get information.

Stef

Hi Eliot, Nicolas' change that we put on the tracker didn't really work as intended (Nicolas did you see that?), but i'll let him respond to the points he was making. For me (point #1) It would be better for you to try and tell us the classes/methods that are either obviously broken or need the attention to get to a series of fixes. e.g., ignoring the loop highlight for the moment, the pc mapping goes wrong right at the start of this example: toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]. by highlighting pc=2 (IIRC) which is the assignment, but the first highlight should be pc=3 which is the send of #new. It doesn't correctly map/avoid the extra instructions for the array initialisation for copying the values outside the block. is it possible to fix that first? where do we look? I am just wondering how this can be approached in stages if possible. i know it is all related at some level. stepping into the code/method that does the majority of the pc mapping is a funny experience ;-) because the method is structured in such a way to expose a number of these bugs. for point #2 ideally we would not ask for a step and have nothing happen. However i wonder if we can get the highlight areas correct, even if we have to "step" through hidden parts. This assumes we can suppress the highlight for the hidden bits which i think we already have...but only if we can do one without the other. I would then write tests for the highlights, and we would have some protection for further changes. thanks, Mike On Mon, Sep 5, 2011 at 5:14 PM, Eliot Miranda <eliot.miranda@gmail.com> wrote:

Hi Nicolas,     that's quite a lot to absorb.  Exactly what do you want me to review?  The first thing it seems to me is to define what we want to be highlighted at different bytecodes in an optimized loop.  A second thing is to think about how the debugger can avoid stepping through the hidden sends that are part of a loop.  What I mean is that     1 to: n do: [:i| ....] is actually      i := 1.      [i <= n] whileTrue: [ and so the debugger steps through i:= 1 and i <= n, even though these aren't visible in the source.  That's why one has to click several times to get into the body of the loop.  The debugger could for example use the source ranges for the bytecodes to step until the source range changes.  So if there is the same source range info for all the bytecodes involved in the loop iteration (not the loop body) the debugger will be able to respond to step properly.  What do you think?

[sorry for not having responded sooner; esug and personal issues have got in the way. apologies] On Thu, Aug 25, 2011 at 1:06 PM, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> wrote:

...

A few more notes:

- Encoder>>rawSourceRanges answer a mapping AST Node -> sourceRange  Gnerally, I would expect sourceRange to be an Interval, but this has to be confirmed.  Theses ranges are constructed at source code Parse time (see senders of #noteSourceRange:forNode:) - The program counters are assigned to AST nodes at byte code #generate time - for inlined macros, like #to:do: in our case, this pc is after the initialize and test statements.

in CompiledMethod>>#rawSourceRangesAndMethodDo: I just evaluated this:

methNode encoder rawSourceRanges collect: [:ival |        sourceText copyFrom: ival first to: ival last]

And what I see is that the original to:do: message (before macros inlining) has the right range {1        to: 5        do: [:index |                temp := index.                collection                        add: [temp]]}->a Text for 'to: 5 do: [ :index |                temp := index.                collection add: [ temp ] ]' This node is the original #to:do: and has pc=42

There is also {a LeafNode}->a Text for '[ :index |                temp := index.                collection add: [ temp ] ]' which has a nil pc so it won't be taken into account in the source map.

But one of the messages produced by the inlining has this curious range: {index <= 5}->a Text for 'to: 5 do: [ :index |                temp := index.                c' This node has pc = 40, so it will be selected before the correct one above has a chance to be. {index <= 5} is the test statement produced by inlining, and this seems to be the incorrect highlighting we see. Thus we know we have to concentrate on macro inlining. This happens in MessageNode>>transformToDo: We'll see this.

In the interim, I played with eliminating the nodes having unitilialized pc Let us try to evaluate this snippet in debugger's inspector: (methNode encoder rawSourceRanges keys select: [:e | e pc > 0]) collect: [:node |        | ival |        ival := methNode encoder rawSourceRanges at: node.        node -> (sourceText copyFrom: ival first to: ival last)]

Oh god, a bug in the debugger: DoItIn: homeContext        ^ ((homeContext namedTempAt: 2) encoder rawSourceRanges keys                select: [:e | e pc > 0])                collect: [:node |                        | ival |                        ival := (node namedTempAt: 2) encoder rawSourceRanges at: node.                        node                                -> (sourceText copyFrom: ival first to: ival last)] (node namedTempAt: 2) does not mean a thing... A confusion occurred between the homeContext (DoItIn: method argument) and node (the block argument) Nevermind... forget about it.

Now let's just concentrate on MessageNode>>transformToDo: We see this code:        test := MessageNode new                                receiver: blockVar                                selector: (increment key > 0 ifTrue: [#<=] ifFalse: [#>=])                                arguments: (Array with: limit)                                precedence: precedence from: encoder                                sourceRange: (myRange first to: blockRange first).

So the intention seems to select 'to: 5 do: '

But we see this: BlockNode>>noteSourceRangeStart:end:encoder:        "Note two source ranges for this node.  One is for the debugger         and is of the last expression, the result of the block.  One is for         source analysis and is for the entire block."        encoder                noteSourceRange: (start to: end)                forNode: self closureCreationNode.        startOfLastStatement                ifNil:                        [encoder                                noteSourceRange: (start to: end)                                forNode: self]                ifNotNil:                        [encoder                                noteSourceRange: (startOfLastStatement to: end - 1)                                forNode: self]

So it seems intentional to select only the last instruction of the block for the debugger. We'd better not change this without prior asking Eliot. But obviously, this is not what is expected by the #transformToDo:

Nicolas

2011/8/25 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

tx nicolas this is a cool way to help :)

Stef

On Aug 24, 2011, at 9:54 PM, Nicolas Cellier wrote:

...

So the entries of interest for highlighting are

Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext:

Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes.

This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap.

By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting.

The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range.

Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now.

So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this

(ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ]

and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications.

Cheers

Nicolas

2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how they slightly diverge. Left Pharo  Right  Squeak

50, 73     71, 73       diff 71, 73     71, 73 50, 73     50, 73 108, 115   79, 121      diff 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146     (diff negative size means no highlight) 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 132, 144   132, 144 147, 146   146, 146 146, 146   146, 146 79, 121    79, 121 108, 115   108, 115 etc... For example the first difference is because Pharo shows the whole assignment of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

-- best, Eliot

On Tue, Sep 6, 2011 at 2:37 PM, Eliot Miranda <eliot.miranda@gmail.com>wrote:

On Tue, Sep 6, 2011 at 2:10 PM, Michael Roberts <mike@mjr104.co.uk> wrote:

...

Hi Eliot,

Nicolas' change that we put on the tracker didn't really work as intended (Nicolas did you see that?), but i'll let him respond to the points he was making.

For me (point #1) It would be better for you to try and tell us the classes/methods that are either obviously broken or need the attention to get to a series of fixes.

e.g., ignoring the loop highlight for the moment, the pc mapping goes wrong right at the start of this example:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ].

by highlighting pc=2 (IIRC) which is the assignment, but the first highlight should be pc=3 which is the send of #new. It doesn't correctly map/avoid the extra instructions for the array initialisation for copying the values outside the block. is it possible to fix that first? where do we look?

Well, this works in Squeak. The pc ranges must be determined after generating the method *with closure analysis*. i.e. the code ranges must be derived form the same resulting bytecode. To derive 2 for the pc of the send of new the system I guess must be compiling for source ranges differently from normal compilation. Haver you compares Squeak to Pharo for this example?

and then things get better, but not ideal, if the send of startOfLastStatement: is removed from Parser>>statements:innerBlock:blockNode:, i.e. if hereType ~~ #rightBracket ifTrue: [[theBlockNode startOfLastStatement: (start := self startOfNextToken). (returns := self matchReturn) ... becomes hereType ~~ #rightBracket ifTrue: [[start := self startOfNextToken. (returns := self matchReturn) ... I think my confusion was that I wanted to remember in the block node the /end/ of the last statement, not the beginning, and I wasn't thinking at all clearly at the time. Looks like startOfLastStatement in BlockNode should be removed. With the above change the debugger highlights the "to: limit do:", but not the entire block as it would for a non-optimized block. For me, I want the debugger to highlight the entire thing, from to: expr though to the closing ']'. So I think more needs to be done (i.e. note the position of the closing ']' and pass this to the BlockNode).

...

I am just wondering how this can be approached in stages if possible. i know it is all related at some level. stepping into the code/method that does the majority of the pc mapping is a funny experience ;-) because the method is structured in such a way to expose a number of these bugs.

for point #2 ideally we would not ask for a step and have nothing happen. However i wonder if we can get the highlight areas correct, even if we have to "step" through hidden parts. This assumes we can suppress the highlight for the hidden bits which i think we already have...but only if we can do one without the other. I would then write tests for the highlights, and we would have some protection for further changes.

Right. First thing is to get the source ranges correct. Next thing is to modify the debugger so that step has a visible effect, i.e. that the debugger steps until the source range changes.

...

thanks, Mike

On Mon, Sep 5, 2011 at 5:14 PM, Eliot Miranda <eliot.miranda@gmail.com> wrote:

...

Hi Nicolas, that's quite a lot to absorb. Exactly what do you want me to review? The first thing it seems to me is to define what we want to be highlighted at different bytecodes in an optimized loop. A second thing is to think about how the debugger can avoid stepping through the hidden sends that are part of a loop. What I mean is that 1 to: n do: [:i| ....] is actually i := 1. [i <= n] whileTrue: [ and so the debugger steps through i:= 1 and i <= n, even though these aren't visible in the source. That's why one has to click several times to get into the body of the loop. The debugger could for example use the source ranges for the bytecodes to step until the source range changes. So if there is the same source range info for all the bytecodes involved in the loop iteration (not the loop body) the debugger will be able to respond to step properly. What do you think?

[sorry for not having responded sooner; esug and personal issues have got in the way. apologies] On Thu, Aug 25, 2011 at 1:06 PM, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> wrote:

...

A few more notes:

- Encoder>>rawSourceRanges answer a mapping AST Node -> sourceRange Gnerally, I would expect sourceRange to be an Interval, but this has to be confirmed. Theses ranges are constructed at source code Parse time (see senders of #noteSourceRange:forNode:) - The program counters are assigned to AST nodes at byte code #generate time - for inlined macros, like #to:do: in our case, this pc is after the initialize and test statements.

in CompiledMethod>>#rawSourceRangesAndMethodDo: I just evaluated this:

methNode encoder rawSourceRanges collect: [:ival | sourceText copyFrom: ival first to: ival last]

And what I see is that the original to:do: message (before macros inlining) has the right range {1 to: 5 do: [:index | temp := index. collection add: [temp]]}->a Text for 'to: 5 do: [ :index | temp := index. collection add: [ temp ] ]' This node is the original #to:do: and has pc=42

There is also {a LeafNode}->a Text for '[ :index | temp := index. collection add: [ temp ] ]' which has a nil pc so it won't be taken into account in the source map.

But one of the messages produced by the inlining has this curious

range:

...

{index <= 5}->a Text for 'to: 5 do: [ :index | temp := index. c' This node has pc = 40, so it will be selected before the correct one above has a chance to be. {index <= 5} is the test statement produced by inlining, and this seems to be the incorrect highlighting we see. Thus we know we have to concentrate on macro inlining. This happens in MessageNode>>transformToDo: We'll see this.

In the interim, I played with eliminating the nodes having unitilialized pc Let us try to evaluate this snippet in debugger's inspector: (methNode encoder rawSourceRanges keys select: [:e | e pc > 0]) collect: [:node | | ival | ival := methNode encoder rawSourceRanges at: node. node -> (sourceText copyFrom: ival first to: ival last)]

Oh god, a bug in the debugger: DoItIn: homeContext ^ ((homeContext namedTempAt: 2) encoder rawSourceRanges keys select: [:e | e pc > 0]) collect: [:node | | ival | ival := (node namedTempAt: 2) encoder rawSourceRanges at: node. node -> (sourceText copyFrom: ival first to: ival last)] (node namedTempAt: 2) does not mean a thing... A confusion occurred between the homeContext (DoItIn: method argument) and node (the block argument) Nevermind... forget about it.

Now let's just concentrate on MessageNode>>transformToDo: We see this code: test := MessageNode new receiver: blockVar selector: (increment key > 0 ifTrue: [#<=] ifFalse: [#>=]) arguments: (Array with: limit) precedence: precedence from: encoder sourceRange: (myRange first to: blockRange first).

So the intention seems to select 'to: 5 do: '

But we see this: BlockNode>>noteSourceRangeStart:end:encoder: "Note two source ranges for this node. One is for the debugger and is of the last expression, the result of the block. One is for source analysis and is for the entire block." encoder noteSourceRange: (start to: end) forNode: self closureCreationNode. startOfLastStatement ifNil: [encoder noteSourceRange: (start to: end) forNode: self] ifNotNil: [encoder noteSourceRange: (startOfLastStatement to: end - 1) forNode: self]

So it seems intentional to select only the last instruction of the block for the debugger. We'd better not change this without prior asking Eliot. But obviously, this is not what is expected by the #transformToDo:

Nicolas

2011/8/25 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

tx nicolas this is a cool way to help :)

Stef

On Aug 24, 2011, at 9:54 PM, Nicolas Cellier wrote:

...

So the entries of interest for highlighting are

Debugger>>contentsSelection Debugger>>pcRange CompiledMethod>>debuggerMap DebuggerMethodMap class>>forMethod: DebuggerMethodMap>>rangeForPC:contextIsActiveContext:

Then you see the DebuggerMethodMap>>forMethod:methodNode: takes both a CompiledMethod and its #methodNode. CompiledMethod>>methodNode invokes the Parser to get the AbstractSyntaxTree from method source, and if it ever fails ends up by trying to decompile the byteCodes.

This is the easy part. Now we to deal with #abstractPCForConcretePC: and #abstractSourceMap.

By reading CompiledMethod>>abstractPCForConcretePC: you should quickly understand that a concrete PC is a byte offset of current byteCode (the offsets displayed in the byteCode view) while the abstractPC is just the rank of current byteCode in the list of byteCodes instructions composing the CompiledMethod. This is just because "byteCodes" may spread on several bytes beside their name... This will use InstructionStream and InstructionClient which are just an iterator and a sort of visitor on byteCode instructions. So this is not really interesting.

The more interesting part is #abstractSourceMap There is a first step to obtain CompiledMethod>>rawSourceRangesAndMethodDo: This is the most important part. The rest is again a mapping from concretePC (instruction byte offset) to abstractPC (instruction rank). And some build of a dictionary mapping instruction rank (abstractPC) -> selected range.

Note that the last trick seems to use a regenerated CompiledMethod (theMethodToScan) rather than the original CompiledMethod. There is no assertion whether these two are equivalent or not. A priori, they should, unless the Compiler changed since last compilation or if its behaviour is affected by some Preferences... Would we introduce some customizable Compiler optimizations that this could become a problem (We would then add to map decompiled AST to source code AST, probably with guesses, unless the CompiledMethod would contain debugger friendly hints...) We will consider this is not a problem by now.

So let's now concentrate on rawSourceRangesAndMethodDo: The nice thing is that you now can just debug this

(ClosureTests>>#testToDoOutsideTemp) methodNode rawSourceRangesAndMethodDo: [:rs :mth | ]

and see how it goes in Squeak. I did not look in Pharo yet, but I would be amazed to see it much different. It's now late, and my spare time is off, but you have clues to get more insights. I wish you good debugging, and come back to me if it ever goes in deeper complications.

Cheers

Nicolas

2011/8/24 Michael Roberts <mike@mjr104.co.uk>:

...

...

Ok I'm curious to know then.

Here is a little trace from this example method:

toDoOutsideTemp | temp collection | collection := OrderedCollection new. 1 to: 5 do: [ :index | temp := index. collection add: [ temp ] ]

Trace is start,stop position of the highlight for each 'step over'.

Whilst the numbers are hard to visualise, below you can see how

they

...

slightly diverge. Left Pharo Right Squeak

50, 73 71, 73 diff 71, 73 71, 73 50, 73 50, 73 108, 115 79, 121 diff 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 (diff negative size means no highlight) 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 132, 144 132, 144 147, 146 146, 146 146, 146 146, 146 79, 121 79, 121 108, 115 108, 115 etc... For example the first difference is because Pharo shows the whole assignment of the first line as the first send, even though it is not. The second difference is that Pharo shows the assignment inside the block as the first highlight of the loop even though the to:do should be highlighted....but both Pharo & Squeak get the to:do: wrong when they choose to show it. hope you get the idea... Mike

-- best, Eliot

-- best, Eliot

-- best, Eliot

Hello Stef, I appreciate the work of the pharo community! We will continue reporting bugs and submit fixes that we made. Of course I do not expect anyone to fix all reported bugs, but there are issues like 4694 (debugger) where we really need your help. Other issues like the buggy code completion can be workarounded by me by disabling code completion ;-) So thanks for the welcome and I am looking forward to having 4694 fixed! Andrea Stéphane Ducasse schrieb:

Thanks andrea

Welcome to the pharo mailing-list and community.

Now I think that this is important that inside netstyle you also consider that if pharo is important for you (which I imagine) that you should also contribute. Bug reporting is already a contribution. Pharo is an open-source software mainly supported by the free time of people, people that are often do not earning their money from their pharo work (I thank them for all that). I also understand that people can get frustrated by changes but what can we do?

May be people can gather and check the fixes that are important to fix but we do not have the force to maintain.

Imagine well that we have one engineer full time since 8 months.

Stef

On Aug 23, 2011, at 8:54 AM, Andrea Brühlmann wrote:

...

Hello,

I reported some bugs on the issue tracker and hope you can help me with them!

4681: Accepting with the enter key does not work http://code.google.com/p/pharo/issues/detail?id=4681

4682: Code completion makes strange cursor placements and too many spaces http://code.google.com/p/pharo/issues/detail?id=4682

4683: Code completion breaks some search fields (errors during typing) http://code.google.com/p/pharo/issues/detail?id=4683

4684: Missing ctrl+w (methodNamesContainingIt:) http://code.google.com/p/pharo/issues/detail?id=4684

4685: Merge dialog: cannot resolve conflict with removed method http://code.google.com/p/pharo/issues/detail?id=4685

4686: Method category change creates no new version http://code.google.com/p/pharo/issues/detail?id=4686

4687: Errors during coding (MessageNotUnderstood: receiver of "morph" is nil) http://code.google.com/p/pharo/issues/detail?id=4687

4688: Progress bar disappears on image save http://code.google.com/p/pharo/issues/detail?id=4688

4689: MessageTally bug http://code.google.com/p/pharo/issues/detail?id=4689

4690: Progress bar position http://code.google.com/p/pharo/issues/detail?id=4690

4691: Bad line breaks in code until window is resized (because of bold text?) http://code.google.com/p/pharo/issues/detail?id=4691

4692: Drop downs should select the default entry (topmost) instead of a middle one http://code.google.com/p/pharo/issues/detail?id=4692

4693: Line breaks in tooltips are wrong http://code.google.com/p/pharo/issues/detail?id=4693

4694: Debugger: stepping over an error can not open a new debugger until I hit cmd+. http://code.google.com/p/pharo/issues/detail?id=4694

4695: Time asString prints nanos unrounded http://code.google.com/p/pharo/issues/detail?id=4695

-- Andrea Brühlmann

-- AB | ANDREA BRÜHLMANN · SOFTWARE ENGINEER | NETSTYLE · TERRASSENWEG 18 · CH-3012 BERN | TEL +41 31 356 42 54 · FAX +41 31 356 42 51 | WWW.NETSTYLE.CH · A.BRUEHLMANN@NETSTYLE.CH

BTW mike roberts is building a tool to be able to understand and fix the code range of the debugger hilighting. Probably your bug is also related to the closure introduction. Now the abstraction used is so low level that this is a pain and fragile. Ideally using an AST would be much better but this is for the future. Stef On Sep 5, 2011, at 9:07 AM, Andrea Brühlmann wrote:

Hello Stef,

I appreciate the work of the pharo community! We will continue reporting bugs and submit fixes that we made. Of course I do not expect anyone to fix all reported bugs, but there are issues like 4694 (debugger) where we really need your help. Other issues like the buggy code completion can be workarounded by me by disabling code completion ;-)

So thanks for the welcome and I am looking forward to having 4694 fixed!

Andrea

Stéphane Ducasse schrieb:

...

Thanks andrea Welcome to the pharo mailing-list and community. Now I think that this is important that inside netstyle you also consider that if pharo is important for you (which I imagine) that you should also contribute. Bug reporting is already a contribution. Pharo is an open-source software mainly supported by the free time of people, people that are often do not earning their money from their pharo work (I thank them for all that). I also understand that people can get frustrated by changes but what can we do? May be people can gather and check the fixes that are important to fix but we do not have the force to maintain. Imagine well that we have one engineer full time since 8 months. Stef On Aug 23, 2011, at 8:54 AM, Andrea Brühlmann wrote:

...

Hello,

I reported some bugs on the issue tracker and hope you can help me with them!

4681: Accepting with the enter key does not work http://code.google.com/p/pharo/issues/detail?id=4681

4682: Code completion makes strange cursor placements and too many spaces http://code.google.com/p/pharo/issues/detail?id=4682

4683: Code completion breaks some search fields (errors during typing) http://code.google.com/p/pharo/issues/detail?id=4683

4684: Missing ctrl+w (methodNamesContainingIt:) http://code.google.com/p/pharo/issues/detail?id=4684

4685: Merge dialog: cannot resolve conflict with removed method http://code.google.com/p/pharo/issues/detail?id=4685

4686: Method category change creates no new version http://code.google.com/p/pharo/issues/detail?id=4686

4687: Errors during coding (MessageNotUnderstood: receiver of "morph" is nil) http://code.google.com/p/pharo/issues/detail?id=4687

4688: Progress bar disappears on image save http://code.google.com/p/pharo/issues/detail?id=4688

4689: MessageTally bug http://code.google.com/p/pharo/issues/detail?id=4689

4690: Progress bar position http://code.google.com/p/pharo/issues/detail?id=4690

4691: Bad line breaks in code until window is resized (because of bold text?) http://code.google.com/p/pharo/issues/detail?id=4691

4692: Drop downs should select the default entry (topmost) instead of a middle one http://code.google.com/p/pharo/issues/detail?id=4692

4693: Line breaks in tooltips are wrong http://code.google.com/p/pharo/issues/detail?id=4693

4694: Debugger: stepping over an error can not open a new debugger until I hit cmd+. http://code.google.com/p/pharo/issues/detail?id=4694

4695: Time asString prints nanos unrounded http://code.google.com/p/pharo/issues/detail?id=4695

-- Andrea Brühlmann

-- AB | ANDREA BRÜHLMANN · SOFTWARE ENGINEER | NETSTYLE · TERRASSENWEG 18 · CH-3012 BERN | TEL +41 31 356 42 54 · FAX +41 31 356 42 51 | WWW.NETSTYLE.CH · A.BRUEHLMANN@NETSTYLE.CH

2011/9/5 Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com>:

2011/9/5 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

BTW mike roberts is building a tool to be able to understand and fix the code range of the debugger hilighting. Probably your bug is also related to the closure introduction.

Now the abstraction used is so low level that this is a pain and fragile. Ideally using an AST would be much better but this is for the future.

Stef

Stef, If you are saying that implementation is not crystal clear, I can only agree with you. Eliot did avoid a full rewrite (I guess he took the shortest path to make the available implementation work with closures), and this design decision can be questioned indeed.

But speaking of abstraction level by itself, I don't understand your sentence. To me, the abstraction is at the correct level. The Debugger has to map the low level execution machinery (Context/program counter/CompiledMethod/byteCodes) to high level specification visible to the user (source code). The Debugger does so by mapping bytecodes to source ranges via AST, no more, no less.

In case of inlined blocks, there are more instructions on the bytecode side than messages sent on the AST side, just to make the problem a bit more complex, so maybe there are more intelligent way to address the mapping problem (maybe you mean via an intermediate transformation of AST), but you'll have to explain this a bit deeper.

Nicolas

By the way, I would find it cool to have an optional byteCode view parallel to source code view and see the execution of byte codes, and why not, a view of Context stack frames. Also, the debugger might step message by message (AST-based) rather than byteCode by byteCode, is this what you mean by wrong abstraction level ? Nicolas

...

On Sep 5, 2011, at 9:07 AM, Andrea Brühlmann wrote:

...

Hello Stef,

I appreciate the work of the pharo community! We will continue reporting bugs and submit fixes that we made. Of course I do not expect anyone to fix all reported bugs, but there are issues like 4694 (debugger) where we really need your help. Other issues like the buggy code completion can be workarounded by me by disabling code completion ;-)

So thanks for the welcome and I am looking forward to having 4694 fixed!

Andrea

Stéphane Ducasse schrieb:

...

Thanks andrea Welcome to the pharo mailing-list and community. Now I think that this is important that inside netstyle you also consider that if pharo is important for you (which I imagine) that you should also contribute. Bug reporting is already a contribution.  Pharo is an open-source software mainly supported by the free time of people, people that are often do not earning their money from their pharo work (I thank them for all that). I also understand that people can get frustrated by changes but what can we do? May be people can gather and check the fixes that are important to fix but we do not have the force to maintain. Imagine well that we have one engineer full time since 8 months. Stef On Aug 23, 2011, at 8:54 AM, Andrea Brühlmann wrote:

...

Hello,

I reported some bugs on the issue tracker and hope you can help me with them!

4681: Accepting with the enter key does not work     http://code.google.com/p/pharo/issues/detail?id=4681

4682: Code completion makes strange cursor placements and too many spaces     http://code.google.com/p/pharo/issues/detail?id=4682

4683: Code completion breaks some search fields (errors during typing)     http://code.google.com/p/pharo/issues/detail?id=4683

4684: Missing ctrl+w (methodNamesContainingIt:)     http://code.google.com/p/pharo/issues/detail?id=4684

4685: Merge dialog: cannot resolve conflict with removed method     http://code.google.com/p/pharo/issues/detail?id=4685

4686: Method category change creates no new version     http://code.google.com/p/pharo/issues/detail?id=4686

4687: Errors during coding (MessageNotUnderstood: receiver of "morph" is nil)     http://code.google.com/p/pharo/issues/detail?id=4687

4688: Progress bar disappears on image save     http://code.google.com/p/pharo/issues/detail?id=4688

4689: MessageTally bug     http://code.google.com/p/pharo/issues/detail?id=4689

4690: Progress bar position     http://code.google.com/p/pharo/issues/detail?id=4690

4691: Bad line breaks in code until window is resized (because of bold text?)     http://code.google.com/p/pharo/issues/detail?id=4691

4692: Drop downs should select the default entry (topmost) instead of a middle one     http://code.google.com/p/pharo/issues/detail?id=4692

4693: Line breaks in tooltips are wrong     http://code.google.com/p/pharo/issues/detail?id=4693

4694: Debugger: stepping over an error can not open a new debugger until I hit cmd+.     http://code.google.com/p/pharo/issues/detail?id=4694

4695: Time asString prints nanos unrounded     http://code.google.com/p/pharo/issues/detail?id=4695

-- Andrea Brühlmann

-- AB    |   ANDREA BRÜHLMANN · SOFTWARE ENGINEER      |   NETSTYLE · TERRASSENWEG 18 · CH-3012 BERN      |   TEL  +41 31 356 42 54 · FAX  +41 31 356 42 51      |   WWW.NETSTYLE.CH · A.BRUEHLMANN@NETSTYLE.CH

We were thinking with marcus that using byte code to go back to sources code is broken by essence and that using an AST is the way to go. Now we would need a good AST-based interpreter to start with.

2011/9/5 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

BTW mike roberts is building a tool to be able to understand and fix the code range of the debugger hilighting. Probably your bug is also related to the closure introduction.

Now the abstraction used is so low level that this is a pain and fragile. Ideally using an AST would be much better but this is for the future.

Stef

Stef, If you are saying that implementation is not crystal clear, I can only agree with you. Eliot did avoid a full rewrite (I guess he took the shortest path to make the available implementation work with closures), and this design decision can be questioned indeed.

But speaking of abstraction level by itself, I don't understand your sentence. To me, the abstraction is at the correct level. The Debugger has to map the low level execution machinery (Context/program counter/CompiledMethod/byteCodes) to high level specification visible to the user (source code). The Debugger does so by mapping bytecodes to source ranges via AST, no more, no less.

In case of inlined blocks, there are more instructions on the bytecode side than messages sent on the AST side, just to make the problem a bit more complex, so maybe there are more intelligent way to address the mapping problem (maybe you mean via an intermediate transformation of AST), but you'll have to explain this a bit deeper.

Nicolas

...

On Sep 5, 2011, at 9:07 AM, Andrea Brühlmann wrote:

...

Hello Stef,

I appreciate the work of the pharo community! We will continue reporting bugs and submit fixes that we made. Of course I do not expect anyone to fix all reported bugs, but there are issues like 4694 (debugger) where we really need your help. Other issues like the buggy code completion can be workarounded by me by disabling code completion ;-)

So thanks for the welcome and I am looking forward to having 4694 fixed!

Andrea

Stéphane Ducasse schrieb:

...

Thanks andrea Welcome to the pharo mailing-list and community. Now I think that this is important that inside netstyle you also consider that if pharo is important for you (which I imagine) that you should also contribute. Bug reporting is already a contribution. Pharo is an open-source software mainly supported by the free time of people, people that are often do not earning their money from their pharo work (I thank them for all that). I also understand that people can get frustrated by changes but what can we do? May be people can gather and check the fixes that are important to fix but we do not have the force to maintain. Imagine well that we have one engineer full time since 8 months. Stef On Aug 23, 2011, at 8:54 AM, Andrea Brühlmann wrote:

...

Hello,

I reported some bugs on the issue tracker and hope you can help me with them!

4681: Accepting with the enter key does not work http://code.google.com/p/pharo/issues/detail?id=4681

4682: Code completion makes strange cursor placements and too many spaces http://code.google.com/p/pharo/issues/detail?id=4682

4683: Code completion breaks some search fields (errors during typing) http://code.google.com/p/pharo/issues/detail?id=4683

4684: Missing ctrl+w (methodNamesContainingIt:) http://code.google.com/p/pharo/issues/detail?id=4684

4685: Merge dialog: cannot resolve conflict with removed method http://code.google.com/p/pharo/issues/detail?id=4685

4686: Method category change creates no new version http://code.google.com/p/pharo/issues/detail?id=4686

4687: Errors during coding (MessageNotUnderstood: receiver of "morph" is nil) http://code.google.com/p/pharo/issues/detail?id=4687

4688: Progress bar disappears on image save http://code.google.com/p/pharo/issues/detail?id=4688

4689: MessageTally bug http://code.google.com/p/pharo/issues/detail?id=4689

4690: Progress bar position http://code.google.com/p/pharo/issues/detail?id=4690

4691: Bad line breaks in code until window is resized (because of bold text?) http://code.google.com/p/pharo/issues/detail?id=4691

4692: Drop downs should select the default entry (topmost) instead of a middle one http://code.google.com/p/pharo/issues/detail?id=4692

4693: Line breaks in tooltips are wrong http://code.google.com/p/pharo/issues/detail?id=4693

4694: Debugger: stepping over an error can not open a new debugger until I hit cmd+. http://code.google.com/p/pharo/issues/detail?id=4694

4695: Time asString prints nanos unrounded http://code.google.com/p/pharo/issues/detail?id=4695

-- Andrea Brühlmann

-- AB | ANDREA BRÜHLMANN · SOFTWARE ENGINEER | NETSTYLE · TERRASSENWEG 18 · CH-3012 BERN | TEL +41 31 356 42 54 · FAX +41 31 356 42 51 | WWW.NETSTYLE.CH · A.BRUEHLMANN@NETSTYLE.CH

On Mon, Sep 5, 2011 at 8:02 AM, Lukas Renggli <renggli@gmail.com> wrote:

Even if you have a AST debugger you still need at some point a bytecode to AST (or source) mapping, because otherwise you cannot just break into a debugger at a random point in the code. I imagine this jump from bytecode to AST interpretation quite difficult; likely you still need a full bytecode stepper to find a position where you can jump into the AST interpreter.

Right. Basically the two levels, source and bytecode are all one needs and a fixation with ASs doesn't really help. The bytecode is quite tractible, and look at the different performance/quality of Smalltalk vs Ruby for what bytecode vs AST architectures get you.

Lukas

On Monday, 5 September 2011, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:

...

We were thinking with marcus that using byte code to go back to sources code is broken by essence and that using an AST is the way to go. Now we would need a good AST-based interpreter to start with.

...

2011/9/5 Stéphane Ducasse <stephane.ducasse@inria.fr>:

...

BTW mike roberts is building a tool to be able to understand and fix the code range of the debugger hilighting. Probably your bug is also related to the closure introduction.

Now the abstraction used is so low level that this is a pain and fragile. Ideally using an AST would be much better but this is for the future.

Stef

Stef, If you are saying that implementation is not crystal clear, I can only agree with you. Eliot did avoid a full rewrite (I guess he took the shortest path to make the available implementation work with closures), and this design decision can be questioned indeed.

But speaking of abstraction level by itself, I don't understand your sentence. To me, the abstraction is at the correct level. The Debugger has to map the low level execution machinery (Context/program counter/CompiledMethod/byteCodes) to high level specification visible to the user (source code). The Debugger does so by mapping bytecodes to source ranges via AST, no more, no less.

In case of inlined blocks, there are more instructions on the bytecode side than messages sent on the AST side, just to make the problem a bit more complex, so maybe there are more intelligent way to address the mapping problem (maybe you mean via an intermediate transformation of AST), but you'll have to explain this a bit deeper.

Nicolas

...

On Sep 5, 2011, at 9:07 AM, Andrea Brühlmann wrote:

...

Hello Stef,

I appreciate the work of the pharo community! We will continue

reporting bugs and submit fixes that

...

...

we made. Of course I do not expect anyone to fix all reported bugs, but there are issues like 4694 (debugger) where we really need your help. Other issues like the buggy code completion can be workarounded by me by disabling code completion ;-)

So thanks for the welcome and I am looking forward to having 4694 fixed!

Andrea

Stéphane Ducasse schrieb:

...

Thanks andrea Welcome to the pharo mailing-list and community. Now I think that this is important that inside netstyle you also consider that if pharo is important for you (which I imagine) that you should also contribute. Bug reporting is already a contribution. Pharo is an open-source software mainly supported by the free time of people, people that are often do not earning their money from their pharo work (I thank them for all that). I also understand that people can get frustrated by changes but what can we do? May be people can gather and check the fixes that are important to fix but we do not have the force to maintain. Imagine well that we have one engineer full time since 8 months. Stef On Aug 23, 2011, at 8:54 AM, Andrea Brühlmann wrote:

...

Hello,

I reported some bugs on the issue tracker and hope you can help me with them!

4681: Accepting with the enter key does not work http://code.google.com/p/pharo/issues/detail?id=4681

4682: Code completion makes strange cursor placements and too many spaces http://code.google.com/p/pharo/issues/detail?id=4682

4683: Code completion breaks some search fields (errors during typing) http://code.google.com/p/pharo/issues/detail?id=4683

4684: Missing ctrl+w (methodNamesContainingIt:) http://code.google.com/p/pharo/issues/detail?id=4684

4685: Merge dialog: cannot resolve conflict with removed method

-- Lukas Renggli www.lukas-renggli.ch

-- best, Eliot

...

This would be too sad...

No. Too sad is being 1 year and a half to release one single release. Don't expect perfection in each release, because otherwise there won't be any release. If you really need this issue, then apply the patch to your own image. Only SERIOUS bugs can be integrated in already released versions. That a progress bar disappears when saving the image doesn't seem to be that serious.

You are completely right :-) We just started to minimize the amount of our personal patches and are getting used to putting them on the issue tracker. But you are right, we will never get rid of personal patches because we need them before they are integrated! And of course progress bars are not so serious :-) Thanks again for all your help. Andrea