557060 - [Windows] After bug 551254, Firefox/SeaMonkey fails to build or run with error like "?_Throw@std@@YAXABVexception@stdext@@@Z"

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Serge Gautherie (:sgautherie)]

I downloaded the "most recent" build and discovered this failure.

I haven't verified but I assume this is a regression from bug 551254.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a4pre) Gecko/20100403 SeaMonkey/2.1a1pre] (nightly)

Fwiw, latest (optimized) nightly works fine.

Comment 2

[Serge Gautherie (:sgautherie)]

The oldest available build was already broken:
/pub/seamonkey/tinderbox-builds/comm-central-trunk-win32-debug/1270270824
"20100402220024 a0eee84feb71"

The newest optimized  build works fine:
/pub/seamonkey/tinderbox-builds/comm-central-trunk-win32/1270349077
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a4pre) Gecko/20100403 SeaMonkey/2.1a1pre]

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-CC) Copy (the useful part of) it [Checkin: See comment 6]

I assume this should help c-c ... If only we were told beforehand!

First reviewer will have my thanks.
(Anyone can check this in if I don't do it first.)

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 436904 [details] [diff] [review]
(Av1-CC) Copy (the useful part of) it
[Checkin: See comment 6]

did not do a "real" review; but as a skim this looks good; I either want to review myself; or have someone else review before we land this though.

Comment 5

[Justin Wood (:Callek)]

(In reply to comment #4)
> (From update of attachment 436904 [details] [diff] [review])
> did not do a "real" review; but as a skim this looks good; I either want to
> review myself; or have someone else review before we land this though.

For the record; if Chris Jones wants to do a review on this [rather than simply feedback], and thinks this will fix us, I'm happy to accept his r+ as "ok to land"

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 436904 [details] [diff] [review]
(Av1-CC) Copy (the useful part of) it
[Checkin: See comment 6]


http://hg.mozilla.org/comm-central/rev/8c3e917767fa
+
http://hg.mozilla.org/comm-central/rev/d0cd47f97d7c
(Bv1) Add dumb 'true'.

Comment 7

[Serge Gautherie (:sgautherie)]

The port is done :-) but it made no difference :-(

Comment 8

[Serge Gautherie (:sgautherie)]

An optimized SeaMonkey local compilation fails too:
{
Building deps for .../modules/libpr0n/build/nsImageModule.cpp
nsImageModule.cpp
.../modules/libpr0n/build/../decoders/bmp\nsBMPDecoder.h(123) : warning C4005: 'BI_RLE8' : macro redefinition
        X:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\include\wingdi.h(750) : see previous definition of 'BI_RLE8'
.../modules/libpr0n/build/../decoders/bmp\nsBMPDecoder.h(124) : warning C4005: 'BI_RLE4' : macro redefinition
        X:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\include\wingdi.h(751) : see previous definition of 'BI_RLE4'
.../modules/libpr0n/build/../decoders/bmp\nsBMPDecoder.h(125) : warning C4005: 'BI_BITFIELDS' : macro redefinition
        X:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\include\wingdi.h(752) : see previous definition of 'BI_BITFIELDS'
Creating Resource file: module.res
   Creating library fake.lib and object fake.exp
imglib2_s.lib(imgLoader.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) void __cdecl std::_Throw(class std::exception const &)" (__imp_?_Throw@std@@YAXABVexception@1@@Z) referenced in function "protected: virtual void __thiscall std::bad_alloc::_Doraise(void)const " (?_Doraise@bad_alloc@std@@MBEXXZ)
imglib2_s.lib(imgLoader.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ) referenced in function "protected: static void __cdecl std::vector<class nsRefPtr<class imgCacheEntry>,class std::allocator<class nsRefPtr<class imgCacheEntry> > >::_Xlen(void)" (?_Xlen@?$vector@V?$nsRefPtr@VimgCacheEntry@@@@V?$allocator@V?$nsRefPtr@VimgCacheEntry@@@@@std@@@std@@KAXXZ)

imglib2.dll : fatal error LNK1120: 2 unresolved externals
make[6]: *** [imglib2.dll] Error 96
}

Comment 9

[Serge Gautherie (:sgautherie)]

Previous summary was:
[Debug Windows SeaMonkey 2.1] Startup fails with Windows error dialog "?_Throw@std@@YAXABVexception@stdext@@@Z entry point not found in MSVCP80D.dll"

****

I just tried a Firefox build (Win2000 + 2003R2 SDK + ...):
{
Building deps for nsRDFResource.cpp
nsRDFResource.cpp
X:\Program Files\Microsoft Visual Studio 8\VC\INCLUDE\string.h(205) : warning C4273: 'moz_strdup' : inconsistent dll linkage
        ...\dist\include\mozilla/mozalloc.h(138): see previous definition of 'moz_strdup'
Creating Resource file: module.res
   Creating library xul.lib and object xul.exp
xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) void __cdecl std::_Throw(class std::exception const &)" (__imp_?_Throw@std@@YAXABVexception@1@@Z) referenced in function "protected: virtual void __thiscall std::bad_alloc::_Doraise(void)const " (?_Doraise@bad_alloc@std@@MBEXXZ)
imglib2.lib(imgLoader.obj) : error LNK2001: unresolved external symbol "__declspec(dllimport) void __cdecl std::_Throw(class std::exception const &)" (__imp_?_Throw@std@@YAXABVexception@1@@Z)
xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ) referenced in function "protected: static void __cdecl std::vector<class google_breakpad::ExceptionHandler *,class std::allocator<class google_breakpad::ExceptionHandler *> >::_Xran(void)" (?_Xran@?$vector@PAVExceptionHandler@google_breakpad@@V?$allocator@PAVExceptionHandler@google_breakpad@@@std@@@std@@KAXXZ)
imglib2.lib(imgLoader.obj) : error LNK2001: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ)
xul.dll : fatal error LNK1120: 2 unresolved externals
make[4]: *** [xul.dll] Error 96
make[4]: Leaving directory `.../toolkit/library'
}

The failure happens at a different location but seems to be basically the same issue.

Comment 10

[[e-mail]]

I was only able to get imgLoader.cpp and nptest.cpp to link by adding STL_FLAGS= and WRAP_STL_INCLUDES= to the appropriate Makefiles :-(

Comment 11

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

Attachment: Attempt to fix win2k bustage, v1

Works for VC9/Win7 SDK, VC9/server2k3 SDK gives redefinition error for std::exception::_Raise (which is expected).  Not sure that this will work, because my local build doesn't seem to be using the xpcom/glue/stdthrow.h definition of _Throw (might be a __declspec issue).

Serge, I'd really appreciate it if you could try this patch locally.  I'll resuming poking at this tomorrow.

Comment 12

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

Comment on attachment 436904 [details] [diff] [review]
(Av1-CC) Copy (the useful part of) it
[Checkin: See comment 6]

Looks OK to me.

Comment 13

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

(In reply to comment #10)
> I was only able to get imgLoader.cpp and nptest.cpp to link by adding
> STL_FLAGS= and WRAP_STL_INCLUDES= to the appropriate Makefiles :-(

The nptest problem is known, patch awaiting review.  Is the imgLoader error you saw what Serge reports above?

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 437803 [details] [diff] [review]
Attempt to fix win2k bustage, v1


With this patch:
{
Building deps for .../modules/libpr0n/src/imgLoader.cpp
imgLoader.cpp
../../../dist/include\mozilla/stdthrow.h(69) : error C2084: function 'void std::exception::_Raise(void) const' already has a body
        X:\Program Files\Microsoft Visual Studio 8\VC\INCLUDE\exception(260) : see previous definition of '_Raise'
make[5]: *** [imgLoader.obj] Error 2
}

Comment 15

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

Same compilation environment?  This error doesn't make sense to me given

(In reply to comment #9)
> I just tried a Firefox build (Win2000 + 2003R2 SDK + ...):
> {
[snip]
> xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol
> "__declspec(dllimport) public: void __thiscall
> std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ)
> referenced in function "protected: static void __cdecl std::vector<class
> google_breakpad::ExceptionHandler *,class std::allocator<class
> google_breakpad::ExceptionHandler *> >::_Xran(void)"
[snip]
> }

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)

> Same compilation environment?

Very same.

> This error doesn't make sense to me given

I'm not sure what you mean would make more sense?

Comment 17

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

(In reply to comment #16)
> (In reply to comment #15)
> > This error doesn't make sense to me given
> 
> I'm not sure what you mean would make more sense?

The original error message complained about std::exception::_Raise being undefined at link time.  I guessed that _Raise wasn't inlined or exported by your VC/SDK/CRT, so the v1 patch added a definition of _Raise.  But with patch applied, MSVC complained about _Raise being multiply defined, which means it *is* being defined inline in your toolkit's headers (the second definition comes from the v1 patch).  That confuses me.

Comment 18

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

Attachment: Attempt to fix win2k bustage, v2

Little more heavyweight solution, but has some ancillary benefits.  Eliminates the dependency on std::_Throw in my tests.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 438139 [details] [diff] [review]
Attempt to fix win2k bustage, v2


This patch fixes the _Throw cases, but not the _Raise ones:
{
nsRDFResource.cpp
Creating Resource file: module.res
   Creating library xul.lib and object xul.exp
xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ) referenced in function "protected: static void __cdecl std::vector<class google_breakpad::ExceptionHandler *,class std::allocator<class google_breakpad::ExceptionHandler *> >::_Xran(void)" (?_Xran@?$vector@PAVExceptionHandler@google_breakpad@@V?$allocator@PAVExceptionHandler@google_breakpad@@@std@@@std@@KAXXZ)
imglib2.lib(imgLoader.obj) : error LNK2001: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ)
xul.dll : fatal error LNK1120: 1 unresolved externals
make[4]: *** [xul.dll] Error 96
make[4]: Leaving directory `.../toolkit/library'
}

