i#1823 dr_insert_mbr_instrumentation() of gs: add a flag for meta far disp base instr

Commit log for first patchset:
---------------
i#1823 dr_insert_mbr_instrumentation() of gs:
add a flag for meta far disp base instr

add a flag define only on unix for meta isntruction who need
segment mangling and use set it on dr_insert_mbr_instrumentation
when the source opnd is a far base disp opnd to fix #1823

Fixes #1823
---------------

[zhaoqin, 2015-11-13 16:08:39]

Sorry for the late review, I thought we decide to do the mangle directly instead
of adding an instruction flag.

By the way, have you checked the CLA form for contributing to DR, xref
https://github.com/DynamoRIO/dynamorio/wiki/Contributing.

Thanks

[ampotos, 2015-11-13 17:13:38]

On 2015/11/13 16:08:39, zhaoqin wrote:
> Sorry for the late review, I thought we decide to do the mangle directly
instead
> of adding an instruction flag.
> 
> By the way, have you checked the CLA form for contributing to DR, xref
> https://github.com/DynamoRIO/dynamorio/wiki/Contributing.
> 
> Thanks

I thought use a flag is better because if an other instrumentation function need
to use segment manling on meta instr the only thing to do is to set this flag.

If you think this is useless I can put the mangling code directly in 
dr_insert_mbr_instrumentation()

Yes I have checked the CLA form.

[zhaoqin, 2015-11-13 17:48:22]

On 2015/11/13 17:13:38, ampotos wrote:
> On 2015/11/13 16:08:39, zhaoqin wrote:
> > Sorry for the late review, I thought we decide to do the mangle directly
> instead
> > of adding an instruction flag.
> > 
> > By the way, have you checked the CLA form for contributing to DR, xref
> > https://github.com/DynamoRIO/dynamorio/wiki/Contributing.
> > 
> > Thanks
> 
> I thought use a flag is better because if an other instrumentation function
need
> to use segment manling on meta instr the only thing to do is to set this flag.
> 
> If you think this is useless I can put the mangling code directly in 
> dr_insert_mbr_instrumentation()

Adding a flag a feasible solution and has its own advantages.
However we have very limited flag bits left, and we do not want to change the
data structure of instr_t due to back compatibility.
Thus I tend to avoid using them unless absolute necessary (e.g., porting to new
arch).
It looks to me we could have alternatives for this solution, so I am still
leaning toward using direct mangling instead.

> 
> Yes I have checked the CLA form.
Thanks

[ampotos, 2015-11-13 18:00:05]

On 2015/11/13 17:48:22, zhaoqin wrote:
> On 2015/11/13 17:13:38, ampotos wrote:
> > On 2015/11/13 16:08:39, zhaoqin wrote:
> > > Sorry for the late review, I thought we decide to do the mangle directly
> > instead
> > > of adding an instruction flag.
> > > 
> > > By the way, have you checked the CLA form for contributing to DR, xref
> > > https://github.com/DynamoRIO/dynamorio/wiki/Contributing.
> > > 
> > > Thanks
> > 
> > I thought use a flag is better because if an other instrumentation function
> need
> > to use segment manling on meta instr the only thing to do is to set this
flag.
> > 
> > If you think this is useless I can put the mangling code directly in 
> > dr_insert_mbr_instrumentation()
> 
> Adding a flag a feasible solution and has its own advantages.
> However we have very limited flag bits left, and we do not want to change the
> data structure of instr_t due to back compatibility.
> Thus I tend to avoid using them unless absolute necessary (e.g., porting to
new
> arch).
> It looks to me we could have alternatives for this solution, so I am still
> leaning toward using direct mangling instead.
> 
> > 
> > Yes I have checked the CLA form.
> Thanks

Ok, I will do the change in the week-end.

[ampotos, 2015-11-15 21:53:39]

Commit log for latest patchset:
---------------
i#1823 dr_insert_mbr_instrumentation() of gs:

