614631 - Landing bug 578546 broke mingw compilation.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jacek Caban]

Attachment: fix v1.0

It removed |+ENABLE_CXX_EXCEPTIONS = 1| from all makefiles, but in some cases it's needed. Both modules/plugin/base/src/ and widget/src/windows really use exceptions and GCC compilation fails if exceptions are not enabled. The attached patch partially reverts the patch from bug 578546.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Really? A MXR search for ENABLE_CXX_EXCEPTIONS or MOZ_CPP_EXCEPTIONS doesn't show anything in either of those dirs.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

... that's his point, isn't it?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Wouldn't there be ifdefs present if it were being used?

Comment 4

[Justin Wood (:Callek)]

(In reply to comment #3)
> Wouldn't there be ifdefs present if it were being used?

http://mxr.mozilla.org/mozilla-central/source/config/rules.mk?mark=375-377#372

The ifdef is present in the Make compilation. And adds it to CXX Flags, and is a syntactic compilation flag. So thats why this is needed in those DIR's.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

But where is MOZ_CPP_EXCEPTIONS being used?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

OK, I think I get it now. It's not the MOZ_CPP_EXCEPTIONS being used but the MOZ_EXCEPTIONS_FLAGS_ON, right? If we're going to restore the ifdef, would it at least make sense to change the ifdef to one for MINGW rather than ifndef WINCE?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is this an issue with SEH or something else?

Comment 8

[Jacek Caban]

(In reply to comment #6)
> OK, I think I get it now. It's not the MOZ_CPP_EXCEPTIONS being used but the
> MOZ_EXCEPTIONS_FLAGS_ON, right?

Yes.

> If we're going to restore the ifdef, would it
> at least make sense to change the ifdef to one for MINGW rather than ifndef
> WINCE?

Isn't it sane to expect that we code that uses exceptions should be allowed to use MOZ_CPP_EXCEPTIONS? If it causes problems, then it should be fixed on different layer IMO. I will analyze it deeper tomorrow.

To be clear, I'm fine with defining it only for mingw, but it feels like a workaround for a deeper problem.

(In reply to comment #7)
> Is this an issue with SEH or something else?

We use standard try/catch syntax here, so although GCC won't use proper SEH for this, it can generate a proper code. The problem is parser that complains about using exceptions with -fno-exceptions flag.

Comment 9

[Benjamin Smedberg]

try/catch or __try/__catch? They are very different. The plugin code doesn't (or shouldn't) use try/catch, only __try/__catch. And as far as I know mingw doesn't support __try/__catch anyway.

I think we're in a similar situation for widget/src/windows. We only want do SEH, not C++ exception handling.

I really don't want to reintroduce -fexceptions.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

My MXR search of widget/src/windows found http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsFilePicker.cpp#239 which is real c++ exception handling

Comment 11

[Jacek Caban]

Attachment: SEH exception based on compiler configuration

I guess that widget code can also be changed to use __try/__except as that what's usually done to recover from crash. The attached patch is an attempt to avoid #ifdefs on each SEH exceptions use. It addes MOZ_SEH_TRY/MOZSEH_EXCEPT macros that are defined to __try/__except or faking them based on compiler. Currently it will be turned on on msvc only, but GCC is working on SEH support and once it will be released (GCC 4.6 should have it for win64, win32 will come later).

It will also allow us to get rid of custom attempts to implement similar feature like:
http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsZipArchive.h#60

As a side effect it fixes the original bug, as these exceptions are disabled for mingw. I'm going to push it to try server.

Comment 12

[Jacek Caban]

Compilation passed on try server.

Comment 13

[Benjamin Smedberg]

Comment on attachment 493295 [details] [diff] [review]
SEH exception based on compiler configuration

Please put the MOZ_SEH_* macros in nscore.h, not nsIException.idl. r=me with that change

Comment 14

[Jacek Caban]

Attachment: SEH exception based on compiler configuration (using nscore.h)

Thanks for the review. I've attached a fixed version.

Comment 15

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/808d5140670c