Faculty of Information Technology
Software Engineering Group

Ticket #205: libcomp_fix_1_of_1_rev_9777fd81cae8_Issue__205__fixed_compilation_of_assignment_into_inlined_block_argument.patch

File libcomp_fix_1_of_1_rev_9777fd81cae8_Issue__205__fixed_compilation_of_assignment_into_inlined_block_argument.patch, 5.1 KB (added by Jan Vrany, 5 years ago)

  • VariableNode.st 

    # HG changeset patch
# User Jan Vrany <jan.vrany@fit.cvut.cz>
# Date 1524201755 -3600
#      Fri Apr 20 06:22:35 2018 +0100
# Branch jv
# Node ID 9777fd81cae8c076d1bb05a24844e4f20ae8e47f
# Parent  c02671bebd0a80223fd0a6dc776f2ed96a88a92c
Issue #205: fixed compilation of assignment into inlined block argument

Due to a bug in, compiler generated a wrong store when compiling an assignment
to a block argument in inlined block. Consider following method:

    foo
        1 to: 10 do:[:i | i := 100]

For this method it generated following (wrong) code:

    decompiling nA: 0 nV: 1 nT: 3

      1: 79                      push1
      2: 08 01                   LINE [1]
      4: 2F                      dup
      5: 10 0A                   pushNum 10
      7: 91 01                   send1 #> [1]
      9: 33 0A                   trueJump 10 (21)
     11: 2F                      dup
     12: 25 01                   storeMethodVar 1
     14: 10 64                   pushNum 100
     16: 65                      storeMethodVar2  "<-- wrong"
     17: 7B 01                   sendPlus1 [1]
     19: 36 ED                   jump -19 (2)
     21: 05                      retSelf
     22: 05                      retSelf

it should generate storeMethodVar1 insteead. When this code is executed,
it stores the value to a first slot in method's operation stack,
overwriting whatever was there.

This could have caused a segmentation violation as demonstrated in
issue #205. The reason is rather interesting:

The bytecode interpreter (in `interpret.c`) maintains a pointer
to the next bytecode as pointer into a `ByteArray` instance that
represents the bytecode. Since the GC may move the byte array at
"any" time, interpreter has to adjust BP ("bytecode pointer") each
time a GC could have happened. To do so, it stores the reference
to the bytearray in first slot of operation stack adjusting SP
accordingly. When interpreter interpreted the code above, the
bytearray got overwritten so BP got messed up after interpreter
adjusted BP after a possible GC.

Interestingly, the code from the issue works for small number of
iterations, because all the code inside inlined block is optimized
and does not involve any message send and thus there cannot be a GC
and thus no need to adjust the BP.

For large amount of interations, however, what happnes is that
some kind of an interrupt happens (IO / timer interrupt likely),
the interpreter has to give control to interrupt handler (which may
involve a GC) and when handled, it adjusts BP to a wrong value
and on subsequent intepretation step it crashes.

Note, that currently this trick with keeping bytearray in first
stack slot is not needed as we keep method in context and we can
reach method's bytecode from there.

Another lesson learned from this is that smalltalk implementation
should have some sort of bytecode verifiers, just like Java does.

https://swing.fit.cvut.cz/projects/stx-jv/ticket/205

diff -r c02671bebd0a -r 9777fd81cae8 VariableNode.st
    a b  
    628628    ].
    629629
    630630    ((type == #BlockVariable)
    631     "/ the following cannot be encountered with Smalltalk code;
    632     "/ however, the VM supports it for other languages (JavaScript, in particular)
     631    "/ Normally one cannot assign to a block argument in smalltalk.
     632    "/ However, this is supported in Smalltalk/X if ParserFlags allowAssignmentToBlockArgument:
     633    "/ is set to true or when compiling other languages (JavaScript in particular).
    633634    or:[(type == #BlockArg)]) ifTrue:[
    634635        bvIdx := index.
    635636       
    … …  
    655656                "/ Generate a storeMVAR
    656657
    657658                bvIdx := block indexOfFirstTemp + index - 1.
    658                 bvIdx := bvIdx + block numArgs.
     659                type == #BlockVariable ifTrue:[
     660                    bvIdx := bvIdx + block numArgs.
     661                ].
    659662                ^ self
    660663                    codeStoreOn:aStream
    661664                    type:#MethodVariable index:bvIdx
    … …  
    667670            "/ Generate a pushBVAR
    668671
    669672            bvIdx := block indexOfFirstTemp + index - 1.
    670             bvIdx := bvIdx + block numArgs.
     673            type == #BlockVariable ifTrue:[     
     674                bvIdx := bvIdx + block numArgs.
     675            ].
    671676        ] ifFalse:[
    672677            block isInlineBlock ifTrue:[
    673678                "/ a var of a block which is inlined in another block.
    674679                "/ Generate a pushBVAR / pushOuterBVAR
    675680                bvIdx := block indexOfFirstTemp + index - 1.
    676                 bvIdx := bvIdx + block numArgs.
     681                type == #BlockVariable ifTrue:[     
     682                    bvIdx := bvIdx + block numArgs.
     683                ].
    677684            ]
    678685        ].
    679686
    … …  
    806813
    807814    "Created: / 25-06-1997 / 16:14:40 / cg"
    808815    "Modified: / 25-10-2011 / 23:41:59 / cg"
     816    "Modified: / 18-04-2018 / 23:34:33 / Jan Vrany <jan.vrany@fit.cvut.cz>"
     817    "Modified (comment): / 20-04-2018 / 06:21:11 / Jan Vrany <jan.vrany@fit.cvut.cz>"
    809818! !
    810819
    811820!VariableNode methodsFor:'enumerating'!
    … …  
    11951204
    11961205version_CVS
    11971206    ^ '$Header$'
     1207!
     1208
     1209version_HG
     1210
     1211    ^ '$Changeset: <not expanded> $'
    11981212! !
    11991213