30 Dec
2008
30 Dec
'08
8:59 p.m.
KJK::Hyperion wrote:
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.
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)
{
Timo Kreuzer wrote:
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.
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