nsFastLoadFile.cpp fails to compile on mingw

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
8 years ago
8 years ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

Trunk
x86_64
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
Created attachment 495254 [details] [diff] [review]
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.
Attachment #495254 - Flags: review?(ben.hsieh)
(Assignee)

Updated

8 years ago
Assignee: nobody → jacek
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

8 years ago
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)
Attachment #495254 - Flags: review?(ben.hsieh) → review-
(Assignee)

Comment 3

8 years ago
(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?
(Assignee)

Comment 4

8 years ago
It's no longer a problem since bug 588873 backout
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.