i#1551 Port DynamoRIO to ARM: tls

Commit log for first patchset:
---------------
i#1551 Port DynamoRIO to ARM: tls
---------------

[zhaoqin, 2015-02-27 22:22:14]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: tls

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-02 06:02:09]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-02 19:34:00]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-02 19:37:42]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[bruening, 2015-03-02 19:45:09]

Most comments are grammar and spelling errors.  There is only one significant
issue I see: the early fake tls should be able to be removed completely, right?

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils.c
File core/arch/arm/emit_utils.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils....
core/arch/arm/emit_utils.c:477: /* ldr dr_reg_stolen, [reg_base,
DR_TLS_BASE_OFFSET */
missing ]

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils....
core/arch/arm/emit_utils.c:488: */
Shouldn't it also say that we set up the stolen reg?

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils....
core/arch/arm/emit_utils.c:641: /* FIXME i#1551: how should we handle register
pc? */
This comment can be removed

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/instr.c
File core/arch/arm/instr.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/instr.c#new...
core/arch/arm/instr.c:705: if (!opnd_is_immed_int(opnd) ||
opnd_get_immed_int(opnd) != 15)
This "15" magic number is in several places.  How about a named constant?

https://codereview.appspot.com/205420043/diff/40001/core/options.c
File core/options.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/options.c#newcode1876
core/options.c:1876: /* to make DR works in gdb, we must use private loader to
make
grammar: s/works/work/

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h
File core/optionsx.h (right):

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode538
core/optionsx.h:538: * On X86, we use -mangle_app_seg to contol if we will steal
app's TLS.
spelling

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode544
core/optionsx.h:544: * On ARM, we want to steal app's TLS for simmilar reason
(better transparaency).
spelling

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode545
core/optionsx.h:545: * In additiona, monitoring app's TLS is easier (we only
need mangle simple
spelling

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode545
core/optionsx.h:545: * In additiona, monitoring app's TLS is easier (we only
need mangle simple
grammar: s/mon/because mon/

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode546
core/optionsx.h:546: * thread register read instruction) and more robust (fewer
assumption about
assumptions

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode548
core/optionsx.h:548: * app's TLS, so no option is needed.
Also, we cannot easily handle raw threads created without CLONE_SETTLS without
stealing TLS

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c#newcod...
core/unix/loader.c:1065: /* thread contol blokc header type from
spelling

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c#newcod...
core/unix/loader.c:1105: byte data[0x450];
Add comment explaining these magic byte array amounts (sthg like the comment
below on line 1189)

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c#newcod...
core/unix/loader.c:1189: /* GDB read some pthread members (e.g., pid, tid), so
we must make sure
grammar: s/read/reads/

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1306
core/unix/os.c:1306: *dr_tls_base_addr == APP_TLS_VAL_EXITED)
So this is all unchanged?  We still store the DR TLS base in a slot at an offset
from the address stored in tpidro?  This is still a good way to check whether
our TLS is initialized (has to return false for new thread before we set up its
DR TLS)?

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1306
core/unix/os.c:1306: *dr_tls_base_addr == APP_TLS_VAL_EXITED)
Should APP_TLS_VAL_EXITED be renamed?

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1597
core/unix/os.c:1597: /* initialization for tls mangling (-mangle_app_seg on
x86),
grammar: s/,/./ and add capitalization per style guide

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1687
core/unix/os.c:1687: /* get application's GS/FS segment base before being
replaced by DR. */
Comment is a little misleading: this call is key for setting up our own seg, not
just about "get GS/FS segment base"

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1931
core/unix/os.c:1931: if (IF_X86_ELSE(INTERNAL_OPTION(mangle_app_seg), true)) {
I've seen this a few times: how about making a define MACHINE_TLS_IS_DR_TLS or
sthg?

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1937
core/unix/os.c:1937: #ifdef CLIENT_INTERFACE
Can you ensure a non-CI build hits an up-front error b/c w/o having
mangle_app_seg option it will just fail, right?  b/c the segment setup is all
inside CI defines?

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode5566
core/unix/os.c:5566: os_tls->os_seg_info.priv_lib_tls_base) == 0;
So this is used around thread creation

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls.h
File core/unix/tls.h (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls.h#newcode298
core/unix/tls.h:298: #endif
/* X86 */

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c
File core/unix/tls_linux_arm.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:72: os_tls->tls_type = TLS_TYPE_SWAP;
Is TLS_TYPE_SWAP still an accurate name?

Certainly the comment in tls.h for this enum value needs to be updated.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:87: * os_tls->app_tls_swap for the case of detach but
we need some way to
Doesn't this need to be updated?

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:103: * and it will keep working seamlessly. 
Strangely, tpidrro is not zero
This seems wrong now: app pthread's syscall will be converted to just storing
the base.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:107: static byte early_app_fake_tls[16];
It's no longer fake app tls, it's preliminary priv tls.  But why do we need this
now?  Won't privload_tls_init() create something for us?  Can't we remove this
whole early fake tls?

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:110: * this set up prioor to creating any threads.
?  It was spelled right before!

[zhaoqin, 2015-03-02 23:36:28]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-02 23:36:59]

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils.c
File core/arch/arm/emit_utils.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils....
core/arch/arm/emit_utils.c:477: /* ldr dr_reg_stolen, [reg_base,
DR_TLS_BASE_OFFSET */
On 2015/03/02 19:45:08, bruening wrote:
> missing ]

