memory/mozalloc/throw_gcc.h:35:1: note: declaration missing '[[noreturn]]' (libc++ 4.0)

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: jbeich, Assigned: dimitry, NeedInfo)

Tracking

Trunk
mozilla54
Unspecified
FreeBSD
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 affected, thunderbird_esr45 affected, firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

3 years ago
Posted file c++ -E output
On platforms using config/gcc_hidden.h (per bug 273336) and with libc++ (FreeBSD and Android after bug 1260208) the build may break after https://github.com/llvm-mirror/libcxx/commit/14c09a2413ed

$ cc -v
FreeBSD clang version 4.0.0 (trunk 291274) (based on LLVM 4.0.0svn)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin
Found CUDA installation: /usr/local/cuda, version 7.5

$ ./mach build
[...]
In file included from mozglue/misc/TimeStamp.cpp:11:
In file included from objdir/dist/include/mozilla/TimeStamp.h:11:
In file included from objdir/dist/stl_wrappers/algorithm:44:
In file included from objdir/dist/system_wrappers/algorithm:3:
In file included from /usr/include/c++/v1/algorithm:640:
In file included from objdir/dist/stl_wrappers/memory:44:
In file included from objdir/dist/system_wrappers/memory:3:
In file included from /usr/include/c++/v1/memory:632:
In file included from objdir/dist/stl_wrappers/new:44:
In file included from objdir/dist/system_wrappers/new:3:
/usr/include/c++/v1/new:132:1: error: function declared '[[noreturn]]' after its first declaration
_LIBCPP_NORETURN _LIBCPP_FUNC_VIS void __throw_bad_alloc();  // not in C++ spec
^
/usr/include/c++/v1/__config:273:30: note: expanded from macro '_LIBCPP_NORETURN'
#  define _LIBCPP_NORETURN [[noreturn]]
                             ^
objdir/dist/include/mozilla/throw_gcc.h:35:1: note:
      declaration missing '[[noreturn]]' attribute is here
__throw_bad_alloc(void)
^
1 error generated.
Comment hidden (mozreview-request)
Reporter

Comment 2

3 years ago
mozreview-review
Comment on attachment 8824829 [details]
Bug 1329520 - Switch MOZ_NORETURN to C++11-style attribute for compatibility with libc++ 4.0.

https://reviewboard.mozilla.org/r/103096/#review103694

::: js/public/Utility.h:41
(Diff revision 1)
>  namespace js {}
>  
>  #define JS_STATIC_ASSERT(cond)           static_assert(cond, "JS_STATIC_ASSERT")
>  #define JS_STATIC_ASSERT_IF(cond, expr)  MOZ_STATIC_ASSERT_IF(cond, expr, "JS_STATIC_ASSERT_IF")
>  
> -extern MOZ_NORETURN MOZ_COLD JS_PUBLIC_API(void)
> +MOZ_NORETURN MOZ_COLD extern JS_PUBLIC_API(void)

Probably a result of namespace pollution. Applying to MFBT would aggravate such bustage.

```c++
In file included from ipc/contentproc/plugin-container.cpp:8:
In file included from objdir/dist/include/nsXULAppAPI.h:15:
In file included from objdir/dist/include/mozilla/XREAppData.h:12:
In file included from objdir/dist/include/nsCOMPtr.h:33:
In file included from objdir/dist/include/nsCycleCollectionNoteChild.h:13:
In file included from objdir/dist/include/nsCycleCollectionTraversalCallback.h:11:
In file included from objdir/dist/include/js/HeapAPI.h:15:
objdir/dist/include/js/Utility.h:41:8: error: an attribute list cannot appear here
extern MOZ_NORETURN MOZ_COLD JS_PUBLIC_API(void)
       ^~~~~~~~~~~~
objdir/dist/include/mozilla/throw_gcc.h:25:22: note: expanded from macro 'MOZ_NORETURN'
#define MOZ_NORETURN _LIBCPP_NORETURN
                     ^~~~~~~~~~~~~~~~
/usr/include/c++/v1/__config:273:28: note: expanded from macro '_LIBCPP_NORETURN'
#  define _LIBCPP_NORETURN [[noreturn]]
                           ^~~~~~~~~~~~
1 error generated.
```
Reporter

