Problem in newspeak cogit code generation

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

Problem in newspeak cogit code generation

Nicolas Cellier
 
Hi all,
I see what looks like a big problem in those files:

The generated code for genSendDirectedSuper:numArgs: is bogus.
the 3 instance variables
- directedSendUsesBinding
- directedSuperSendTrampolines
- directedSuperBindingSendTrampolines
have been translated into local (thus uninitialized) variables...

How can this possibly work?
Is it related to the CI failures?


Reply | Threaded
Open this post in threaded view
|

Re: Problem in newspeak cogit code generation

David T. Lewis
 
On Sat, Oct 26, 2019 at 11:45:08PM +0200, Nicolas Cellier wrote:

>  
> Hi all,
> I see what looks like a big problem in those files:
> https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspur64src/vm/cogitX64SysV.c
> https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspursrc/vm/cogitIA32.c
>
> The generated code for genSendDirectedSuper:numArgs: is bogus.
> the 3 instance variables
> - directedSendUsesBinding
> - directedSuperSendTrampolines
> - directedSuperBindingSendTrampolines
> have been translated into local (thus uninitialized) variables...
>
> How can this possibly work?
> Is it related to the CI failures?

Very likely the variables are being generated as local because they are
not referenced elsewhere. For example, directedSuperSendTrampolines is
initialized in simulation only by Cogit>>setInterpreter: but apparently
is not set anywhere in the actual translated code.

I don't think that uninitialized locals are set to null in C, so yes
this could be a source of random behavior in the CI tests.

A solution would be to initialize the three variables, possibly just
by referencing them explicitly in one of the declareCVarsIn: methods.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: Problem in newspeak cogit code generation

Nicolas Cellier
 
Hi David,
OK, the variables are initialized conditionally in Cogit>>setInterpreter:

     BytecodeSetHasDirectedSuperSend ifTrue:
         [directedSuperSendTrampolines := CArrayAccessor on: (Array new: NumSendTrampolines).
         directedSuperBindingSendTrampolines := CArrayAccessor on: (Array new: NumSendTrampolines).
         directedSendUsesBinding := false].

So maybe it's not the case of Newspeak bytecode set, and maybe these methods won't be used, but it's not a nice way to do it.
We shall not generate unused incorrect methods, it spoils compiler warning examination...

Le dim. 27 oct. 2019 à 19:58, David T. Lewis <[hidden email]> a écrit :
 
On Sat, Oct 26, 2019 at 11:45:08PM +0200, Nicolas Cellier wrote:

> Hi all,
> I see what looks like a big problem in those files:
> https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspur64src/vm/cogitX64SysV.c
> https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspursrc/vm/cogitIA32.c
>
> The generated code for genSendDirectedSuper:numArgs: is bogus.
> the 3 instance variables
> - directedSendUsesBinding
> - directedSuperSendTrampolines
> - directedSuperBindingSendTrampolines
> have been translated into local (thus uninitialized) variables...
>
> How can this possibly work?
> Is it related to the CI failures?

Very likely the variables are being generated as local because they are
not referenced elsewhere. For example, directedSuperSendTrampolines is
initialized in simulation only by Cogit>>setInterpreter: but apparently
is not set anywhere in the actual translated code.

I don't think that uninitialized locals are set to null in C, so yes
this could be a source of random behavior in the CI tests.

A solution would be to initialize the three variables, possibly just
by referencing them explicitly in one of the declareCVarsIn: methods.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: Problem in newspeak cogit code generation

David T. Lewis
 
On Mon, Oct 28, 2019 at 10:52:38PM +0100, Nicolas Cellier wrote:

>  
> Hi David,
> OK, the variables are initialized conditionally in Cogit>>setInterpreter:
>
>      BytecodeSetHasDirectedSuperSend ifTrue:
>          [directedSuperSendTrampolines := CArrayAccessor on: (Array new:
> NumSendTrampolines).
>          directedSuperBindingSendTrampolines := CArrayAccessor on: (Array
> new: NumSendTrampolines).
>          directedSendUsesBinding := false].
>

There is a <doNotGenerate> directive for the method, so the initialization
that you see there is never part of the generated C code.


> So maybe it's not the case of Newspeak bytecode set, and maybe these
> methods won't be used, but it's not a nice way to do it.
> We shall not generate unused incorrect methods, it spoils compiler warning
> examination...
>