Done.

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils....
core/arch/arm/emit_utils.c:488: */
On 2015/03/02 19:45:07, bruening wrote:
> Shouldn't it also say that we set up the stolen reg?

Done.

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/emit_utils....
core/arch/arm/emit_utils.c:641: /* FIXME i#1551: how should we handle register
pc? */
On 2015/03/02 19:45:07, bruening wrote:
> This comment can be removed

Done.

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/instr.c
File core/arch/arm/instr.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/arch/arm/instr.c#new...
core/arch/arm/instr.c:705: if (!opnd_is_immed_int(opnd) ||
opnd_get_immed_int(opnd) != 15)
On 2015/03/02 19:45:08, bruening wrote:
> This "15" magic number is in several places.  How about a named constant?

Done.

https://codereview.appspot.com/205420043/diff/40001/core/options.c
File core/options.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/options.c#newcode1876
core/options.c:1876: /* to make DR works in gdb, we must use private loader to
make
On 2015/03/02 19:45:08, bruening wrote:
> grammar: s/works/work/

Done.

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h
File core/optionsx.h (right):

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode538
core/optionsx.h:538: * On X86, we use -mangle_app_seg to contol if we will steal
app's TLS.
On 2015/03/02 19:45:08, bruening wrote:
> spelling

Done.

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode544
core/optionsx.h:544: * On ARM, we want to steal app's TLS for simmilar reason
(better transparaency).
On 2015/03/02 19:45:08, bruening wrote:
> spelling

Done.

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode545
core/optionsx.h:545: * In additiona, monitoring app's TLS is easier (we only
need mangle simple
On 2015/03/02 19:45:08, bruening wrote:
> spelling

Done.

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode545
core/optionsx.h:545: * In additiona, monitoring app's TLS is easier (we only
need mangle simple
On 2015/03/02 19:45:08, bruening wrote:
> grammar: s/mon/because mon/

Done.

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode546
core/optionsx.h:546: * thread register read instruction) and more robust (fewer
assumption about
On 2015/03/02 19:45:08, bruening wrote:
> assumptions

Done.

https://codereview.appspot.com/205420043/diff/40001/core/optionsx.h#newcode548
core/optionsx.h:548: * app's TLS, so no option is needed.
On 2015/03/02 19:45:08, bruening wrote:
> Also, we cannot easily handle raw threads created without CLONE_SETTLS without
> stealing TLS

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c#newcod...
core/unix/loader.c:1065: /* thread contol blokc header type from
On 2015/03/02 19:45:08, bruening wrote:
> spelling

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c#newcod...
core/unix/loader.c:1105: byte data[0x450];
On 2015/03/02 19:45:08, bruening wrote:
> Add comment explaining these magic byte array amounts (sthg like the comment
> below on line 1189)

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/loader.c#newcod...
core/unix/loader.c:1189: /* GDB read some pthread members (e.g., pid, tid), so
we must make sure
On 2015/03/02 19:45:08, bruening wrote:
> grammar: s/read/reads/

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1306
core/unix/os.c:1306: *dr_tls_base_addr == APP_TLS_VAL_EXITED)
On 2015/03/02 19:45:08, bruening wrote:
> Should APP_TLS_VAL_EXITED be renamed?

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1306
core/unix/os.c:1306: *dr_tls_base_addr == APP_TLS_VAL_EXITED)
On 2015/03/02 19:45:08, bruening wrote:
> So this is all unchanged?  We still store the DR TLS base in a slot at an
offset
> from the address stored in tpidro?  This is still a good way to check whether
> our TLS is initialized (has to return false for new thread before we set up
its
> DR TLS)?

I think so, or we can check if the TLS base is dynamo's memory?

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1597
core/unix/os.c:1597: /* initialization for tls mangling (-mangle_app_seg on
x86),
On 2015/03/02 19:45:08, bruening wrote:
> grammar: s/,/./ and add capitalization per style guide

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1687
core/unix/os.c:1687: /* get application's GS/FS segment base before being
replaced by DR. */
On 2015/03/02 19:45:09, bruening wrote:
> Comment is a little misleading: this call is key for setting up our own seg,
not
> just about "get GS/FS segment base"

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1931
core/unix/os.c:1931: if (IF_X86_ELSE(INTERNAL_OPTION(mangle_app_seg), true)) {
On 2015/03/02 19:45:08, bruening wrote:
> I've seen this a few times: how about making a define MACHINE_TLS_IS_DR_TLS or
> sthg?

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1937
core/unix/os.c:1937: #ifdef CLIENT_INTERFACE
On 2015/03/02 19:45:08, bruening wrote:
> Can you ensure a non-CI build hits an up-front error b/c w/o having
> mangle_app_seg option it will just fail, right?  b/c the segment setup is all
> inside CI defines?

Done.
add on in tls_linux_arm.c

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode5566
core/unix/os.c:5566: os_tls->os_seg_info.priv_lib_tls_base) == 0;
On 2015/03/02 19:45:09, bruening wrote:
> So this is used around thread creation

