Decompiler issues

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Decompiler issues

Levente Uzonyi-2
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.


Cheers,
Levente

Reply | Threaded
Open this post in threaded view
|

Re: Decompiler issues

Nicolas Cellier
2010/3/1 Levente Uzonyi <[hidden email]>:

> 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...

Eliot might be more efficient than me...

Nicolas

>
> Cheers,
> Levente
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Decompiler issues

Nicolas Cellier
2010/3/1 Nicolas Cellier <[hidden email]>:

> 2010/3/1 Levente Uzonyi <[hidden email]>:
>> 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>}} )

Nicolas

> Eliot might be more efficient than me...
>
> Nicolas
>
>>
>> Cheers,
>> Levente
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Decompiler issues

Nicolas Cellier
2010/3/1 Nicolas Cellier <[hidden email]>:

> 2010/3/1 Nicolas Cellier <[hidden email]>:
>> 2010/3/1 Levente Uzonyi <[hidden email]>:
>>> 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
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Decompiler issues

Nicolas Cellier
2010/3/1 Nicolas Cellier <[hidden email]>:

> 2010/3/1 Nicolas Cellier <[hidden email]>:
>> 2010/3/1 Nicolas Cellier <[hidden email]>:
>>> 2010/3/1 Levente Uzonyi <[hidden email]>:
>>>> 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?
>

I suggest replacing:
        temporaries remove: aTempVariableNode ifAbsent: ['].
with:
        self actualScope temporaries remove: aTempVariableNode ifAbsent: ['].

Nicolas

> Nicolas
>
>> Nicolas
>>
>>> Eliot might be more efficient than me...
>>>
>>> Nicolas
>>>
>>>>
>>>> Cheers,
>>>> Levente
>>>>
>>>>
>>>
>>
>