Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(58)

Issue 314160043: TBR: i#2089 TLS init: use a magic field to indicate init

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 4 months ago by bruening
Modified:
7 years, 4 months ago
Reviewers:
zhaoqin
CC:
dynamorio-devs_googlegroups.com
Visibility:
Public.

Description

Commit log for first patchset: --------------- i#2089 TLS init: use a magic field to indicate init Puts in a new scheme for is_thread_tls_initialized(): rather than reading thread id fields and comparing to expensive syscalls, and trying to distinguish different fork and clone children, we instead use a very simple approach. We add a magic number field to our TLS and is_thread_tls_initialized() simply does a safe read of that field: if it equals our magic number, it's initialized. On a clone, the parent swaps to a separate private TLS with an invalid magic field. The child inherits this and thus starts out correctly uninitialized, while the parent's syscall gencode still works as it ignores the magic field. The parent's TLS is restored first thing in dispatch(). On a fork, nothing is done, and thus the magic number remains and the child is correctly initialized. We avoid safe read faults on thread exit by pointing the exiting thread's TLS at a fake TLS with an invalid magic number. This is efficient for regular threads and required for threads missing CLONE_SIGHAND, for which we must also skip the segment zeroing at thread exit. On swapping to native, we also put in the private TLS. On re-take-over, we add new queries (including using a special "invalid" magic number to distinguish from a completely unknown thread) and TLS swaps to several attach points to avoid thinking that a NULL dcontext or !is_thread_tls_initialized() means an unknown thread. Generalizes the segment swapping code to correctly swap DR's segment too instead of what it did before where it took in a segment parameter but then hardcoded the register to use. The app using the aux segment is not yet supported: that's i#2088/i#107. The new scheme is under the -safe_read_tls_init option, which is now on by default. Once we're sure it's stable we'll remove the option and the old code. Adds a new test linux.clone-pie, a PIE build of linux.clone, which forces the use of the MSR for TLS instead of the GDT and results in a crash with -no_safe_read_tls_init. Expands the linux.clone* tests to also test a thread without CLONE_SIGHAND. Fixes #2089 ---------------

Patch Set 1 #

Patch Set 2 : Committed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+390 lines, -111 lines) Patch
M core/arch/emit_utils_shared.c View 1 chunk +12 lines, -10 lines 0 comments Download
M core/arch/x86/x86.asm View 1 chunk +5 lines, -5 lines 0 comments Download
M core/arch/x86_code.c View 1 chunk +7 lines, -0 lines 0 comments Download
M core/dispatch.c View 1 chunk +7 lines, -0 lines 0 comments Download
M core/dynamo.c View 1 chunk +0 lines, -2 lines 0 comments Download
M core/optionsx.h View 1 chunk +4 lines, -5 lines 0 comments Download
M core/os_shared.h View 1 chunk +6 lines, -0 lines 0 comments Download
M core/synch.c View 1 chunk +2 lines, -1 line 0 comments Download
M core/unix/os.c View 28 chunks +265 lines, -62 lines 0 comments Download
M core/unix/os_asm_defines.asm View 1 chunk +6 lines, -6 lines 0 comments Download
M core/unix/os_exports.h View 1 chunk +3 lines, -0 lines 0 comments Download
M core/unix/os_private.h View 4 chunks +6 lines, -2 lines 0 comments Download
M core/unix/signal.c View 1 chunk +1 line, -1 line 0 comments Download
M core/unix/tls.h View 2 chunks +16 lines, -8 lines 0 comments Download
M core/unix/tls_linux_x86.c View 1 chunk +1 line, -0 lines 0 comments Download
M core/win32/os.c View 1 chunk +7 lines, -0 lines 0 comments Download
M suite/tests/CMakeLists.txt View 1 chunk +5 lines, -0 lines 0 comments Download
M suite/tests/linux/clone.c View 6 chunks +23 lines, -9 lines 0 comments Download
M suite/tests/linux/clone.expect View 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 2
bruening
7 years, 4 months ago (2016-12-07 21:18:33 UTC) #1
bruening
7 years, 4 months ago (2016-12-07 21:18:36 UTC) #2
Committed as
https://github.com/DynamoRIO/dynamorio/commit/184dd8b8b053975cea28659c02a0b4f...

Final commit log: 
---------------
i#2089 TLS init: use a magic field to indicate init

Puts in a new scheme for is_thread_tls_initialized(): rather than reading
thread id fields and comparing to expensive syscalls, and trying to
distinguish different fork and clone children, we instead use a very simple
approach.  We add a magic number field to our TLS and
is_thread_tls_initialized() simply does a safe read of that field: if it
equals our magic number, it's initialized.

On a clone, the parent swaps to a separate private TLS with an invalid
magic field.  The child inherits this and thus starts out correctly
uninitialized, while the parent's syscall gencode still works as it ignores
the magic field.  The parent's TLS is restored first thing in dispatch().
On a fork, nothing is done, and thus the magic number remains and the child
is correctly initialized.

We avoid safe read faults on thread exit by pointing the exiting thread's
TLS at a fake TLS with an invalid magic number.  This is efficient for regular
threads and required for threads missing CLONE_SIGHAND, for which we must
also skip the segment zeroing at thread exit.

On swapping to native, we also put in the private TLS.  On re-take-over, we
add new queries (including using a special "invalid" magic number to
distinguish from a completely unknown thread) and TLS swaps to several
attach points to avoid thinking that a NULL dcontext or
!is_thread_tls_initialized() means an unknown thread.

Generalizes the segment swapping code to correctly swap DR's segment too
instead of what it did before where it took in a segment parameter but then
hardcoded the register to use.

The app using the aux segment is not yet supported: that's i#2088/i#107.

The new scheme is under the -safe_read_tls_init option, which is now on by
default.  Once we're sure it's stable we'll remove the option and the old
code.

Adds a new test linux.clone-pie, a PIE build of linux.clone, which forces
the use of the MSR for TLS instead of the GDT and results in a crash with
-no_safe_read_tls_init.

Expands the linux.clone* tests to also test a thread without CLONE_SIGHAND.

Fixes #2089

Review-URL: https://codereview.appspot.com/314160043
---------------
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b