Nit: The nsprpub/configure diff should not be in this patch.

Comment 20

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

Sigh, had hoped fixing _Throw() would make these magically go away.

This error still doesn't make sense to me.  Serge, may I ask a favor?  Would you run

 $ make -C $objdir/modules/libpr0n/src imgLoader.i

and upload that?  (Warning: it'll be pretty large.)

Comment 21

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

Thanks Serge.  So, the problem is what I feared: these VC/SDK headers are idiotically declaring

  class __declspec(dllimport) exception {

even though all the methods of "exception" are inline.  __declspec(dllimport) semantics (which I learned about after the failed attempt of comment 11) say that even if a __declspec(dllimport) symbol has an inline definition, the translation unit retains a dependency on the __declspec(dllimport) symbol.  This means that even though std::exception::_Raise() is defined inline to call std::_Throw(), and we've interposed _Throw() successfully, libxul retains a meaningless dependency on the std::exception::_Raise symbol (would never be used).

This is good and bad news.  Good news is that while the v2 patch above won't compile on MSVC7/2k3SDK, tinderbox and release builds including it *will* run on win2k, because they won't emit the braindead dependency on std::exception::_Raise() and will have the interposed std::_Throw().  So I'll clean up that patch and get the ball rolling.  (The interposed _Throw() is useful for other reasons too.)

Bad news is that it looks to be much harder to work around the _Raise issue than the _Throw one.  I think the best solution there is just to disable STL wrappers for older build environments.

Serge, you said you were using the Win2k3 server SDK, but which compiler?  VC7?  (I tried the 2k3 SDK on my machine with MSVC9, but couldn't repro the bustage, so I think these headers are coming from MSVC.)

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)

> I think the best solution there is just to disable STL
> wrappers for older build environments.

Might be: it seems Neil (successfully) tried that in comment 10.

> Serge, you said you were using the Win2k3 server SDK, but which compiler?

'Visual C++ 8 Express' + 'Platform SDK for Windows Server 2003 R2' +
{
ac_add_options --with-windows-version=502
ac_add_options --disable-ctypes
ac_add_options --disable-ipc
}

Comment 23

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

Attachment: Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc

(r? -> ehsan for the MSVC bits, if he wishes to review them.)

Before getting into the means of this patch, consider the ends

 - Mozilla-interposed exception handling on MSVC and gcc.  We can define whatever "throwing an exception from the STL" should mean.
 - Centralized "throw" logic; MSVC and gcc share the same.
 - Exception safety (on Tier I's) for all of Gecko, including ipc/chromium/, for the first time ever AFAIK

Background: For code compiled by MSVC with _HAS_EXCEPTIONS=0, its headers call |std::_Throw(e)| instead of |throw e| when an exception is to be "thrown" from VC headers.  std::_Throw() prints an error message and then aborts().

Means: Let's pretend that this bug didn't exist, and std::_Throw() worked on all windows versions.  According to http://hg.mozilla.org/mozilla-central/file/b66b7330b505/xpcom/base/nsDebugImpl.cpp#l374, abort() can't be trusted to invoke breakpad on windows.  We definitely want breakpad for STL exceptions, so interposing a Mozilla std::_Throw() would be propitious.  This is a bug in its own right.

Interposing a Mozilla std::_Throw() is gross, granted.  This patch does |#define _Throw(e) mozilla_Throw(e)| before including <exception> (which declares std::_Throw), then exports std::mozilla_Throw from mozalloc.dll.  The indirection includes a compile-time guard that prevents any Gecko code from linking against std::_Throw(), so new code can't regress the interposition.

std::mozilla_Throw() does the same thing as the interposed std::__throw_[exception]()s on gcc: calls mozalloc_abort().  mozalloc_handle_oom() also calls that now.

This fixes Serge's bug because msvcrt7.dll (or whatever the win2k CRT is) doesn't export std::_Throw().  I think this omission is analogous to gcc's "support" for -fshort-wchar (and others): "Oh, you just need to recompile the entire runtime."  Luckily it's been fixed in later versions.

Comment 24

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

Serge, this build

  http://build.mozilla.org/tryserver-builds/[e-mail]-try-0291b4f0fc92

should work on win2k.  Do you mind trying it out?

Comment 25

[Serge Gautherie (:sgautherie)]

Optimized builds work fine:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 Minefield/3.7a5pre] (nightly)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 Minefield/3.7a5pre] (mozilla-central-win32/1270897138)
+
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 SeaMonkey/2.1a1pre] (nightly)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 SeaMonkey/2.1a1pre] (comm-central-trunk-win32/1270896177)

