i#864: Don't set dcontext's x86_mode when recreating app state.

Reviewer: rnk@google.com
i#864: Don't set dcontext's x86_mode when recreating app state.

i#864: Don't set dcontext's x86_mode when recreating app state.

Fixes issue 864.

[bruening, 2012-08-06 23:18:11]

This doesn't look right to me: if in mixed-mode one thread can suspend
another and ask for its translation, which must temporarily set the mode
for its suspended fragment.


On Mon, Aug 6, 2012 at 6:26 PM, <yangt@google.com> wrote:

> Reviewers: Reid Kleckner (google),
>
> Description:
> Reviewer: rnk@google.com
> i#864: Don't set dcontext's x86_mode when recreating app state.
>
> i#864: Don't set dcontext's x86_mode when recreating app state.
>
> Fixes issue 864.
>
>
> Please review this at
http://codereview.appspot.com/**6448116/<http://codereview.appspot.com/6448116/>
>
> Affected files:
>   M trunk/core/x86/arch.c
>   M trunk/core/x86/interp.c
>
>
> Index: trunk/core/x86/arch.c
> diff --git a/trunk/core/x86/arch.c b/trunk/core/x86/arch.c
> index 2f43f45883e84a5ec8d661d525c51b**738713378a..**
> df9c6dacb7a13b72cc9dc3fd787fad**d98d626499 100644
> --- a/trunk/core/x86/arch.c
> +++ b/trunk/core/x86/arch.c
> @@ -3165,7 +3165,6 @@ recreate_app_state_internal(**dcontext_t
> *tdcontext, priv_mcontext_t *mcontext,
>          instrlist_t *ilist = NULL;
>          fragment_t *f = owning_f;
>          bool alloc = false;
> -        IF_X64(bool old_mode;)
>  #ifdef CLIENT_INTERFACE
>          dr_restore_state_info_t client_info;
>          dr_mcontext_t xl8_mcontext;
> @@ -3260,8 +3259,10 @@ recreate_app_state_internal(**dcontext_t
> *tdcontext, priv_mcontext_t *mcontext,
>              mcontext->pc = cti_pc;
>          }
>
> -        /* Recreate in same mode as original fragment */
> -        IF_X64(old_mode = set_x86_mode(tdcontext, FRAG_IS_32(f->flags)));
> +#ifdef X64
> +        /* must be recreating in the same code cache mode as original
> fragment */
> +        ASSERT(BOOLS_MATCH(X64_CACHE_**MODE_DC(tdcontext),
> !FRAG_IS_32(f->flags)));
> +#endif
>
>          /* now recreate the state */
>  #ifdef CLIENT_INTERFACE
> @@ -3286,7 +3287,6 @@ recreate_app_state_internal(**dcontext_t
> *tdcontext, priv_mcontext_t *mcontext,
>                                                  mcontext, just_pc,
> f->flags);
>              STATS_INC(recreate_via_app_**ilist);
>          }
> -        IF_X64(set_x86_mode(tdcontext, old_mode));
>
>  #ifdef STEAL_REGISTER
>          /* FIXME: conflicts w/ PR 263407 reg spill tracking */
> Index: trunk/core/x86/interp.c
> diff --git a/trunk/core/x86/interp.c b/trunk/core/x86/interp.c
> index 237e0d6afe2de5209185cd802033b5**968aa44130..**
> 6dd8acc1405531171360bbdb9db87e**2f5e99a6fc 100644
> --- a/trunk/core/x86/interp.c
> +++ b/trunk/core/x86/interp.c
> @@ -4407,7 +4407,6 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
> byte *pc,
>      instrlist_t *ilist;
>      bool alloc = false;
>      monitor_data_t md = {0,};
> -    IF_X64(bool old_mode = true;)
>      /* check synchronization, we need to make sure no one flushes the
>       * fragment we just looked up while we are recreating it, if it's the
>       * caller's dcontext then just need to be couldbelinking, otherwise
> need
> @@ -4442,8 +4441,10 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
> byte *pc,
>          }
>      }
>
> -    /* Recreate in same mode as original fragment */
> -    IF_X64(old_mode = set_x86_mode(dcontext, FRAG_IS_32(f->flags)));
> +#ifdef X64
> +    /* must be recreating in the same code cache mode as original
> fragment */
> +    ASSERT(BOOLS_MATCH(X64_CACHE_**MODE_DC(dcontext),
> !FRAG_IS_32(f->flags)));
> +#endif
>
>      if ((f->flags & FRAG_IS_TRACE) == 0) {
>          /* easy case: just a bb */
> @@ -4606,7 +4607,6 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
> byte *pc,
>          *alloc_res = alloc;
>      if (f_res == NULL && alloc)
>          fragment_free(dcontext, f);
> -    IF_X64(set_x86_mode(dcontext, old_mode));
>      return ilist;
>  }
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "DynamoRIO Devs" group.
> To post to this group, send email to
dynamorio-devs@googlegroups.**com<dynamorio-devs@googlegroups.com>
> .
> To unsubscribe from this group, send email to dynamorio-devs+unsubscribe@*
> *googlegroups.com <dynamorio-devs%2Bunsubscribe@googlegroups.com>.
> For more options, visit this group at http://groups.google.com/**
>
group/dynamorio-devs?hl=en<http://groups.google.com/group/dynamorio-devs?hl=en>
> .
>
>

