Mac x86_64: Mach exception support

Mac x86-64: Mach exception support.

This extends Mach exception support to x86-64 on Mac. The Mach exception
handler maps threads from their Mach port number in the local (task/process)
port namespace to their Native Client thread index by consulting a new Mach
thread map which is maintained on x86-64 as Native Client threads come in and
out of existence.

Additional changes:
 - Provides an x86-64 implementation of NaClMachThreadIsInUntrusted.
 - On x86-64 and x86-32 alike, it now bails out on trying to handle an
   exception if no untrusted exception handler is set before setting the flag
   that declares that an untrusted exception handler has been invoked. With
   this change, all tests now pass with platform=x86-64.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3087
TEST=scons platform=x86-64 exception_tests
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=10861

[Mark Mentovai, 2013-02-13 23:11:10]

This is more of a checkpoint than something ready for checkin. Certain chunks
are pretty littered with #ifdefs, crash_filter should probably be renamed now,
and there are a couple other warts remaining. I wanted to send it around for
early feedback to get your thoughts on a few things, like:

What do you think of the mutex? The dumb loop? (Windows has a dumb loop too but
avoids the mutex by suspending the process. We can’t suspend the process on the
Mac because the Mach exception handler is in the same process. Although if on
Windows, you’re not concerned about partial stores, then maybe we don’t need to
be concerned about it on Mac either. This should be fine on x86, but I wrote
this a bit more generically.)

Is the way I’ve handled platform-specific #includes OK, or do you have a
preferred local style for this kind of thing? How about the stub implementation
of mach_thread_map for 32-bit x86? (That was an effort to reduce the #ifdef
soup.)

If the mutex is kept, is there a better initialization pattern for it? I managed
to put it in a reasonable early-init point for everything but the tests that
check early crashes, which required their own initialization. Should I
initialize it during Mach EH initialization or during the initialization of the
first NaCl thread, whichever happens first? I could track that using a
non-thread-safe static, on the assumption that at that early stage of the game,
things are still pretty serialized, but I don’t know if it’s actually safe to
assume that two NaCl threads won’t start in parallel and race.

