Re: [ros-dev] [ros-diffs] [hyperion] 38461: modified tests/pseh2/psehtest.c finally_13 test re-enabled, as it doesn't crash anymore finally_14 test fixed. Now we know how exceptions thrown in a __finally are supposed to be handled PSEH test suite now has 91 tests and passes all of them with both GCC and Visual C++

[Aleksey Bragin]

Just wanted to tell this is a great work, and with such test suite we  
will always notice if something breaks.


WBR,
Aleksey Bragin.

On Dec 30, 2008, at 7:04 AM, hyperion(a)svn.reactos.org wrote:

>   Author: hyperion
>  Date: Mon Dec 29 22:04:51 2008
>  New Revision: 38461
>
>  URL: http://svn.reactos.org/svn/reactos?rev=38461&view=rev
>  Log:
>  modified   tests/pseh2/psehtest.c
>     finally_13 test re-enabled, as it doesn't crash anymore
>     finally_14 test fixed. Now we know how exceptions thrown in a  
>  __finally are supposed to be handled
>     PSEH test suite now has 91 tests and passes all of them with  
>  both GCC and Visual C++

[Timo Kreuzer]

About the SEH tests.
The tests are nice, but there are 2 things that makes me think the tests
are cheating.

1.) All used variables are static. That means the optimizer is mostly
out of the game. Is this planned to be a requirement (static or volatile
variables)?
2.) The tests use return_positive() and return_zero() functions. Another
way to stop the optimizer from doing his work, because function calls
act as sequence points and the result isn't known at compile time. But
this is far from practical. If you replace them with compiler constants,
the optimizer will try to remove these assignmnents if possible.

So I did some testing. After removing the static, I got 1 error:
psehtest.c:2490: Test failed: test_continue_search_3 failed

After replacing the return_x, ... functions with macros I got 3 errors:
psehtest.c:2486: Test failed: test_continue_search_6 failed
psehtest.c:2486: Test failed: test_execute_handler_12 failed
psehtest.c:2486: Test failed: test_continue_execution_12 failed

Same with full and normal optimisation.

Timo


Aleksey Bragin schrieb:
>   Just wanted to tell this is a great work, and with
> such test suite we  
>  will always notice if something breaks.
>
>
>  WBR,
>  Aleksey Bragin.
>
>  On Dec 30, 2008, at 7:04 AM, hyperion(a)svn.reactos.org wrote:
>
>    
> >   Author: hyperion
> >  Date: Mon Dec 29 22:04:51 2008
> >  New Revision: 38461
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=38461&view=rev
> >  Log:
> >  modified   tests/pseh2/psehtest.c
> >     finally_13 test re-enabled, as it doesn't crash anymore
> >     finally_14 test fixed. Now we know how exceptions thrown in a  
> >  __finally are supposed to be handled
> >     PSEH test suite now has 91 tests and passes all of them with  
> >  both GCC and Visual C++
> >       
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>

[KJK::Hyperion]

Timo Kreuzer wrote:
>   1.) All used variables are static. That means the
> optimizer is mostly
>  out of the game. 
It's intentional. Using functions instead of constants is intentional,
too. Ensures variables are actually instanced, even if they are
"unused". And PSEH macros have slightly different behavior when
compile-time constants are used in some places. I'm unsure how to test
*reliably* interactions between optimizer bugs and PSEH

>   So I did some testing. After removing the static, I
> got 1 error:
>  psehtest.c:2490: Test failed: test_continue_search_3 failed 
The return value of return_positive() is not stored anywhere, and ret
remains uninitialized. Congratulations! you found a compiler bug

>   After replacing the return_x, ... functions with
> macros I got 3 errors: 
I don't. Send me your modifications

[Timo Kreuzer]

KJK::Hyperion wrote:
>   Timo Kreuzer wrote:
>    
> >   1.) All used variables are static. That means the
> > optimizer is mostly
> >  out of the game.
> >       
>  It's intentional. Using functions instead of constants is intentional,
>  too. Ensures variables are actually instanced, even if they are
>  "unused". And PSEH macros have slightly different behavior when
>  compile-time constants are used in some places. I'm unsure how to test
>  *reliably* interactions between optimizer bugs and PSEH
>     
I just suggest testing real situations instead of hypothetical
situations. And it's a fact that we normally don't use static variables
and bot only functions but also constants in our code. So these tests
are of small value regarding the usage in reactos. And you shouldn't
simply blame the optmimizer to have bugs when it doesn't do what you
would like it to do.