Yes, in handle_clone or something, I do not remember why we need switch back to
APP's TLS, but more likely gdb will crash without switch or something

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls.h
File core/unix/tls.h (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls.h#newcode298
core/unix/tls.h:298: #endif
On 2015/03/02 19:45:09, bruening wrote:
> /* X86 */

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c
File core/unix/tls_linux_arm.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:72: os_tls->tls_type = TLS_TYPE_SWAP;
On 2015/03/02 19:45:09, bruening wrote:
> Is TLS_TYPE_SWAP still an accurate name?
> 
> Certainly the comment in tls.h for this enum value needs to be updated.

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:87: * os_tls->app_tls_swap for the case of detach but
we need some way to
On 2015/03/02 19:45:09, bruening wrote:
> Doesn't this need to be updated?

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:103: * and it will keep working seamlessly. 
Strangely, tpidrro is not zero
On 2015/03/02 19:45:09, bruening wrote:
> This seems wrong now: app pthread's syscall will be converted to just storing
> the base.

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:107: static byte early_app_fake_tls[16];
On 2015/03/02 19:45:09, bruening wrote:
> It's no longer fake app tls, it's preliminary priv tls.  But why do we need
this
> now?  Won't privload_tls_init() create something for us?  Can't we remove this
> whole early fake tls?

Done.

https://codereview.appspot.com/205420043/diff/40001/core/unix/tls_linux_arm.c...
core/unix/tls_linux_arm.c:110: * this set up prioor to creating any threads.
On 2015/03/02 19:45:09, bruening wrote:
> ?  It was spelled right before!

Done.

[bruening, 2015-03-03 05:11:12]

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/205420043/diff/40001/core/unix/os.c#newcode1306
core/unix/os.c:1306: *dr_tls_base_addr == APP_TLS_VAL_EXITED)
On 2015/03/02 23:36:58, zhaoqin wrote:
> On 2015/03/02 19:45:08, bruening wrote:
> > So this is all unchanged?  We still store the DR TLS base in a slot at an
> offset
> > from the address stored in tpidro?  This is still a good way to check
whether
> > our TLS is initialized (has to return false for new thread before we set up
> its
> > DR TLS)?
> 
> I think so, or we can check if the TLS base is dynamo's memory?

No, that leads to an infinite loop: I know b/c there used to be such a check
here and I had to remove it.  See the comment below which I put there to prevent
anyone from adding such a check!

[bruening, 2015-03-03 05:30:03]

The rebase makes it hard to review the changes since patchset 3: could you post
a version w/o the rebase?

[zhaoqin, 2015-03-03 19:00:57]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-03 19:07:24]

Reviewer: bruening@google.com
using upload for review

[zhaoqin, 2015-03-03 19:08:42]

Reviewer: bruening@google.com
using upload for review

i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043

[bruening, 2015-03-03 19:22:49]

LGTM

https://codereview.appspot.com/205420043/diff/120002/core/optionsx.h
File core/optionsx.h (right):

https://codereview.appspot.com/205420043/diff/120002/core/optionsx.h#newcode549
core/optionsx.h:549: * raw threads created without CLONE_SETTLS without stealig
TLS.
spelling

https://codereview.appspot.com/205420043/diff/120002/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/205420043/diff/120002/core/unix/os.c#newcode5563
core/unix/os.c:5563: byte **app_lib_tls_swap_slot = (byte **)
This needs a comment as it's not clear why you need to clobber the app's memory.

https://codereview.appspot.com/205420043/diff/120002/core/unix/os.c#newcode5572
core/unix/os.c:5572: byte **app_lib_tls_swap_slot = (byte **)
Ditto

[zhaoqin, 2015-03-03 20:19:36]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-03 20:26:15]

Commit log for latest patchset:
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------

[zhaoqin, 2015-03-03 20:55:18]

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

Final commit log: 
---------------
i#1551 Port DynamoRIO to ARM: add private loader TLS support on ARM
- remove old implementation for APP_TLS_SWAP
- implement private loader TLS setup
- implement TLS mangling
- implement TLS switch between app and DR

Review-URL: https://codereview.appspot.com/205420043
---------------