Closed Bug 578546 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: XPCOM, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

(Whiteboard: [build_warning])

Attachments

(2 files, 1 obsolete file)

Another big source of warning spam when building with MSVC10.

In dist\include\mozilla\mozalloc.h(224)
This also happens on MSVC8.
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.
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.
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
(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.
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.
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.
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #487798 - Flags: review?(jones.chris.g)
(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.
MOZ_CPP_EXCEPTIONS shouldn't be set unless you build with --enable-cpp-exceptions, or are in code where it's forced on.
Attachment #487798 - Flags: review?(jones.chris.g) → review+
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.
Attachment #487798 - Flags: approval2.0?
Forgot to say, rules.mk is what does the CXXFLAGS setting.
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#375
This works.
Attachment #488020 - Flags: review?(jones.chris.g)
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.
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
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.
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.
Warning C4290 is completely gone with this patch and the first one applied. Victory!
Attachment #488020 - Attachment is obsolete: true
Attachment #488379 - Flags: review?(jones.chris.g)
Attachment #488379 - Flags: approval2.0?
Attachment #488020 - Flags: review?(jones.chris.g)
Comment on attachment 488379 [details] [diff] [review]
Remove ENABLE_CXX_EXCEPTIONS from tree

Looks awesome, if tryserver is happy :).
Attachment #488379 - Flags: review?(jones.chris.g) → review+
Passes try with only the usual random orange suspects. Who can a2.0+ these patches?
Attachment #487798 - Flags: approval2.0? → approval2.0+
Attachment #488379 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/539b6dd6ded6
http://hg.mozilla.org/mozilla-central/rev/a42e9b001bc8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Depends on: 614631
You need to log in before you can comment on or make changes to this bug.