Closed
Bug 904108
Opened 11 years ago
Closed 11 years ago
js/src/vm/PropertyKey.cpp won't compile under Apple clang 4.1
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: abr, Assigned: abr)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
1.16 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Add operator=() to js::Value, with normal "default" behavior.
Attachment #789015 -
Flags: review?(luke)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #789015 -
Attachment is obsolete: true
Attachment #789015 -
Flags: review?(luke)
Assignee | ||
Updated•11 years ago
|
Attachment #789017 -
Flags: review?(luke)
![]() |
||
Comment 3•11 years ago
|
||
Waldo: do you know what might be going on here? It seems strange for clang to have such a seemingly simple bug.
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
(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. :)
Assignee | ||
Comment 6•11 years ago
|
||
[Adding 'needinfo' for comment 3, as patch approval is presumably waiting on a response]
Flags: needinfo?(jwalden+bmo)
Comment 7•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(jwalden+bmo)
![]() |
||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #789017 -
Attachment is obsolete: true
Attachment #789017 -
Flags: review?(luke)
Assignee | ||
Updated•11 years ago
|
Attachment #789342 -
Flags: review?(luke)
Assignee | ||
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
Comment on attachment 789342 [details] [diff] [review] Add explicit assignment operator to js::Value Thanks!
Attachment #789342 -
Flags: review?(luke) → review+
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdd168a8518
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9cdd168a8518
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 15•11 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•11 years ago
|
Flags: needinfo?(adam)
Assignee | ||
Comment 17•11 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(adam)
Resolution: --- → FIXED
Comment 18•11 years ago
|
||
(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>.
Assignee | ||
Comment 19•11 years ago
|
||
(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.
Assignee | ||
Comment 20•11 years ago
|
||
Sorry, I didn't mean to close this before. Fixing back to how Ehsan had it set.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: js/src/vm/PropertyKey.cpp won't compile under clang 4.1 → js/src/vm/PropertyKey.cpp won't compile under Apple clang 4.1
Comment 21•11 years ago
|
||
Thanks for the confirmation. Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/72800965e07b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → WONTFIX
Comment 22•11 years ago
|
||
Merge of backout: https://hg.mozilla.org/mozilla-central/rev/72800965e07b
Target Milestone: mozilla26 → ---
Comment 24•11 years ago
|
||
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•11 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•