Fix fast TLS support on Mac.

Fix fast TLS support on Mac.

[Mark Mentovai, 2011-04-01 14:28:01]

I haven’t tested this yet.

http://codereview.chromium.org/6706018/diff/1/src/platform-tls-mac.h
File src/platform-tls-mac.h (right):

http://codereview.chromium.org/6706018/diff/1/src/platform-tls-mac.h#newcode48
src/platform-tls-mac.h:48: asm("movl %%gs:(%1), %0;"
It seems like you should be able to move the arithmetic used to compute |offset|
on line 45 right here into the addressing mode. You can treat kMacTlsBaseOffset
as the base, index as the index, and 4 (or 8) as the width.

[Avi (use Gerrit), 2011-04-01 15:40:57]

http://codereview.chromium.org/6706018/diff/1/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/1/src/platform-macos.cc#newcode548
src/platform-macos.cc:548: }
else if kernel_version_major == 11, else can we fall back to just calling the
pthread function? Can we avoid this crash for post-Lion?

[Vitaly Repeshko, 2011-04-01 15:45:17]

http://codereview.chromium.org/6706018/diff/1/src/platform-tls-mac.h
File src/platform-tls-mac.h (right):

http://codereview.chromium.org/6706018/diff/1/src/platform-tls-mac.h#newcode48
src/platform-tls-mac.h:48: asm("movl %%gs:(%1), %0;"
On 2011/04/01 14:28:01, Mark Mentovai wrote:
> It seems like you should be able to move the arithmetic used to compute
|offset|
> on line 45 right here into the addressing mode. You can treat
kMacTlsBaseOffset
> as the base, index as the index, and 4 (or 8) as the width.

Given that we have to load the base from a static variable I thought it wouldn't
be worth the extra complexity in the inline assembly, but after checking what
gcc produces, it seems to be worth it. So I changed the code here.

[Vitaly Repeshko, 2011-04-01 15:48:30]

http://codereview.chromium.org/6706018/diff/1/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/1/src/platform-macos.cc#newcode548
src/platform-macos.cc:548: }
On 2011/04/01 15:40:57, Avi wrote:
> else if kernel_version_major == 11, else can we fall back to just calling the
> pthread function? Can we avoid this crash for post-Lion?

To fall back on the pthread function, we'd have to check some global static each
time when doing a TLS load. This is significantly more expensive and we might as
well always use the pthread function.

[Mark Mentovai, 2011-04-01 16:37:16]

I’m going to test this patch shortly.

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc#newcod...
src/platform-macos.cc:546: // 11.x.x (Lion) changed the offset.
Perhaps Avi would be satisfied if you had a test here for (kernel_version_major
> 11) and assumed the Lion behavior (kMacTlsBaseOffset = 0) but also logged a
warning. This would make it obvious that the assumption needs to be verified on
a subsequent kernel version, and that if something crashes inside V8, this
code’s assumptions would be a candidate for blame.

http://codereview.chromium.org/6706018/diff/2002/src/platform-tls-mac.h
File src/platform-tls-mac.h (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-tls-mac.h#newco...
src/platform-tls-mac.h:49: :"r"(kMacTlsBaseOffset), "r"(index));
You might do better by letting the output share a register with one of the
inputs as was done previously. Since this function is inline, it might let the
compiler avoid having to spill and reload in some cases.

[Avi (use Gerrit), 2011-04-01 16:41:52]

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc#newcod...
src/platform-macos.cc:546: // 11.x.x (Lion) changed the offset.
On 2011/04/01 16:37:16, Mark Mentovai wrote:
> Perhaps Avi would be satisfied if [...]

Yes, that would make me happy.

[Vitaly Repeshko, 2011-04-01 16:47:54]

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc#newcod...
src/platform-macos.cc:546: // 11.x.x (Lion) changed the offset.
On 2011/04/01 16:41:52, Avi wrote:
> On 2011/04/01 16:37:16, Mark Mentovai wrote:
> > Perhaps Avi would be satisfied if [...]
> 
> Yes, that would make me happy.

Guys, do you want this in addition to CheckFastTls? If yes, should it just be
fprintf(stderr, "Warning\n")?

http://codereview.chromium.org/6706018/diff/2002/src/platform-tls-mac.h
File src/platform-tls-mac.h (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-tls-mac.h#newco...
src/platform-tls-mac.h:49: :"r"(kMacTlsBaseOffset), "r"(index));
On 2011/04/01 16:37:16, Mark Mentovai wrote:
> You might do better by letting the output share a register with one of the
> inputs as was done previously. Since this function is inline, it might let the
> compiler avoid having to spill and reload in some cases.

GCC docs say the compiler is free to reuse a register with the current
constraints and that extra constraints are actually required to make it allocate
additional registers.

Search for "Unless an output operand" in
http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

[Mark Mentovai, 2011-04-01 17:54:25]

I’ve tested this now and it works as advertised.

LGTM as-is if you prefer, your Check should be sufficient.

[Vitaly Repeshko, 2011-04-04 05:54:57]

Thanks a lot for reviewing and testing this! Landed on bleeding_edge in r7488.
This should make it into the regular Monday's push to trunk. If not, I'll merge
it separately.


-- Vitaly

[fschneider, 2011-04-04 07:39:26]

One comment. LGTM otherwise.

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc#newcod...
src/platform-macos.cc:515: intptr_t kMacTlsBaseOffset = 0;
not sure: Can you guarantee that writes to kMacTlsBaseOffset are atomic? e.g.
unaligned store, splitting a cache line or the like.

Maybe make this use atomic-ops to for initializing, reading should still be
fast, right?

[Vitaly Repeshko, 2011-04-04 21:15:43]

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6706018/diff/2002/src/platform-macos.cc#newcod...
src/platform-macos.cc:515: intptr_t kMacTlsBaseOffset = 0;
On 2011/04/04 07:39:26, fschneider wrote:
> not sure: Can you guarantee that writes to kMacTlsBaseOffset are atomic? e.g.
> unaligned store, splitting a cache line or the like.
> 
> Maybe make this use atomic-ops to for initializing, reading should still be
> fast, right?

Good question. Technically to prevent the offset from temporarily being in an
inconsistent state we should either use atomic ops when both storing and loading
it or use a mutex in the initialization code. But given that we know it all
happens on either ia32 or x64 (platform-tls-mac checks it) and given that the
linker must allocate the offset naturally aligned, we can rely on it being
stored/loaded atomically.