[Reid Kleckner (google), 2012-08-07 03:30:48]

Currently fragments have no idea whether they were translated from 32-bit
code or not.  For state translation, we should be able to just get the
current mode.  But we'll also need the mode for pc lookup, which can happen
at any time for any fragment, right?  Seems like we might want to add a
flag.


On Mon, Aug 6, 2012 at 7:18 PM, Derek Bruening <bruening@google.com> wrote:

> This doesn't look right to me: if in mixed-mode one thread can suspend
> another and ask for its translation, which must temporarily set the mode
> for its suspended fragment.
>
>
> On Mon, Aug 6, 2012 at 6:26 PM, <yangt@google.com> wrote:
>
>> Reviewers: Reid Kleckner (google),
>>
>> Description:
>> Reviewer: rnk@google.com
>> i#864: Don't set dcontext's x86_mode when recreating app state.
>>
>> i#864: Don't set dcontext's x86_mode when recreating app state.
>>
>> Fixes issue 864.
>>
>>
>> Please review this at
http://codereview.appspot.com/**6448116/<http://codereview.appspot.com/6448116/>
>>
>> Affected files:
>>   M trunk/core/x86/arch.c
>>   M trunk/core/x86/interp.c
>>
>>
>> Index: trunk/core/x86/arch.c
>> diff --git a/trunk/core/x86/arch.c b/trunk/core/x86/arch.c
>> index 2f43f45883e84a5ec8d661d525c51b**738713378a..**
>> df9c6dacb7a13b72cc9dc3fd787fad**d98d626499 100644
>> --- a/trunk/core/x86/arch.c
>> +++ b/trunk/core/x86/arch.c
>> @@ -3165,7 +3165,6 @@ recreate_app_state_internal(**dcontext_t
>> *tdcontext, priv_mcontext_t *mcontext,
>>          instrlist_t *ilist = NULL;
>>          fragment_t *f = owning_f;
>>          bool alloc = false;
>> -        IF_X64(bool old_mode;)
>>  #ifdef CLIENT_INTERFACE
>>          dr_restore_state_info_t client_info;
>>          dr_mcontext_t xl8_mcontext;
>> @@ -3260,8 +3259,10 @@ recreate_app_state_internal(**dcontext_t
>> *tdcontext, priv_mcontext_t *mcontext,
>>              mcontext->pc = cti_pc;
>>          }
>>
>> -        /* Recreate in same mode as original fragment */
>> -        IF_X64(old_mode = set_x86_mode(tdcontext, FRAG_IS_32(f->flags)));
>> +#ifdef X64
>> +        /* must be recreating in the same code cache mode as original
>> fragment */
>> +        ASSERT(BOOLS_MATCH(X64_CACHE_**MODE_DC(tdcontext),
>> !FRAG_IS_32(f->flags)));
>> +#endif
>>
>>          /* now recreate the state */
>>  #ifdef CLIENT_INTERFACE
>> @@ -3286,7 +3287,6 @@ recreate_app_state_internal(**dcontext_t
>> *tdcontext, priv_mcontext_t *mcontext,
>>                                                  mcontext, just_pc,
>> f->flags);
>>              STATS_INC(recreate_via_app_**ilist);
>>          }
>> -        IF_X64(set_x86_mode(tdcontext, old_mode));
>>
>>  #ifdef STEAL_REGISTER
>>          /* FIXME: conflicts w/ PR 263407 reg spill tracking */
>> Index: trunk/core/x86/interp.c
>> diff --git a/trunk/core/x86/interp.c b/trunk/core/x86/interp.c
>> index 237e0d6afe2de5209185cd802033b5**968aa44130..**
>> 6dd8acc1405531171360bbdb9db87e**2f5e99a6fc 100644
>> --- a/trunk/core/x86/interp.c
>> +++ b/trunk/core/x86/interp.c
>> @@ -4407,7 +4407,6 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
>> byte *pc,
>>      instrlist_t *ilist;
>>      bool alloc = false;
>>      monitor_data_t md = {0,};
>> -    IF_X64(bool old_mode = true;)
>>      /* check synchronization, we need to make sure no one flushes the
>>       * fragment we just looked up while we are recreating it, if it's the
>>       * caller's dcontext then just need to be couldbelinking, otherwise
>> need
>> @@ -4442,8 +4441,10 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
>> byte *pc,
>>          }
>>      }
>>
>> -    /* Recreate in same mode as original fragment */
>> -    IF_X64(old_mode = set_x86_mode(dcontext, FRAG_IS_32(f->flags)));
>> +#ifdef X64
>> +    /* must be recreating in the same code cache mode as original
>> fragment */
>> +    ASSERT(BOOLS_MATCH(X64_CACHE_**MODE_DC(dcontext),
>> !FRAG_IS_32(f->flags)));
>> +#endif
>>
>>      if ((f->flags & FRAG_IS_TRACE) == 0) {
>>          /* easy case: just a bb */
>> @@ -4606,7 +4607,6 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
>> byte *pc,
>>          *alloc_res = alloc;
>>      if (f_res == NULL && alloc)
>>          fragment_free(dcontext, f);
>> -    IF_X64(set_x86_mode(dcontext, old_mode));
>>      return ilist;
>>  }
>>
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "DynamoRIO Devs" group.
>> To post to this group, send email to
dynamorio-devs@googlegroups.**com<dynamorio-devs@googlegroups.com>
>> .
>> To unsubscribe from this group, send email to dynamorio-devs+unsubscribe@
>> **googlegroups.com <dynamorio-devs%2Bunsubscribe@googlegroups.com>.
>> For more options, visit this group at http://groups.google.com/**
>>
group/dynamorio-devs?hl=en<http://groups.google.com/group/dynamorio-devs?hl=en>
>> .
>>
>>
>