>     
> >   So I did some testing. After removing the static,
> > I got 1 error:
> >  psehtest.c:2490: Test failed: test_continue_search_3 failed
> >       
>  The return value of return_positive() is not stored anywhere, and ret
>  remains uninitialized. Congratulations! you found a compiler bug
>     
No, it's a valid optimisation:
1. The return value of return_positive() is stored in edx
2. The code branches into the 1st try and except block
  2a) except block (will not be called here): edx is put on the stack,
return_arg() is called and the result copied to ebx
  2b) try block: again 2 different branches, in both of them
return_zero() is called and the result stored in ebx
3.) ebx is compared to the return value of return_positive()

The compiler generates a new instance of the ret variable each time it's
assigned and it's free to use different registers and/or memory
locations each time, as long as it stays consistent for each possible
code path.

The test fails, because when executing the exception block, edx doesn't
contain what it should, as it's zeroed in the try block before the
exception happens. Exceptions are simply not considered as possible code
pathes.
This can and will happen unless
a) We require that all variables that are changed inside a try block and
referenced inside the except block must be declared volatile
b) We find a way to tell gcc to not change the location of a variable
during a try block.

>     
> >   After replacing the return_x, ... functions with
> > macros I got 3 errors:
> >       
>  I don't. Send me your modifications
>     
I attached 2 patches.

Regards,
Timo






Index: psehtest.c
===================================================================
--- psehtest.c	(Revision 38469)
+++ psehtest.c	(Arbeitskopie)
@@ -66,6 +66,39 @@
 
 #define DEFINE_TEST(NAME_) static int test_ ## NAME_(void)
 
