Re: [Libreoffice-commits] Resolves: #i123747# allow treating Window's SEH events as C++ exceptions

On 11/26/2013 10:27 AM, Stephan Bergmann wrote:
> Is this sound?  I am no expert with Windows SEH, does it actually
> guarantee that the low-level process state is also a valid state at the
> higher-level C++ semantics when a SEH exception is thrown?  (That would
> be a prerequisite for this patch, and e.g. the missing guarantee about
> that consistency for C/POSIX signals is the reason why we cannot
> automatically translate such signals into C++ exceptions.)

So there is /EHa (enabling C++ to catch asynchronous (SEH) exceptions) 
vs. /EHs (allowing C++ to only catch C++ (synchronous) exceptions), and 
we're currently using /EHa, and that presumably makes the #i123747# 
patch work.

However, /EHa does at least have a space overhead (and 
<http://stackoverflow.com/a/4415711> claims it also has a time overhead 
on x86), see below, so my natural reaction would be to switch to /EHs 
and be happy.  (After all, SEH exceptions are a result of invoking 
undefined behavior at the C++ level, and its a fallacy to assume you can 
meaningfully interact with that at the C++ level in general.  And little 
if any of our code base would apparently depend on it, given that an 
/EHs "make check" works, as detailed below.)

However, as other parts of that stackoverflow page allude to, there may 
be implications for interaction with the CLR, i.e., for our CLI-UNO 
bridge.  I'd hoped to verify that with the 
testtools/source/bridgetest/cli/ code, but that's apparently rotting 
dead at least since gbuild'ification, and I ditched my fruitless 
attempts to revive it after an hour of frustration...  Anybody else 
wanting to pick that up?

Anyway, for the record, here's how I measured impact of /EHa vs. /EHs 
vs. /EHsc (which additionally assumes that C functions never throw C++ 
exceptions) on current master (at rev. 
2fbf95f5efb0e7e2781fa8546845a084721ee4e7).  The build is implicitly 
--disable-debug --disable-dbgutil, but I had to explicitly --disable-lto 
because I only had a 32bit Windows 7 available and that isn't able to 
give the linker enough heap to do LTO on our huge libraries.

It is the definition of gb_LinkTarget_EXCEPTIONFLAGS in 
solenv/gbuild/platform/com_MSC_defs.mk that specifies -EHa, and I 
exchanged that with -EHs and -EHsc, resp., and measured the sizes of all 
instdir *.com, *.dll, and *.exe files for each case (forgot to include 
*.bin, too).  The attached eh.ods gives the detailed numbers; there is a 
roughly 20% decrease in overall binary file size from /EHa to /EHs, 
while there is only little further decrease towards /EHsc.

To make all three builds work, I had to apply the attached eh.patch:
* Compiling /clr code apparently explicitly requires /EHa (and 
specifying /EHa after /EHs[c] only results in a linker warning and uses 
/EHa).
* The #i123747# patch's _set_se_translator requires /EHa (and this is 
about reverting it anyway).
* Some function definitions in winaccessibility are boilerplate-armored 
with try/catch(...) around their bodies even if those are known to not 
throw any (C++) exceptions, and the compiler warns about that under 
/EHs[c], so I needed to silence that for --enable-werror.

Stephan

Attachment: eh.ods
Description: application/vnd.oasis.opendocument.spreadsheet