[Mark Seaborn, 2013-02-14 00:37:53]

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_app_thread.c (right):

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_app_thread.c:26: #if NACL_OSX
I'd be inclined to put this #if into the header so that use #include sites are
tidier.

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
File src/trusted/service_runtime/osx/crash_filter.c (right):

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/crash_filter.c:41: if (!nacl_thread_index) {
Make this "if (nacl_thread_index == NACL_TLS_INDEX_INVALID)"

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
File src/trusted/service_runtime/osx/mach_exception_handler.c (right):

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_exception_handler.c:166: if
(!nacl_thread_index) {
Make this "if (nacl_thread_index == NACL_TLS_INDEX_INVALID)"

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_exception_handler.c:261: frame_addr_user =
(regs.uts.ts64.__rsp & 0xffffffff) - NACL_STACK_RED_ZONE;
Casting to uint32_t would be preferred over '& 0xffffffff'.  You can make
frame_addr_user uint32_t.

You can do "frame_addr_user -= NACL_STACK_RED_ZONE" outside the #ifs.

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_exception_handler.c:314: * Put registers in
right place to land at NaClSwitchNoSSEViaECX
This comment should be inside "#if NACL_BUILD_SUBARCH == 32"

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_thread_map.c:28:
NaClXMutexLock(&mach_threads_mu);
I'd be inclined not to use a lock for this (as on Windows), since the updates
should all be atomic.

Taking a fault is meant to be synchronising so we wouldn't need the memory
barriers implied by lock/unlock either.

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_thread_map.c:38: return nacl_thread_index
== NACL_THREAD_MAX ? 0 : nacl_thread_index;
Use NACL_TLS_INDEX_INVALID rather than 0

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
File src/trusted/service_runtime/osx/mach_thread_map.h (right):

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_thread_map.h:21: size_t
GetNaClThreadIndexForMachThread(mach_port_t mach_thread);
These should have "NaCl" prefixes, e.g. NaClGetThreadIndexForMachThread()

[Mark Mentovai, 2013-02-14 19:35:40]

I’ve made the changes you suggested, and a few more.

I think this is approaching production quality now if you’d like to do a full
review.

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/...
src/trusted/service_runtime/osx/mach_thread_map.c:28:
NaClXMutexLock(&mach_threads_mu);
Mark Seaborn wrote:
> I'd be inclined not to use a lock for this (as on Windows), since the updates
> should all be atomic.
> 
> Taking a fault is meant to be synchronising so we wouldn't need the memory
> barriers implied by lock/unlock either.

I’m dropping the lock since this is safe as used here on x86_64.

I don’t see how a fault on one thread results in any synchronization, though.

I guess you’re not concerned about looping through all slots? Presumably faults
happen infrequently enough and code that does handle them doesn’t expect them to
be all that performant.

[Mark Seaborn, 2013-02-14 22:52:44]

On 14 February 2013 11:35, <mark@chromium.org> wrote:

> https://codereview.chromium.**org/12207165/diff/12/src/**
>
trusted/service_runtime/osx/**mach_thread_map.c<https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/osx/mach_thread_map.c>
> File src/trusted/service_runtime/**osx/mach_thread_map.c (right):
>
> https://codereview.chromium.**org/12207165/diff/12/src/**
>
trusted/service_runtime/osx/**mach_thread_map.c#newcode28<https://codereview.chromium.org/12207165/diff/12/src/trusted/service_runtime/osx/mach_thread_map.c#newcode28>
> src/trusted/service_runtime/**osx/mach_thread_map.c:28:
> NaClXMutexLock(&mach_threads_**mu);
> Mark Seaborn wrote:
>
>> I'd be inclined not to use a lock for this (as on Windows), since the
>> updates
>>
> should all be atomic.
>>
>
>  Taking a fault is meant to be synchronising so we wouldn't need the memory
>>
> barriers implied by lock/unlock either.
>>
>
> I’m dropping the lock since this is safe as used here on x86_64.
>
> I don’t see how a fault on one thread results in any synchronization,
> though.
>

By "synchronisation" I meant that it implies a memory barrier.  So the
thread will do the following:

 1) Assign to an element of mach_threads[]
 2) Enter untrusted code
 3) Fault inside untrusted code

The fault at step (3) should imply a memory barrier so that the other
thread handling the fault should see the write from step (1).  Without the
implied memory barrier there wouldn't be a guarantee that the handler
thread would see the change to mach_threads[].


I guess you’re not concerned about looping through all slots? Presumably
> faults happen infrequently enough and code that does handle them doesn’t
> expect them to be all that performant.
>

Right, I'm not too worried at the moment.  I think it's better to be
consistent with what the x86-64 Windows implementation does for now.  At
some point in the future we could optimise this more, and change both Mac
and Windows versions.

If you have a small number of threads, you'll find the thread quickly
anyway, because the thread will have a small index number.

I've been gradually removing uses of the thread indexes (I have a change
out that removes a major use of them --
https://codereview.chromium.org/12218089/), and the x86-64 exception
handlers' use of them would be one of the last uses.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Mark Seaborn, 2013-02-14 23:55:02]

Can you add exception_tests to buildbot_standard.py in this change too, please?

Also say in the commit message that you're implementing
NaClMachThreadIsInUntrusted() for x86-64.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/crash_filter.h (left):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.h:1: /*
Don't delete this file, please -- it's #included from the Chrome side by
breakpad_mac.mm and so is part of our interface to Chrome.  (At some point I
want to make this interface clearer by creating a directory for public headers:
https://code.google.com/p/nativeclient/issues/detail?id=2832)

Moving the file means I'd have to change the Chrome side when rolling DEPS, and
our general policy is to avoid changes that require this.

Can you rename it back to crash_filter.{h,c}?

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_exception_handler.c (right):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:97: #define TS_xCX
TSx.__ecx
I'm not overly keen on adding macros for accessing *all* of these fields.  It
adds extra levels of indirection that the reader needs to follow to see the
underlying struct.

Maybe just add macros for fields that are common between 32-bit and 64-bit?

e.g.

#define X86_FLAGS(regs) ((regs).uts.ts32.__eflags)
#define X86_PROG_CTR(regs) ((regs).uts.ts32.__eip)

Also making this look like a function is more compositional than having a
#define for a compound field name like "ts32.__eflags".

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:199: #if
NACL_BUILD_SUBARCH == 64
It looks like you don't need this #if, and your intention was that
NaClMachThreadStateIsInUntrusted() should work for x86-32 too?

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:288: if
(nap->exception_handler == 0) {
I can see why you reordered this before checking exception_flag (though this
seems like an unrelated change that should maybe be separate?).

But I think it's better to check nap->exception_handler before looking at
natp->exception_stack.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.c:14: #if
NACL_ARCH(NACL_BUILD_ARCH) != NACL_x86 || NACL_BUILD_SUBARCH != 32
Again, probably better to say:
#if NACL_ARCH(NACL_BUILD_ARCH) == NACL_x86 && NACL_BUILD_SUBARCH = 64

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.c:37: mach_port_t mach_thread =
pthread_mach_thread_np(pthread_self());
It might be worth commenting that:

We assume that the pthread library does not close and re-acquire the Mach port
for the thread.  If that happens, Mach could assign the port a different number
in the process's port table.

I assume OS X does not provide any globally-valid thread IDs that we can use for
this, unlike on Windows and Linux?

We're also assuming that when the kernel sends the thread port to our Mach
exception handler, Mach reuses the same index for the port in the process's port
table.  (Presumably Mach doesn't have any other mechanism for comparing two
ports for equality?)

That's my understanding of this, anyway. :-)

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_thread_map.h (right):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.h:21: #if
NACL_ARCH(NACL_BUILD_ARCH) != NACL_x86 || NACL_BUILD_SUBARCH != 32
Probably better to say:
#if NACL_ARCH(NACL_BUILD_ARCH) == NACL_x86 && NACL_BUILD_SUBARCH = 64
since if we ever handled ARM here, we might use a combination of pc and r9 to
recover the thread identity.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.h:33: void
NaClSetCurrentMachThreadForThreadIndex(size_t nacl_thread_index);
Elsewhere, thread indexes are all uint32_t

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.h:53: CHECK(0);
I'd suggest omitting this function definition since it's not used on x86-32 and
doesn't work there.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.h:60: /* No-op for 32-bit x86.
*/
Nit: '32-bit x86' -> 'x86-32' (since that's the usual term in the NaCl
codebase).

[Mark Mentovai, 2013-02-15 17:36:42]

Most of your comments are uncontroversial and I’ll take care of them later. I do
have a couple more for you on cross-examination first, though.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/crash_filter.h (left):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.h:1: /*
Mark Seaborn wrote:
> Don't delete this file, please -- it's #included from the Chrome side by
> breakpad_mac.mm and so is part of our interface to Chrome.  (At some point I
> want to make this interface clearer by creating a directory for public
headers:
> https://code.google.com/p/nativeclient/issues/detail?id=2832)
> 
> Moving the file means I'd have to change the Chrome side when rolling DEPS,
and
> our general policy is to avoid changes that require this.
> 
> Can you rename it back to crash_filter.{h,c}?

This file is more than just a “crash filter” now, and I prefer not to leave old
misleading names behind. If Chrome is the only concern, I think we can handle
this easily.

What if I kept a crash_filter.h here that did nothing other than #include the
header under its new name? Then the Chromium DEPS can be rolled without having
to make any changes on the Chrome side. Once that’s done, I’ll fix the
Chrome-side #include to use the new name, and remove the transitional
crash_filter.h?

Since Chrome picks up the .cc file via the .gyp file in Native Client’s
repository, there shouldn’t be anything extra needed to account for the .cc file
changing names.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_exception_handler.c (right):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:288: if
(nap->exception_handler == 0) {
Mark Seaborn wrote:
> I can see why you reordered this before checking exception_flag (though this
> seems like an unrelated change that should maybe be separate?).
> 
> But I think it's better to check nap->exception_handler before looking at
> natp->exception_stack.

I can put this back how it was and address it in a follow-up.

Actually, I was thinking of just using “goto” to clear exception_flag for the
late failure cases, mach_vm_write and thread_set_state. I’ll just work all of
that into the follow-up.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.c:37: mach_port_t mach_thread =
pthread_mach_thread_np(pthread_self());
Mark Seaborn wrote:
> It might be worth commenting that:
> 
> We assume that the pthread library does not close and re-acquire the Mach port
> for the thread.  If that happens, Mach could assign the port a different
number
> in the process's port table.
> 
> I assume OS X does not provide any globally-valid thread IDs that we can use
for
> this, unlike on Windows and Linux?
> 
> We're also assuming that when the kernel sends the thread port to our Mach
> exception handler, Mach reuses the same index for the port in the process's
port
> table.  (Presumably Mach doesn't have any other mechanism for comparing two
> ports for equality?)
> 
> That's my understanding of this, anyway. :-)

Your understanding is correct.

As of Mac OS X 10.6, there actually is a system-wide thread ID that’s exposed to
user space. I believe that this would also work for our needs, but I wasn’t sure
if Native Client still needed to support Mac OS X 10.5. Chrome dropped 10.5
support last summer. Did Native Client too?

I have a response ready for the rest of your comment, but it might be moot
depending on the answer to this question.

[Mark Mentovai, 2013-02-15 22:30:30]

I’ve updated this per the comments, except for the questions I had earlier.

I did reintroduce crash_filter.h but left the files renamed as suggested. Let me
know if you’re OK with this.

I also haven’t touched mach_thread_map with respect to comments or switching it
to use thread IDs, because I’m waiting to hear back from you about whether
Native Client needs to support Mac OS X 10.5.

https://codereview.chromium.org/12207165/diff/21002/buildbot/buildbot_standar...
File buildbot/buildbot_standard.py (right):

https://codereview.chromium.org/12207165/diff/21002/buildbot/buildbot_standar...
buildbot/buildbot_standard.py:347: args=['exception_tests',
I could add more tests here since they do all pass now, but I’m going on the
assumption that this still isn’t desirable until there are dedicated bots per
the comment above.

https://codereview.chromium.org/12207165/diff/21002/src/trusted/service_runti...
File src/trusted/service_runtime/nacl_app_thread.c (right):

https://codereview.chromium.org/12207165/diff/21002/src/trusted/service_runti...
src/trusted/service_runtime/nacl_app_thread.c:47: #elif NACL_OSX
It occurs to me that I could get rid of the #ifdef around this by letting the
no-op stubs inlined into the header function for non-Mac in addition to
Mac-non-x86_64. I haven’t done that because the code might look a little
unseemly calling into Mach functions without obvious platform guards, but if you
like that idea, it’ll save a couple of lines.

[Mark Seaborn, 2013-02-15 22:48:08]

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/crash_filter.h (left):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.h:1: /*
On 2013/02/15 17:36:42, Mark Mentovai wrote:
> Mark Seaborn wrote:
> > Don't delete this file, please -- it's #included from the Chrome side by
> > breakpad_mac.mm and so is part of our interface to Chrome.  (At some point I
> > want to make this interface clearer by creating a directory for public
> > headers:
> > https://code.google.com/p/nativeclient/issues/detail?id=2832)
> > 
> > Moving the file means I'd have to change the Chrome side when rolling DEPS,
> > and
> > our general policy is to avoid changes that require this.
> > 
> > Can you rename it back to crash_filter.{h,c}?
> 
> This file is more than just a “crash filter” now, and I prefer not to leave
old
> misleading names behind. If Chrome is the only concern, I think we can handle
> this easily.

Hmm, the new function NaClMachThreadStateIsInUntrusted() is still just a
predicate for filtering trusted vs. untrusted crashes, so the name
crash_filter.h still seems relevant to me.  mach_thread_trusted_state.h seems
more vague, but maybe that's just me.


> What if I kept a crash_filter.h here that did nothing other than #include the
> header under its new name? Then the Chromium DEPS can be rolled without having
> to make any changes on the Chrome side. Once that’s done, I’ll fix the
> Chrome-side #include to use the new name, and remove the transitional
> crash_filter.h?

That would work technically.

But bear in mind that I would like to remove NaClMachThreadIsInUntrusted() and
Chromium's use of it relatively soon.  (See my comment at
https://code.google.com/p/nativeclient/issues/detail?id=3087#c4)

I started working on that the other day.  My work-in-progress is here:
https://codereview.chromium.org/12278019  I realised that this work-in-progress
depends on having Mac x86-64 exception handling support in first, otherwise it
will regress run_signal_handler_test for Mac x86-64.

Implementing NaClMachThreadIsInUntrusted() for x86-64 for now seems fine, since
I know it's the last piece you need to get Chrome to link when NaCl is enabled. 
But tweaking the header file that provides it might not be worthwhile.

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/14011/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.c:37: mach_port_t mach_thread =
pthread_mach_thread_np(pthread_self());
On 2013/02/15 17:36:42, Mark Mentovai wrote:
> Mark Seaborn wrote:
> > It might be worth commenting that:
> > 
> > We assume that the pthread library does not close and re-acquire the Mach
port
> > for the thread.  If that happens, Mach could assign the port a different
> > number
> > in the process's port table.
> > 
> > I assume OS X does not provide any globally-valid thread IDs that we can use
> > for
> > this, unlike on Windows and Linux?
> > 
> > We're also assuming that when the kernel sends the thread port to our Mach
> > exception handler, Mach reuses the same index for the port in the process's
> > port
> > table.  (Presumably Mach doesn't have any other mechanism for comparing two
> > ports for equality?)
> > 
> > That's my understanding of this, anyway. :-)
> 
> Your understanding is correct.
> 
> As of Mac OS X 10.6, there actually is a system-wide thread ID that’s exposed
to
> user space.

What's this interface called?

Whether it's worth using it depends on how likely it is that Apple will change
it.

As I understand it, even the Mach interfaces are not officially supported,
although they aren't likely to change too much since the Mach layer is fairly
stable (or stale :-) ).  An alternative thread ID might be more likely to be
removed in future.

> I believe that this would also work for our needs, but I wasn’t sure
> if Native Client still needed to support Mac OS X 10.5. Chrome dropped 10.5
> support last summer. Did Native Client too?

Yes, we don't need to support 10.5.

[Mark Mentovai, 2013-02-15 23:16:31]

Mark Seaborn wrote:
> On 2013/02/15 17:36:42, Mark Mentovai wrote:
> > As of Mac OS X 10.6, there actually is a system-wide thread ID that’s
exposed
> > to user space.
> 
> What's this interface called?

The two places I’d pick it up are pthread_threadid_np (<pthread.h>) and
thread_info (<mach/thread_act.h>) with flavor THREAD_IDENTIFIER_INFO and
thread_info_out of type thread_identifier_info_data_t* (<mach/thread_info.h>).

> Whether it's worth using it depends on how likely it is that Apple will change
> it.
> 
> As I understand it, even the Mach interfaces are not officially supported,
> although they aren't likely to change too much since the Mach layer is fairly
> stable (or stale :-) ).  An alternative thread ID might be more likely to be
> removed in future.

You can unfortunately make the same argument about just about any interface
Apple provides. Even those that are fully-supported find themselves on the
“deprecated” plank fairly rapidly. We’ve been treating anything that shows up in
a public header as something that Apple can’t break, even if they decide to
deprecate them.

> Yes, we don't need to support 10.5.

I’ll need to flip the -mmacosx-version-min from 10.5 to 10.6 in the root
SConstruct to support a change to thread IDs, but the thread ID map may be the
best way to go, then.

I did think of another way to implement this, although it relies on pthread
internals. We could use pthread_from_mach_thread_np to transform the exception
thread’s Mach port into a pthread_t (internally, the pthread implementation just
walks through its list of pthreads and compares Mach ports, similar to what I’ve
written but with a lock on the thread list). Given the pthread_t and the value
of nacl_thread_info_key, we can read the thread ID right out of the thread’s
thread-specific data area, even from the exception handler thread, from
nacl_thread_index_tls_offset bytes higher than the pointer value of the
pthread_t. This would be safe to do from the handler thread because the faulted
thread would be suspended. The unclean aspect of this is that it relies on
knowledge of the pthread implementation’s internals, but Native Client’s TLS
implementation for Mac x86_64 already does this, and already computes
nacl_thread_index_tls_offset
(trusted/service_runtime/arch/x86_64/nacl_tls_64.c).

I don’t think that this is actually any cleaner or better than the Mach thread
map or hypothetical thread ID map, but if there’s “Apple’s going to deprecate
the world” paranoia, this option does have the advantage of not taking advantage
of any NEW unpublished system internals, just getting in deeper with the
internals that NaCl already uses.

[Mark Seaborn, 2013-02-16 00:01:33]

On 15 February 2013 15:16, <mark@chromium.org> wrote:

> Mark Seaborn wrote:
>
>> On 2013/02/15 17:36:42, Mark Mentovai wrote:
>> > As of Mac OS X 10.6, there actually is a system-wide thread ID that’s
>> exposed
>>
>>> > to user space.
>>>
>>
>>  What's this interface called?
>>>
>>
>> The two places I’d pick it up are pthread_threadid_np (<pthread.h>) and
> thread_info (<mach/thread_act.h>) with flavor THREAD_IDENTIFIER_INFO and
> thread_info_out of type thread_identifier_info_data_t*
> (<mach/thread_info.h>).
>

That sounds good to me.

BTW, for the record, your original approach of using Mach thread ports is
OK with me too.  I think the assumption of stable port IDs is pretty safe,
I just wanted there to be a comment about it in the code.

I'll leave it up to you whether to stick with Mach ports or switch to
pthread_threadid_np().


>
>  Yes, we don't need to support 10.5.
>>
>
> I’ll need to flip the -mmacosx-version-min from 10.5 to 10.6 in the root
> SConstruct to support a change to thread IDs,


That's fine.


> but the thread ID map may be the
> best way to go, then.
>
> I did think of another way to implement this, although it relies on pthread
> internals. We could use pthread_from_mach_thread_np to transform the
> exception
> thread’s Mach port into a pthread_t (internally, the pthread
> implementation just
> walks through its list of pthreads and compares Mach ports, similar to
> what I’ve
> written but with a lock on the thread list).


Hmm, I would prefer to avoid taking locks in the Mach exception handler if
possible, because if trusted code crashes while holding the same lock, we
will deadlock and not be able to report the crash via Breakpad.

On the other hand, maybe calls like thread_get_state() already call
malloc() or take locks internally, and this battle is already lost.

It looks like pthread_from_mach_thread_np() just uses a spinlock.  If it
used pthread_mutex_lock(), it would be a little worse because
pthread_mutex_lock() can call realloc() internally (which I discovered
recently: https://code.google.com/p/nativeclient/issues/detail?id=3279),
which would mean that a crash inside malloc() could deadlock the Mach
exception handler.


Given the pthread_t and the value
> of nacl_thread_info_key, we can read the thread ID right out of the
> thread’s
> thread-specific data area, even from the exception handler thread, from
> nacl_thread_index_tls_offset bytes higher than the pointer value of the
> pthread_t. This would be safe to do from the handler thread because the
> faulted
> thread would be suspended. The unclean aspect of this is that it relies on
> knowledge of the pthread implementation’s internals, but Native Client’s
> TLS
> implementation for Mac x86_64 already does this, and already computes
> nacl_thread_index_tls_offset
> (trusted/service_runtime/arch/x86_64/nacl_tls_64.c).
>

Yes, that would be very similar to the existing assumption.  But I think
you would also be assuming that %gs points to the start of pthread_t,
wouldn't you?

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Mark Mentovai, 2013-02-19 17:09:42]

Ready for another look. All feedback incorporated.

[Mark Seaborn, 2013-02-20 18:33:17]

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
File src/trusted/service_runtime/osx/crash_filter.c (right):

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.c:33: CHECK(kr == KERN_SUCCESS);
Nit: The NaClLog() call you're replacing here had a more informative error
message.  Could you restore that?

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.c:37: nacl_thread_index =
state.uts.ts32.__gs >> 3;
As an aside, it's kind of weird that this value doesn't get used.  You could set
nacl_thread_index to anything here and the code would work.  I suppose it
doesn't matter though.

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.c:46: if (nacl_thread_index ==
NACL_TLS_INDEX_INVALID) {
See other comment -- for consistency with mach_exception_handler.c, it would be
better to put this inside the "#elif NACL_BUILD_SUBARCH == 64".

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_exception_handler.c (right):

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:149: if
(nacl_thread_index == NACL_TLS_INDEX_INVALID) {
This check should really go inside the "#elif NACL_BUILD_SUBARCH == 64" before. 
Otherwise you're assuming that trusted code doesn't run with %gs == 0, which
some future version of OS X might do (they might use %fs instead).  If %gs == 0,
the special cases later for single-stepping wouldn't get run.

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:155: if (!natp) {
Nit: NaCl style is "natp == NULL"

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:168: #if
NACL_BUILD_SUBARCH == 64
This check can be unconditional -- it's not specific to x86-64.

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:175: *is_untrusted = 0;
Adding an early return here would be an additional safeguard, since we don't
want the check for NaClSwitchRemainingRegsViaECX to allow the thread to continue
executing.

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_exception_handler.c:247: /* Don't handle if
no exception handler is set. */
Can you note this change in the commit message, please?

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.c:29: break;
An early return would be clearer now that you don't need to unlock a mutex.

[Mark Mentovai, 2013-02-20 20:15:48]

Thanks for the feedback. This is ready again now.

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
File src/trusted/service_runtime/osx/crash_filter.c (right):

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.c:33: CHECK(kr == KERN_SUCCESS);
Mark Seaborn wrote:
> Nit: The NaClLog() call you're replacing here had a more informative error
> message.  Could you restore that?

Done, but I think that the more succinct CHECK form is more “do what I mean,” in
that the important thing is the assertion, and not so much that the message gets
logged with an error code.

https://codereview.chromium.org/12207165/diff/33001/src/trusted/service_runti...
src/trusted/service_runtime/osx/crash_filter.c:37: nacl_thread_index =
state.uts.ts32.__gs >> 3;
Mark Seaborn wrote:
> As an aside, it's kind of weird that this value doesn't get used.  You could
set
> nacl_thread_index to anything here and the code would work.  I suppose it
> doesn't matter though.

I don’t like having this dangling unused variable. As you saw in an earlier
version, I got rid of the extra parameter to the inner function entirely at one
point, but I thought the #ifdef soup was too hairy. If I declare it and don’t
give it a value, it looks like an uninitialized use, so I have to either give it
a fake value or a real value, and I figured, why not a real one?

I’m happy to do this a different way if there’s something you really like, but
none of the solutions are really awesome. Part of the problem is the strict
C89/C90 intolerance of mixed declarations and code, and having to add the one
extra #ifdef in the declarations section tips the balance in my mind against
doing any #ifdefs at all.

https://codereview.chromium.org/12207165/diff/55003/src/trusted/service_runti...
File src/trusted/service_runtime/osx/mach_thread_map.c (right):

https://codereview.chromium.org/12207165/diff/55003/src/trusted/service_runti...
src/trusted/service_runtime/osx/mach_thread_map.c:80:
DCHECK(NaClGetThreadIndexForMachThread(mach_thread) ==
Note that I’ve added this DCHECK in as an additional map integrity safeguard.

[Mark Seaborn, 2013-02-20 20:24:27]

LGTM

[Mark Mentovai, 2013-02-20 21:45:30]

Committed patchset #6 manually as r10861 (presubmit successful).