Debug builds fail to start:
Bug 557060 '_Throw' (+ Bug 558559 'RtlCaptureStackBackTrace') : [FF] (mozilla-central-win32-debug/1270891739)
Bug 557060 '_Throw' : [SM] (comm-central-trunk-win32-debug/1270896370)

As a reminder, there's 2 issues (wrt this bug) on (my) Windows 2000:
*Starting "official" Debug builds.
*Compiling local (Optimized) builds.


(In reply to comment #23)
> This fixes Serge's bug because msvcrt7.dll (or whatever the win2k CRT is)
> doesn't export std::_Throw().

(I'm not sure what this patch is fixing for me.)

Debug builds are loading msvcr80d.dll (v8.0.50727.42).
It's the only Debug dll I have, whereas I have (msvcr71.dll,) msvcr80.dll and msvcr90.dll.
Notice that Debug builds package their own msvcr80.dll (v8.0.50727.762, which they're not using):
Is it worth they package the Optimized dlls?
Should they package the 8.0 Debug dlls instead, fwiw!?
Should they moreover package 9.0 Debug dlls, if that exists??

NB: Optimized builds load their own mozcrt19.dll...


(In reply to comment #24)
> Do you mind trying it out?

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100409 Minefield/3.7a5pre] ([e-mail]-try-0291b4f0fc92)
"--enable-optimize --disable-debug --enable-jemalloc"

This Optimized try build _still_ works :-)

Yet, this (new) patch gives me the same comment 19 errors.
NB: Fwiw, my local builds are without --enable-jemalloc.

Comment 26

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

Which debug build fails to start?

To repeat what I said in comment 21, the latest patch won't fix your local build errors.  A configure.in patch is needed for that.

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)
> Which debug build fails to start?

Tinderbox ones.

Comment 28

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

OK.  That shouldn't have changed, since they're not building with attachment 438239 [details] [diff] [review].

Comment 29

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

Attachment: Check for |class __declspec(dllimport) exception| bug in older VC/SDKs [Checkin: Comment 46]

This (should) fix compilation errors by disabling STL_WRAPPERS for older build environments.  (Test wfm locally in that it doesn't detect the bug.)

Comment 30

[(no longer active)]

Comment on attachment 438239 [details] [diff] [review]
Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc

>-COMPILE_CXXFLAGS = $(VISIBILITY_FLAGS) $(STL_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
>+COMPILE_CXXFLAGS = $(STL_FLAGS) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)

Why are these changes necessary?

>diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
>+#if defined(_WIN32)
>+#  if !defined(WINCE)
>+    //This should exit us
>+    raise(SIGABRT);
>+#  endif

Would it make sense here to call RaiseException, at least for Windows CE?

>+    //If we are ignored exit this way..
>+    _exit(3);
>+#elif defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
>+    abort();
>+#else
>+#  warning not attempting to abort() on this platform

Shouldn't this be an #error?

>+#endif
>+
>+    // Still haven't aborted?  Try dereferencing null.
>+    // (Written this way to lessen the likelihood of it being optimized away.)
>+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
>+    // this is crazy

In order to make sure that we'll cause a crash, there is a better technique.

  __debugbreak();

This is guaranteed to work, and won't get optimized away.

>diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
>+#define _Throw(_e)  mozilla_Throw(_e)

Change this to:

#define _Throw mozilla_Throw

to make sure that any possible crazy code which attempts to get the address of std::_Throw is also handled.

>diff --git a/memory/mozalloc/throw_msvc.cpp b/memory/mozalloc/throw_msvc.cpp
>+namespace std {
>+
>+__declspec(dllexport) void mozilla_Throw(const exception& e);
>+
>+void
>+mozilla_Throw(const exception& e)
>+{
>+    mozalloc_abort(e.what());
>+}
>+
>+}  // namespace std

Since C++ programs aren't officially supposed to touch the std namespace, maybe add a comment here explaining why this is OK.  The same thing goes for the gcc version of this code.

Comment 31

[[e-mail]]

Comment on attachment 438279 [details] [diff] [review]
Check for |class __declspec(dllimport) exception| bug in older VC/SDKs
[Checkin: Comment 46]

This successfully disables the STL bits for me.

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 438279 [details] [diff] [review]
Check for |class __declspec(dllimport) exception| bug in older VC/SDKs
[Checkin: Comment 46]


With this patch alone, I can build Firefox again :-)

Does this have to be an AC_TRY_LINK() or could it be based on the SDK version or --with-windows-version value for example?
I'm not sure what disabling this "feature" means: would it make sense to explicitly "warn" the developer?

Comment 33

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

(In reply to comment #32)
> (From update of attachment 438279 [details] [diff] [review])
> Does this have to be an AC_TRY_LINK() or could it be based on the SDK version
> or --with-windows-version value for example?

It's not clear to me which VC/SDKs trigger the failure.  Based on what I've seen so far, it appears that

  VC9/win7:    fine (my build configuration)
  VC9/win2k3:  fine (I tested this)
  VC8/win7:    fine (build slaves)
  VC8/win2k3   broken (Serge's build config)

So, I prefer the feature test.

> I'm not sure what disabling this "feature" means: would it make sense to
> explicitly "warn" the developer?

It means the build won't be exception safe.  Not sure if it's worth warning about, since it's only disabled for pretty old build setups.

Comment 34

[Serge Gautherie (:sgautherie)]

(In reply to comment #33)

> So, I prefer the feature test.

Thanks for the list, it's just fine ftr ;-)

> It means the build won't be exception safe.

Ftr, could you give an example of such an exception (case) and how an unsafe build will behave?
(Also for "me" to know if/when it happens with such a build and not mistake that for a bug...)

Comment 35

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

(In reply to comment #34)
> (In reply to comment #33)
> > It means the build won't be exception safe.
> 
> Ftr, could you give an example of such an exception (case) and how an unsafe
> build will behave?
> (Also for "me" to know if/when it happens with such a build and not mistake
> that for a bug...)

Well TBH, Gecko has never been exception safe, so if you haven't seen any bugs yet you probably won't see new ones ;).  The problem with exceptions is plugins and binary extensions: if one installs an exception handler and calls into Gecko, and Gecko throws an exception, then the plugin/extension will let execution resume even though the heap is in an unknown and possibly inconsistent/corrupted state.

But, if your plugins and binary extensions don't do that, then in practice there's no difference between "exception safe" and "exception unsafe" builds.

Comment 36

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

(In reply to comment #30)
> (From update of attachment 438239 [details] [diff] [review])
> >-COMPILE_CXXFLAGS = $(VISIBILITY_FLAGS) $(STL_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> >+COMPILE_CXXFLAGS = $(STL_FLAGS) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> 
> Why are these changes necessary?
> 

gcc needs to find headers in $DIST/stl_wrappers before $DIST/system_wrappers, because the system_wrappers/ headers change symbol visibility, and stl_wrappers/ headers include other Gecko headers.

> >diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
> >+#if defined(_WIN32)
> >+#  if !defined(WINCE)
> >+    //This should exit us
> >+    raise(SIGABRT);
> >+#  endif
> 
> Would it make sense here to call RaiseException, at least for Windows CE?
> 

I don't know, would it?  I copied and pasted this code from xpcom/base/nsDebugImpl.cpp:Abort().

> >+    //If we are ignored exit this way..
> >+    _exit(3);
> >+#elif defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
> >+    abort();
> >+#else
> >+#  warning not attempting to abort() on this platform
> 
> Shouldn't this be an #error?
> 

Why do you think so?  We fall through to code that dereferences NULL and then calls _exit(), so I'm not sure that the lack of abort() adds risk.

> >+#endif
> >+
> >+    // Still haven't aborted?  Try dereferencing null.
> >+    // (Written this way to lessen the likelihood of it being optimized away.)
> >+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
> >+    // this is crazy
> 
> In order to make sure that we'll cause a crash, there is a better technique.
> 
>   __debugbreak();
> 
> This is guaranteed to work, and won't get optimized away.
> 

Interesting!  This should be fixed in nsDebugImpl.cpp as well.  Mind if we do that in a followup bug?

> >diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
> >+#define _Throw(_e)  mozilla_Throw(_e)
> 
> Change this to:
> 
> #define _Throw mozilla_Throw
> 
> to make sure that any possible crazy code which attempts to get the address of
> std::_Throw is also handled.
> 

The CRT doesn't do that (sez grep).  Would you r+ a Gecko patch that tried to take the address of a private MSVC-only STL symbol? ;)

Comment 37

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

(In reply to comment #36)
> (In reply to comment #30)
> > (From update of attachment 438239 [details] [diff] [review] [details])
> > >-COMPILE_CXXFLAGS = $(VISIBILITY_FLAGS) $(STL_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> > >+COMPILE_CXXFLAGS = $(STL_FLAGS) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> > 
> > Why are these changes necessary?
> > 
> 
> gcc needs to find headers in $DIST/stl_wrappers before $DIST/system_wrappers,
> because the system_wrappers/ headers change symbol visibility, and
> stl_wrappers/ headers include other Gecko headers.
> 

Oh, and I should add that the reason why *that's* important is that STL headers are in both the stl-headers and system-wrappers list, because we build code that uses our hidden-visibility flags but can't use our STL wrappers.

Comment 38

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

Er, not sure how I set that flag.  Unintentional.

Comment 39

[(no longer active)]

(In reply to comment #36)
> > >diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
> > >+#if defined(_WIN32)
> > >+#  if !defined(WINCE)
> > >+    //This should exit us
> > >+    raise(SIGABRT);
> > >+#  endif
> > 
> > Would it make sense here to call RaiseException, at least for Windows CE?
> > 
> 
> I don't know, would it?  I copied and pasted this code from
> xpcom/base/nsDebugImpl.cpp:Abort().

I think we should do this, yes.

> > >+    //If we are ignored exit this way..
> > >+    _exit(3);
> > >+#elif defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
> > >+    abort();
> > >+#else
> > >+#  warning not attempting to abort() on this platform
> > 
> > Shouldn't this be an #error?
> > 
> 
> Why do you think so?  We fall through to code that dereferences NULL and then
> calls _exit(), so I'm not sure that the lack of abort() adds risk.

Oh, right.  My oversight.

> > >+#endif
> > >+
> > >+    // Still haven't aborted?  Try dereferencing null.
> > >+    // (Written this way to lessen the likelihood of it being optimized away.)
> > >+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
> > >+    // this is crazy
> > 
> > In order to make sure that we'll cause a crash, there is a better technique.
> > 
> >   __debugbreak();
> > 
> > This is guaranteed to work, and won't get optimized away.
> > 
> 
> Interesting!  This should be fixed in nsDebugImpl.cpp as well.  Mind if we do
> that in a followup bug?

Sure, no problem.  Will you file that or do you want me to?

> > >diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
> > >+#define _Throw(_e)  mozilla_Throw(_e)
> > 
> > Change this to:
> > 
> > #define _Throw mozilla_Throw
> > 
> > to make sure that any possible crazy code which attempts to get the address of
> > std::_Throw is also handled.
> > 
> 
> The CRT doesn't do that (sez grep).  Would you r+ a Gecko patch that tried to
> take the address of a private MSVC-only STL symbol? ;)

No!  But I wasn't talking about Gecko code, I was talking about STL code (in a future VC version, for example.)  Better safe than sorry, right?

Comment 40

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

(In reply to comment #39)
> (In reply to comment #36)
> > > >diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
> > > >+#if defined(_WIN32)
> > > >+#  if !defined(WINCE)
> > > >+    //This should exit us
> > > >+    raise(SIGABRT);
> > > >+#  endif
> > > 
> > > Would it make sense here to call RaiseException, at least for Windows CE?
> > > 
> > 
> > I don't know, would it?  I copied and pasted this code from
> > xpcom/base/nsDebugImpl.cpp:Abort().
> 
> I think we should do this, yes.
> 
> > > >+#endif
> > > >+
> > > >+    // Still haven't aborted?  Try dereferencing null.
> > > >+    // (Written this way to lessen the likelihood of it being optimized away.)
> > > >+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
> > > >+    // this is crazy
> > > 
> > > In order to make sure that we'll cause a crash, there is a better technique.
> > > 
> > >   __debugbreak();
> > > 
> > > This is guaranteed to work, and won't get optimized away.
> > > 
> > 
> > Interesting!  This should be fixed in nsDebugImpl.cpp as well.  Mind if we do
> > that in a followup bug?
> 
> Sure, no problem.  Will you file that or do you want me to?
> 

Filed 558928 for these two items.

> > > >diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
> > > >+#define _Throw(_e)  mozilla_Throw(_e)
> > > 
> > > Change this to:
> > > 
> > > #define _Throw mozilla_Throw
> > > 
> > > to make sure that any possible crazy code which attempts to get the address of
> > > std::_Throw is also handled.
> > > 
> > 
> > The CRT doesn't do that (sez grep).  Would you r+ a Gecko patch that tried to
> > take the address of a private MSVC-only STL symbol? ;)
> 
> No!  But I wasn't talking about Gecko code, I was talking about STL code (in a
> future VC version, for example.)  Better safe than sorry, right?

*shrug*.  I tend to disagree, but it's not worth getting hung up on ;).

Comment 41

[(no longer active)]

Comment on attachment 438239 [details] [diff] [review]
Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc

I thought I'd set the flag here.  I'd prefer that you change the _Throw macro, but I won't hold up the review on that.  :-)

Comment 42

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

No problem.  I was going to post a new patch updated per your comments after fixing a mac tinderbox issue, but I can't repro locally so hopefully I pushed a bad base rev.

Comment 43

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

(In reply to comment #42)
> No problem.  I was going to post a new patch updated per your comments after
> fixing a mac tinderbox issue, but I can't repro locally so hopefully I pushed a
> bad base rev.

Yup, re-pushed with a different base rev and got all green.  Guess I'll chalk up the earlier errors to cosmic rays.

Comment 44

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

Attachment: Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc, v2 [Checkin: Comment 46]

Updated per Ehsan's comments.

Comment 45

[Benjamin Smedberg]

Comment on attachment 438679 [details] [diff] [review]
Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc, v2
[Checkin: Comment 46]

The only worry I have about this is making it more future-proof: what if a future version of MSVC doesn't use std::_Throw? Could we add a configure test to ensure that *calling* std::_Throw compiles, to at least detect this case?

Comment 46

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

http://hg.mozilla.org/mozilla-central/rev/f0ccd76765b9
http://hg.mozilla.org/mozilla-central/rev/f7f54ede7aea

Comment 47

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That future problem is "now" -- VS2010 does not have _Throw and doesn't define it in <exception>, so the configure check fails; not possible to build with VS2010 after this landed.

Comment 48

[Serge Gautherie (:sgautherie)]

(In reply to comment #47)
> That future problem is "now" -- VS2010 does not have _Throw and doesn't define
> it in <exception>, so the configure check fails; not possible to build with
> VS2010 after this landed.

Now filed as bug 560723.

Comment 49

[Takanori MATSUURA]

After f0ccd76765b9, I got the following error on Linux.

../../dist/include/mozilla/throw_gcc.h: In function ‘void std::__throw_system_error(int)’:
../../dist/include/mozilla/throw_gcc.h:147: error: ‘strerror’ was not declared in this scope

Comment 50

[Takanori MATSUURA]

Attachment: Fix for comment #49 [Checkin: Comment 56]

Add string.h to #include.

Comment 51

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-CC) Copy config.mk part of m-c fix [Checkin: Comment 55]

(Whoever review it first...)

Comment 52

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1-CC) Copy (the useful part of) m-c configure.in fix [Supeseded by bug 562655]

(Whoever review it first...)

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 440450 [details] [diff] [review]
(Fv1-CC) Copy config.mk part of m-c fix
[Checkin: Comment 55]

Callek can, I'm busy with lots of other things.

Comment 54

[Jacek Caban]

Attachment: mingw fix [Checkin: Comment 56]

Changes to configure.in from this bug broke mingw build. std::_Throw and |class __declspec(dllimport) exception| are specific to MSVC, so they should be moved to MSVC-specific part.

Comment 55

[Serge Gautherie (:sgautherie)]

Comment on attachment 440450 [details] [diff] [review]
(Fv1-CC) Copy config.mk part of m-c fix
[Checkin: Comment 55]


http://hg.mozilla.org/comm-central/rev/7d00aae318b9

Comment 56

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

http://hg.mozilla.org/mozilla-central/rev/774f9718376f
http://hg.mozilla.org/mozilla-central/rev/6b72a39eaa2a

Please let's be done with this bug.

Comment 57

[Serge Gautherie (:sgautherie)]

Comment on attachment 440452 [details] [diff] [review]
(Gv1-CC) Copy (the useful part of) m-c configure.in fix
[Supeseded by bug 562655]


Looking for "approval-thunderbird3.1b2=?" actually.

Comment 58

[Mark Banner (:standard8)]

Comment on attachment 440452 [details] [diff] [review]
(Gv1-CC) Copy (the useful part of) m-c configure.in fix
[Supeseded by bug 562655]

Without any description as to why this should get approval to land in a code freeze you're not going to get it - especially given that it doesn't even affect comm-1.9.2 which is what we're frozen for.

Comment 59

[Serge Gautherie (:sgautherie)]

(In reply to comment #58)
> especially given that it doesn't even
> affect comm-1.9.2 which is what we're frozen for.

Indeed: that's exactly why I don't see why a "c-1.9.2" freeze should block this c-c sync with m-c :-/

Comment 60

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Chris, is it possible that this broke XULRunner? See bug 559447 comment 8?

Comment 61

[Serge Gautherie (:sgautherie)]

Comment on attachment 440452 [details] [diff] [review]
(Gv1-CC) Copy (the useful part of) m-c configure.in fix
[Supeseded by bug 562655]


No comment :-|

Comment 62

[Serge Gautherie (:sgautherie)]

Ftr, I just found
http://code.google.com/p/chromiumembedded/issues/detail?id=67
"Issue 67: Build cefclient with vc 2005 express edition"