904108 - js/src/vm/PropertyKey.cpp won't compile under Apple clang 4.1

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Adam Roach [:abr]]

Sometime in the past few days, something changed (probably in
js/public/Value.cpp) that broke builds for clang 4.1:

>In file included from .../js/src/vm/PropertyKey.cpp:9:
>./../../dist/include/js/PropertyKey.h:35:7: error: no member named 'operator=' in 'JS::Value'
>class PropertyKey
>      ^~~~~~~~~~~
>./../../dist/include/js/PropertyKey.h:87:14: note: implicit default move assignment operator for 'JS::PropertyKey' first required here
>        *key = PropertyKey(uint32_t(v.toInt32()));
>             ^
>1 error generated.

Comment 1

[Adam Roach [:abr]]

Attachment: Add explicit assignment operator to js::Value

Add operator=() to js::Value, with normal "default" behavior.

Comment 2

[Adam Roach [:abr]]

Attachment: Add explicit assignment operator to js::Value

Comment 3

[Luke Wagner [:luke]]

Waldo: do you know what might be going on here?  It seems strange for clang to have such a seemingly simple bug.

Comment 4

[Stephen A Pohl [:spohl] (OOO until 9/9)]

Just fyi: I ran into this problem earlier today and updating my command line tools (via Prefences > Downloads in XCode) fixed this problem for me. New clang version is 4.2.

Comment 5

[Adam Roach [:abr]]

(In reply to Stephen Pohl [:spohl] from comment #4)
> Just fyi: I ran into this problem earlier today and updating my command line
> tools (via Prefences > Downloads in XCode) fixed this problem for me. New
> clang version is 4.2.

Thanks -- I presumed it worked under 4.2, since the tbpl builds weren't failing. My understanding is that clang 4.1 is still a supported compiler. We can, of course, have a broader conversation about deprecating it; but, while it is, we need to make sure the tree builds with it.

It is, in fact, mostly for this reason that I haven't upgraded to 4.2 yet. Apparently, I'm one of the very few devs who still runs 4.1, since I seem to be catching 4.1-related regressions before anyone else does. :)

Comment 6

[Adam Roach [:abr]]

[Adding 'needinfo' for comment 3, as patch approval is presumably waiting on a response]

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 789017 [details] [diff] [review]
Add explicit assignment operator to js::Value

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

As I recall from mailing-list-skimming, clang's implementation of all the implicit defaulting/deleting of special member functions was completed somewhat late, compared to most of their C++11 support.  Possibly this just didn't come til a little bit too late, but I dunno, really.

