Smalltalk › Squeak › Squeak VM

Fix for label duplications in Slang's case expansion

‹ Previous Topic Next Topic ›
 
classic Classic list List threaded Threaded
♦
♦
 8 messages Options
Guillermo Polito
Reply | Threaded
Open this post in threaded view
♦
♦
|

Fix for label duplications in Slang's case expansion

Guillermo Polito
 
Hi all,

with Federico, a student from Argentina, we have found a problem in the label generation inside expanded cases in Slang.
The problem comes from the fact that expanded cases (using the pragma <expandCase>, typically used for bytecodes), duplicate the entire tree with its inlines too, also duplicating labels inside it, which generates compilation errors due to label redefinition.

I’ve written a test for the case:

        | translation method codeGenerator |
        method := self class >> #methodWithCases.
        translation := method asTranslationMethodOfClass: TMethod.
        codeGenerator := CCodeGeneratorGlobalStructure new.
        codeGenerator addMethod: ((self class >> #expanded) asTranslationMethodOfClass: TMethod).
        codeGenerator addMethod: translation.

        codeGenerator prepareMethods.
        codeGenerator inlineDispatchesInMethodNamed: #methodWithCases localizingVars: #().
        self assert: translation statements first cases first statements last isLabel.
        self assert: translation statements first cases third statements last isLabel.

        self
                deny: translation statements first cases first statements last label
                equals: translation statements first cases third statements last label.



And I’ve made a fix that renames labels on each expanded case, to ensure non shared labels are unique within each case.
I’ve put it in vmmaker's inbox. I hope there are no problems regarding timestamps, I’ve been careful about that ^^.

Commit + commit message below:

Name: VMMaker.oscog-GuillermoPolito.2676
Author: GuillermoPolito
Time: 24 January 2020, 4:27:09.847804 pm
UUID: 49bcc820-4659-0d00-b258-043c01043dd2
Ancestors: VMMaker.oscog-eem.2675

Fix in slang case statement expansion labels.

During expansion in case statements, trees are duplicated and expanded.
However, labels inside those trees are duplicated using the same name, producing compilation problems due to label conflicts/redefinition.

This fix uses #renameLabelsForInliningInto: when expanding case statements to rewrite labels generating unique labels.

Cheers,
Guille
Guillermo Polito
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Guillermo Polito
 
Of course, for the test to run, it needs these other two methods :)

methodWithCases

        self dispatchOn: currentBytecode in: #( expanded expanded expanded )


expanded
        <expandCases>
        ^ (currentBytecode bitAnd: 15) << 2

> El 24 ene 2020, a las 16:57, Guillermo Polito <[hidden email]> escribió:
>
> Hi all,
>
> with Federico, a student from Argentina, we have found a problem in the label generation inside expanded cases in Slang.
> The problem comes from the fact that expanded cases (using the pragma <expandCase>, typically used for bytecodes), duplicate the entire tree with its inlines too, also duplicating labels inside it, which generates compilation errors due to label redefinition.
>
> I’ve written a test for the case:
>
> | translation method codeGenerator |
> method := self class >> #methodWithCases.
> translation := method asTranslationMethodOfClass: TMethod.
> codeGenerator := CCodeGeneratorGlobalStructure new.
> codeGenerator addMethod: ((self class >> #expanded) asTranslationMethodOfClass: TMethod).
> codeGenerator addMethod: translation.
>
> codeGenerator prepareMethods.
> codeGenerator inlineDispatchesInMethodNamed: #methodWithCases localizingVars: #().
> self assert: translation statements first cases first statements last isLabel.
> self assert: translation statements first cases third statements last isLabel.
>
> self
> deny: translation statements first cases first statements last label
> equals: translation statements first cases third statements last label.
>
>
>
> And I’ve made a fix that renames labels on each expanded case, to ensure non shared labels are unique within each case.
> I’ve put it in vmmaker's inbox. I hope there are no problems regarding timestamps, I’ve been careful about that ^^.
>
> Commit + commit message below:
>
> Name: VMMaker.oscog-GuillermoPolito.2676
> Author: GuillermoPolito
> Time: 24 January 2020, 4:27:09.847804 pm
> UUID: 49bcc820-4659-0d00-b258-043c01043dd2
> Ancestors: VMMaker.oscog-eem.2675
>
> Fix in slang case statement expansion labels.
>
> During expansion in case statements, trees are duplicated and expanded.
> However, labels inside those trees are duplicated using the same name, producing compilation problems due to label conflicts/redefinition.
>
> This fix uses #renameLabelsForInliningInto: when expanding case statements to rewrite labels generating unique labels.
>
> Cheers,
> Guille

Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Eliot Miranda-2
In reply to this post by Guillermo Polito
 
Thanks Guille!

_,,,^..^,,,_

> On Jan 24, 2020, at 7:57 AM, Guillermo Polito <[hidden email]> wrote:
>
> 
> Hi all,
>
> with Federico, a student from Argentina, we have found a problem in the label generation inside expanded cases in Slang.
> The problem comes from the fact that expanded cases (using the pragma <expandCase>, typically used for bytecodes), duplicate the entire tree with its inlines too, also duplicating labels inside it, which generates compilation errors due to label redefinition.
>
> I’ve written a test for the case:
>
>    | translation method codeGenerator |
>    method := self class >> #methodWithCases.
>    translation := method asTranslationMethodOfClass: TMethod.
>    codeGenerator := CCodeGeneratorGlobalStructure new.
>    codeGenerator addMethod: ((self class >> #expanded) asTranslationMethodOfClass: TMethod).
>    codeGenerator addMethod: translation.
>
>    codeGenerator prepareMethods.
>    codeGenerator inlineDispatchesInMethodNamed: #methodWithCases localizingVars: #().
>    self assert: translation statements first cases first statements last isLabel.
>    self assert: translation statements first cases third statements last isLabel.
>
>    self
>        deny: translation statements first cases first statements last label
>        equals: translation statements first cases third statements last label.
>
>
>
> And I’ve made a fix that renames labels on each expanded case, to ensure non shared labels are unique within each case.
> I’ve put it in vmmaker's inbox. I hope there are no problems regarding timestamps, I’ve been careful about that ^^.
>
> Commit + commit message below:
>
> Name: VMMaker.oscog-GuillermoPolito.2676
> Author: GuillermoPolito
> Time: 24 January 2020, 4:27:09.847804 pm
> UUID: 49bcc820-4659-0d00-b258-043c01043dd2
> Ancestors: VMMaker.oscog-eem.2675
>
> Fix in slang case statement expansion labels.
>
> During expansion in case statements, trees are duplicated and expanded.
> However, labels inside those trees are duplicated using the same name, producing compilation problems due to label conflicts/redefinition.
>
> This fix uses #renameLabelsForInliningInto: when expanding case statements to rewrite labels generating unique labels.
>
> Cheers,
> Guille
Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Eliot Miranda-2
In reply to this post by Guillermo Polito
 
Hi Guille,

On Fri, Jan 24, 2020 at 7:57 AM Guillermo Polito <[hidden email]> wrote:
 
Hi all,

with Federico, a student from Argentina, we have found a problem in the label generation inside expanded cases in Slang.
The problem comes from the fact that expanded cases (using the pragma <expandCase>, typically used for bytecodes), duplicate the entire tree with its inlines too, also duplicating labels inside it, which generates compilation errors due to label redefinition.

I’ve written a test for the case:

        | translation method codeGenerator |
        method := self class >> #methodWithCases.
        translation := method asTranslationMethodOfClass: TMethod.
        codeGenerator := CCodeGeneratorGlobalStructure new.
        codeGenerator addMethod: ((self class >> #expanded) asTranslationMethodOfClass: TMethod).
        codeGenerator addMethod: translation.

        codeGenerator prepareMethods.
        codeGenerator inlineDispatchesInMethodNamed: #methodWithCases localizingVars: #().
        self assert: translation statements first cases first statements last isLabel.
        self assert: translation statements first cases third statements last isLabel.

        self
                deny: translation statements first cases first statements last label
                equals: translation statements first cases third statements last label.



And I’ve made a fix that renames labels on each expanded case, to ensure non shared labels are unique within each case.
I’ve put it in vmmaker's inbox. I hope there are no problems regarding timestamps, I’ve been careful about that ^^.

Well, all the timestamps are gone.  But it took me a couple of minutes to extend the Monticello browser to filter-out all the methods that only differ in timestamps.  And that is the point.  We are in control of our destiny using Monticello, not nearly as much as with git.  

Anyway, thank you.  i can now see the two changed methods, can easily integrate, and give them a meaningful timestamp.  Can you email me the test method?
 

Commit + commit message below:

Name: VMMaker.oscog-GuillermoPolito.2676
Author: GuillermoPolito
Time: 24 January 2020, 4:27:09.847804 pm
UUID: 49bcc820-4659-0d00-b258-043c01043dd2
Ancestors: VMMaker.oscog-eem.2675

Fix in slang case statement expansion labels.

During expansion in case statements, trees are duplicated and expanded.
However, labels inside those trees are duplicated using the same name, producing compilation problems due to label conflicts/redefinition.

This fix uses #renameLabelsForInliningInto: when expanding case statements to rewrite labels generating unique labels.

Cheers,
Guille


--
_,,,^..^,,,_
best, Eliot
Guillermo Polito
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Guillermo Polito
 


El 24 ene 2020, a las 21:22, Eliot Miranda <[hidden email]> escribió:

Hi Guille,

On Fri, Jan 24, 2020 at 7:57 AM Guillermo Polito <[hidden email]> wrote:
 
Hi all,

with Federico, a student from Argentina, we have found a problem in the label generation inside expanded cases in Slang.
The problem comes from the fact that expanded cases (using the pragma <expandCase>, typically used for bytecodes), duplicate the entire tree with its inlines too, also duplicating labels inside it, which generates compilation errors due to label redefinition.

I’ve written a test for the case:

        | translation method codeGenerator |
        method := self class >> #methodWithCases.
        translation := method asTranslationMethodOfClass: TMethod.
        codeGenerator := CCodeGeneratorGlobalStructure new.
        codeGenerator addMethod: ((self class >> #expanded) asTranslationMethodOfClass: TMethod).
        codeGenerator addMethod: translation.

        codeGenerator prepareMethods.
        codeGenerator inlineDispatchesInMethodNamed: #methodWithCases localizingVars: #().
        self assert: translation statements first cases first statements last isLabel.
        self assert: translation statements first cases third statements last isLabel.

        self
                deny: translation statements first cases first statements last label
                equals: translation statements first cases third statements last label.



And I’ve made a fix that renames labels on each expanded case, to ensure non shared labels are unique within each case.
I’ve put it in vmmaker's inbox. I hope there are no problems regarding timestamps, I’ve been careful about that ^^.

Well, all the timestamps are gone.  But it took me a couple of minutes to extend the Monticello browser to filter-out all the methods that only differ in timestamps.  And that is the point.  We are in control of our destiny using Monticello, not nearly as much as with git.  

Inspecting the mcz, I see the timestamps were gone, sorry.
Still, I think the problem is deeper than git/not git. Because when doing the patch I carefully loaded latest VMMaker.oscog version.
I supposed that forcing loading in monticello would reload everything. I (wrongly) guessed it should have installed timestamps too.
Then I applied my change from there.

Even just now, I got a latest fresh pharo9,
 - downloaded latest VMMaker from source.squeak.org
 - made a diff and a merge with the version I generated in the inbox
    => both empty => I cannot see problems related to missing timestamps in the UI :/
And no git was involved *at all* here.

I was maybe supposed to see changes in timestamps here? Does MC diff show that at all?
Now that I think about it, I don’t have any memories of it. But that could have helped me spot the problem before.
Maybe I could patch MC diff in Pharo to take timestamps into account?



Anyway, thank you.  i can now see the two changed methods, can easily integrate, and give them a meaningful timestamp.  Can you email me the test method?

I’ll attach a changeset in a couple of minutes ^^

 

Commit + commit message below:

Name: VMMaker.oscog-GuillermoPolito.2676
Author: GuillermoPolito
Time: 24 January 2020, 4:27:09.847804 pm
UUID: 49bcc820-4659-0d00-b258-043c01043dd2
Ancestors: VMMaker.oscog-eem.2675

Fix in slang case statement expansion labels.

During expansion in case statements, trees are duplicated and expanded.
However, labels inside those trees are duplicated using the same name, producing compilation problems due to label conflicts/redefinition.

This fix uses #renameLabelsForInliningInto: when expanding case statements to rewrite labels generating unique labels.

Cheers,
Guille


-- 
_,,,^..^,,,_
best, Eliot
Guillermo Polito
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Guillermo Polito
In reply to this post by Eliot Miranda-2
 
Please, find attached a changeset with the test + method.
Still, my gut tells me this test is not representative enough of the error we had, but it’s better than nothing.




El 24 ene 2020, a las 21:22, Eliot Miranda <[hidden email]> escribió:

Hi Guille,

On Fri, Jan 24, 2020 at 7:57 AM Guillermo Polito <[hidden email]> wrote:
 
Hi all,

with Federico, a student from Argentina, we have found a problem in the label generation inside expanded cases in Slang.
The problem comes from the fact that expanded cases (using the pragma <expandCase>, typically used for bytecodes), duplicate the entire tree with its inlines too, also duplicating labels inside it, which generates compilation errors due to label redefinition.

I’ve written a test for the case:

        | translation method codeGenerator |
        method := self class >> #methodWithCases.
        translation := method asTranslationMethodOfClass: TMethod.
        codeGenerator := CCodeGeneratorGlobalStructure new.
        codeGenerator addMethod: ((self class >> #expanded) asTranslationMethodOfClass: TMethod).
        codeGenerator addMethod: translation.

        codeGenerator prepareMethods.
        codeGenerator inlineDispatchesInMethodNamed: #methodWithCases localizingVars: #().
        self assert: translation statements first cases first statements last isLabel.
        self assert: translation statements first cases third statements last isLabel.

        self
                deny: translation statements first cases first statements last label
                equals: translation statements first cases third statements last label.



And I’ve made a fix that renames labels on each expanded case, to ensure non shared labels are unique within each case.
I’ve put it in vmmaker's inbox. I hope there are no problems regarding timestamps, I’ve been careful about that ^^.

Well, all the timestamps are gone.  But it took me a couple of minutes to extend the Monticello browser to filter-out all the methods that only differ in timestamps.  And that is the point.  We are in control of our destiny using Monticello, not nearly as much as with git.  

Anyway, thank you.  i can now see the two changed methods, can easily integrate, and give them a meaningful timestamp.  Can you email me the test method?
 

Commit + commit message below:

Name: VMMaker.oscog-GuillermoPolito.2676
Author: GuillermoPolito
Time: 24 January 2020, 4:27:09.847804 pm
UUID: 49bcc820-4659-0d00-b258-043c01043dd2
Ancestors: VMMaker.oscog-eem.2675

Fix in slang case statement expansion labels.

During expansion in case statements, trees are duplicated and expanded.
However, labels inside those trees are duplicated using the same name, producing compilation problems due to label conflicts/redefinition.

This fix uses #renameLabelsForInliningInto: when expanding case statements to rewrite labels generating unique labels.

Cheers,
Guille


-- 
_,,,^..^,,,_
best, Eliot


VMCodeGenerationTest.GuillermoPolito.cs (2K) Download Attachment
Jakob Reschke
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Jakob Reschke
In reply to this post by Guillermo Polito
 
Sounds as if Pharo had dropped the timestamps from the Monticello model completely. Or all reading and writing of them.


Guillermo Polito <[hidden email]> schrieb am Mo., 27. Jan. 2020, 14:00:

Inspecting the mcz, I see the timestamps were gone, sorry.
Still, I think the problem is deeper than git/not git. Because when doing the patch I carefully loaded latest VMMaker.oscog version.
I supposed that forcing loading in monticello would reload everything. I (wrongly) guessed it should have installed timestamps too.
Then I applied my change from there.

Even just now, I got a latest fresh pharo9,
 - downloaded latest VMMaker from source.squeak.org
 - made a diff and a merge with the version I generated in the inbox
    => both empty => I cannot see problems related to missing timestamps in the UI :/
And no git was involved *at all* here.

I was maybe supposed to see changes in timestamps here? Does MC diff show that at all?
Now that I think about it, I don’t have any memories of it. But that could have helped me spot the problem before.
Maybe I could patch MC diff in Pharo to take timestamps into account?
Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Fix for label duplications in Slang's case expansion

Eliot Miranda-2
 


On Jan 31, 2020, at 12:08 AM, Jakob Reschke <[hidden email]> wrote:


Sounds as if Pharo had dropped the timestamps from the Monticello model completely. Or all reading and writing of them.

That’s indeed what they have done.  It’s easily worked around, as my modification to Monticello shows.



Guillermo Polito <[hidden email]> schrieb am Mo., 27. Jan. 2020, 14:00:

Inspecting the mcz, I see the timestamps were gone, sorry.
Still, I think the problem is deeper than git/not git. Because when doing the patch I carefully loaded latest VMMaker.oscog version.
I supposed that forcing loading in monticello would reload everything. I (wrongly) guessed it should have installed timestamps too.
Then I applied my change from there.

Even just now, I got a latest fresh pharo9,
 - downloaded latest VMMaker from source.squeak.org
 - made a diff and a merge with the version I generated in the inbox
    => both empty => I cannot see problems related to missing timestamps in the UI :/
And no git was involved *at all* here.

I was maybe supposed to see changes in timestamps here? Does MC diff show that at all?
Now that I think about it, I don’t have any memories of it. But that could have helped me spot the problem before.
Maybe I could patch MC diff in Pharo to take timestamps into account?
Free forum by Nabble Edit this page