616704 - nsFastLoadFile.cpp fails to compile on mingw

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[Jacek Caban]

Attachment: fix v1.0

After langing bug 588873, nsFastLoadFile.cpp uses SEH exceptions that are not (yet) supported by GCC. Bug 614631 (waiting for approval to land) introduces new macros to deal with it in a more generic way. My proposed fix changes the code to use those macros instead of MOZ_WIN_MEM_TRY_* ones, fixing the compilation.

Comment 1

[Benedict Hsieh [:benedict]]

I think you'd be better off asking Taras for a review on this. I do like that the new macros have curly braces in the try-block. Is the lack of space between brace and catch like

>}MOZ_SEH_EXCEPT(GetExceptionCode() == EXCEPTION_IN_PAGE_ERROR

an unfortunate necessity?

Comment 2

[(dormant account)]

Comment on attachment 495254 [details] [diff] [review]
fix v1.0

> #ifdef XP_WIN
> #include <windows.h>
> #include "private/pprio.h"  // To get PR_ImportFile
>-
>-#define MOZ_WIN_MEM_TRY_BEGIN __try {
>-#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                \
>-  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \
>-           EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH)   \
>-  {                                                                 \
>-    NS_WARNING("EXCEPTION_IN_PAGE_ERROR in " __FUNCTION__);         \
>-    cmd;                                                            \
>-  }
>-#else
>-#define MOZ_WIN_MEM_TRY_BEGIN {
>-#define MOZ_WIN_MEM_TRY_CATCH(cmd) }
> #endif

So why not do the simple thing and and change this to 
#ifdef XP_WIN
include windows.h/pprio.h
#endif

Then for the rest of the stuff change the guard to
#if defined(XP_WIN) and defined(_MSC_VER)

Comment 3

[Jacek Caban]

(In reply to comment #2)

> So why not do the simple thing and and change this to 
> #ifdef XP_WIN
> include windows.h/pprio.h
> #endif
> 
> Then for the rest of the stuff change the guard to
> #if defined(XP_WIN) and defined(_MSC_VER)

Having it centralized and depending on configure script is better for at least two reasons:

1. No logic duplication. We don't need to expand #ifdef logic every time we have to use SEH exceptions. This bug is a good example, that having such custom solutions for each use case leads to problems.

2. General good practice is that compiler capabilities checking belongs to configure script. The whole SEH problem is a good example: GCC is going to support it for win64 in 4.6 version and win32 probably in 4.7. Once the first version will be released, all we have to do to enable it everywhere without breaking backward compatibility is a simple check in configure.in. Otherwise I will have to go through all use cases and add yet another #ifdef changes.

Why don't you like my patch?

Comment 4

[Jacek Caban]

It's no longer a problem since bug 588873 backout