Comment 3

3 years ago
mozreview-review-reply
Comment on attachment 8824829 [details]
Bug 1329520 - Switch MOZ_NORETURN to C++11-style attribute for compatibility with libc++ 4.0.

https://reviewboard.mozilla.org/r/103096/#review103694

Consider this v0. Actual fix is probably to use `[[noreturn]]` in MFBT.
Comment hidden (mozreview-request)
Reporter

Comment 5

3 years ago
The underlying issue here is a generic Clang vs. GCC incompatibility. However, libc++ is rarely used together with GCC and OS X opts out of using mozilla/throw_gcc.h by not defining WRAP_STL_INCLUDES.

$ cat >a.cc
#include <stdlib.h>

__attribute__((noinline)) [[noreturn]] static void foo(void) 
{
  exit(0);
}

int main(void)
{
  foo();
}

$ g++6 -std=c++11 a.cc

$ clang++ -std=c++11 a.cc
a.cc:3:27: error: an attribute list cannot appear here
__attribute__((noinline)) [[noreturn]] static void foo(void)
                          ^~~~~~~~~~~~
1 error generated.
Reporter

Comment 6

3 years ago
mozreview-review-reply
Comment on attachment 8824829 [details]
Bug 1329520 - Switch MOZ_NORETURN to C++11-style attribute for compatibility with libc++ 4.0.

https://reviewboard.mozilla.org/r/103096/#review103694

```c++
In file included from obj-firefox/gfx/tests/gtest/Unified_cpp_gfx_tests_gtest0.cpp:2:
In file included from gfx/2d/unittest/TestBase.cpp:6:
In file included from gfx/2d/unittest/TestBase.h:8:
In file included from obj-firefox/dist/stl_wrappers/string:44:
In file included from obj-firefox/dist/system_wrappers/string:3:
In file included from clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/string:40:
In file included from clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/char_traits.h:39:
In file included from clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/stl_algobase.h:64:
In file included from clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/stl_pair.h:59:
In file included from clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/move.h:57:
In file included from obj-firefox/dist/stl_wrappers/type_traits:66:
obj-firefox/dist/include/mozilla/throw_gcc.h:28:1: error: function declared '[[noreturn]]' after its first declaration
MOZ_NORETURN MOZ_ALWAYS_INLINE MOZ_EXPORT void
^
obj-firefox/dist/include/mozilla/Attributes.h:112:33: note: expanded from macro 'MOZ_NORETURN'
#  define MOZ_NORETURN          MOZ_HAVE_NORETURN
                                ^
obj-firefox/dist/include/mozilla/Attributes.h:65:44: note: expanded from macro 'MOZ_HAVE_NORETURN'
#      define MOZ_HAVE_NORETURN          [[noreturn]]
                                           ^
clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/functexcept.h:48:3: note: declaration missing '[[noreturn]]' attribute is here
  __throw_bad_exception(void) __attribute__((__noreturn__));
  ^
```
Reporter

Updated

3 years ago
Attachment #8824829 - Flags: review?(nfroyd)
Reporter

Comment 7

3 years ago
Dimitry, is it possible to force libc++ use __attribute__((noreturn)) over [[return]] even in C++11 mode for compatibility with libstdc++? I can't test Clang on Linux behavior (outside of Try builds) because on FreeBSD --gcc-toolchain or -stdlib=libstdc++ are no longer supported.
Flags: needinfo?(dimitry)
Assignee

Comment 8

