578546 - warning C4290: C++ exception specification ignored except to indicate a function is not __declspec(nothrow)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Another big source of warning spam when building with MSVC10.

In dist\include\mozilla\mozalloc.h(224)

Comment 1

[David Zbarsky (:dzbarsky)]

This also happens on MSVC8.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Chris, any chance this can get any love? It makes it very hard to look for other warnings because this and bug 578540 are so overwhelmingly spammed to the console.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, was on PTO.  I won't be able to look at this until probably next week.  If someone else wants to take a crack, the patch will likely reduce to making the |throw()| specifier disappear on MSVC.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

According to MSDN, we can otherwise kill this with #pragma warning( disable : 4290 )
http://msdn.microsoft.com/en-us/library/sa28fef8%28VS.80%29.aspx

Otherwise, do I understand your last comment to mean changing the conditions on the ifdef on line #207 of mozalloc.h?
http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#207

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Otherwise, do I understand your last comment to mean changing the conditions on
> the ifdef on line #207 of mozalloc.h?
> http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#207

Sure, sounds good.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

To make sure I understand what's going on here, right now we're defining MOZALLOC_THROW_IF_HAS_EXCEPTIONS as throw() for all compilers other than Android. Since we aren't building with exceptions enabled on MSVC, the __declspec(nothrow) is being automatically used to disable that throw() along with warning about having done so.

Therefore, we should just change the ifdef on line #207 to include both Android and MSVC to make this warning go away, and doing so shouldn't have any other effect since the throw() is effectively being disabled anyway right now.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Attachment: Change ifdef on line 207 to include MSVC

Starting at line 207, I made the following change:
-#ifdef ANDROID
+#if defined(ANDROID) || defined(_MSC_VER)
 /*
  * Android doesn't fully support exceptions, so its <new> header
- * has operators that don't specify throw() at all.
+ * has operators that don't specify throw() at all. Also include MSVC
+ * to suppress build warning spam (bug 578546).
  */

Seems to have worked, except I'm realizing that we're also getting the same warning on lines 225 and 237 (on the MOZALLOC_THROW_BAD_ALLOC lines). However, I can't say with certainty that those are new and were just missed before due to the prevalence of the other warnings. Starting on line 217, we have:
#ifdef MOZ_CPP_EXCEPTIONS
#define MOZALLOC_THROW_BAD_ALLOC throw(std::bad_alloc)
#else
#define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_IF_HAS_EXCEPTIONS
#endif

So my assumption is the MOZ_CPP_EXCEPTIONS is set for MSVC and is hence giving us the throw(std::bad_alloc) line. Not sure how to proceed on this one since I'm assuming MOZ_CPP_EXCEPTIONS is defined on purpose here.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #6)
> To make sure I understand what's going on here, right now we're defining
> MOZALLOC_THROW_IF_HAS_EXCEPTIONS as throw() for all compilers other than
> Android. Since we aren't building with exceptions enabled on MSVC, the
> __declspec(nothrow) is being automatically used to disable that throw() along
> with warning about having done so.

Not quite ... the MSDN page explains

  A function is declared using exception specification, which Visual C++ accepts but does not implement. Code with exception specifications that are ignored during compilation may need to be recompiled and linked to be reused in future versions supporting exception specifications.

VC parses |throw([X])| syntax but ignores it thereafter.  Technically these functions *are* __declspec(nothrow) but since we don't build with exceptions it shouldn't matter.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

MOZ_CPP_EXCEPTIONS shouldn't be set unless you build with --enable-cpp-exceptions, or are in code where it's forced on.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looks like MOZ_CPP_EXCEPTIONS is coming in from the IPC code setting ENABLE_CXX_EXCEPTIONS=1 which then adds MOZ_CPP_EXCEPTIONS=1 to CXXFLAGS. Another one of those situations where I don't understand the code well enough to know what we should do here.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Forgot to say, rules.mk is what does the CXXFLAGS setting.
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#375

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Attachment: Change second ifdef to exclude MSVC

This works.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hmm, I sure thought we had removed ENABLE_CXX_EXCEPTIONS=1 from the IPC stuff.  bent, do you remember why we needed it in the first place?

Ryan, I would try building without it.  It'd be better to just remove that than work around it.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

hmm, ENABLE_CXX_EXCEPTIONS=1 is in more places than just the IPC code. Not sure if it's safe to remove them all...
http://mxr.mozilla.org/mozilla-central/search?find=%2F&string=ENABLE_CXX_EXCEPTIONS

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The only two I'm not sure about there are in lib7z and widget/src/windows.  The others were added for ipc code or testing (or activex, which we don't care about).  widget/src/windows is IPC-enabled now (not sure why ...) so I'm pretty sure we ENABLE_CXX_EXCEPTIONS=1 there for that too.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

widget/src/windows goes back to the original cvs-->hg merge. I'm going to try removing it and see what happens. However, I don't see anything within widget/src/windows actually using MOZ_CPP_EXCEPTIONS or MOZ_CXX_EXCEPTIONS, so I'm going to go ahead and just remove it as well.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Remove ENABLE_CXX_EXCEPTIONS from tree

Warning C4290 is completely gone with this patch and the first one applied. Victory!

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 488379 [details] [diff] [review]
Remove ENABLE_CXX_EXCEPTIONS from tree

Looks awesome, if tryserver is happy :).

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Passes try with only the usual random orange suspects. Who can a2.0+ these patches?

Comment 20

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/539b6dd6ded6
http://hg.mozilla.org/mozilla-central/rev/a42e9b001bc8