add mangling of gs segment in dr_insert_mbr_instrumentation

Fixes #1823
---------------

[ampotos, 2015-11-15 22:06:47]

Commit log for latest patchset:
---------------
i#1823 dr_insert_mbr_instrumentation() of gs:

add mangling of gs segment in dr_insert_mbr_instrumentation

Fixes #1823
---------------

[zhaoqin, 2015-11-15 23:25:49]

A few issues need to be fixed, also it would be nice to add a test for it.

https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h
File core/arch/arch.h (right):

https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h#newcode540
core/arch/arch.h:540: /* mangle the instruction that reference memory via
segment register */
The comment above is for mangle_seg_ref not mangle_seg_ref_opnd.
You could move the comment for mangle_seg_ref_opnd from core/arch/x86/mangle.c
to here or add a short comment here.

Also, shouldn't we add conditional macro for UNIX too?

https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c
File core/arch/mangle_shared.c (right):

https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c...
core/arch/mangle_shared.c:741: if (INTERNAL_OPTION(mangle_app_seg) &&
(instr_is_app(instr))) {
why add extra parentheses?

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
File core/lib/instrument.c (left):

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#old...
core/lib/instrument.c:5797: 
why remove the line?

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
core/lib/instrument.c:5794: opnd_get_segment(src) == DR_SEG_GS)
It is should also include the case if seg reg is DR_SEG_FS.

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist, instr,
src, reg_target);
according to mangle_seg_ref_opnd, reg_target must not be used by src, however,
it seems src could use ECX, e.g., gs:ecx. In that case, we should steal another
register for the mangling.

[ampotos, 2015-11-16 11:45:47]

On 2015/11/15 23:25:49, zhaoqin wrote:
> A few issues need to be fixed, also it would be nice to add a test for it.
> 
> https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h
> File core/arch/arch.h (right):
> 
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h#newcode540
> core/arch/arch.h:540: /* mangle the instruction that reference memory via
> segment register */
> The comment above is for mangle_seg_ref not mangle_seg_ref_opnd.
> You could move the comment for mangle_seg_ref_opnd from core/arch/x86/mangle.c
> to here or add a short comment here.
> 
> Also, shouldn't we add conditional macro for UNIX too?
> 
> https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c
> File core/arch/mangle_shared.c (right):
> 
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c...
> core/arch/mangle_shared.c:741: if (INTERNAL_OPTION(mangle_app_seg) &&
> (instr_is_app(instr))) {
> why add extra parentheses?
> 
> https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> File core/lib/instrument.c (left):
> 
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#old...
> core/lib/instrument.c:5797: 
> why remove the line?
> 
> https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> File core/lib/instrument.c (right):
> 
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> core/lib/instrument.c:5794: opnd_get_segment(src) == DR_SEG_GS)
> It is should also include the case if seg reg is DR_SEG_FS.
> 
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist, instr,
> src, reg_target);
> according to mangle_seg_ref_opnd, reg_target must not be used by src, however,
> it seems src could use ECX, e.g., gs:ecx. In that case, we should steal
another
> register for the mangling.
When I was looking at the code to look how you choose the register to stole for
the case when the ECX is used in src I found 2 other where that issue is present
(in mangle_indirect_call and mangle_indirect_jump). Do you me to fix this 2 ?

[ampotos, 2015-11-16 14:33:40]

