1269171 - mozalloc.h:184:33: error: 'bad_alloc' in namespace 'std' does not name a type

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jan Beich]

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.

Comment 1

[Jan Beich]

Attachment: MozReview Request: Bug 1269171 - Unbreak libc++ build after bug 1268816. r?lsalzman

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

Comment 2

[Jan Beich]

Comment on attachment 8747519 [details]
MozReview Request: Bug 1269171 - Unbreak libc++ build after bug 1268816. r?lsalzman

Alternatively, I can sprinkle DISABLE_STL_WRAPPING like in bug 1259537 but it'd affect more than one directory all of which would be unrelated to skia.

Comment 3

[Lee Salzman [:lsalzman]]

Comment on attachment 8747519 [details]
MozReview Request: Bug 1269171 - Unbreak libc++ build after bug 1268816. r?lsalzman

https://reviewboard.mozilla.org/r/49917/#review46815

Comment 4

[Ralph Giles (:rillian)]

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

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Ralph Giles (:rillian)]

(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.

Comment 7

[Mike Hommey [:glandium]]

(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.

Comment 8

[Frederic Bezies]

(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 :(

Comment 9

[Jan Beich]

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
                                              ^~~~~~~~~

Comment 10

[Jan Beich]

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

Comment 11

[Marcia Knous [:marcia]]

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

Comment 12

[Alex Henrie]

Attachment: [PATCH] Don't include mozalloc.h from the type_traits wrapper

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 13

[Lee Salzman [:lsalzman]]

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 14

[Jan Beich]

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

Also fixes libc++ build.

Comment 18

[Mike Hommey [:glandium]]

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.

Comment 19

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1269171 - Backout ccff1c4580ab (bug 1270832) because it doesn't actually work properly and blocks upcoming changes.

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/

Comment 20

[Mike Hommey [:glandium]]

(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.

Comment 21

[Mike Hommey [:glandium]]

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.

Comment 22

[Alex Henrie]

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.

Comment 23

[Mihamina Rakotomandimby]

> ... 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?

Comment 24

[Mike Hommey [:glandium]]

(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.

Comment 25

[Mike Hommey [:glandium]]

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

Comment 26

[Mike Hommey [:glandium]]

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

Comment 27

[Alex Henrie]

Attachment: build log after applying rb53474.patch

(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.

Comment 29

[Mike Hommey [:glandium]]

Attachment: 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 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 30

[Mike Hommey [:glandium]]

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 31

[Nathan Froyd [:froydnj]]

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 32

[Nathan Froyd [:froydnj]]

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.

Comment 33

[Mike Hommey [:glandium]]

(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 34

[Mike Hommey [:glandium]]

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 35

[Nathan Froyd [:froydnj]]

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.

Comment 36

[Pulsebot]

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

Comment 37

[Carsten Book [:Tomcat]]

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

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68da139d0866