::: js/public/Value.h
@@ +909,5 @@
> +    /*
> +     * clang 4.1 doesn't corectly create a default assignment operator
> +     * for this class, so we need to explicitly declare one.
> +     */
> +    Value &operator=(const Value r) {

Shouldn't this be const Value &r?

Comment 8

[Luke Wagner [:luke]]

Ah, if this is an "old clang version" issue, then perhaps we can put the operator= behind a versioned #ifdef with a comment?  Then it'll be natural to remove it when 4.1 is ancient history.

Comment 9

[Jeff Walden [:Waldo]]

There's no way to test that.  The "version numbers" exposed by __clang_minor__ and __clang_major__ (?) are "marketing version numbers", which different vendors permissibly set to different values.  The feature-testing macros are the only way to detect stuff, but there's no feature for this, and no way to expose the SVN revision number or similar to do a newer-than check.

Comment 10

[Adam Roach [:abr]]

Attachment: Add explicit assignment operator to js::Value

Comment 11

[Adam Roach [:abr]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> There's no way to test that.  The "version numbers" exposed by
> __clang_minor__ and __clang_major__ (?) are "marketing version numbers",
> which different vendors permissibly set to different values.

While this is true, the only platform that I'm aware we support clang for is OS X, and we know where the line between "broken" and "working" is for Apple's on version of clang. If there were macros to test for specific bugs, I'd happily use them instead. :)

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 789342 [details] [diff] [review]
Add explicit assignment operator to js::Value

Thanks!

Comment 13

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdd168a8518

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cdd168a8518

Comment 15

[(no longer active)]

This fix is incorrect because of the version number issue that was raised before, and we need to back it out since it affects llvm clang builds, which is what we use at least in our infrastructure.

If we really want to fix this properly, we should add a configure check which detects the bug in Apple clang and adds the workaround if that condition is true.

Comment 16

[(no longer active)]

Should I backout the patch myself pending a correct fix?

Comment 17

[Adam Roach [:abr]]

Ehsan: I don't see it breaking anything in its current form -- in fact, I think it would have been fine to land without any conditional compilation, but it seemed to make Luke happy. Anyway, since it doesn't cause any demonstrable harm and *does* improve the situation for some subset of developers, I'm not really enthusiastic about investing much effort in removing it.

The other side of the argument is that the conversation on the dev list appears to be converging towards not supporting 4.1, so backing it out would only formalize a policy of no longer catering to Apple clang 4.1. I've given up and upgraded to 4.2; so, if you decide to back this patch out, I won't object. But if you do back it out, please just close this bug as "WONTFIX".

The problem with your proposed "proper" solution is that it would take a rather large effort to track down a minimal test case that triggers this bug for the purpose of integration into configure-time checks (the compiler bug itself is somewhat bizarre). I certainly have bigger things on my plate at the moment.

Comment 18

[(no longer active)]

(In reply to comment #17)
> Ehsan: I don't see it breaking anything in its current form -- in fact, I think
> it would have been fine to land without any conditional compilation, but it
> seemed to make Luke happy. Anyway, since it doesn't cause any demonstrable harm
> and *does* improve the situation for some subset of developers, I'm not really
> enthusiastic about investing much effort in removing it.

Well I'm not familiar with the semantics of this code so I believe you that it's not doing any harm.  But at the very least it's going to be bad precedence for other people.  It's just that __clang_major__ and __clang_minor__ are not really useful for anything in practice.  Just taking out that #ifdef would be a nice fix as far as I'm concerned here!

> The other side of the argument is that the conversation on the dev list appears
> to be converging towards not supporting 4.1, so backing it out would only
> formalize a policy of no longer catering to Apple clang 4.1. I've given up and
> upgraded to 4.2; so, if you decide to back this patch out, I won't object. But
> if you do back it out, please just close this bug as "WONTFIX".

OK, but I don't want to make your life unnecessarily painful, which is why I did not back this out myself.  :-)  Please note that the Apple clang versions are pretty much useless!

If you can confirm that this doesn't break your build with the newer Apple clang, I can back it out and mark this bug as WONTFIX.

> The problem with your proposed "proper" solution is that it would take a rather
> large effort to track down a minimal test case that triggers this bug for the
> purpose of integration into configure-time checks (the compiler bug itself is
> somewhat bizarre). I certainly have bigger things on my plate at the moment.

Yeah the proper solution is definitely much more involved, and I totally understand you not having enough time to fix this.  I would fix it "properly" myself but I think keeping support for older clang releases will be a losing battle!

Anyway, I encourage you to do your own clang builds.  See <http://ehsanakhgari.org/blog/2011-10-17/why-you-should-switch-clang-today-and-how> as a guide to build clang.  Our supported llvm svn ID can always be found here: <http://mxr.mozilla.org/mozilla-central/source/browser/config/tooltool-manifests/linux64/clang.manifest#3>.

Comment 19

[Adam Roach [:abr]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)

Like I said, I've already upgraded my compiler, so it's really no impact on me one way or another whether 4.1 works. Breaking it will probably make other people unhappy, but it sounds like we're on the way to making those people unhappy anyway.

I'm comfortable that backing the patch out will not break the 4.2 that I'm running, since tbpl didn't choke on the code prior to this patch going in.

Comment 20

[Adam Roach [:abr]]

Sorry, I didn't mean to close this before. Fixing back to how Ehsan had it set.

Comment 21

[(no longer active)]

Thanks for the confirmation.  Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/72800965e07b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/72800965e07b

Comment 24

[John Daggett (:jtd)]

Added a note to the OSX build requirements page to note that clang 4.2 is required:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites

Kinda sucks that the build system can't catch this but at least we can document the error and point folks in the right direction.

Comment 25

[(no longer active)]

(In reply to comment #24)
> Added a note to the OSX build requirements page to note that clang 4.2 is
> required:
> 
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites

Thanks!

> Kinda sucks that the build system can't catch this but at least we can document
> the error and point folks in the right direction.

See bug 905876.