On Tue, Sep 6, 2011 at 2:10 PM, Michael Roberts <[email protected]> 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? > 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 <[email protected]> > 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 > > <[email protected]> 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 <[email protected]>: > >> > 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 <[email protected]>: > >> >>> > >> >>>> > >> >>>> 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