3 years ago
(In reply to Jan Beich from comment #7)
> Dimitry, is it possible to force libc++ use __attribute__((noreturn)) over
> [[return]] even in C++11 mode for compatibility with libstdc++?

The declaration in libc++ of its own macro for this purpose goes like this:

#if __has_feature(cxx_attributes)
#  define _LIBCPP_NORETURN [[noreturn]]
#else
#  define _LIBCPP_NORETURN __attribute__ ((noreturn))
#endif

and the cxx_attributes feature is always enabled for C++11.

That said, the complaint from comment 6 seems to indicate that it doesn't accept "mixing and matching" __attribute__((noreturn)) and [[noreturn]] either.  For example, this little fragment:

__attribute__((noreturn)) void foo(void);
[[noreturn]] void foo(void);

isn't accepted by clang in C++11 mode:

foo.cpp:2:3: error: function declared '[[noreturn]]' after its first declaration
[[noreturn]] void foo(void);
  ^
foo.cpp:1:32: note: declaration missing '[[noreturn]]' attribute is here
__attribute__((noreturn)) void foo(void);
                               ^
1 error generated.

while g++ 6.2.0 does accept it.  I'm unsure what to do here.

One question is why does throw_gcc.h try to redeclare __throw_bad_exception at all? Can't it just use the declaration from the system headers?
From the header itself:
// For gcc, we define these inline to abort so that we're absolutely
// certain that (i) no exceptions are thrown from Gecko; (ii) these
// errors are always terminal and caught by breakpad.
Reporter

Comment 10

3 years ago
(In reply to Dimitry Andric from comment #8)
> isn't accepted by clang in C++11 mode:
> 
> foo.cpp:2:3: error: function declared '[[noreturn]]' after its first
> declaration
> [[noreturn]] void foo(void);
>   ^
> foo.cpp:1:32: note: declaration missing '[[noreturn]]' attribute is here
> __attribute__((noreturn)) void foo(void);

__attribute__((foo)) often translates to [[gnu::foo]] but [[noreturn]] is special because it's also part of C++11. If [[gnu::noreturn]] and [[noreturn]] have different semantics then it's understandable why Clang throws an error. However, the following is accepted

  [[noreturn]] void foo(void);
  [[gnu::noreturn]] void foo(void);

but if swapped it's not

  [[gnu::noreturn]] void foo(void);
  [[noreturn]] void foo(void);
Reporter

Updated

3 years ago
Attachment #8824829 - Attachment is obsolete: true
Assignee

Comment 11

3 years ago
I've proposed a fix for libc++ here: https://reviews.llvm.org/D28981

Comment 12

2 years ago
__throw_bad_alloc is an internal implementation detail of libstdc++ and mozilla has no business whatsoever trying to redefine it. Get off my lawn.

Comment 13

2 years ago
But if you do insist on this kind of foul play, it's your job to get it right, you can't expect standard library implementations to adjust their implementation details to accommodate misuse of them.

I'm half tempted to randomly add and remove attributes from the libstdc++ header to prevent this kind of tomfoolery.

Comment 14

2 years ago
I suggest making a feature request to libstdc++ so that (for example) the functions get defined as inline when a particular macro is defined. That would be a supported API for doing what you want, not a hack that subverts the standard library's implementation details.

I'm certainly in favour of making -fno-exceptions more useful with libstdc++ (because currently you need to recompile libstdc++ itself to really get no exceptions). But not by some unsupported backdoor.
(In reply to Jonathan Wakely from comment #14)
> I suggest making a feature request to libstdc++ so that (for example) the
> functions get defined as inline when a particular macro is defined. That
> would be a supported API for doing what you want, not a hack that subverts
> the standard library's implementation details.

This probably would have been better than whatever we do now.  But sometimes the expedient hack is more useful than the proper fix (especially with the GCC review -> release -> usable in Firefox lead cycle).  Propriety of futzing with implementation details aside, what we have at least doesn't change the contract of the implementation-defined functions, but simply makes them usable with -fno-exceptions.

There's a bug filed for this upstream: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69388

> I'm certainly in favour of making -fno-exceptions more useful with libstdc++
> (because currently you need to recompile libstdc++ itself to really get no
> exceptions). But not by some unsupported backdoor.

We would love to work on a solution that's acceptable to the libstdc++ maintainers.  But this particular bug is probably not the place to do that.

Comment 16

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #15)
> There's a bug filed for this upstream:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69388

Oh yes, I created that last time I was offended by these replacements.

> > I'm certainly in favour of making -fno-exceptions more useful with libstdc++
> > (because currently you need to recompile libstdc++ itself to really get no
> > exceptions). But not by some unsupported backdoor.
> 
> We would love to work on a solution that's acceptable to the libstdc++
> maintainers.  But this particular bug is probably not the place to do that.

Agreed, the GCC bug is where it belongs. Thanks for reminding me of it.
Assignee

Comment 17

2 years ago
Here's the patch I have submitted to the FreeBSD bug.  This prefixes all the __throw_xxx functions with [[noreturn]], but *only* for clang, and specifically for libc++ 4.0.0 and higher.
Reporter

Comment 18

2 years ago
Comment on attachment 8829928 [details] [diff] [review]
mozcentral-fix-libc++-noreturn-1.diff

How hard it'd be to fix comment 10 instead? Does MSVC accept the following form unlike clang-cl?

  void foo(void);
  [[noreturn]] void foo(void);

Otherwise, mozilla/throw_gcc.h can be turned off for more libc++ platforms following OS X due to lacking wrappers for

  __throw_bad_any_cast
  __throw_bad_array_length
  __throw_bad_optional_access
  __throw_bad_variant_access
  __throw_bad_weak_ptr
(In reply to Nathan Froyd [:froydnj] from comment #15)
> This probably would have been better than whatever we do now.  But sometimes
> the expedient hack is more useful than the proper fix (especially with the
> GCC review -> release -> usable in Firefox lead cycle).

Considering we rely on system libstdc++, the "usable in Firefox lead cycle" starts with "is available on all supported distros".

Comment 21

2 years ago
(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Nathan Froyd [:froydnj] from comment #15)
> > This probably would have been better than whatever we do now.  But sometimes
> > the expedient hack is more useful than the proper fix (especially with the
> > GCC review -> release -> usable in Firefox lead cycle).
> 
> Considering we rely on system libstdc++, the "usable in Firefox lead cycle"
> starts with "is available on all supported distros".

That doesn't make it right to rely on the hacks forever. Start the conversation upstream and eventually it will be fixed and you can remove these hacks. Don't have the conversation and you'll be trying to be compatible with internal implementation details (on all supported distros!) forever.
Assignee

Comment 22

2 years ago
The patch I submitted in comment 17 did not compile with gcc, since it does not recognize __has_feature(), sorry about that.  Here is a second patch, that ensures __has_feature is defined if it isn't available.
Attachment #8829928 - Attachment is obsolete: true

Comment 23

2 years ago
It's undefined behaviour to #define __has_feature, and even ignoring that it's a bad idea to do so in a header because other code might try to use it and could assume the fact it's defined means it's supported by the compiler. Mozilla headers should not be defining macros in the implementation namespace. Mozilla is not the C++ implementation.

This seems a better approach:

#if defined(__clang__)
#  if __has_feature(cxx_attributes) \
     && defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 4000
#    define MOZ_NORETURN_CXX11 [[noreturn]]
#  endif
#endif
#ifndef MOZ_NORETURN_CXX11
#  define MOZ_NORETURN_CXX11
#endif
Assignee

Comment 24

2 years ago
I'm fine with any approach that works, so that looks good to me.  (It could be that very old versions of clang don't support __has_feature(), but we should probably not bother with those anyway.)
Assignee

Comment 25

2 years ago
Updated with Jonathan's suggestion.
Attachment #8830273 - Attachment is obsolete: true
Reporter

Comment 26

2 years ago
Comment on attachment 8830499 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

https://treeherder.mozilla.org/#/jobs?repo=try&revision=702a46a802a1920fd46bf40158b0744e232b0d07
Attachment #8830499 - Flags: review?(nfroyd)
Comment on attachment 8830499 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

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

This seems mostly OK.  I'm cancelling the review just because I want to know whether my suggestion below will work, but if it's not feasible, I have no problem r+'ing the revised patch.

::: memory/mozalloc/throw_gcc.h
@@ +25,5 @@
> +#    define MOZ_NORETURN_CXX11 [[noreturn]]
> +#  endif
> +#endif
> +#ifndef MOZ_NORETURN_CXX11
> +#  define MOZ_NORETURN_CXX11

Whatever macro we #define here, we need to #undef at the end of the file.

Also, there needs to be a comment somewhere about why we have this separate noreturn attribute for these cases.

Can we do something like:

#if defined(__clang__)
  ... #define MOZ_NORETURN_THROW [[noreturn]]
#endif
#ifndef MOZ_NORETURN_THROW
#  define MOZ_NORETURN_THROW MOZ_NORETURN
#endif

MOZ_EXPORT MOZ_NORETURN_THROW MOZ_ALWAYS_INLINE void
__throw_foo(void)
{
  ...
}

?  We might have to swap the order of some of these depending on how [[attribute]] in the grammer intermixes with __attribute__, but that seems nicer to read than MOZ_NORETURN_CXX11...MOZ_NORETURN.
Attachment #8830499 - Flags: review?(nfroyd) → feedback+
Assignee

Comment 28

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #27)
> ::: memory/mozalloc/throw_gcc.h
> @@ +25,5 @@
> > +#    define MOZ_NORETURN_CXX11 [[noreturn]]
> > +#  endif
> > +#endif
> > +#ifndef MOZ_NORETURN_CXX11
> > +#  define MOZ_NORETURN_CXX11
> 
> Whatever macro we #define here, we need to #undef at the end of the file.

Ah yes, understood.


> Also, there needs to be a comment somewhere about why we have this separate
> noreturn attribute for these cases.

Sure, I can add that just before the #ifdef block.


> Can we do something like:
> 
> #if defined(__clang__)
>   ... #define MOZ_NORETURN_THROW [[noreturn]]
> #endif
> #ifndef MOZ_NORETURN_THROW
> #  define MOZ_NORETURN_THROW MOZ_NORETURN
> #endif
> 
> MOZ_EXPORT MOZ_NORETURN_THROW MOZ_ALWAYS_INLINE void
> __throw_foo(void)
> {
>   ...
> }
> 
> ?  We might have to swap the order of some of these depending on how
> [[attribute]] in the grammer intermixes with __attribute__, but that seems
> nicer to read than MOZ_NORETURN_CXX11...MOZ_NORETURN.

Well, the [[noreturn]] attribute must appear first on the line, before any __attribute__ ones, otherwise clang won't accept it.  (Apparently gcc has no such qualms. :)

I tried to avoid swapping the MOZ_EXPORT and MOZ_NORETURN, since I am not entirely sure how that plays out on e.g. Windows, where MOZ_EXPORT expands to __declspec(export), I believe.  Does Microsoft C++ accept __declspec(noreturn) before __declspec(export)?  If so, your approach is probably fine.
(In reply to Dimitry Andric from comment #28)
> (In reply to Nathan Froyd [:froydnj] from comment #27)
> > Can we do something like:
> > 
> > #if defined(__clang__)
> >   ... #define MOZ_NORETURN_THROW [[noreturn]]
> > #endif
> > #ifndef MOZ_NORETURN_THROW
> > #  define MOZ_NORETURN_THROW MOZ_NORETURN
> > #endif
> > 
> > MOZ_EXPORT MOZ_NORETURN_THROW MOZ_ALWAYS_INLINE void
> > __throw_foo(void)
> > {
> >   ...
> > }
> > 
> > ?  We might have to swap the order of some of these depending on how
> > [[attribute]] in the grammer intermixes with __attribute__, but that seems
> > nicer to read than MOZ_NORETURN_CXX11...MOZ_NORETURN.
> 
> Well, the [[noreturn]] attribute must appear first on the line, before any
> __attribute__ ones, otherwise clang won't accept it.  (Apparently gcc has no
> such qualms. :)

That's unfortunate, but I guess it makes some things inside clang easier.

> I tried to avoid swapping the MOZ_EXPORT and MOZ_NORETURN, since I am not
> entirely sure how that plays out on e.g. Windows, where MOZ_EXPORT expands
> to __declspec(export), I believe.  Does Microsoft C++ accept
> __declspec(noreturn) before __declspec(export)?  If so, your approach is
> probably fine.

I don't know.  I suspect that it does, since they're both just __declspec and for there to be an ordering requirement between __declspecs would be...unusual.  I am willing to try test patches for you, or you can request tryserver access:

https://wiki.mozilla.org/ReleaseEngineering/TryServer

and push test patches yourself.
Assignee

Comment 30

2 years ago
Updated with Nathan's suggestions from comment 27:
* Add explanatory comment before defining MOZ_NORETURN_THROW
* Use only one instance of MOZ_NORETURN_THROW, at the start of function declarations
* Undefine MOZ_NORETURN_THROW at the end of the header
Attachment #8830499 - Attachment is obsolete: true
Reporter

Comment 31

2 years ago
Comment on attachment 8831204 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5e46bbf599a92fb32e34baa12a9882dcd8c5043
Attachment #8831204 - Flags: review?(nfroyd)
Comment on attachment 8831204 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

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

This looks fine; two issues noted below.

::: memory/mozalloc/throw_gcc.h
@@ +37,5 @@
>  // NB: user code is not supposed to touch the std:: namespace.  We're
>  // doing this after careful review because we want to define our own
>  // exception throwing semantics.  Don't try this at home!
>  
> +MOZ_NORETURN_THROW MOZ_EXPORT MOZ_ALWAYS_INLINE void

I think this reads slightly better as MOZ_THROW_NORETURN; the MOZ_NORETURN at the beginning of the line threw me, and then I realized there was an underscore following it, instead of the space.

WDYT?  Can you s/MOZ_NORETURN_THROW/MOZ_THROW_NORETURN/g if you agree?

@@ +140,4 @@
>  
>  } // namespace std
>  
> +#undef MOZ_RETURN_THROW

Surely this should be MOZ_NORETURN_THROW?
Attachment #8831204 - Flags: review?(nfroyd) → review+
Assignee

Comment 33

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #32)
> > +MOZ_NORETURN_THROW MOZ_EXPORT MOZ_ALWAYS_INLINE void
> 
> I think this reads slightly better as MOZ_THROW_NORETURN; the MOZ_NORETURN
> at the beginning of the line threw me, and then I realized there was an
> underscore following it, instead of the space.
> 
> WDYT?  Can you s/MOZ_NORETURN_THROW/MOZ_THROW_NORETURN/g if you agree?

Sure, substituted.


> > +#undef MOZ_RETURN_THROW
> 
> Surely this should be MOZ_NORETURN_THROW?

Yes, that was a copy/paste error.  Fixed it now.
Attachment #8831204 - Attachment is obsolete: true
Reporter

Comment 34

2 years ago
Comment on attachment 8831309 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1504807bebe2f7cc0e49ee39ac829f0452acf6e2
Attachment #8831309 - Flags: review?(nfroyd)
Attachment #8831309 - Flags: review?(nfroyd) → review+
Thank you!
Reporter

Updated

2 years ago
Keywords: checkin-needed

Comment 36

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b96baf833a5
Fix "memory/mozalloc/throw_gcc.h:35:1: note: declaration missing '[[noreturn]]' (libc++ 4.0)" r=nfroyd
Keywords: checkin-needed
Reporter

Updated

2 years ago
Assignee: nobody → dimitry

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b96baf833a5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter

Comment 38

2 years ago
Comment on attachment 8831309 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

Would be nice to have on ESR52 to help regression testing downstream. For ESR45 it's too late (only 2 releases are left) and the patch doesn't apply cleanly, anyway.

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Broken build on platforms shipping Clang/libc++ 4.0 by default e.g., FreeBSD 11.1, OS X 10.13
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, by me and dowstream e.g.,
http://package18.nyi.freebsd.org/data/headi386PR216008-default/2017-02-01_06h23m11s/logs/firefox-51.0.1,1.log
[Needs manual test from QE? If yes, steps to reproduce]: Probably not.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only break build for Linux and Android. Windows uses memory/mozalloc/throw_msvc.h instead while OS X - neither.
[String changes made/needed]: None
Attachment #8831309 - Flags: approval-mozilla-beta?
Attachment #8831309 - Flags: approval-mozilla-aurora?
Comment on attachment 8831309 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

Fix a build issue on platforms shipping Clang/libc++ 4.0 by default. Aurora53+.
Attachment #8831309 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8831309 [details] [diff] [review]
Use [[noreturn]] for __throw_xxx wrappers, if libc++ 4.0.0 or higher

work around libc++ wonkiness, beta52+
Attachment #8831309 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1337119
Too late for 51. Mark 51 won't fix.

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.