2010/3/1 Nicolas Cellier nicolas.cellier.aka.nice@gmail.com:
2010/3/1 Nicolas Cellier nicolas.cellier.aka.nice@gmail.com:
2010/3/1 Levente Uzonyi leves@elte.hu:
Hi,
I recently ran into a new decompiler issue which doesn't allow the debugger to open. Details here: http://bugs.squeak.org/view.php?id=7467
So I thought it's time to revisit my decision of making DecompilerTests a subclass of LongTestCase, because I'm pretty sure noone is running these tests. According to my measurements 60-70% of their runtime is spent with garbage collecting. DecompilerTests >> #decompileClassesSelect: forces a full GC for every class in the image (>2000 in a clear trunk image). Is this GC still necessary?
There are also two new methods besides EventSensor >> #eventTickler which give a Syntax Error when running these tests. The new methods are SHMCClassDefinition >> #withAllSuperclasses and SHMCClassDefinition >> #allInstVarNames. This problem occurs when a temporary is declared in an inlined block which is the receiver of #whileTrue, #whileTrue:, #whileFalse or #whileFalse and the variable is also referenced from a normal block. For example: [ | foo | [ foo := false ] value ] whileTrue will be decompiled as: [[_r1 := false] value] whileTrue.
A lot of decompiler tests are failing, because the temporaries are indexed in a different order during the second decompilation.
I hope someone with enough knowledge will fix these issues.
I have no special knowledge. The code is quite complex (measured with # of instVars, # of methods, and # line of codes in certain methods). But, thanks you provided a good test case.
The first problem I see right from beginning is the order of trailer temp names: trailer tempNames -> '[input i index1 ps k count (digits)[each (digits)]]' I guess parentheses (digits) expresses a copiedValue.
Let explore the byte codes to see if it fits:
'1 0' readStream 81 <22> pushConstant: '1 0' 82 <D1> send: readStream
[:input: | ...] block argument 83 <8F 01 00 67> closureNumCopied: 0 numArgs: 1 bytes 87 to 189 87 <73> pushConstant: nil => i 88 <73> pushConstant: nil => index1 89 <73> pushConstant: nil => ps 90 <73> pushConstant: nil => k 91 <73> pushConstant: nil => count 92 <73> pushConstant: nil => UNUSED SLOT ????? 93 <73> pushConstant: nil => copiedValues #( digits ) 94 <10> pushTemp: 0 ( input ) 95 <DE> send: skipSeparators 96 <87> pop 97 <50> pushLit: Integer 98 <10> pushTemp: 0 ( input ) 99 <EF> send: readFrom: 100 <69> popIntoTemp: 1 ( i := ) 101 <11> pushTemp: 1 ( i ) 102 <75> pushConstant: 0 103 <B6> send: =
whileFalse: [... 104 <A8 52> jumpTrue: 188 106 <8A 01> push: (Array new: 1) 108 <6F> popIntoTemp: 7 ( #( digits ) ) copiedValues... Note that this is at rank 8, not 7 as expected from trailer tempNames... 109 <76> pushConstant: 1 110 <6C> popIntoTemp: 4 ( k :=) 111 <75> pushConstant: 0 112 <6D> popIntoTemp: 5 ( count := ) 113 <44> pushLit: Array 114 <25> pushConstant: 10 115 <75> pushConstant: 0 116 <F3> send: new:withAll: 117 <8E 00 07> popIntoTemp: 0 inVectorAt: 7 ( digits := )
120 <15> pushTemp: 5 ( count ) 121 <11> pushTemp: 1 ( i ) 122 <B2> send: <
whileTrue: [ 123 <AC 3A> jumpFalse: 183 125 <14> pushTemp: 4 ( k ) 126 <D6> send: printString 127 <6B> popIntoTemp: 3 ( ps := ) 128 <13> pushTemp: 3 129 <2C> pushConstant: $1 130 <EB> send: indexOf: 131 <81 42> storeIntoTemp: 2 ( index1 := ) 133 <75> pushConstant: 0 134 <B6> send: = 135 <99> jumpFalse: 138 136 <71> pushConstant: true 137 <95> jumpTo: 144 138 <13> pushTemp: 3 139 <2A> pushConstant: $3 140 <12> pushTemp: 2 ( index1 ) 141 <F9> send: indexOf:startingAt: 142 <75> pushConstant: 0 143 <B6> send: =
ifTrue: [ 144 <AC 1F> jumpFalse: 177 146 <15> pushTemp: 5 ( count ) 147 <76> pushConstant: 1 148 <B0> send: + 149 <6D> popIntoTemp: 5 ( count := ) 150 <13> pushTemp: 3 ( ps ) 151 <17> pushTemp: 7 ( #( digits) ) 152 <8F 11 00 13> closureNumCopied: 1 numArgs: 1 bytes 156 to 174 156 <8C 00 01> pushTemp: 0 inVectorAt: 1 ( digits ) 159 <10> pushTemp: 0 ( each ) 160 <D7> send: asciiValue 161 <28> pushConstant: 47 162 <B1> send: - 163 <8C 00 01> pushTemp: 0 inVectorAt: 1 ( digits ) 166 <10> pushTemp: 0 ( each ) 167 <D7> send: asciiValue 168 <28> pushConstant: 47 169 <B1> send: - 170 <C0> send: at: 171 <76> pushConstant: 1 172 <B0> send: + 173 <C1> send: at:put: 174 <7D> blockReturn 175 <CB> send: do: 176 <87> pop 177 <14> pushTemp: 4 ( k ) 178 <76> pushConstant: 1 179 <B0> send: + 180 <6C> popIntoTemp: 4 ( k := ) 181 <A3 C1> jumpTo: 120 183 <70> self 184 <DD> send: halt 185 <87> pop 186 <A3 A2> jumpTo: 94 188 <73> pushConstant: nil 189 <7D> blockReturn 190 <E0> send: in: 191 <7C> returnTop
What look *** STRANGE *** is the UNUSED slot reserved in [:input | ... ] temp vector...
Now decompiling in initSymbols: >> mapFromBlockStartsIn:toTempVarsFrom:constructor:
I got this map: there are only 3 blocks: (the other onesare inlined)
- the whole method (startpc=81)
- the [:input | ...] block (startpc=87)
- the [:each | ...] block (startpc=156)
And the temps names are (for each block indexed by start pc) a Dictionary( 81->#() 87->#( #('input' 1) #('i' 2) #('index1' 3) #('ps' 4) #('k' 5) #('count' 6) #('digits' #(7 1))) 156->#( #('each' 1) #('digits' #(2 1))) ) I think we got a problem here... digits should be #(8 1) to fit the compiled code and the UNUSED slot...
the tempVector has just 7 slots: {{input} . {i} . {index1} . {ps} . {k} . {count} . {{digits}}} later replaced with: {{input} . {i} . {index1} . {ps} . {k} . {count} . {_r7}}
Then, when decompiling: 108 <6F> popIntoTemp: 7 ( #( digits ) ) copiedValues... Note that this is at rank 8, not 7 as expected from trailer tempNames... the assertion fails because we only have 7 temps vars and try to assign temp of rank 8...
Now, just have to discover how the UNUSED slot was ever compiled...
Debugging this: Compiler evaluate: '''1 0'' readStream in: [ :input | | i | [ input skipSeparators. i := Integer readFrom: input. i = 0 ] whileFalse: [ | k count digits | k := 1. count := 0. digits := Array new: 10 withAll: 0. [ count < i ] whileTrue: [ | index1 ps | ps := k printString. ((index1 := ps indexOf: $1) = 0 or: [ (ps indexOf: $3 startingAt: index1) = 0 ]) ifTrue: [ count := count + 1. ps do: [ :each | digits at: each asciiValue - 47 put: (digits at: each asciiValue - 47) + 1 ] ]. k := k + 1 ]. self halt ] ] '
Visibly, the digits temp has two slots reserved, one for itself, one for copiedValues <2-8>: encoder blockExtentsToLocal -> a Dictionary( (0 to: 10)->#() (2 to: 8)->{{input} . {i} . {index1} . {ps} . {k} . {count} . {digits} . {<2-8>}} (4 to: 6)->{{each} . {<2-8>}} )
BlockNode>>addRemoteTemp: aTempVariableNode rootNode: rootNode "<MethodNode>" ...snip... temporaries remove: aTempVariableNode ifAbsent: [self halt: 'debug me'].
triggers the halt because temporaries isEmpty... in block node [ | k count digits | k := 1. ...]
temporaries isEmpty because block is optimized... So the block does not really exist, and the temps are stored in parent block... So when block is optimized, we should better not ask it to remove: aTempVariableNode, should we?
Nicolas
Nicolas
Eliot might be more efficient than me...
Nicolas
Cheers, Levente