libc++ build is broken with exceptions disabled (bug 551254) in <type_traits>. OS X doesn't use -I${DIST}/stl_wrappers thus not affected. In file included from mozglue/misc/TimeStamp.cpp:11: In file included from dist/include/mozilla/TimeStamp.h:13: In file included from dist/include/mozilla/FloatingPoint.h:15: In file included from dist/include/mozilla/MathAlgorithms.h:15: In file included from dist/stl_wrappers/cmath:28: In file included from dist/system_wrappers/new:3: In file included from /usr/include/c++/v1/new:70: In file included from dist/system_wrappers/exception:3: In file included from /usr/include/c++/v1/exception:82: In file included from dist/stl_wrappers/type_traits:34: dist/include/mozilla/mozalloc.h:184:33: error: no type named 'bad_alloc' in namespace 'std' void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC ^~~~~~~~~~~~~~~~~~~~~~~~ dist/include/mozilla/mozalloc.h:175:34: note: expanded from macro 'MOZALLOC_THROW_BAD_ALLOC' #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dist/include/mozilla/mozalloc.h:172:63: note: expanded from macro 'MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS' #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS throw(std::bad_alloc) ~~~~~^ dist/include/mozilla/mozalloc.h:190:44: error: no type named 'nothrow_t' in namespace 'std' void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ~~~~~^ dist/include/mozilla/mozalloc.h:196:35: error: no type named 'bad_alloc' in namespace 'std' void* operator new[](size_t size) MOZALLOC_THROW_BAD_ALLOC ^~~~~~~~~~~~~~~~~~~~~~~~ dist/include/mozilla/mozalloc.h:175:34: note: expanded from macro 'MOZALLOC_THROW_BAD_ALLOC' #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dist/include/mozilla/mozalloc.h:172:63: note: expanded from macro 'MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS' #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS throw(std::bad_alloc) ~~~~~^ dist/include/mozilla/mozalloc.h:202:46: error: no type named 'nothrow_t' in namespace 'std' void* operator new[](size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ~~~~~^ dist/include/mozilla/mozalloc.h:214:44: error: no type named 'nothrow_t' in namespace 'std' void operator delete(void* ptr, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ~~~~~^ dist/include/mozilla/mozalloc.h:226:46: error: no type named 'nothrow_t' in namespace 'std' void operator delete[](void* ptr, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ~~~~~^ 6 errors generated.

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

Comment on attachment 8747519 [details] MozReview Request: Bug 1269171 - Unbreak libc++ build after bug 1268816. r?lsalzman https://reviewboard.mozilla.org/r/49917/#review46815

This also fixes my broken linux64 build with gcc 6.0 on Fedora 24. Seems to require a CLOBBER update though?

Comment on attachment 8747519 [details] MozReview Request: Bug 1269171 - Unbreak libc++ build after bug 1268816. r?lsalzman This most certainly is not the right fix for this. I'll need to think about it.