On 2015/11/16 11:45:47, ampotos wrote:
> On 2015/11/15 23:25:49, zhaoqin wrote:
> > A few issues need to be fixed, also it would be nice to add a test for it.
> > 
> > https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h
> > File core/arch/arch.h (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h#newcode540
> > core/arch/arch.h:540: /* mangle the instruction that reference memory via
> > segment register */
> > The comment above is for mangle_seg_ref not mangle_seg_ref_opnd.
> > You could move the comment for mangle_seg_ref_opnd from
core/arch/x86/mangle.c
> > to here or add a short comment here.
> > 
> > Also, shouldn't we add conditional macro for UNIX too?
> > 
> >
https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c
> > File core/arch/mangle_shared.c (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c...
> > core/arch/mangle_shared.c:741: if (INTERNAL_OPTION(mangle_app_seg) &&
> > (instr_is_app(instr))) {
> > why add extra parentheses?
> > 
> > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > File core/lib/instrument.c (left):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#old...
> > core/lib/instrument.c:5797: 
> > why remove the line?
> > 
> > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > File core/lib/instrument.c (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > core/lib/instrument.c:5794: opnd_get_segment(src) == DR_SEG_GS)
> > It is should also include the case if seg reg is DR_SEG_FS.
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist,
instr,
> > src, reg_target);
> > according to mangle_seg_ref_opnd, reg_target must not be used by src,
however,
> > it seems src could use ECX, e.g., gs:ecx. In that case, we should steal
> another
> > register for the mangling.
> When I was looking at the code to look how you choose the register to stole
for
> the case when the ECX is used in src I found 2 other where that issue is
present
> (in mangle_indirect_call and mangle_indirect_jump). Do you me to fix this 2 ?

For the test, how can I add a test in client_interface only for linux ?

[zhaoqin, 2015-11-16 15:22:10]

On 2015/11/16 11:45:47, ampotos wrote:
> On 2015/11/15 23:25:49, zhaoqin wrote:
> > A few issues need to be fixed, also it would be nice to add a test for it.
> > 
> > https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h
> > File core/arch/arch.h (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h#newcode540
> > core/arch/arch.h:540: /* mangle the instruction that reference memory via
> > segment register */
> > The comment above is for mangle_seg_ref not mangle_seg_ref_opnd.
> > You could move the comment for mangle_seg_ref_opnd from
core/arch/x86/mangle.c
> > to here or add a short comment here.
> > 
> > Also, shouldn't we add conditional macro for UNIX too?
> > 
> >
https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c
> > File core/arch/mangle_shared.c (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c...
> > core/arch/mangle_shared.c:741: if (INTERNAL_OPTION(mangle_app_seg) &&
> > (instr_is_app(instr))) {
> > why add extra parentheses?
> > 
> > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > File core/lib/instrument.c (left):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#old...
> > core/lib/instrument.c:5797: 
> > why remove the line?
> > 
> > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > File core/lib/instrument.c (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > core/lib/instrument.c:5794: opnd_get_segment(src) == DR_SEG_GS)
> > It is should also include the case if seg reg is DR_SEG_FS.
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist,
instr,
> > src, reg_target);
> > according to mangle_seg_ref_opnd, reg_target must not be used by src,
however,
> > it seems src could use ECX, e.g., gs:ecx. In that case, we should steal
> another
> > register for the mangling.
> When I was looking at the code to look how you choose the register to stole
for
> the case when the ECX is used in src I found 2 other where that issue is
present
> (in mangle_indirect_call and mangle_indirect_jump). Do you me to fix this 2 ?

It looks like we have never encounter ecx used in far indirect jump with ecx.
You can just do the same thing as those two, i.e., add comment and assert
ASSERT_BUG_NUM(107, !opnd_uses_reg(target, REG_XCX));

[zhaoqin, 2015-11-16 15:24:54]

On 2015/11/16 14:33:40, ampotos wrote:
> On 2015/11/16 11:45:47, ampotos wrote:
> > On 2015/11/15 23:25:49, zhaoqin wrote:
> > > A few issues need to be fixed, also it would be nice to add a test for it.
> > > 
> > > https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h
> > > File core/arch/arch.h (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/arch.h#newcode540
> > > core/arch/arch.h:540: /* mangle the instruction that reference memory via
> > > segment register */
> > > The comment above is for mangle_seg_ref not mangle_seg_ref_opnd.
> > > You could move the comment for mangle_seg_ref_opnd from
> core/arch/x86/mangle.c
> > > to here or add a short comment here.
> > > 
> > > Also, shouldn't we add conditional macro for UNIX too?
> > > 
> > >
> https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c
> > > File core/arch/mangle_shared.c (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/arch/mangle_shared.c...
> > > core/arch/mangle_shared.c:741: if (INTERNAL_OPTION(mangle_app_seg) &&
> > > (instr_is_app(instr))) {
> > > why add extra parentheses?
> > > 
> > > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > > File core/lib/instrument.c (left):
> > > 
> > >
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#old...
> > > core/lib/instrument.c:5797: 
> > > why remove the line?
> > > 
> > > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > > File core/lib/instrument.c (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > > core/lib/instrument.c:5794: opnd_get_segment(src) == DR_SEG_GS)
> > > It is should also include the case if seg reg is DR_SEG_FS.
> > > 
> > >
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > > core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist,
> instr,
> > > src, reg_target);
> > > according to mangle_seg_ref_opnd, reg_target must not be used by src,
> however,
> > > it seems src could use ECX, e.g., gs:ecx. In that case, we should steal
> > another
> > > register for the mangling.
> > When I was looking at the code to look how you choose the register to stole
> for
> > the case when the ECX is used in src I found 2 other where that issue is
> present
> > (in mangle_indirect_call and mangle_indirect_jump). Do you me to fix this 2
?
> 
> For the test, how can I add a test in client_interface only for linux ?

In suite/tests/CMakeLists.txt, use if (UNIX) to add test for UNIX only.
xref client.partial_module_map test at line 1776.

[bruening, 2015-11-18 14:41:48]

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist, instr,
src, reg_target);
What is the plan for solving this problem in clients?  A client cannot call this
routine.

[ampotos, 2015-11-18 20:18:50]

On 2015/11/18 14:41:48, bruening wrote:
> https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> File core/lib/instrument.c (right):
> 
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist, instr,
> src, reg_target);
> What is the plan for solving this problem in clients?  A client cannot call
this
> routine.

I'm busy this week and next week, I will finish the test and the fix after that.

For fixing the problem in clients I don't really have a proposition for now.

Maybe a new API function is needed, something who just select a register and
call mangle_seg_ref_opnd ? But with the client have to check if a segment
reference is used in an instruction it want to load, maybe there another way
who make things easier for clients.

[ampotos, 2015-12-01 14:23:26]

On 2015/11/18 20:18:50, ampotos wrote:
> On 2015/11/18 14:41:48, bruening wrote:
> > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > File core/lib/instrument.c (right):
> > 
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist,
instr,
> > src, reg_target);
> > What is the plan for solving this problem in clients?  A client cannot call
> this
> > routine.
> 
> I'm busy this week and next week, I will finish the test and the fix after
that.
> 
> For fixing the problem in clients I don't really have a proposition for now.
> 
> Maybe a new API function is needed, something who just select a register and
> call mangle_seg_ref_opnd ? But with the client have to check if a segment
> reference is used in an instruction it want to load, maybe there another way
> who make things easier for clients.

I'm trying to make the test but I have a problem. I made a little program who
use
gs in 64 bit en fs in 32 to make some call (http://pastebin.com/4bw9j6z0).
In the client I want to check the value of the calle in the
mbr_instrumentation's
callback with the address of test_func. But when I use a client with an
empty dr_init it's segfault in 64bit and in 32bit I have :
"Internal Error: DynamoRIO debug check failure: 
/mnt/data/b/build/slave/linux-dr-package/dynamorio/core/unix/os.c:5854 
BOOLS_MATCH(to_app, os_using_app_state(dcontext))"

This happen because a try to use the same segment register than dynamoRIO, but I
didn't find an other way to do this test in a portable way. (I know gs is used
for
vsyscall in 32bit but I don't thing check the callee addr with a hardcoded
address
is a good idea).

[zhaoqin, 2015-12-01 17:32:57]

On 2015/12/01 14:23:26, ampotos wrote:
> On 2015/11/18 20:18:50, ampotos wrote:
> > On 2015/11/18 14:41:48, bruening wrote:
> > > https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c
> > > File core/lib/instrument.c (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/273200043/diff/40001/core/lib/instrument.c#new...
> > > core/lib/instrument.c:5795: src = mangle_seg_ref_opnd(dcontext, ilist,
> instr,
> > > src, reg_target);
> > > What is the plan for solving this problem in clients?  A client cannot
call
> > this
> > > routine.
> > 
> > I'm busy this week and next week, I will finish the test and the fix after
> that.
> > 
> > For fixing the problem in clients I don't really have a proposition for now.
> > 
> > Maybe a new API function is needed, something who just select a register and
> > call mangle_seg_ref_opnd ? But with the client have to check if a segment
> > reference is used in an instruction it want to load, maybe there another way
> > who make things easier for clients.
> 
> I'm trying to make the test but I have a problem. I made a little program who
> use
> gs in 64 bit en fs in 32 to make some call (http://pastebin.com/4bw9j6z0).
> In the client I want to check the value of the calle in the
> mbr_instrumentation's
> callback with the address of test_func. But when I use a client with an
> empty dr_init it's segfault in 64bit and in 32bit I have :
> "Internal Error: DynamoRIO debug check failure: 
> /mnt/data/b/build/slave/linux-dr-package/dynamorio/core/unix/os.c:5854 
> BOOLS_MATCH(to_app, os_using_app_state(dcontext))"
> 
> This happen because a try to use the same segment register than dynamoRIO, but
I
> didn't find an other way to do this test in a portable way. (I know gs is used
> for
> vsyscall in 32bit but I don't thing check the callee addr with a hardcoded
> address
> is a good idea).

It looks to me a potential bug in DynamoRIO.
It is rare that gs in 64-bit or fs in 32-bit is used in real world application,
so it is possible some bugs there handling them correctly.

I would suggest:
1. file an issue with details of how to reproduce that bug
2. simply the test to test fs in 64-bit and gs in 32 only first, add comment
FIXME about adding other test later.

[ampotos, 2015-12-02 16:12:25]

Commit log for latest patchset:
---------------
i#1823 dr_insert_mbr_instrumentation() of gs:

remove "opnd_get_segment(src) == DR_SEG_GS" because
mangle_seg_ref_opnd already check for the presence of gs or fs
and add test for 32 bit en 64 bit build

Fixes #1823
---------------

[zhaoqin, 2015-12-04 20:41:43]

a lot of style problems to be fixed.

https://codereview.appspot.com/273200043/diff/60001/core/arch/arch.h
File core/arch/arch.h (right):

https://codereview.appspot.com/273200043/diff/60001/core/arch/arch.h#newcode540
core/arch/arch.h:540: #ifdef UNIX
nit: # ifdef UNIX

https://codereview.appspot.com/273200043/diff/60001/core/arch/arch.h#newcode545
core/arch/arch.h:545: #endif
# endif

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c
File core/lib/instrument.c (left):

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c#old...
core/lib/instrument.c:5788: reg_target = REG_XCX; /* we use movzx below */
hmm, do we adjust the reg_target?
Maybe not, the only case is if sz is OPSZ_4 in x64, could it happen?

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c#new...
core/lib/instrument.c:5745: /*
nit, start real comment from this line.

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c#new...
core/lib/instrument.c:5749: for (reg_target = REG_XAX; reg_target <= REG_XBX;
reg_target++)
style, use {} for multiline body

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c#new...
core/lib/instrument.c:5797: #ifdef UNIX
nit, should be # ifdef UNIX

https://codereview.appspot.com/273200043/diff/60001/core/lib/instrument.c#new...
core/lib/instrument.c:5801: #endif
ditto

https://codereview.appspot.com/273200043/diff/60001/suite/tests/CMakeLists.txt
File suite/tests/CMakeLists.txt (right):

https://codereview.appspot.com/273200043/diff/60001/suite/tests/CMakeLists.tx...
suite/tests/CMakeLists.txt:1778: if (UNIX AND NOT ARM) #FIXME when i#1833 is
fixed uncomment code in mbr_instrumentation_segment.dll.c
style, max line wide: 90

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
File suite/tests/client-interface/mbr_instrumentation_segment.c (right):

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:1: #include <stdio.h>
need license header

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:53: * FIXME i#1833 :
the following code with gs don't run properly with dynamoRIO
i#1833:
doesn't
DynamoRIO.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:54: * when it will,
uncomment the following code to add test for gs segment
Disable the following code for gs segment test.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:63: /*               
    : : "m" (val) : "eax", "ebx"); */
we do not have comment out code, use if 0 if we have to.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:65: __asm__ volatile
("mov %0, %%fs; \n\
Xref vbjmp-rac-test.c about writting inline asm in DR.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:85: * FIXME i#18233 :
Actually only fs is test because gs is used by dynamoRIO
18233?

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:92: /*
arch_prctl(ARCH_SET_GS, (unsigned long)funcs); */
ditto about comment code.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
File suite/tests/client-interface/mbr_instrumentation_segment.dll.c (right):

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:1: #include
"dr_api.h"
need license header

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:7: static void
mbr_instru_test(app_pc __attribute__((unused))caller, app_pc callee)
I would prfer
void static
mbr_instru_test...

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:9: if (func_pc &&
callee == func_pc)
style indentation 4 instead of 2.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:13: static
dr_emit_flags_t bb_event(void *drcontext, void __attribute__((unused))*tag,
ditto

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:54: DR_EXPORT
void dr_init(client_id_t __attribute__((unused))id)
we want to use dr_client_main for new code.
Do we need __attribute__((unused))?

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:57:
dr_register_exit_event(&exit_event);
we want to use drmgr in sample, but I guess it is ok to use dr's event directly.

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
File suite/tests/client-interface/mbr_instrumentation_segment.expect (right):

https://codereview.appspot.com/273200043/diff/60001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.expect:1: Call to
test_func
need license header

[ampotos, 2015-12-07 16:34:09]

Commit log for latest patchset:
---------------
i#1823 dr_insert_mbr_instrumentation() of gs

Add segment mangling on mbr_instrumentation for gs and fs.

Fixes #1823
---------------

[ampotos, 2015-12-07 16:36:13]

On 2015/12/07 16:34:09, ampotos wrote:
> Commit log for latest patchset:
> ---------------
> i#1823 dr_insert_mbr_instrumentation() of gs
> 
> Add segment mangling on mbr_instrumentation for gs and fs.
> 
> Fixes #1823
> ---------------

I used __attribute__((unused)) because I compile with gcc 5.2 and there is a
warning without it.
The licence header is absent on all present .expect file so i didn't add it.

[zhaoqin, 2015-12-10 21:21:21]

a few more style problems, but I do not think we need another round of review,
so LGTM.
Please fix those style problems and perform full pre-commit test on both Windows
and Linux before commit.

https://github.com/DynamoRIO/dynamorio/wiki/Commit-Criteria

https://codereview.appspot.com/273200043/diff/80001/core/arch/arch.h
File core/arch/arch.h (right):

https://codereview.appspot.com/273200043/diff/80001/core/arch/arch.h#newcode541
core/arch/arch.h:541: /* Mangle reference of fs/gs semgents, reg must not be
used in the oldop */
style, . at the end to complete the sentence.

https://codereview.appspot.com/273200043/diff/80001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/273200043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5745: /* Is possible for mbr instruction to use XCX
register, so we have
It is

https://codereview.appspot.com/273200043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5746: * to use an unsed register
. at the end.

https://codereview.appspot.com/273200043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5793: reg_target = reg_target + (DR_REG_START_32 -
DR_REG_START_64);
you may want to use reg_64_to_32() insetad.

https://codereview.appspot.com/273200043/diff/80001/suite/tests/CMakeLists.txt
File suite/tests/CMakeLists.txt (right):

https://codereview.appspot.com/273200043/diff/80001/suite/tests/CMakeLists.tx...
suite/tests/CMakeLists.txt:1779: #FIXME when i#1833 is fixed uncomment code in
mbr_instrumentation_segment.dll.c
uncomment => enable

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
File suite/tests/client-interface/mbr_instrumentation_segment.c (right):

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:2: * Copyright (c)
2011-2015 Google, Inc.  All rights reserved.
2015 would be enough

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:74: /* if a 32 bit
program run on a 64 bit system, the first free gdt slot is 12
If

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:121: * (for example
to store the canary)
.

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.c:123: * the test for
gs
. at the end.

https://github.com/DynamoRIO/dynamorio/wiki/Code-Style
xref code style, either complete sentence properly capitalized and punctuated,
or in-complete sentence, not in the middle.

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
File suite/tests/client-interface/mbr_instrumentation_segment.dll.c (right):

https://codereview.appspot.com/273200043/diff/80001/suite/tests/client-interf...
suite/tests/client-interface/mbr_instrumentation_segment.dll.c:2: * Copyright
(c) 2011-2015 Google, Inc.  All rights reserved.
2015

[bruening, 2015-12-10 23:03:12]

On 2015/12/07 16:34:09, ampotos wrote:
> Fixes #1823

We split the general issue into #1834, so this line is now accurate.

Please put a XXX xref to i#1834 in a comment in the code that we would like a
more general solution.

[ampotos, 2015-12-28 21:47:44]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/b4cf375c45faa4b33fbdf0fc5414f50...

Final commit log: 
---------------
i#1823 dr_insert_mbr_instrumentation() of gs

Add segment mangling on mbr_instrumentation for gs and fs.

Fixes #1823
---------------

[ampotos, 2016-01-03 14:25:47]

On 2015/12/28 21:47:44, ampotos wrote:
> Committed as
>
https://github.com/DynamoRIO/dynamorio/commit/b4cf375c45faa4b33fbdf0fc5414f50...
> 
> Final commit log: 
> ---------------
> i#1823 dr_insert_mbr_instrumentation() of gs
> 
> Add segment mangling on mbr_instrumentation for gs and fs.
> 
> Fixes #1823
> ---------------

I think there was a problem with the commit, I didn't have commit access on the
repot when I used dcommit.

[zhaoqin, 2016-01-04 17:40:22]

I just added ampotos in the repository, please try again if you can commit
the CL.

On Sun, Jan 3, 2016 at 9:25 AM, <ampotos@gmail.com> wrote:

> On 2015/12/28 21:47:44, ampotos wrote:
>
>> Committed as
>>
>
>
>
https://github.com/DynamoRIO/dynamorio/commit/b4cf375c45faa4b33fbdf0fc5414f50...
>
> Final commit log:
>> ---------------
>> i#1823 dr_insert_mbr_instrumentation() of gs
>>
>
> Add segment mangling on mbr_instrumentation for gs and fs.
>>
>
> Fixes #1823
>> ---------------
>>
>
> I think there was a problem with the commit, I didn't have commit access
> on the repot when I used dcommit.
>
> https://codereview.appspot.com/273200043/
>

[ampotos, 2016-01-04 18:20:07]

On 2016/01/04 17:40:22, zhaoqin wrote:
> I just added ampotos in the repository, please try again if you can commit
> the CL.
> 
> On Sun, Jan 3, 2016 at 9:25 AM, <mailto:ampotos@gmail.com> wrote:
> 
> > On 2015/12/28 21:47:44, ampotos wrote:
> >
> >> Committed as
> >>
> >
> >
> >
>
https://github.com/DynamoRIO/dynamorio/commit/b4cf375c45faa4b33fbdf0fc5414f50...
> >
> > Final commit log:
> >> ---------------
> >> i#1823 dr_insert_mbr_instrumentation() of gs
> >>
> >
> > Add segment mangling on mbr_instrumentation for gs and fs.
> >>
> >
> > Fixes #1823
> >> ---------------
> >>
> >
> > I think there was a problem with the commit, I didn't have commit access
> > on the repot when I used dcommit.
> >
> > https://codereview.appspot.com/273200043/
> >

It's commit.

[bruening, 2016-01-08 21:42:21]

This broke the Mac build:

debug-internal-32: **** 4 build errors ****
/.../dynamorio/suite/tests/client-interface/mbr_instrumentation_segment.c:36:11:
fatal error: 'linux/unistd.h' file not found


On Mon, Jan 4, 2016 at 1:20 PM, <ampotos@gmail.com> wrote:

> On 2016/01/04 17:40:22, zhaoqin wrote:
>
>> I just added ampotos in the repository, please try again if you can
>>
> commit
>
>> the CL.
>>
>
> On Sun, Jan 3, 2016 at 9:25 AM, <mailto:ampotos@gmail.com> wrote:
>>
>
> > On 2015/12/28 21:47:44, ampotos wrote:
>> >
>> >> Committed as
>> >>
>> >
>> >
>> >
>>
>
>
>
https://github.com/DynamoRIO/dynamorio/commit/b4cf375c45faa4b33fbdf0fc5414f50...
>
>> >
>> > Final commit log:
>> >> ---------------
>> >> i#1823 dr_insert_mbr_instrumentation() of gs
>> >>
>> >
>> > Add segment mangling on mbr_instrumentation for gs and fs.
>> >>
>> >
>> > Fixes #1823
>> >> ---------------
>> >>
>> >
>> > I think there was a problem with the commit, I didn't have commit
>>
> access
>
>> > on the repot when I used dcommit.
>> >
>> > https://codereview.appspot.com/273200043/
>> >
>>
>
> It's commit.
>
> https://codereview.appspot.com/273200043/
>

[bruening, 2016-01-08 21:56:21]

Also, there are numerous style violations (indentation is off in both the
cmake and C files).

On Fri, Jan 8, 2016 at 4:42 PM, Derek Bruening <bruening@google.com> wrote:

> This broke the Mac build:
>
> debug-internal-32: **** 4 build errors ****
>
/.../dynamorio/suite/tests/client-interface/mbr_instrumentation_segment.c:36:11:
> fatal error: 'linux/unistd.h' file not found
>
>
> On Mon, Jan 4, 2016 at 1:20 PM, <ampotos@gmail.com> wrote:
>
>> On 2016/01/04 17:40:22, zhaoqin wrote:
>>
>>> I just added ampotos in the repository, please try again if you can
>>>
>> commit
>>
>>> the CL.
>>>
>>
>> On Sun, Jan 3, 2016 at 9:25 AM, <mailto:ampotos@gmail.com> wrote:
>>>
>>
>> > On 2015/12/28 21:47:44, ampotos wrote:
>>> >
>>> >> Committed as
>>> >>
>>> >
>>> >
>>> >
>>>
>>
>>
>>
https://github.com/DynamoRIO/dynamorio/commit/b4cf375c45faa4b33fbdf0fc5414f50...
>>
>>> >
>>> > Final commit log:
>>> >> ---------------
>>> >> i#1823 dr_insert_mbr_instrumentation() of gs
>>> >>
>>> >
>>> > Add segment mangling on mbr_instrumentation for gs and fs.
>>> >>
>>> >
>>> > Fixes #1823
>>> >> ---------------
>>> >>
>>> >
>>> > I think there was a problem with the commit, I didn't have commit
>>>
>> access
>>
>>> > on the repot when I used dcommit.
>>> >
>>> > https://codereview.appspot.com/273200043/
>>> >
>>>
>>
>> It's commit.
>>
>> https://codereview.appspot.com/273200043/
>>
>
>