Re: [Qemu-devel] Crash due to invalid env->current_tb

[Laurent Vivier]

Le mercredi 30 avril 2008 à 17:11 +0200, Adam Lackorzynski a écrit :
> On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
> >
> > On Apr 29, 2008, at 8:40 PM, Adam Lackorzynski wrote:
> >
> >>
> >> On Tue Apr 29, 2008 at 20:09:00 +0300, Blue Swirl wrote:
> >>> On 4/29/08, Adam Lackorzynski <address@hidden> wrote:
> >>>> Hi,
> >>>>
> >>>> I've been experiencing crashes of latest svn Qemu, host ia32 and  
> >>>> target
> >>>> arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
> >>>> The segfault happens because of an invalid env->current_tb which  
> >>>> seems
> >>>> to be caused by generated code. The following code in cpu_exec
> >>>>
> >>>>   tc_ptr = tb->tc_ptr;
> >>>>   env->current_tb = tb;
> >>>>   gen_func = (void *)tc_ptr;
> >>>>   T0 = gen_func();
> >>>>   env->current_tb = NULL;
> >>>>
> >>>> is being compiled to the following
> >>>>
> >>>>   mov    0x14(%ecx),%eax
> >>>>   mov    %ecx,0x56c(%ebp)
> >>>>   xor    %edi,%edi
> >>>>   call   *%eax
> >>>>   mov    %edi,0x56c(%ebp)
> >>>>
> >>>> After the call edi isn't 0 anymore and gets the bogus value. As  
> >>>> edi is
> >>>> callee saved the code itself seems ok.
> >>>> When I add a barrier before "env->current_tb = NULL" the xor is  
> >>>> placed
> >>>> after the call and everything works fine. So might the problem be  
> >>>> that
> >>>> generated code isn't preserving edi/registers?
> >>>
> >>> Right. How did you make the barrier? My version (attached) just
> >>> crashes, I'm not fluent on i386 assembly. Maybe your version could
> >>> serve as a temporary fix.
> >>
> >> I just added an 'asm volatile("")' to stop reordering of instructions
> >> which of course isn't enough. The following works for me:
> >>
> >> ===================================================================
> >> --- cpu-exec.c     (revision 4276)
> >> +++ cpu-exec.c     (working copy)
> >> @@ -690,6 +691,11 @@
> >>            fp.ip = tc_ptr;
> >>            fp.gp = code_gen_buffer + 2 * (1 << 20);
> >>            (*(void (*)(void)) &fp)();
> >> +#elif defined(__i386)
> >> +          asm volatile ("call *%1\n"
> >> +                        : "=a" (T0)
> >> +                        : "r" (gen_func)
> >> +                        : "esi", "edi");
> >> #else
> >>                 T0 = gen_func();
> >> #endif
> >
> > There was a comment from Fabrice on how to do prologues in TCG to save / 
> > restore the clobbered values. Btw, ebx gets clobbered as well.
> 
> tcg/README says that some registers are clobbered. So something like
> this should be safe:
> 
> Index: cpu-exec.c
> ===================================================================
> --- cpu-exec.c        (revision 4276)
> +++ cpu-exec.c        (working copy)
> @@ -690,6 +691,15 @@
>               fp.ip = tc_ptr;
>               fp.gp = code_gen_buffer + 2 * (1 << 20);
>               (*(void (*)(void)) &fp)();
> +#elif defined(__i386)
> +             asm volatile ("push %%ebp\n"
> +                           "push %%ebx\n"
> +                           "call *%1\n"
> +                           "pop %%ebx\n"
> +                           "pop %%ebp\n"
> +                           : "=a" (T0)
> +                           : "r" (gen_func)
> +                           : "esi", "edi", "ecx", "edx");

Why don't you add ebp and ebx in the clobbered registers list (like
"esi", "edi", "ecx", "edx") ?

>  #else
>                  T0 = gen_func();
>  #endif
> 
> 
> 
> 
> Adam
-- 
------------- address@hidden ---------------
"The best way to predict the future is to invent it."
- Alan Kay