(In reply to Mike Hommey [:glandium] from comment #5) > This most certainly is not the right fix for this. I'll need to think about > it. Then please open a new bug for followup. If we have a work-around that should land in the meantime to fix the regression and unblock people working on gcc 6 issues.

(In reply to Ralph Giles (:rillian) needinfo me from comment #6) > (In reply to Mike Hommey [:glandium] from comment #5) > > > This most certainly is not the right fix for this. I'll need to think about > > it. > > Then please open a new bug for followup. If we have a work-around that > should land in the meantime to fix the regression and unblock people working > on gcc 6 issues. This bug is about libc++. Firefox builds fine with gcc 6 without this patch. Here is proof: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86d7709526d4ddacaac38fd3b31cbdb0b900b2a6 . If it fails for you, then file a different bug with details about your issue.

(In reply to Mike Hommey [:glandium] from comment #7) > (In reply to Ralph Giles (:rillian) needinfo me from comment #6) > > (In reply to Mike Hommey [:glandium] from comment #5) > > > > > This most certainly is not the right fix for this. I'll need to think about > > > it. > > > > Then please open a new bug for followup. If we have a work-around that > > should land in the meantime to fix the regression and unblock people working > > on gcc 6 issues. > > This bug is about libc++. Firefox builds fine with gcc 6 without this patch. > Here is proof: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=86d7709526d4ddacaac38fd3b31cbdb0b900b2a6 . If it > fails for you, then file a different bug with details about your issue. Sorry to spam this bug, but I'm seeing the same problem on my ArchLinux with testing repositories enabled and gcc 6.1... Only this patch help build process going on :(

I also can reproduce with GCC 6.1.1 (20160428) and 7.0.0 (20160501) but not 5.3.0 or 4.9.4 (20160427). In file included from dist/stl_wrappers/type_traits:39:0, from /usr/local/lib/gcc6/include/c++/bits/move.h:57, from /usr/local/lib/gcc6/include/c++/bits/nested_exception.h:40, from /usr/local/lib/gcc6/include/c++/exception:171, from dist/system_wrappers/exception:3, from /usr/local/lib/gcc6/include/c++/new:40, from dist/system_wrappers/new:3, from dist/stl_wrappers/cmath:33, from dist/include/mozilla/MathAlgorithms.h:15, from dist/include/mozilla/FloatingPoint.h:15, from dist/include/mozilla/TimeStamp.h:13, from mozglue/misc/TimeStamp.cpp:11: dist/include/mozilla/mozalloc.h:184:33: error: 'bad_alloc' in namespace 'std' does not name a type void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC ^ dist/include/mozilla/mozalloc.h:190:44: error: 'nothrow_t' in namespace 'std' does not name a type void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ^~~~~~~~~ dist/include/mozilla/mozalloc.h:196:35: error: 'bad_alloc' in namespace 'std' does not name a type void* operator new[](size_t size) MOZALLOC_THROW_BAD_ALLOC ^ dist/include/mozilla/mozalloc.h:202:46: error: 'nothrow_t' in namespace 'std' does not name a type void* operator new[](size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ^~~~~~~~~ dist/include/mozilla/mozalloc.h:214:44: error: 'nothrow_t' in namespace 'std' does not name a type void operator delete(void* ptr, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ^~~~~~~~~ dist/include/mozilla/mozalloc.h:226:46: error: 'nothrow_t' in namespace 'std' does not name a type void operator delete[](void* ptr, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS ^~~~~~~~~

Recent Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75c352f8a002 (In reply to Mike Hommey [:glandium] from comment #7) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=86d7709526d4ddacaac38fd3b31cbdb0b900b2a6 Make sure to test *with* bug 1268816. Notice type_traits line is missing in https://hg.mozilla.org/try/file/86d7709526d4/config/stl-headers

Tracking for 49 - we want to keep an eye on this and make sure it gets straightened out.

I'm having the same problem on Arch Linux. In GCC 6.1.1, <new> includes <type_traits>. mozalloc.h depends on <new> and therefore on <type_traits>, so the wrapper for <type_traits> needs to not include mozalloc.h to avoid a circular dependency. I think we can do the same thing here as was done for bug 1245076.

Comment on attachment 8753597 [details] [diff] [review] [PATCH] Don't include mozalloc.h from the type_traits wrapper Since there's precedent for this, while still letting us wrap the header, I think this variant is sensible. Let's see what Mike and/or Nathan think.

Comment on attachment 8753597 [details] [diff] [review] [PATCH] Don't include mozalloc.h from the type_traits wrapper Also fixes libc++ build.

Comment on attachment 8753597 [details] [diff] [review] [PATCH] Don't include mozalloc.h from the type_traits wrapper Taking a step back, I think there is a better and future-proof alternative.

Since the introduction of the STL wrappers, they have included mozalloc.h, and multiple times, we've hit header reentrancy problems, and worked around them as best as we could. Taking a step back, all mozalloc.h does is: - declare moz_* allocator functions. - define inline implementations of various operator new/delete variants. The first only requires the functions are declared before they are used, so mozalloc.h only needs to be included before anything that would use those functions. The second doesn't actually require a specific order, as long as the declaration for those functions comes before their use, and they are either declared in <new> or implicitly by the C++ compiler. So all in all, it doesn't matter that mozalloc.h is included before the wrapped STL headers. What matters is that it's included when STL headers are included. Review commit: https://reviewboard.mozilla.org/r/53474/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53474/

(In reply to Mike Hommey [:glandium] from comment #19) > Created attachment 8753714 [details] > MozReview Request: Bug 1269171 - Change how mozalloc.h is hooked in STL > wrappers > > Since the introduction of the STL wrappers, they have included > mozalloc.h, and multiple times, we've hit header reentrancy problems, > and worked around them as best as we could. > > Taking a step back, all mozalloc.h does is: > - declare moz_* allocator functions. > - define inline implementations of various operator new/delete variants. > > The first only requires the functions are declared before they are used, > so mozalloc.h only needs to be included before anything that would use > those functions. > > The second doesn't actually require a specific order, as long as the > declaration for those functions comes before their use, and they are > either declared in <new> or implicitly by the C++ compiler. > > So all in all, it doesn't matter that mozalloc.h is included before the > wrapped STL headers. What matters is that it's included when STL headers > are included. > > Review commit: https://reviewboard.mozilla.org/r/53474/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/53474/ That has a crazy failure on Linux debug builds, and screwed up the msvc header, but I'm still interested in feedback on whether this sounds crazy or not.

Haha, on Windows, this retriggers bug 1189891. Worst case scenario, we can leave the Windows wrapper untouched. Still need to figure out what's going funky on Linux.

It doesn't fix the problem because std::nothrow_t and std::bad_alloc must be defined before including mozalloc.h. When you include <new>, before getting to the definitions of std::nothrow_t and std::bad_alloc, <new> includes <type_traits>, and <type_traits> includes mozalloc.h even though we haven't finished including <new> yet. I also wish that we could find a simpler solution, but I really don't think there is one.

> ... I'm seeing the same problem on my ArchLinux with > testing repositories enabled and gcc 6.1... Only this patch help build > process going on :( gcc6.1 hit stable a couple of days ago and I also encounter the problem. What patch did you apply?

(In reply to Alex Henrie from comment #22) > It doesn't fix the problem Yes it does. Did you try it? > because std::nothrow_t and std::bad_alloc must be > defined before including mozalloc.h. And they are because mozalloc.h includes <new> > When you include <new>, before getting > to the definitions of std::nothrow_t and std::bad_alloc, <new> includes > <type_traits>, and <type_traits> includes mozalloc.h even though we haven't > finished including <new> yet. My patch makes things such that mozalloc.h is included once _after_ all recursively included STL headers have been included.

Mmmm turns out it does fail for files that include new on their own...

And that's because the reentrancy thing is badly placed.

(In reply to Mike Hommey [:glandium] from comment #24) > (In reply to Alex Henrie from comment #22) > > It doesn't fix the problem > > Yes it does. Did you try it? Here's what I'm doing: $ wget https://reviewboard.mozilla.org/r/53474/diff/raw/ -O - | patch -p1 $ ./mach clobber $ ./mach build -j 1 I'm attaching the standard output from the last command.

Since the introduction of the STL wrappers, they have included mozalloc.h, and multiple times, we've hit header reentrancy problems, and worked around them as best as we could. Taking a step back, all mozalloc.h does is: - declare moz_* allocator functions. - define inline implementations of various operator new/delete variants. The first only requires the functions to be declared before they are used, so mozalloc.h only needs to be included before anything that would use those functions. The second doesn't actually require a specific order, as long as the declaration for those functions comes before their use, and they are either declared in <new> or implicitly by the C++ compiler. So all in all, it doesn't matter that mozalloc.h is included before the wrapped STL headers. What matters is that it's included when STL headers are included. So arrange things such that mozalloc.h is included after the first wrapped STL header is fully preprocessed (and all its includes have been included). Review commit: https://reviewboard.mozilla.org/r/53874/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53874/

Comment on attachment 8753714 [details] MozReview Request: Bug 1269171 - Backout ccff1c4580ab (bug 1270832) because it doesn't actually work properly and blocks upcoming changes. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53474/diff/1-2/

Comment on attachment 8754260 [details] MozReview Request: Bug 1269171 - Change how mozalloc.h is hooked in STL wrappers https://reviewboard.mozilla.org/r/53874/#review50728 I'm not sure this got tested properly due to the double definitions outlined below. I think I understand what's going on here, but I'd like to see something more polished before approving it. I think we're relying on something like the following not happening: // 1. stdlib defines new/delete/etc. // 2. stdlib code uses those definitions, not our definitions // 3. mozalloc.h gets included, defines new/delete/etc. // 4. other stdlib/gecko code uses mozalloc's definitions // 5. bad things happen because we're implicitly assuming that step 1 is always done via including <new>, and we catch that by providing our own definitions after <new>, correct? I guess if it was done with some other header, we'd have to figure out how to wrap that. ::: config/gcc-stl-wrapper.template.h:27 (Diff revision 1) > +// > +// FIXME/bug 551254: gcc's debug STL implementation requires -frtti. > +// Figure out how to resolve this with -fno-rtti. Maybe build with > +// -frtti in DEBUG builds? > +// > +// # define _GLIBCXX_DEBUG 1 This change needs to be rebased. ::: config/gcc-stl-wrapper.template.h:36 (Diff revision 1) > -// named, starting with the directory in the search path after the > -// one where the current file was found. > -# include_next <new> > > +#if !defined(MOZ_INCLUDE_MOZALLOC_H_FROM) && \ > + !defined(moz_dont_include_mozalloc_for_${HEADER}) This check is basically for the cstdlib bit above, since we don't care about moz_dont_include_mozalloc_for anywhere else? ::: config/gcc-stl-wrapper.template.h:37 (Diff revision 1) > +# define MOZ_INCLUDE_MOZALLOC_H_FROM ${HEADER} > +# define MOZ_INCLUDE_MOZALLOC_H_FROM_${HEADER} This define is so important you needed to define it twice? Or did you mean to define the moz_dont_include_mozalloc_for bit? ::: config/msvc-stl-wrapper.template.h:17 (Diff revision 1) > +# define MOZ_INCLUDE_MOZALLOC_H_FROM ${HEADER} > +# define MOZ_INCLUDE_MOZALLOC_H_FROM_${HEADER} Same comment here as for the GCC case. ::: memory/mozalloc/mozalloc.h:19 (Diff revision 1) > +# include <cstring> > +#else > +# include "stdlib.h" > +# include "string.h" Why are we including cstring and string.h when we already included them prior to this? Are these includes for the C versions meant to pick up our wrappers instead of the system headers? A comment answering these sorts of questions seems warranted.

Comment on attachment 8753714 [details] MozReview Request: Bug 1269171 - Backout ccff1c4580ab (bug 1270832) because it doesn't actually work properly and blocks upcoming changes. https://reviewboard.mozilla.org/r/53474/#review50730 What is the "not working properly" bit? ::: config/gcc-stl-wrapper.template.h:49 (Diff revision 2) > +// FIXME/bug 551254: gcc's debug STL implementation requires -frtti. > +// Figure out how to resolve this with -fno-rtti. Maybe build with > +// -frtti in DEBUG builds? > +// > +// # define _GLIBCXX_DEBUG 1 Oh. I see, your other patch was on top of this one.

(In reply to Nathan Froyd [:froydnj] from comment #31) > Comment on attachment 8754260 [details] > MozReview Request: Bug 1269171 - Change how mozalloc.h is hooked in STL > wrappers > > https://reviewboard.mozilla.org/r/53874/#review50728 > > I'm not sure this got tested properly due to the double definitions outlined > below. It was tested on try and allowed full builds with GCC 6 there! > I think I understand what's going on here, but I'd like to see > something more polished before approving it. > > I think we're relying on something like the following not happening: > > // 1. stdlib defines new/delete/etc. > > // 2. stdlib code uses those definitions, not our definitions > > // 3. mozalloc.h gets included, defines new/delete/etc. > > // 4. other stdlib/gecko code uses mozalloc's definitions > > // 5. bad things happen > > because we're implicitly assuming that step 1 is always done via including > <new>, and we catch that by providing our own definitions after <new>, > correct? I guess if it was done with some other header, we'd have to figure > out how to wrap that. The order doesn't matter. If stdlib uses operator new before mozalloc.h is included, it still uses mozalloc.h definition, because what the stdlib has is declarations, not definitions. A reduced testcase for this would look like this: #include <new> #include <cstdlib> void *foo(int size) { return new char[size]; } inline void* operator new[](std::size_t size) throw() { return malloc(size); } In that testcase, foo will always use malloc. It would still be true if foo were defined in one of the STL headers. > ::: config/gcc-stl-wrapper.template.h:36 > (Diff revision 1) > > -// named, starting with the directory in the search path after the > > -// one where the current file was found. > > -# include_next <new> > > > > +#if !defined(MOZ_INCLUDE_MOZALLOC_H_FROM) && \ > > + !defined(moz_dont_include_mozalloc_for_${HEADER}) > > This check is basically for the cstdlib bit above, since we don't care about > moz_dont_include_mozalloc_for anywhere else? Half of the test is for the include to happen only once. The other is for the include not to happen for cstdlib. > ::: config/gcc-stl-wrapper.template.h:37 > (Diff revision 1) > > +# define MOZ_INCLUDE_MOZALLOC_H_FROM ${HEADER} > > +# define MOZ_INCLUDE_MOZALLOC_H_FROM_${HEADER} > > This define is so important you needed to define it twice? Or did you mean > to define the moz_dont_include_mozalloc_for bit? There is a subtle difference between the two. It would probably be clearer if the first line was "# define MOZ_INCLUDE_MOZALLOC_H_FROM", which would work as well. > ::: memory/mozalloc/mozalloc.h:19 > (Diff revision 1) > > +# include <cstring> > > +#else > > +# include "stdlib.h" > > +# include "string.h" > > Why are we including cstring and string.h when we already included them > prior to this? Because I forgot to remove it.

Comment on attachment 8754260 [details] MozReview Request: Bug 1269171 - Change how mozalloc.h is hooked in STL wrappers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53874/diff/1-2/

Comment on attachment 8754260 [details] MozReview Request: Bug 1269171 - Change how mozalloc.h is hooked in STL wrappers https://reviewboard.mozilla.org/r/53874/#review51004 Thanks for the revision and the added explanations.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a640e6fa8ab9 https://hg.mozilla.org/integration/mozilla-inbound/rev/68da139d0866