Re: [Libjit] Miscompilation problem

[Aleksey Demakov]

Hi David,

I finally applied my version of the fix. Your test seems to work now.
If you still use libjit please test it too.

Regards,
Aleksey

On Thu, Jan 18, 2018 at 2:21 AM, Aleksey Demakov <address@hidden> wrote:
> Hi David,
>
> Unfortunately not yet. Your patch is probably correct but perhaps
> there is a somewhat simpler fix. Something like replacing in
> reset_value() this line:
>
> value->in_global_register = 0;
>
> with
>
> value->in_global_register = value->has_global_register;
>
> I'm not sure this will work either, didn't check it yet.
>
> Regards,
> Aleksey
>
> On Wed, Jan 17, 2018 at 10:39 PM, David Meyer <address@hidden> wrote:
>> Hi Aleksey,
>>
>> Any progress on this? I am using the following patch to fix the bug for my 
>> own purposes, although I’m not sure if this solution is ideal:
>>
>> diff --git a/jit/jit-compile.c b/jit/jit-compile.c
>> index ba310e4..7b34514 100644
>> --- a/jit/jit-compile.c
>> +++ b/jit/jit-compile.c
>> @@ -308,6 +308,7 @@ reset_value(jit_value_t value)
>>         value->reg = -1;
>>         value->in_register = 0;
>>         value->in_global_register = 0;
>> +       value->has_global_register = 0;
>>         value->in_frame = 0;
>>  }
>>
>> @@ -743,6 +744,9 @@ compile(_jit_compile_t *state, jit_function_t func)
>>                 /* Clean up the compilation state */
>>                 cleanup_on_restart(&state->gen, state->func);
>>
>> +               /* Prepare data needed for code generation */
>> +               codegen_prepare(state);
>> +
>>                 /* Allocate more space */
>>                 memory_realloc(state);
>>         }
>>
>> - David
>>
>> On 1/9/18, 10:18 PM, "Aleksey Demakov" <address@hidden> wrote:
>>
>>     Thanks for the report, I will look into this on the weekend.
>>
>>     On Mon, Jan 8, 2018 at 4:39 PM, David Meyer <address@hidden> wrote:
>>     > I’ve been encountering this sporadically for several weeks. I finally 
>> have
>>     > an isolated example (attached, minimal.c). It will attempt to 
>> reproduce the
>>     > miscompilation. There is some trial and error involved, since it 
>> requires
>>     > triggering an out-of-memory compile restart at just the right time. 
>> When it
>>     > detects a miscompile, it dumps the corresponding object code to
>>     > /tmp/minimal.dump. Here is what it looks like when it miscompiles:
>>     >
>>     >     7ffff7fb402f:       55                      push   %rbp
>>     >
>>     >     7ffff7fb4030:       48 8b ec                mov    %rsp,%rbp
>>     >
>>     >     7ffff7fb4033:       48 83 ec 30             sub    $0x30,%rsp
>>     >
>>     >     7ffff7fb4037:       4c 89 34 24             mov    %r14,(%rsp)
>>     >
>>     >     7ffff7fb403b:       4c 89 7c 24 08          mov    %r15,0x8(%rsp)
>>     >
>>     >     7ffff7fb4040:       4c 8b f7                mov    %rdi,%r14
>>     >
>>     >     7ffff7fb4043:       89 75 f8                mov    %esi,-0x8(%rbp)
>>     >
>>     >     7ffff7fb4046:       89 55 f0                mov    %edx,-0x10(%rbp)
>>     >
>>     >     7ffff7fb4049:       e9 78 78 00 00          jmpq   0x7ffff7fbb8c6
>>     >
>>     >     7ffff7fb404e:       44 8b 7d e8             mov    
>> -0x18(%rbp),%r15d
>>     > <<<<<<<<<<
>>     >
>>     >     7ffff7fb4052:       41 83 c7 00             add    $0x0,%r15d
>>     >
>>     >     7ffff7fb4056:       e9 7b 78 00 00          jmpq   0x7ffff7fbb8d6
>>     >
>>     >
>>     >                ... large dummy jumptable section...
>>     >
>>     >
>>     >     7ffff7fbb8c6:       45 8b fe                mov    %r14d,%r15d
>>     >
>>     >     7ffff7fbb8c9:       44 03 7d f8             add    -0x8(%rbp),%r15d
>>     >
>>     >     7ffff7fbb8cd:       44 03 7d f0             add    
>> -0x10(%rbp),%r15d
>>     >
>>     >     7ffff7fbb8d1:       e9 78 87 ff ff          jmpq   0x7ffff7fb404e
>>     >
>>     >     7ffff7fbb8d6:       41 83 fe 64             cmp    $0x64,%r14d
>>     >
>>     >     7ffff7fbb8da:       0f 84 7b 87 ff ff       je     0x7ffff7fb405b
>>     >
>>     >     7ffff7fbb8e0:       41 8b c7                mov    %r15d,%eax
>>     >
>>     >     7ffff7fbb8e3:       4c 8b 34 24             mov    (%rsp),%r14
>>     >
>>     >     7ffff7fbb8e7:       4c 8b 7c 24 08          mov    0x8(%rsp),%r15
>>     >
>>     >     7ffff7fbb8ec:       48 8b e5                mov    %rbp,%rsp
>>     >
>>     >     7ffff7fbb8ef:       5d                      pop    %rbp
>>     >
>>     >     7ffff7fbb8f0:       c3                      retq
>>     >
>>     >
>>     >
>>     >
>>     > The code at the top of the function is confused about where the value 
>> “v”
>>     > (defined on line 44 of minimal.c) lives. The code at the bottom 
>> assumes that
>>     > the value lives in register %r15, but the code at the top pulls it 
>> from the
>>     > stack at -0x18(%rbp), even through it is never written there.
>>     >
>>     > I have only a vague idea of what might be happening. The value “v” is 
>> bound
>>     > to a global register by codegen_prepare() (which runs on the first 
>> compile,
>>     > but not after restarts). The memory exception triggers reset_value() 
>> on “v”
>>     > (from cleanup_on_restart), which clears v->in_global_register and
>>     > v->in_register, leaving “v” in a slightly different state the second 
>> time
>>     > codegen starts. So, when the second compilation hits the add 
>> instruction at
>>     > the very top (the first instruction that uses v in the 
>> block/instruction
>>     > iteration order), it assumes it needs to load it from the stack.
>>     >
>>     >
>>     > I’m not sure what the correct fix is here, but it seems like the value 
>> state
>>     > should be consistent across compile restarts.
>>
>>