diff --git a/cli_ure/Executable_climaker.mk b/cli_ure/Executable_climaker.mk
index 2d99b1f..298f01e 100644
--- a/cli_ure/Executable_climaker.mk
+++ b/cli_ure/Executable_climaker.mk
@@ -15,7 +15,7 @@ $(eval $(call gb_Executable_use_package,climaker,\
 
 $(eval $(call gb_Executable_add_cxxflags,climaker,\
        -AI $(INSTDIR)/$(LIBO_URE_LIB_FOLDER) \
-       -clr \
+       -EHa -clr \
        -LN \
        -wd4339 \
        -wd4715 \
diff --git a/cli_ure/Library_cli_cppuhelper_native.mk b/cli_ure/Library_cli_cppuhelper_native.mk
index 39c13c8..7aae2d6 100644
--- a/cli_ure/Library_cli_cppuhelper_native.mk
+++ b/cli_ure/Library_cli_cppuhelper_native.mk
@@ -15,7 +15,7 @@ $(eval $(call gb_Library_Assembly,cli_cppuhelper))
 # in CLR meta-data - use of this type may lead to a runtime exception":
 $(eval $(call gb_Library_add_cxxflags,cli_cppuhelper,\
        -AI $(INSTDIR)/$(LIBO_URE_LIB_FOLDER) \
-       -clr \
+       -EHa -clr \
        -wd4339 \
 ))
 
diff --git a/cli_ure/Library_cli_uno.mk b/cli_ure/Library_cli_uno.mk
index 58b2d4f..4d3a2c5 100644
--- a/cli_ure/Library_cli_uno.mk
+++ b/cli_ure/Library_cli_uno.mk
@@ -11,7 +11,7 @@ $(eval $(call gb_Library_Library,cli_uno))
 
 $(eval $(call gb_Library_add_cxxflags,cli_uno,\
        -AI $(INSTDIR)/$(LIBO_URE_LIB_FOLDER) \
-       -clr \
+       -EHa -clr \
        -wd4339 \
 ))
 
diff --git a/sal/osl/w32/signal.cxx b/sal/osl/w32/signal.cxx
index a63e692..74bad9d 100644
--- a/sal/osl/w32/signal.cxx
+++ b/sal/osl/w32/signal.cxx
@@ -444,7 +444,8 @@ sal_Bool SAL_CALL osl_setErrorReporting( sal_Bool bEnable )
     if( !bEnable) // if the crash reporter is disabled
     {
         // fall back to handle Window's SEH events as C++ exceptions
-        _set_se_translator( win_seh_translator);
+// warning C4535: calling _set_se_translator requires /EHa
+//      _set_se_translator( win_seh_translator);
     }
 
     return bOld;
diff --git a/solenv/gbuild/platform/com_MSC_defs.mk b/solenv/gbuild/platform/com_MSC_defs.mk
diff --git a/winaccessibility/source/UAccCOM/AccRelation.cxx 
b/winaccessibility/source/UAccCOM/AccRelation.cxx
index 32721e0..4ddd1ad 100644
--- a/winaccessibility/source/UAccCOM/AccRelation.cxx
+++ b/winaccessibility/source/UAccCOM/AccRelation.cxx
@@ -56,11 +56,11 @@ STDMETHODIMP CAccRelation::get_localizedRelationType(BSTR *)
 {
 
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
     return S_OK;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -72,7 +72,7 @@ STDMETHODIMP CAccRelation::get_nTargets(long * nTargets)
 {
 
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
     CHECK_ENABLE_INF
     if (nTargets == NULL)
@@ -82,7 +82,7 @@ STDMETHODIMP CAccRelation::get_nTargets(long * nTargets)
     *nTargets = xTargets.getLength();
     return S_OK;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 /**
diff --git a/winaccessibility/source/UAccCOM/AccTable.cxx 
b/winaccessibility/source/UAccCOM/AccTable.cxx
index 095b56b..05d3c84 100644
--- a/winaccessibility/source/UAccCOM/AccTable.cxx
+++ b/winaccessibility/source/UAccCOM/AccTable.cxx
@@ -100,11 +100,11 @@ STDMETHODIMP CAccTable::get_caption(IUnknown * *)
 {
 
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
     return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -959,11 +959,11 @@ STDMETHODIMP CAccTable::get_rowColumnExtentsAtIndex(long,
 
     CHECK_ENABLE_INF
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
     return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 STDMETHODIMP CAccTable::get_modelChange(IA2TableModelChange  *)
diff --git a/winaccessibility/source/UAccCOM/AccTextBase.cxx 
b/winaccessibility/source/UAccCOM/AccTextBase.cxx
index e63afe4..ca0a010 100644
--- a/winaccessibility/source/UAccCOM/AccTextBase.cxx
+++ b/winaccessibility/source/UAccCOM/AccTextBase.cxx
@@ -913,22 +913,22 @@ STDMETHODIMP CAccTextBase::scrollSubstringToPoint(long, long, 
IA2CoordinateType,
 {
 
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
     return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 STDMETHODIMP CAccTextBase::scrollSubstringTo(long, long, IA2ScrollType)
 {
 
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
     return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 /**
diff --git a/winaccessibility/source/UAccCOM/MAccessible.cxx 
b/winaccessibility/source/UAccCOM/MAccessible.cxx
index 73897c4..10c6ed2 100644
--- a/winaccessibility/source/UAccCOM/MAccessible.cxx
+++ b/winaccessibility/source/UAccCOM/MAccessible.cxx
@@ -1135,7 +1135,7 @@ STDMETHODIMP CMAccessible::put_accValue(VARIANT varChild, BSTR szValue)
 STDMETHODIMP CMAccessible::Put_XAccName(const OLECHAR __RPC_FAR *pszName)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         // #CHECK#
         if(pszName == NULL)
@@ -1149,7 +1149,7 @@ STDMETHODIMP CMAccessible::Put_XAccName(const OLECHAR __RPC_FAR *pszName)
             return E_FAIL;
         return S_OK;
 
-        LEAVE_PROTECTED_BLOCK
+        //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -1206,7 +1206,7 @@ STDMETHODIMP CMAccessible::SetState(DWORD pXSate)
 STDMETHODIMP CMAccessible::Put_XAccDescription(const OLECHAR __RPC_FAR *pszDescription)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         // #CHECK#
         if(pszDescription == NULL)
@@ -1221,7 +1221,7 @@ STDMETHODIMP CMAccessible::Put_XAccDescription(const OLECHAR __RPC_FAR 
*pszDescr
             return E_FAIL;
         return S_OK;
 
-        LEAVE_PROTECTED_BLOCK
+        //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -1232,7 +1232,7 @@ STDMETHODIMP CMAccessible::Put_XAccDescription(const OLECHAR __RPC_FAR 
*pszDescr
 STDMETHODIMP CMAccessible::Put_XAccValue(const OLECHAR __RPC_FAR *pszAccValue)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         // #CHECK#
         if(pszAccValue == NULL)
@@ -1245,7 +1245,7 @@ STDMETHODIMP CMAccessible::Put_XAccValue(const OLECHAR __RPC_FAR *pszAccValue)
             return E_FAIL;
         return S_OK;
 
-        LEAVE_PROTECTED_BLOCK
+        //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -1257,12 +1257,12 @@ STDMETHODIMP CMAccessible::Put_XAccValue(const OLECHAR __RPC_FAR 
*pszAccValue)
 STDMETHODIMP CMAccessible::Put_XAccWindowHandle(HWND hwnd)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         m_hwnd = hwnd;
     return S_OK;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -1814,13 +1814,13 @@ STDMETHODIMP CMAccessible::get_relations( long, IAccessibleRelation 
__RPC_FAR *_
 
 STDMETHODIMP CMAccessible::role(long __RPC_FAR *role)
 {
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
 
         (*role) = m_iRole;
 
     return S_OK;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 
@@ -1857,21 +1857,21 @@ STDMETHODIMP CMAccessible:: get_nActions(long __RPC_FAR *nActions)
 STDMETHODIMP CMAccessible:: scrollToPoint(enum IA2CoordinateType, long, long)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
     ISDESTROY()
     return E_NOTIMPL;
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 
 }
 STDMETHODIMP CMAccessible:: scrollTo(enum IA2ScrollType)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
     ISDESTROY()
 
     return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 static XAccessible* getTheParentOfMember(XAccessible* pXAcc)
@@ -2076,19 +2076,19 @@ STDMETHODIMP CMAccessible:: get_extendedStates( long, BSTR __RPC_FAR 
*__RPC_FAR
 {
 
     CHECK_ENABLE_INF
-        ENTER_PROTECTED_BLOCK
+        //ENTER_PROTECTED_BLOCK
         ISDESTROY()
 
         return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 
 STDMETHODIMP CMAccessible:: get_uniqueID(long __RPC_FAR *uniqueID)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         // #CHECK#
         if(uniqueID == NULL)
@@ -2098,13 +2098,13 @@ STDMETHODIMP CMAccessible:: get_uniqueID(long __RPC_FAR *uniqueID)
         *uniqueID = m_dChildID;
         return S_OK;
 
-        LEAVE_PROTECTED_BLOCK
+        //LEAVE_PROTECTED_BLOCK
 }
 
 STDMETHODIMP CMAccessible:: get_windowHandle(HWND __RPC_FAR *windowHandle)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         // #CHECK#
         if(windowHandle == NULL)
@@ -2130,7 +2130,7 @@ STDMETHODIMP CMAccessible:: get_windowHandle(HWND __RPC_FAR *windowHandle)
         *windowHandle = nHwnd;
         return S_OK;
 
-        LEAVE_PROTECTED_BLOCK
+        //LEAVE_PROTECTED_BLOCK
 }
 
 /**
@@ -2496,7 +2496,7 @@ HRESULT STDMETHODCALLTYPE CMAccessible::accDoDefaultAction(VARIANT varChild)
 STDMETHODIMP CMAccessible::Put_ActionDescription( const OLECHAR* szAction)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         // #CHECK#
         if(szAction == NULL)
@@ -2507,7 +2507,7 @@ STDMETHODIMP CMAccessible::Put_ActionDescription( const OLECHAR* szAction)
         m_pszActionDescription = SysAllocString( szAction );
         return S_OK;
 
-        LEAVE_PROTECTED_BLOCK
+        //LEAVE_PROTECTED_BLOCK
 }
 
 BOOL CMAccessible::GetXInterfaceFromXAccessible(XAccessible* pXAcc, XInterface** ppXI, int index)
@@ -3110,44 +3110,44 @@ STDMETHODIMP CMAccessible:: get_extendedRole( BSTR __RPC_FAR *  )
 {
 
     CHECK_ENABLE_INF
-        ENTER_PROTECTED_BLOCK
+        //ENTER_PROTECTED_BLOCK
         ISDESTROY()
 
         return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 STDMETHODIMP CMAccessible:: get_localizedExtendedRole( BSTR __RPC_FAR *  )
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 STDMETHODIMP CMAccessible:: get_nExtendedStates( long __RPC_FAR * )
 {
 
     CHECK_ENABLE_INF
-        ENTER_PROTECTED_BLOCK
+        //ENTER_PROTECTED_BLOCK
         ISDESTROY()
 
         return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 
 STDMETHODIMP CMAccessible:: get_localizedExtendedStates( long, BSTR __RPC_FAR *__RPC_FAR *, long 
__RPC_FAR *)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         return E_NOTIMPL;
 
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 
 
@@ -3248,47 +3248,47 @@ DWORD GetMSAAStateFromUNO(short xState)
 STDMETHODIMP CMAccessible:: get_appName( BSTR __RPC_FAR *name)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         if(name == NULL)
             return E_INVALIDARG;
 
     *name = SysAllocString(OLESTR("Hannover"));
     return S_OK;
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 STDMETHODIMP CMAccessible:: get_appVersion(BSTR __RPC_FAR *version)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         if(version == NULL)
             return E_INVALIDARG;
     *version=SysAllocString(OLESTR("3.0"));
     return S_OK;
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 STDMETHODIMP CMAccessible:: get_toolkitName(BSTR __RPC_FAR *name)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         if(name == NULL)
             return E_INVALIDARG;
     *name = SysAllocString(OLESTR(" "));
     return S_OK;
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }
 STDMETHODIMP CMAccessible:: get_toolkitVersion(BSTR __RPC_FAR *version)
 {
 
-    ENTER_PROTECTED_BLOCK
+    //ENTER_PROTECTED_BLOCK
         ISDESTROY()
         if(version == NULL)
             return E_INVALIDARG;
     *version = SysAllocString(OLESTR(" "));
     return S_OK;
-    LEAVE_PROTECTED_BLOCK
+    //LEAVE_PROTECTED_BLOCK
 }