Introduce temporary stack variables for getfield/-static and ?load

[mgudemann]

When a value is pushed on the stack, it must be ensured that modification of the
origin will not modify the values on the stack.

[mgudemann]

this fixes the obvious cases, regression tests and thinking about possibly other cases is needed

[mgudemann]

strong candidates for additional cases:

• ?aload - returns a reference to an element in an array
• dup? - duplicates entries on the stack, only values should be duplicated

[peterschrammel]

Do you have tests that expose the bug?

[mgudemann]

one is the code in #926 where the assert currently still succeeds (which is shouldn't) but the accessed index is the correct one now

[mgudemann]

instead of

        n = n + (int)1;
        // 63 file incr.java line 7 function java::incr.f:(I)V bytecode_index 8
        ASSERT n >= 0 // Array index < 0
        // 64 file incr.java line 7 function java::incr.f:(I)V
        ASSERT !(this == null) // reference is null in *((struct incr *)(void *)this)
        // 65 file incr.java line 7 function java::incr.f:(I)V
        ASSERT !(this->nums == null) // reference is null in *((struct java::array[int] *)((struct incr *)(void *)this)->nums)
        // 66 file incr.java line 7 function java::incr.f:(I)V bytecode_index 8
        ASSERT n < ((struct java::array[int] *)((struct incr *)(void *)this)->nums)->length // Array index >= length
        // 67 file incr.java line 7 function java::incr.f:(I)V
        ASSERT !(this->nums->data + n == null) // reference is null in this->nums->data[n]
        // 68 file incr.java line 7 function java::incr.f:(I)V bytecode_index 10
        IF this->nums->data[n] == 2 THEN GOTO 1

it's now

        stack_field_tmp2 = ((struct incr *)stack_var_tmp1)->nums;
        // 78 file incr.java line 7 function java::incr.f:(I)V bytecode_index 6
        stack_var_tmp3 = n;
        // 79 file incr.java line 7 function java::incr.f:(I)V bytecode_index 7
        n = n + (int)1;
        // 80 file incr.java line 7 function java::incr.f:(I)V bytecode_index 8
        ASSERT stack_var_tmp3 >= 0 // Array index < 0
        // 81 file incr.java line 7 function java::incr.f:(I)V
        ASSERT !(stack_field_tmp2 == null) // reference is null in *((struct java::array[int] *)stack_field_tmp2)
        // 82 file incr.java line 7 function java::incr.f:(I)V bytecode_index 8
        ASSERT stack_var_tmp3 < ((struct java::array[int] *)stack_field_tmp2)->length // Array index >= length
        // 83 file incr.java line 7 function java::incr.f:(I)V
        ASSERT !(stack_field_tmp2->data + stack_var_tmp3 == null) // reference is null in stack_field_tmp2->data[stack_var_tmp3]
        // 84 file incr.java line 7 function java::incr.f:(I)V bytecode_index 10
        IF !(stack_field_tmp2->data[stack_var_tmp3] == 2) THEN GOTO 2

[smowton]

A possibility for the future to obtain correctness without dramatic uglification of the code: create the temp variables only when we come across an instruction that writes while there are reads on the stack (e.g. before an astore, for each operand it would not remove from the stack and which is non-constant, now synthesise temporary names to store them; in the common case that there are no such non-constants held over the write op (i.e. the write op drains the stack) we could still produce brief code as we do now.

However this is trickier, so this fix is probably the right way to go for now.

[tautschnig]

Can this please not be merged without regression tests being added? (Also all three checks are failing at the moment.)

[mgudemann]

@tautschnig the regression tests are fixed, I will add specific ones for this, too
currently I am adapting the variants of dup

[mgudemann]

@tautschnig I added regression tests for the bytecode instructions in question

@smowton you are right about polluting the GOTOs, effectively this partially recreates the stack state now, which is sub-optimal.
If I understand correctly, you propose changing the "dual" instructions (?store instead of ?load, ?putfield instead of ?getfield etc.) in a way that saves the variables / references that remain on the stack after the write into temporary variables. Now that regression tests exist to validate the implementation, I will have a look at that.

[tautschnig]

Why is this file (and the other .j files) included?

[mgudemann]

these are the source files I used to create the classes. This is very bytecode specific, in particular the dup2_x? are very rare, so I used the jasmin assembler to create the regression tests.

[tautschnig]

Ah, cool. Would you mind adding information in either the commit message or some extra readme file so that these steps become reproducible? I had naively thought that the .class files had been produced from the accompanying .java.

[mgudemann]

ok, will do, the stack_test.java is always the entry point and creates an object instance of the class created via jasmin.

[tautschnig]

Would it be possible to add as many details as possible, such as the precise command line? This is information that you've now got right at hand, but it will be much more work to reproduce later on. Memory is weak, git is strong!

[mgudemann]

:-) it is actually jasmin $FILE.j but I'll add this to the READMEs

[mgudemann]

I added some comments about jasmin, how to get it and how to use it on .j files

[tautschnig]

Thank you!

[mgudemann]

see #951 for an alternative fix working on the write instructions

[mgudemann]

superseded by #951 which takes a different approach that creates less temporary variables