1256136 - Enable std::is_destructible with clang/libc++ again

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Jan Beich]

Upstream fixed libc++ bug which was later backported to FreeBSD 10.2. Let's back out bug 1028428.

https://github.com/llvm-mirror/libcxx/commit/2b44e3dd49136179c49a81f34f14ce31005c6792

Comment 1

[Jan Beich]

Attachment: MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

Review commit: https://reviewboard.mozilla.org/r/39629/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39629/

Comment 2

[Jan Beich]

re-Try without unrelated changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67af485491e5

Comment 3

[Nathan Froyd [:froydnj]]

This doesn't seem like a good change unless we can verify that the libc++ that we will be using in the future (Android/OS X) has the necessary changes.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 8729964 [details]
MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

per last comment.

Also - dbaron or gps might be a better reviewer.

Comment 5

[Jan Beich]

Comment on attachment 8729964 [details]
MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

@dbaron, the patch removes libc++ workaround that only affected old (and no longer supported) version of FreeBSD. Android isn't using libc++ yet but OS X builds fine as it was originally with bug 1027251.

(In reply to Nathan Froyd [:froydnj] from comment #3)
> This doesn't seem like a good change unless we can verify that the libc++
> that we will be using in the future (Android/OS X) has the necessary changes.

Link to relevant bugs, CC parties in the know or this is a FUD about something too far off into the future. Recent enough libc++ isn't affected as shown by FreeBSD 11.0 (clang/libc++ 3.8).

Android (bug 1260208): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac181e1dbca5
OS X (10.7): https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca8619eb5e1

Treeherder seems to have OS X 10.10 slave(s) but I don't know how to force build (as opposed to running tests) there.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Jan Beich from comment #5)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > This doesn't seem like a good change unless we can verify that the libc++
> > that we will be using in the future (Android/OS X) has the necessary changes.
> 
> Link to relevant bugs, CC parties in the know or this is a FUD about
> something too far off into the future. Recent enough libc++ isn't affected
> as shown by FreeBSD 11.0 (clang/libc++ 3.8).
> 
> Android (bug 1260208):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac181e1dbca5
> OS X (10.7):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca8619eb5e1

I don't think either of these builds have the necessary patches to build with libc++...at least the parent revision of the patch from this bug is just a generic inbound or central commit, rather than the patch stacks from the relevant bugs (bug 1260208 for Android, bug 1246743 for OS X).  Also, given that we're dropping OS X 10.6-10.8 support, we'll just be building with the 10.9 (?) SDK for OS X and whatever libc++ revision that has, rather than the patches from bug 1246743, I think.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8729964 [details]
MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

https://reviewboard.mozilla.org/r/39629/#review46723

Like comment 6 says, I don't have any confidence this actually works with our libc++-using platforms.

A quick check of Android shows that its libc++ does not have this fix, so things would break when we switched Android over.

Comment 8

[Jan Beich]

Comment on attachment 8729964 [details]
MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b52c038d81b0

(In reply to Nathan Froyd [:froydnj] from comment #6)
> > Android (bug 1260208):
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac181e1dbca5
> > OS X (10.7):
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca8619eb5e1
> 
> I don't think either of these builds have the necessary patches to build
> with libc++...at least the parent revision of the patch from this bug is
> just a generic inbound or central commit

Android parent revision was actually part of bug 1260208... OS X ended up being after bug 1269542 which made testing bug 1246743 somewhat pointless.

(In reply to Nathan Froyd [:froydnj] from comment #7)
> A quick check of Android shows that its libc++ does not have this fix, so
> things would break when we switched Android over.

The patch here can only break build and won't be merged into Aurora.

Comment 9

[Jan Beich]

Also, only clang/libc++ was affected, not gcc/libc++ which Android uses after bug 1260208.

Comment 10

[Jan Beich]

Comment on attachment 8729964 [details]
MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

:dougt transferred r? to :froydnj via comment 4, so dropping confusing r-.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8729964 [details]
MozReview Request: Bug 1256136 - Enable std::is_destructible with clang/libc++ again. r?dougt

https://reviewboard.mozilla.org/r/39629/#review47765

Ah, thanks for pointing out that this is clang/libc++ specific.  I missed that the first time around.

So two things.  One is that we do want to try to make Android builds compile with clang (I'm going to try and do that this quarter), and this change would break that.  Two is that it's not clear to me that the OS X builds are actually using libc++; your bug link is just about hiding jobs, not actually changing build configurations.  (I see no evidence in the build log that we're using something like -stdlib=libc++, for instance, and rummaging around in clang source code, it looks like the default is libstdc++: lib/Driver/Toolchain.cpp, ToolChain::GetCXXStdlibType.)

I don't think we're far enough away from the busted libc++ that we can assume everything we're building on has the fix.

Comment 12

[Jan Beich]

Bug 1303302 removed the affected code.