+#if 0
+
+#define no_op()
+#define return_arg(i) (i)
+
+#define return_zero() (0)
+#define return_positive() (1234)
+#define return_negative() (-1234)
+#define return_one() (1)
+#define return_minusone() (-1)
+
+#define return_zero_2(p) (0)
+#define return_positive_2(p) (1234)
+#define return_negative_2(p) (-1234)
+#define return_one_2(p) (1)
+#define return_minusone_2(p) (-1)
+
+#define return_zero_3(i) (0)
+#define return_positive_3(i) (1234)
+#define return_negative_3(i) (-1234)
+#define return_one_3(i) (1)
+#define return_minusone_3(i) (-1)
+
+#define return_zero_4(p, i) (0)
+#define return_positive_4(p, i) (1234)
+#define return_negative_4(p, i) (-1234)
+#define return_one_4(p, i) (1)
+#define return_minusone_4(p, i) (-1)
+
+#define set_positive(p) (*(p))=1
+
+#endif
+
 /* Empty statements *///{{{
 DEFINE_TEST(empty_1)
 {
@@ -119,7 +152,7 @@
 /* Static exception filters *///{{{
 DEFINE_TEST(execute_handler_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -139,7 +172,7 @@
 
 DEFINE_TEST(continue_execution_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -159,7 +192,7 @@
 
 DEFINE_TEST(continue_search_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -187,7 +220,7 @@
 
 DEFINE_TEST(execute_handler_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -207,7 +240,7 @@
 
 DEFINE_TEST(continue_execution_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -229,7 +262,7 @@
 /* Dynamic exception filters *///{{{
 DEFINE_TEST(execute_handler_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -249,7 +282,7 @@
 
 DEFINE_TEST(continue_execution_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -269,7 +302,7 @@
 
 DEFINE_TEST(continue_search_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -297,7 +330,7 @@
 
 DEFINE_TEST(execute_handler_4)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -317,7 +350,7 @@
 
 DEFINE_TEST(continue_execution_4)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -339,7 +372,7 @@
 /* Dynamic exception filters, using _SEH2_GetExceptionInformation() *///{{{
 DEFINE_TEST(execute_handler_5)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -359,7 +392,7 @@
 
 DEFINE_TEST(continue_execution_5)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -379,7 +412,7 @@
 
 DEFINE_TEST(continue_search_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_positive();
 
@@ -407,7 +440,7 @@
 
 DEFINE_TEST(execute_handler_6)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -427,7 +460,7 @@
 
 DEFINE_TEST(continue_execution_6)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -449,7 +482,7 @@
 /* Dynamic exception filters, using _SEH2_GetExceptionCode() *///{{{
 DEFINE_TEST(execute_handler_7)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -469,7 +502,7 @@
 
 DEFINE_TEST(continue_execution_7)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -489,7 +522,7 @@
 
 DEFINE_TEST(continue_search_4)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -517,7 +550,7 @@
 
 DEFINE_TEST(execute_handler_8)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -537,7 +570,7 @@
 
 DEFINE_TEST(continue_execution_8)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -559,7 +592,7 @@
 /* Dynamic exception filters, using _SEH2_GetExceptionInformation() and
_SEH2_GetExceptionCode() *///{{{
 DEFINE_TEST(execute_handler_9)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -579,7 +612,7 @@
 
 DEFINE_TEST(continue_execution_9)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -599,7 +632,7 @@
 
 DEFINE_TEST(continue_search_5)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -627,7 +660,7 @@
 
 DEFINE_TEST(execute_handler_10)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -647,7 +680,7 @@
 
 DEFINE_TEST(continue_execution_10)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -669,7 +702,7 @@
 /* Constant exception filters with side effects *///{{{
 DEFINE_TEST(execute_handler_11)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -689,7 +722,7 @@
 
 DEFINE_TEST(continue_execution_11)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -709,8 +742,8 @@
 
 DEFINE_TEST(continue_search_6)
 {
-	static int ret;
-	static int ret2;
+	int ret;
+	int ret2;
 
 	ret = return_zero();
 	ret2 = return_zero();
@@ -742,7 +775,7 @@
 
 DEFINE_TEST(execute_handler_12)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -762,7 +795,7 @@
 
 DEFINE_TEST(continue_execution_12)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -784,7 +817,7 @@
 /* _SEH2_LEAVE *///{{{
 DEFINE_TEST(leave_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -805,7 +838,7 @@
 
 DEFINE_TEST(leave_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -828,7 +861,7 @@
 
 DEFINE_TEST(leave_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -852,7 +885,7 @@
 
 DEFINE_TEST(leave_4)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -883,7 +916,7 @@
 
 DEFINE_TEST(leave_5)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -909,7 +942,7 @@
 
 DEFINE_TEST(leave_6)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1106,7 +1139,7 @@
 /* Termination blocks *///{{{
 DEFINE_TEST(finally_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1125,7 +1158,7 @@
 
 DEFINE_TEST(finally_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1145,7 +1178,7 @@
 
 DEFINE_TEST(finally_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1191,7 +1224,7 @@
 
 DEFINE_TEST(finally_5)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1219,7 +1252,7 @@
 
 DEFINE_TEST(finally_6)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1248,7 +1281,7 @@
 
 DEFINE_TEST(finally_7)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1278,7 +1311,7 @@
 
 DEFINE_TEST(finally_8)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1344,7 +1377,7 @@
 
 DEFINE_TEST(finally_10)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1382,7 +1415,7 @@
 
 DEFINE_TEST(finally_11)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1420,7 +1453,7 @@
 
 DEFINE_TEST(finally_12)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1494,7 +1527,7 @@
 
 DEFINE_TEST(finally_13)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1559,7 +1592,7 @@
 
 DEFINE_TEST(finally_14)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1601,7 +1634,7 @@
 
 DEFINE_TEST(xpointers_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1620,7 +1653,7 @@
 
 DEFINE_TEST(xpointers_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1639,8 +1672,8 @@
 
 DEFINE_TEST(xpointers_3)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1659,8 +1692,8 @@
 
 DEFINE_TEST(xpointers_4)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1679,8 +1712,8 @@
 
 DEFINE_TEST(xpointers_5)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1699,8 +1732,8 @@
 
 DEFINE_TEST(xpointers_6)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1719,7 +1752,7 @@
 
 DEFINE_TEST(xpointers_7)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1739,8 +1772,8 @@
 
 DEFINE_TEST(xpointers_8)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1760,8 +1793,8 @@
 
 DEFINE_TEST(xpointers_9)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1781,8 +1814,8 @@
 
 DEFINE_TEST(xpointers_10)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1802,8 +1835,8 @@
 
 DEFINE_TEST(xpointers_11)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1823,7 +1856,7 @@
 
 DEFINE_TEST(xpointers_12)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1850,8 +1883,8 @@
 
 DEFINE_TEST(xpointers_13)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1878,8 +1911,8 @@
 
 DEFINE_TEST(xpointers_14)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1906,8 +1939,8 @@
 
 DEFINE_TEST(xpointers_15)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1934,8 +1967,8 @@
 
 DEFINE_TEST(xpointers_16)
 {
-	static int ret;
-	static const ULONG_PTR args[] = { 1, 2, 12345 };
+	int ret;
+	const ULONG_PTR args[] = { 1, 2, 12345 };
 
 	ret = return_zero();
 
@@ -1975,7 +2008,7 @@
 
 DEFINE_TEST(xcode_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -1995,7 +2028,7 @@
 
 DEFINE_TEST(xcode_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2015,7 +2048,7 @@
 
 DEFINE_TEST(xcode_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2045,7 +2078,7 @@
 /* _SEH2_AbnormalTermination() *///{{{
 DEFINE_TEST(abnorm_1)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2064,7 +2097,7 @@
 
 DEFINE_TEST(abnorm_2)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2083,7 +2116,7 @@
 
 DEFINE_TEST(abnorm_3)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2103,7 +2136,7 @@
 
 DEFINE_TEST(abnorm_4)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2131,7 +2164,7 @@
 
 DEFINE_TEST(abnorm_5)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2158,7 +2191,7 @@
 
 DEFINE_TEST(abnorm_6)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2185,7 +2218,7 @@
 
 DEFINE_TEST(abnorm_7)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 
@@ -2213,7 +2246,7 @@
 
 DEFINE_TEST(abnorm_8)
 {
-	static int ret;
+	int ret;
 
 	ret = return_zero();
 

Index: psehtest.c
===================================================================
--- psehtest.c	(Revision 38469)
+++ psehtest.c	(Arbeitskopie)
@@ -66,6 +66,39 @@
 
 #define DEFINE_TEST(NAME_) static int test_ ## NAME_(void)
 
+#if 1
+
+#define no_op()
+#define return_arg(i) (i)
+
+#define return_zero() (0)
+#define return_positive() (1234)
+#define return_negative() (-1234)
+#define return_one() (1)
+#define return_minusone() (-1)
+
+#define return_zero_2(p) (0)
+#define return_positive_2(p) (1234)
+#define return_negative_2(p) (-1234)
+#define return_one_2(p) (1)
+#define return_minusone_2(p) (-1)
+
+#define return_zero_3(i) (0)
+#define return_positive_3(i) (1234)
+#define return_negative_3(i) (-1234)
+#define return_one_3(i) (1)
+#define return_minusone_3(i) (-1)
+
+#define return_zero_4(p, i) (0)
+#define return_positive_4(p, i) (1234)
+#define return_negative_4(p, i) (-1234)
+#define return_one_4(p, i) (1)
+#define return_minusone_4(p, i) (-1)
+
+#define set_positive(p) (*(p))=1
+
+#endif
+
 /* Empty statements *///{{{
 DEFINE_TEST(empty_1)
 {

[KJK::Hyperion]

Timo Kreuzer wrote:
>   I just suggest testing real situations instead of
> hypothetical
>  situations. 
Every test tests a real situation I encountered during development of
both PSEH 1 and PSEH 2 (infinite loops, stack misalignment, compilation
failures, faulty logic, paths in library code that were never executed,
compile-time constants with vs without side effects). Hypothetical
situations are only situations that haven't happened yet

If you have tests to propose, propose tests

> >   The return
> > value of return_positive() is not stored anywhere, and ret
> >  remains uninitialized. Congratulations! you found a compiler bug 
>  No, it's a
> valid optimisation: 
_SEH2EnterFrame is marked returns_twice, GCC should ensure all register
variables to be "dead" (i.e. copied to the stack frame) before calling
it, and warn if they could be clobbered. Trusting the contents of a
register after a call to _SEH2EnterFrame is a bug. Either that, or the
gotos must be confusing GCC's flow control analysis. But the gotos are
necessary for the pretty syntax, so we'll have to pay with yet more
redundant code

> > >   After replacing the return_x, ... functions with
> > > macros I got 3 errors: 
> >  I don't. Send me your modifications 
>  I attached 2
> patches. 
I believe this is what younger, ruder team members would refer to as
"epic fail". Not me, of course. Me, I just wish I wasn't expected to
find bugs in my bug reports

[Timo Kreuzer]

KJK::Hyperion schrieb:
>   Every test tests a real situation I encountered during
> development of
>  both PSEH 1 and PSEH 2 (infinite loops, stack misalignment, compilation
>  failures, faulty logic, paths in library code that were never executed,
>  compile-time constants with vs without side effects). Hypothetical
>  situations are only situations that haven't happened yet
>
>  If you have tests to propose, propose tests
>
>     
Ok these test are fine to test that everything works as it should under
some save assumptions.
I suggest adding tests that cover fully optimized code. Non-volatile
variables, constants, lots of code needing multiple registers in the try
and except block, inline functions, inline assembly, etc... Everything
that might be used anywhere in our code and might be totally clobbered
by the optimizer.

> > >   The return value of return_positive() is not stored
> > > anywhere, and ret
> > >  remains uninitialized. Congratulations! you found a compiler bug
> > >         
> >  No, it's a valid optimisation:
> >       
>  _SEH2EnterFrame is marked returns_twice, GCC should ensure all register
>  variables to be "dead" (i.e. copied to the stack frame) before calling
>  it, and warn if they could be clobbered. Trusting the contents of a
>  register after a call to _SEH2EnterFrame is a bug. Either that, or the
>  gotos must be confusing GCC's flow control analysis. But the gotos are
>  necessary for the pretty syntax, so we'll have to pay with yet more
>  redundant code
>     
The gotos are not "confusing" gcc. SEH2EnterFrame isn't called in
the 
first except branch, so the compiler is free to keep ret in any register
it likes during that branch. The try branch has no need to keep ret in
any memory location or register at all, because it's not referenced any
more. It's only set to the result of return_zero(). It wouldn't make
sense to store the old result as it will be droped anyway. In other
words the register *is dead* on entering the try branch. Sorry, but you
still didn't find a compiler bug.

>     
> > > >   After replacing the return_x, ... functions with
> > > > macros I got 3 errors:
> > > >           
> > >  I don't. Send me your modifications
> > >         
> >  I attached 2 patches.
> >       
>  I believe this is what younger, ruder team members would refer to as
>  "epic fail". Not me, of course. Me, I just wish I wasn't expected to
>  find bugs in my bug reports
>     
Lol, ok, I'd call it a typo ;-)

[KJK::Hyperion]

Timo Kreuzer wrote:
>   In other words the register *is dead* on entering the
> try branch. 
Read and compare the disassembled code of continue_search_3 before and
after my last change

[Timo Kreuzer]

KJK::Hyperion schrieb:
>   Timo Kreuzer wrote:
>    
> >   In other words the register *is dead* on entering
> > the try branch.
> >       
>  Read and compare the disassembled code of continue_search_3 before and
>  after my last change
>     
Yes, it's fixed now. At least this case. It's still not the holy grail
I
think.

What about this one:

DEFINE_TEST(set_before_exception)
{
    int ret1, ret2;

    ret2 = ret1 = 0;

    _SEH2_TRY
    {
        ret2 = 1;
        volatile char * p = 0;
        *p = 0;
        ret1 = 1;
    }
    _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
    {
        ret1 = -1;
    }
    _SEH2_END;

    return (ret1 == -1 && ret2 == 1);
}

The compiler assumes that on entering the except block, that not a
single instruction of the try block has been executed. And it will use
this to optimize away everything it can. MSVC will preserve the
instruction order and make sure all variables are stored in memory
locations.
I don't think you can get this handled equally without hacking it into gcc.

This case isn't that bad. We just have the following requirement:
Do not make any assumptions about the order in which code is executed
inside the try block!
Not even if normally you could say that something must be executed in a
special order (like x = func1(); y = func2(x);)

But there's another thing that bothers me. It's guaranteed that all
variables that are used after the call to _SEH2EnterFrame are either
constants or stored in a memory location at the time that SEH2EnterFrame
is called. No more, no less. It is not guaranteed that the variables
stay there.
Now the compiler could decide to move a variable into a register
directly after the call (maybe because it's needed in this register in
both branches) and before the branch. Then in the try branch later it's
moved into another register just before the exception occurs.

Now this is hypothetical, because I couldn't find a real situation where
this happens. I just think it might be possible.

Regards,
Timo

[Thomas Bluemel]

Timo Kreuzer wrote:
>   This case isn't that bad. We just have the
> following requirement:
>  Do not make any assumptions about the order in which code is executed 
>  inside the try block!
>  Not even if normally you could say that something must be executed in a 
>  special order (like x = func1(); y = func2(x);) 
Functions are called in the order you expect, unless the compiler is 
able to analyze/inline them (such as static/inline functions) or unless 
the code is written in a manner where the order is not defined by the 
specification of the language (e.g. multiple calls to functions in 
arguments for a function call). Anything the compiler doesn't know at 
compile-time won't be re-ordered. This applies to all optimizers 
(link-time optimizations may allow to re-order them though).

Thomas

[KJK::Hyperion]

Timo Kreuzer wrote:
>   Yes, it's fixed now. At least this case. It's
> still not the holy grail I
>  think. 
Fix it then

>   The compiler assumes that on entering the except
> block, that not a
>  single instruction of the try block has been executed. 
This is a reasonable assumption. It's also the same identical thing that
happened with PSEH 1. A __try is supposed to imply barriers around any
instruction, because any instruction could throw. We can't do that. The
best we can do is make critical variables volatile, which implies a
barrier in every access to them

>   I don't think you can get this handled equally
> without hacking it into gcc. 
No, you think I should use a nested function for __except bodies but,
for some reason, you prefer not to share this notion with me