[yangt, 2012-08-07 15:38:16]

But we're running out of fragment flags. For now can I do it in the way
below (see new diff)?


On Mon, Aug 6, 2012 at 11:30 PM, Reid Kleckner <rnk@google.com> wrote:

> Currently fragments have no idea whether they were translated from 32-bit
> code or not.  For state translation, we should be able to just get the
> current mode.  But we'll also need the mode for pc lookup, which can happen
> at any time for any fragment, right?  Seems like we might want to add a
> flag.
>
>
> On Mon, Aug 6, 2012 at 7:18 PM, Derek Bruening <bruening@google.com>wrote:
>
>> This doesn't look right to me: if in mixed-mode one thread can suspend
>> another and ask for its translation, which must temporarily set the mode
>> for its suspended fragment.
>>
>>
>>  On Mon, Aug 6, 2012 at 6:26 PM, <yangt@google.com> wrote:
>>
>>>  Reviewers: Reid Kleckner (google),
>>>
>>> Description:
>>> Reviewer: rnk@google.com
>>> i#864: Don't set dcontext's x86_mode when recreating app state.
>>>
>>> i#864: Don't set dcontext's x86_mode when recreating app state.
>>>
>>> Fixes issue 864.
>>>
>>>
>>> Please review this at
http://codereview.appspot.com/**6448116/<http://codereview.appspot.com/6448116/>
>>>
>>> Affected files:
>>>   M trunk/core/x86/arch.c
>>>   M trunk/core/x86/interp.c
>>>
>>>
>>> Index: trunk/core/x86/arch.c
>>> diff --git a/trunk/core/x86/arch.c b/trunk/core/x86/arch.c
>>> index 2f43f45883e84a5ec8d661d525c51b**738713378a..**
>>> df9c6dacb7a13b72cc9dc3fd787fad**d98d626499 100644
>>> --- a/trunk/core/x86/arch.c
>>> +++ b/trunk/core/x86/arch.c
>>> @@ -3165,7 +3165,6 @@ recreate_app_state_internal(**dcontext_t
>>> *tdcontext, priv_mcontext_t *mcontext,
>>>          instrlist_t *ilist = NULL;
>>>          fragment_t *f = owning_f;
>>>          bool alloc = false;
>>> -        IF_X64(bool old_mode;)
>>>  #ifdef CLIENT_INTERFACE
>>>          dr_restore_state_info_t client_info;
>>>          dr_mcontext_t xl8_mcontext;
>>> @@ -3260,8 +3259,10 @@ recreate_app_state_internal(**dcontext_t
>>> *tdcontext, priv_mcontext_t *mcontext,
>>>              mcontext->pc = cti_pc;
>>>          }
>>>
>>> -        /* Recreate in same mode as original fragment */
>>> -        IF_X64(old_mode = set_x86_mode(tdcontext,
>>> FRAG_IS_32(f->flags)));
>>> +#ifdef X64
>>> +        /* must be recreating in the same code cache mode as original
>>> fragment */
>>> +        ASSERT(BOOLS_MATCH(X64_CACHE_**MODE_DC(tdcontext),
>>> !FRAG_IS_32(f->flags)));
>>> +#endif
>>>
>>>          /* now recreate the state */
>>>  #ifdef CLIENT_INTERFACE
>>> @@ -3286,7 +3287,6 @@ recreate_app_state_internal(**dcontext_t
>>> *tdcontext, priv_mcontext_t *mcontext,
>>>                                                  mcontext, just_pc,
>>> f->flags);
>>>              STATS_INC(recreate_via_app_**ilist);
>>>          }
>>> -        IF_X64(set_x86_mode(tdcontext, old_mode));
>>>
>>>  #ifdef STEAL_REGISTER
>>>          /* FIXME: conflicts w/ PR 263407 reg spill tracking */
>>> Index: trunk/core/x86/interp.c
>>> diff --git a/trunk/core/x86/interp.c b/trunk/core/x86/interp.c
>>> index 237e0d6afe2de5209185cd802033b5**968aa44130..**
>>> 6dd8acc1405531171360bbdb9db87e**2f5e99a6fc 100644
>>> --- a/trunk/core/x86/interp.c
>>> +++ b/trunk/core/x86/interp.c
>>> @@ -4407,7 +4407,6 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
>>> byte *pc,
>>>      instrlist_t *ilist;
>>>      bool alloc = false;
>>>      monitor_data_t md = {0,};
>>> -    IF_X64(bool old_mode = true;)
>>>      /* check synchronization, we need to make sure no one flushes the
>>>       * fragment we just looked up while we are recreating it, if it's
>>> the
>>>       * caller's dcontext then just need to be couldbelinking, otherwise
>>> need
>>> @@ -4442,8 +4441,10 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
>>> byte *pc,
>>>          }
>>>      }
>>>
>>> -    /* Recreate in same mode as original fragment */
>>> -    IF_X64(old_mode = set_x86_mode(dcontext, FRAG_IS_32(f->flags)));
>>> +#ifdef X64
>>> +    /* must be recreating in the same code cache mode as original
>>> fragment */
>>> +    ASSERT(BOOLS_MATCH(X64_CACHE_**MODE_DC(dcontext),
>>> !FRAG_IS_32(f->flags)));
>>> +#endif
>>>
>>>      if ((f->flags & FRAG_IS_TRACE) == 0) {
>>>          /* easy case: just a bb */
>>> @@ -4606,7 +4607,6 @@ recreate_fragment_ilist(**dcontext_t *dcontext,
>>> byte *pc,
>>>          *alloc_res = alloc;
>>>      if (f_res == NULL && alloc)
>>>          fragment_free(dcontext, f);
>>> -    IF_X64(set_x86_mode(dcontext, old_mode));
>>>      return ilist;
>>>  }
>>>
>>>
>>>
>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups "DynamoRIO Devs" group.
>>> To post to this group, send email to
dynamorio-devs@googlegroups.**com<dynamorio-devs@googlegroups.com>
>>> .
>>> To unsubscribe from this group, send email to
>>>
dynamorio-devs+unsubscribe@**googlegroups.com<dynamorio-devs%2Bunsubscribe@go...
>>> .
>>> For more options, visit this group at http://groups.google.com/**
>>>
group/dynamorio-devs?hl=en<http://groups.google.com/group/dynamorio-devs?hl=en>
>>> .
>>>
>>>
>>
>  --
> You received this message because you are subscribed to the Google Groups
> "DynamoRIO Devs" group.
> To post to this group, send email to dynamorio-devs@googlegroups.com.
> To unsubscribe from this group, send email to
> dynamorio-devs+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/dynamorio-devs?hl=en.
>



-- 
Yang Tang | Software Engineering Intern | yangt@google.com | US-NYC-9TH
12F104F

[bruening, 2012-08-07 18:15:30]

http://codereview.appspot.com/6448116/diff/5001/trunk/core/x86/arch.c
File trunk/core/x86/arch.c (right):

http://codereview.appspot.com/6448116/diff/5001/trunk/core/x86/arch.c#newcode...
trunk/core/x86/arch.c:3266: old_mode = get_x86_mode(tdcontext);
state translation is used for much more than just faults.  it's used to relocate
threads for some types of cache flushing and GC as well as to provide a state
for thread suspension.  those events can all happen at arbitrary points in the
cache.  so the current mode of the target thread's dcontext is not the right
mode to use: we need the mode of the fragment.

if -x86_to_x64 did not support 32-bit fragments, you could use the FRAG_32_BIT
flag, but that would get messy b/c other code uses the flag in situations where
you don't want to set it.

you could take FRAG_DO_NOT_SIDELINE at least for now.  nobody is building
SIDELINE.  it could be incompat w/ x64 as a at-least-temp solution.

[bruening, 2012-08-13 17:57:59]

LGTM

http://codereview.appspot.com/6448116/diff/9001/trunk/core/emit.c
File trunk/core/emit.c (right):

http://codereview.appspot.com/6448116/diff/9001/trunk/core/emit.c#newcode455
trunk/core/emit.c:455: else if (get_x86_mode(dcontext))
I guess we don't support the client having other-mode instrs in there: have to
be translated before get to here

http://codereview.appspot.com/6448116/diff/9001/trunk/core/fragment.h
File trunk/core/fragment.h (right):

http://codereview.appspot.com/6448116/diff/9001/trunk/core/fragment.h#newcode149
trunk/core/fragment.h:149: /* this fragment contains code translated from 32-bit
app code */
64-bit code

[yangt, 2012-08-13 22:05:02]

Unfortunately, this breaks after r1525. I'm trying to fix it. So I won't commit
for now.

http://codereview.appspot.com/6448116/diff/9001/trunk/core/emit.c
File trunk/core/emit.c (right):

http://codereview.appspot.com/6448116/diff/9001/trunk/core/emit.c#newcode455
trunk/core/emit.c:455: else if (get_x86_mode(dcontext))
On 2012/08/13 17:58:00, bruening wrote:
> I guess we don't support the client having other-mode instrs in there: have to
> be translated before get to here

Right.

http://codereview.appspot.com/6448116/diff/9001/trunk/core/fragment.h
File trunk/core/fragment.h (right):

http://codereview.appspot.com/6448116/diff/9001/trunk/core/fragment.h#newcode149
trunk/core/fragment.h:149: /* this fragment contains code translated from 32-bit
app code */
On 2012/08/13 17:58:00, bruening wrote:
> 64-bit code

Done.

[bruening, 2012-08-14 17:06:16]

http://codereview.appspot.com/6448116/diff/14001/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6448116/diff/14001/trunk/core/x86/emit_utils.c#...
trunk/core/x86/emit_utils.c:6986:
link_shared_syscall_common(SHARED_GENCODE(true));
you need to link the 64-bit version b/c there are 64-bit source fragments

http://codereview.appspot.com/6448116/diff/14001/trunk/core/x86/emit_utils.c#...
trunk/core/x86/emit_utils.c:7026:
unlink_shared_syscall_common(SHARED_GENCODE(true));
ditto

[bruening, 2012-08-14 18:38:50]

LGTM w/ comments

http://codereview.appspot.com/6448116/diff/8003/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6448116/diff/8003/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6984: *
link_shared_syscall_common(SHARED_GENCODE(true));
shared_syscall only exists for windows so we don't need a FIXME, just something
like "XXX i#821: there are no 32-bit syscalls for WOW64 with 64-bit DR".

can you update the other PR 284029 ref to i#821 while you're at it?

http://codereview.appspot.com/6448116/diff/8003/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:7023: /* FIXME PR 284029: for now we assume there
are no syscalls in x86 code.
ditto