Actually, I think that it /is/ used (or at least it could be used if
#isDirected is true, which probably amounts to the same thing). All of
the unreferenced methods will be optimized away by the inliner, but there
is one remaining usage, and it does look like a possible source of trouble.

Dave

 

> Le dim. 27 oct. 2019 ?? 19:58, David T. Lewis <[hidden email]> a ??crit :
>
> >
> > On Sat, Oct 26, 2019 at 11:45:08PM +0200, Nicolas Cellier wrote:
> > >
> > > Hi all,
> > > I see what looks like a big problem in those files:
> > >
> > https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspur64src/vm/cogitX64SysV.c
> > >
> > https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspursrc/vm/cogitIA32.c
> > >
> > > The generated code for genSendDirectedSuper:numArgs: is bogus.
> > > the 3 instance variables
> > > - directedSendUsesBinding
> > > - directedSuperSendTrampolines
> > > - directedSuperBindingSendTrampolines
> > > have been translated into local (thus uninitialized) variables...
> > >
> > > How can this possibly work?
> > > Is it related to the CI failures?
> >
> > Very likely the variables are being generated as local because they are
> > not referenced elsewhere. For example, directedSuperSendTrampolines is
> > initialized in simulation only by Cogit>>setInterpreter: but apparently
> > is not set anywhere in the actual translated code.
> >
> > I don't think that uninitialized locals are set to null in C, so yes
> > this could be a source of random behavior in the CI tests.
> >
> > A solution would be to initialize the three variables, possibly just
> > by referencing them explicitly in one of the declareCVarsIn: methods.
> >
> > Dave
> >
> >

Reply | Threaded
Open this post in threaded view
|

Re: Problem in newspeak cogit code generation

Nicolas Cellier
 
Follow-up:

I tried to generate empty function rather than broken function:


This failed because the non local return did result in such generated code:

        return result;
        return 0;
    }

So I tried to remove the function entirely:

