1357897 - Building on OSX requires Xcode 8

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Building with Xcode 7 started failing with bug 1348419.  OSX 10.11 will need Xcode 8.2.1 and 10.12 will need 8.3.*.  Config should probably check for this.

note: I'm still building now that I updated to 8.2.1, will update this bug if that fails.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

To make this easier for others to find, here's the error I got when building on macOS 10.11.6 with Xcode 7.3.1:

…/js/src/jscntxt.h:74:8: error: thread-local storage is not supported for the current target

Comment 2

[Nathan Froyd [:froydnj]]

We may want to disable the thread_local support on OS X, then, because I think we still permit building with Xcode 7...I'm not really sure if we have a formal policy for which versions of Xcode we support beyond having a new enough clang and an appropriate SDK.  Is Xcode 8 available for 10.9?

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> We may want to disable the thread_local support on OS X

might be nice to version detect xcode and support if possible.

> Is Xcode 8 available for 10.9?

The only list I found showed:

xcode for osx
6.2 for 10.9
7.2.1 for 10.10
8.2.1 for 10.11
>= 8.3 for 10.12

Comment 4

[Tom Tromey :tromey]

First, I'm sorry about the difficulties here.  I was operating under the assumption that try
would faithfully reflect the minimum tool versions.  But you know what they say about assumptions.

One option would be a version check in ThreadLocal.h.  I don't know if there's a handy define
for this.

Another would be a configure check.

A third option would be to simply drop XP_MACOSX from the conditions in ThreadLocal.h.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: ensure compiler supports thread-local storage before using it on Mac

(In reply to Tom Tromey :tromey from comment #4)
> First, I'm sorry about the difficulties here.  I was operating under the
> assumption that try
> would faithfully reflect the minimum tool versions.  But you know what they
> say about assumptions.
> 
> One option would be a version check in ThreadLocal.h.  I don't know if
> there's a handy define
> for this.

It's possible to check for a particular version of Xcode via something like:

> #if defined(__clang__) && defined(__APPLE__) && \
>     defined(__apple_build_version__) && (__clang_major__ >= 8)

But it's probably better to check for the presence of the feature itself via `__has_feature(cxx_thread_local)`, per <http://clang.llvm.org/docs/LanguageExtensions.html#c-11-thread-local>.  We should just first check that `defined(__has_feature)`.

(Alternately, we could check that `defined(__clang)`, which also appears in Mozilla code.  But `defined(__has_feature)` seems preferable, because it's a feature check, just like __has_feature itself.)

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8860596 [details] [diff] [review]
ensure compiler supports thread-local storage before using it on Mac

Review of attachment 8860596 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I don't know whether __thread works on OS X in all the configurations we support and because putting the check here seems ungood in other ways.

::: mfbt/ThreadLocal.h
@@ +180,5 @@
>  #endif
>  }
>  
>  #ifdef MOZ_HAS_THREAD_LOCAL
> +#if defined(XP_WIN) || (defined(XP_MACOSX) && defined(__has_feature) && __has_feature(cxx_thread_local))

Does our automation setup support __thread on OS X?  It may just be simpler to move this check to the definition of MOZ_HAS_THREAD_LOCAL, otherwise things are not going to work very well.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: ensure compiler supports thread-local storage before enabling it on Mac

(In reply to Nathan Froyd [:froydnj] from comment #7)
> ::: mfbt/ThreadLocal.h
> @@ +180,5 @@
> >  #endif
> >  }
> >  
> >  #ifdef MOZ_HAS_THREAD_LOCAL
> > +#if defined(XP_WIN) || (defined(XP_MACOSX) && defined(__has_feature) && __has_feature(cxx_thread_local))
> 
> Does our automation setup support __thread on OS X?

I don't know if tryserver can give a complete answer to this question, but here are builds of the original patch on the two Mac platforms available in it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aad9113bd1b6125435e8b5d1e69634f9c8995d57


> It may just be simpler
> to move this check to the definition of MOZ_HAS_THREAD_LOCAL, otherwise
> things are not going to work very well.

I can add the check to the definition of MOZ_HAS_THREAD_LOCAL (and should, upon further review), but I'll still need to include it in the definition of MOZ_THREAD_LOCAL.  Otherwise, that definition will always try to use thread_local on Mac if MOZ_HAS_THREAD_LOCAL, even if MOZ_HAS_THREAD_LOCAL is defined because of HAVE_THREAD_TLS_KEYWORD.

Here's a patch that adds the check.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8861314 [details] [diff] [review]
ensure compiler supports thread-local storage before enabling it on Mac

Review of attachment 8861314 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9809ac55fe34
ensure __has_feature(thread_local) before using it on Mac; r=froydnj

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage at ThreadLocal.h:35:67:

https://hg.mozilla.org/integration/mozilla-inbound/rev/30c8bc2292771a746c4926ff3ae57415d862f813

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9809ac55fe346363ca5e28a556621e9349c85e7d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=94150748&repo=mozilla-inbound

[task 2017-04-25T19:07:36.075015Z] 19:07:36     INFO -  In file included from /home/worker/workspace/build/src/js/src/vm/Runtime.h:16:0,
[task 2017-04-25T19:07:36.075279Z] 19:07:36     INFO -                   from /home/worker/workspace/build/src/js/src/jscntxt.h:21,
[task 2017-04-25T19:07:36.075491Z] 19:07:36     INFO -                   from /home/worker/workspace/build/src/js/src/vm/RegExpObject.h:13,
[task 2017-04-25T19:07:36.075696Z] 19:07:36     INFO -                   from /home/worker/workspace/build/src/js/src/builtin/RegExp.h:10,
[task 2017-04-25T19:07:36.075902Z] 19:07:36     INFO -                   from /home/worker/workspace/build/src/js/src/builtin/RegExp.cpp:7:
[task 2017-04-25T19:07:36.076109Z] 19:07:36     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ThreadLocal.h:35:67: error: missing binary operator before token "("
[task 2017-04-25T19:07:36.076273Z] 19:07:36     INFO -       (defined(XP_MACOSX) && defined(__has_feature) && __has_feature(cxx_thread_local))
[task 2017-04-25T19:07:36.076416Z] 19:07:36     INFO -                                                                     ^
[task 2017-04-25T19:07:36.076572Z] 19:07:36     INFO -  /home/worker/workspace/build/src/config/rules.mk:1029: recipe for target 'RegExp.o' failed

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: ensure compiler supports thread-local storage before enabling it on Mac

The previous patch was busted on Windows/Linux because the preprocessor choked on the __has_feature expression, which it doesn't support.

This version of the patch tests that expression only on Mac.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4fe7d6edaec5b0a3a8c9e036b26beaa2b9649a3

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8861600 [details] [diff] [review]
ensure compiler supports thread-local storage before enabling it on Mac

Review of attachment 8861600 [details] [diff] [review]:
-----------------------------------------------------------------

This is cleaner, thank you!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de88e4dc9d99
ensure __has_feature(thread_local) before using it on Mac; r=froydnj

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de88e4dc9d99