This failed, because the function is used (that's why it was not eliminated):

Undefined symbols for architecture x86_64:
"_genSendDirectedSupernumArgs", referenced from:
_genExtSendSuperBytecode in cogit.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build/vm/NewspeakVirtualMachine] Error 1
The command "$CHROOT ./scripts/ci/travis_build.sh" failed and exited with 1 during .
Of course, I should have seen it in Smalltalk:
genExtSendSuperBytecode
     "239 11101111 i i i i i j j j Send To Superclass Literal Selector #iiiii (+ Extend A * 32) with jjj (+ Extend B * 8) Arguments"
     | isDirected litIndex nArgs |
     (isDirected := extB >= 64) ifTrue:
    ...snip...
     ^isDirected
        ifTrue: [self genSendDirectedSuper: litIndex numArgs: nArgs]
         ifFalse: [...

And this is used in NewspeakV4 bytecode set too:
initializeBytecodeTableForNewspeakV4
        ...snip...
         (2 239 239 genExtSendSuperBytecode isMapped)

Funnily (sadly?), when I removed the generation of genDirectedSuper:numArgs: send, the generator started to generate the (now unused) global scope variables!


My conclusion is:
- NewspeakV4 includes a bytecode that potentially generate directed super send
- but BytecodeSetHasDirectedSuperSend is set to false in Newspeak flavor
- this mismatch lead to incorrect code generation.

Shouldn't we set BytecodeSetHasDirectedSuperSend := true for NewspeakV4?
I'm going to try...

Le mar. 29 oct. 2019 à 00:57, David T. Lewis <[hidden email]> a écrit :
 
On Mon, Oct 28, 2019 at 10:52:38PM +0100, Nicolas Cellier wrote:

> Hi David,
> OK, the variables are initialized conditionally in Cogit>>setInterpreter:
>
>      BytecodeSetHasDirectedSuperSend ifTrue:
>          [directedSuperSendTrampolines := CArrayAccessor on: (Array new:
> NumSendTrampolines).
>          directedSuperBindingSendTrampolines := CArrayAccessor on: (Array
> new: NumSendTrampolines).
>          directedSendUsesBinding := false].
>

There is a <doNotGenerate> directive for the method, so the initialization
that you see there is never part of the generated C code.


> So maybe it's not the case of Newspeak bytecode set, and maybe these
> methods won't be used, but it's not a nice way to do it.
> We shall not generate unused incorrect methods, it spoils compiler warning
> examination...
>

Actually, I think that it /is/ used (or at least it could be used if
#isDirected is true, which probably amounts to the same thing). All of
the unreferenced methods will be optimized away by the inliner, but there
is one remaining usage, and it does look like a possible source of trouble.

Dave


> Le dim. 27 oct. 2019 ?? 19:58, David T. Lewis <[hidden email]> a ??crit :
>
> >
> > On Sat, Oct 26, 2019 at 11:45:08PM +0200, Nicolas Cellier wrote:
> > >
> > > Hi all,
> > > I see what looks like a big problem in those files:
> > >
> > https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspur64src/vm/cogitX64SysV.c
> > >
> > https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspursrc/vm/cogitIA32.c
> > >
> > > The generated code for genSendDirectedSuper:numArgs: is bogus.
> > > the 3 instance variables
> > > - directedSendUsesBinding
> > > - directedSuperSendTrampolines
> > > - directedSuperBindingSendTrampolines
> > > have been translated into local (thus uninitialized) variables...
> > >
> > > How can this possibly work?
> > > Is it related to the CI failures?
> >
> > Very likely the variables are being generated as local because they are
> > not referenced elsewhere. For example, directedSuperSendTrampolines is
> > initialized in simulation only by Cogit>>setInterpreter: but apparently
> > is not set anywhere in the actual translated code.
> >
> > I don't think that uninitialized locals are set to null in C, so yes
> > this could be a source of random behavior in the CI tests.
> >
> > A solution would be to initialize the three variables, possibly just
> > by referencing them explicitly in one of the declareCVarsIn: methods.
> >
> > Dave
> >
> >

Reply | Threaded
Open this post in threaded view
|

Re: Problem in newspeak cogit code generation

Nicolas Cellier
 
So I tried this:


regenerated the ns source code:


then got traditional random newspeak bootstrap/test failures, so this was not the CI problem...



+/home/travis/build/OpenSmalltalk/opensmalltalk-vm/products/nscogspurlinux/bin/nsvm -headless ns-2019-11-09.32.image NewspeakBootstrap.st
Warning, attempt to use method with selector (null) and selector parserToForwardTo
Warning, attempt to use method with selector (null) and selector atIndex:inTable:occupiedSlotDo:emptySlotDo:deletedSlotDo:
Warning, attempt to use method with selector (null) and selector atIndex:inTable:occupiedSlotDo:emptySlotDo:deletedSlotDo:

...
Loading changeset Tools-eem.300.3.cs. Done in 14 ms.
Deserializing bootstrap runtime. Done in 238 ms.
Instantiating bootstrap runtime. Done in 1 ms.
Compiling module APIManager.ns. Done in 100 ms.
Compiling module AccessModifierTestApp.ns9 November 2019 10:12:18.322723 am
VM: unix - Smalltalk
Image: Squeak5.1 [latest update: #16548]
SecurityManager state:
Restricted: false
FileAccess: true
SocketAccess: true
Working Dir /tmp/tmp.yFdIe0kqdQ/nsboot
Trusted Dir /tmp/tmp.yFdIe0kqdQ/nsboot/secure
Untrusted Dir /tmp/tmp.yFdIe0kqdQ/nsboot/My Squeak
UnresolvedSendAST`731589(Object)>>doesNotUnderstand: #'init`start:'
Receiver: <<error during printing>>
Arguments and temporary variables:
<<error during printing>
Receiver's instance variables:
start: nil
end: nil
message: nil
receiver: nil
UnresolvedSendAST`731589(AST)>>initializer`new
Receiver: <<error during printing>>
Arguments and temporary variables:
<<error during printing>
Receiver's instance variables:
start: nil
end: nil
message: nil
receiver: nil
UnresolvedSendAST`731589(UnresolvedSendAST)>>initializer`new
Receiver: <<error during printing>>
Arguments and temporary variables:
<<error during printing>
Receiver's instance variables:
start: nil
end: nil
message: nil
receiver: nil
UnresolvedSendAST`731589 class(UnresolvedSendAST class)>>new
Receiver: UnresolvedSendAST`731589
Arguments and temporary variables:
<<error during printing>
Receiver's instance variables:
superclass: SendAST`731589
methodDict: a MethodDictionary(#apply:->(UnresolvedSendAST mixin>>#apply: "a CompiledMethod...etc...
format: 65540
mixinSlot: UnresolvedSendAST mixin
enclosingObjectSlot: NewspeakASTs#731589
subclassesSlot: nil
nameSlot: #'UnresolvedSendAST`731589'
Parser`2523583(Parser)>>hereSendFromMsg:
Receiver: <<error during printing>>
Arguments and temporary variables:
<<error during printing>
Receiver's instance variables:
name: name
forwardReferenceTable: MutableHashedMap(#letter -> ForwardReferenceParser`3011573...etc...
selfMirror: <<error during printing>>
[] in Parser`2523583(Parser)>>slotName
Receiver: <<error during printing>>
Arguments and temporary variables:
<<error during printing>
Receiver's instance variables:
name: name
forwardReferenceTable: MutableHashedMap(#letter -> ForwardReferenceParser`3011573...etc...
selfMirror: <<error during printing>>
...


Le sam. 9 nov. 2019 à 09:57, Nicolas Cellier <[hidden email]> a écrit :
Follow-up:

I tried to generate empty function rather than broken function:


This failed because the non local return did result in such generated code:

        return result;
        return 0;
    }

So I tried to remove the function entirely:

This failed, because the function is used (that's why it was not eliminated):

Undefined symbols for architecture x86_64:
"_genSendDirectedSupernumArgs", referenced from:
_genExtSendSuperBytecode in cogit.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build/vm/NewspeakVirtualMachine] Error 1
The command "$CHROOT ./scripts/ci/travis_build.sh" failed and exited with 1 during .
Of course, I should have seen it in Smalltalk:
genExtSendSuperBytecode
     "239 11101111 i i i i i j j j Send To Superclass Literal Selector #iiiii (+ Extend A * 32) with jjj (+ Extend B * 8) Arguments"
     | isDirected litIndex nArgs |
     (isDirected := extB >= 64) ifTrue:
    ...snip...
     ^isDirected
        ifTrue: [self genSendDirectedSuper: litIndex numArgs: nArgs]
         ifFalse: [...

And this is used in NewspeakV4 bytecode set too:
initializeBytecodeTableForNewspeakV4
        ...snip...
         (2 239 239 genExtSendSuperBytecode isMapped)

Funnily (sadly?), when I removed the generation of genDirectedSuper:numArgs: send, the generator started to generate the (now unused) global scope variables!


My conclusion is:
- NewspeakV4 includes a bytecode that potentially generate directed super send
- but BytecodeSetHasDirectedSuperSend is set to false in Newspeak flavor
- this mismatch lead to incorrect code generation.

Shouldn't we set BytecodeSetHasDirectedSuperSend := true for NewspeakV4?
I'm going to try...

Le mar. 29 oct. 2019 à 00:57, David T. Lewis <[hidden email]> a écrit :
 
On Mon, Oct 28, 2019 at 10:52:38PM +0100, Nicolas Cellier wrote:

> Hi David,
> OK, the variables are initialized conditionally in Cogit>>setInterpreter:
>
>      BytecodeSetHasDirectedSuperSend ifTrue:
>          [directedSuperSendTrampolines := CArrayAccessor on: (Array new:
> NumSendTrampolines).
>          directedSuperBindingSendTrampolines := CArrayAccessor on: (Array
> new: NumSendTrampolines).
>          directedSendUsesBinding := false].
>

There is a <doNotGenerate> directive for the method, so the initialization
that you see there is never part of the generated C code.


> So maybe it's not the case of Newspeak bytecode set, and maybe these
> methods won't be used, but it's not a nice way to do it.
> We shall not generate unused incorrect methods, it spoils compiler warning
> examination...
>

Actually, I think that it /is/ used (or at least it could be used if
#isDirected is true, which probably amounts to the same thing). All of
the unreferenced methods will be optimized away by the inliner, but there
is one remaining usage, and it does look like a possible source of trouble.

Dave


> Le dim. 27 oct. 2019 ?? 19:58, David T. Lewis <[hidden email]> a ??crit :
>
> >
> > On Sat, Oct 26, 2019 at 11:45:08PM +0200, Nicolas Cellier wrote:
> > >
> > > Hi all,
> > > I see what looks like a big problem in those files:
> > >
> > https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspur64src/vm/cogitX64SysV.c
> > >
> > https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/nsspursrc/vm/cogitIA32.c
> > >
> > > The generated code for genSendDirectedSuper:numArgs: is bogus.
> > > the 3 instance variables
> > > - directedSendUsesBinding
> > > - directedSuperSendTrampolines
> > > - directedSuperBindingSendTrampolines
> > > have been translated into local (thus uninitialized) variables...
> > >
> > > How can this possibly work?
> > > Is it related to the CI failures?
> >
> > Very likely the variables are being generated as local because they are
> > not referenced elsewhere. For example, directedSuperSendTrampolines is
> > initialized in simulation only by Cogit>>setInterpreter: but apparently
> > is not set anywhere in the actual translated code.
> >
> > I don't think that uninitialized locals are set to null in C, so yes
> > this could be a source of random behavior in the CI tests.
> >
> > A solution would be to initialize the three variables, possibly just
> > by referencing them explicitly in one of the declareCVarsIn: methods.
> >
> > Dave
> >
> >