[PATCH v3 0/1] MR3458: ddraw/tests: Fix overflow bugs in compare_float.
1. Subtracting integers may result in an overflow or underflow. 2. Right at the 'edge' of underflowing, the result of subtraction may be `INT_MIN`, and the call to `abs()` will also result in `INT_MIN`. This fix accounts for all of these conditions. EXAMPLES: 1. can be encountered by comparing 2.0 and -2.0 2. can be encountered by comparing -2.0 and 2.0 NOTE: There are 14 more instances of `compare_float` across several modules. I would like to make sure the logic is sound with this MR, then I can take on the rest. -- v3: ddraw/tests: Fix overflow bugs in compare_float. https://gitlab.winehq.org/wine/wine/-/merge_requests/3458
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 27 ++++++++++++--------------- dlls/ddraw/tests/ddraw2.c | 27 ++++++++++++--------------- dlls/ddraw/tests/ddraw4.c | 27 ++++++++++++--------------- dlls/ddraw/tests/ddraw7.c | 27 ++++++++++++--------------- 4 files changed, 48 insertions(+), 60 deletions(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index cde80c58e99..bb90b77d527 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -50,20 +50,24 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; - if (x < 0) - x = INT_MIN - x; - if (y < 0) - y = INT_MIN - y; - - if (abs(x - y) > ulps) - return FALSE; + if (x >= 0) + x = -(INT_MIN + x); + if (y >= 0) + y = -(INT_MIN + y); - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, float w, unsigned int ulps) @@ -74,13 +78,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) diff --git a/dlls/ddraw/tests/ddraw2.c b/dlls/ddraw/tests/ddraw2.c index 86056014f96..d627696720e 100644 --- a/dlls/ddraw/tests/ddraw2.c +++ b/dlls/ddraw/tests/ddraw2.c @@ -51,20 +51,24 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; - if (x < 0) - x = INT_MIN - x; - if (y < 0) - y = INT_MIN - y; - - if (abs(x - y) > ulps) - return FALSE; + if (x >= 0) + x = -(INT_MIN + x); + if (y >= 0) + y = -(INT_MIN + y); - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, float w, unsigned int ulps) @@ -75,13 +79,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index 4458cea5145..adbd111746d 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -57,20 +57,24 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; - if (x < 0) - x = INT_MIN - x; - if (y < 0) - y = INT_MIN - y; - - if (abs(x - y) > ulps) - return FALSE; + if (x >= 0) + x = -(INT_MIN + x); + if (y >= 0) + y = -(INT_MIN + y); - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, float w, unsigned int ulps) @@ -81,13 +85,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index bbb78169aea..e26a6ad7a8e 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -68,20 +68,24 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; - if (x < 0) - x = INT_MIN - x; - if (y < 0) - y = INT_MIN - y; - - if (abs(x - y) > ulps) - return FALSE; + if (x >= 0) + x = -(INT_MIN + x); + if (y >= 0) + y = -(INT_MIN + y); - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec3(struct vec3 *vec, float x, float y, float z, unsigned int ulps) @@ -99,13 +103,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3458
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135488 Your paranoid android. === w10pro64 (32 bit report) === ddraw: ddraw7.c:10266: Test failed: Failed to set foreground window. ddraw7.c:10268: Test failed: Got unexpected hr 0. ddraw7.c:10270: Test failed: Got unexpected hr 0. ddraw7.c:10272: Test failed: Got unexpected hr 0. ddraw7.c:10274: Test failed: Got unexpected hr 0. ddraw7.c:10276: Test failed: Got unexpected hr 0. ddraw7.c:10286: Test failed: Got unexpected hr 0. ddraw7.c:10288: Test failed: Got unexpected hr 0. ddraw7.c:10290: Test failed: Got unexpected hr 0. ddraw7.c:10294: Test failed: Failed to set foreground window. ddraw7.c:10298: Test failed: Got unexpected hr 0. ddraw7.c:10300: Test failed: Got unexpected hr 0. ddraw7.c:10306: Test failed: Got unexpected hr 0. ddraw7.c:10330: Test failed: Got unexpected hr 0. ddraw7.c:10332: Test failed: Got unexpected hr 0x887600e1. ddraw7.c:10338: Test failed: Got unexpected hr 0. === w10pro64 (64 bit report) === ddraw: ddraw7.c:17254: Test failed: WM_KILLFOCUS was not received. ddraw7.c:17416: Test failed: Got unexpected hr 0x887600e1. ddraw7.c:17419: Test failed: Got unexpected hr 0x887600ff. 2070:ddraw7: unhandled exception c0000005 at 00000000004F63F0
I'm going with a solution that
1. uses `compare_uint`, 1. ensures the things that should *definitively* return false *will* return false (e.g. 2 vs -2), and 1. other results stay the same as before (in the 0.0/-0.0/subnormal range), even if of dubious value.
I think Henri is right, though; we don't want positive and negative denormals to compare as equal. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40954
While I'm still not a fan of changing the current behavior for subnormals, I will follow consensus and update the MR accordingly. Note that with just the compare_uint change, while it does leave 0.0 and -0.0 comparing equal, and makes positive/negative subnormals compare unequal, for comparisons of pos/neg subnormals with pos/neg 0 the results seem a bit erratic. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40969
It might be a good idea to make it so that zero compared with non-zero is consistently unequal (rather than erratically as with only the `compare_uint` change). Doing so could look like this: ```diff - if (x < 0) - x = INT_MIN - x; - if (y < 0) - y = INT_MIN - y; + if (x == INT_MIN) + x = 0; + if (y == INT_MIN) + y = 0; + if (x == 0 || y == 0) + return x == 0 && y == 0; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40970
Perhaps fortunately, this isn't my call, but I'm having a hard time seeing the denormal behaviour as something that's in dire need of touching. Beyond that, all the usual guidelines still apply, of course; one logical change per commit, clearly explain what the commit does and why it's necessary in the commit message, and so on. You may find the earlier referenced vkd3d commit helpful as a reference. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40971
Perhaps fortunately, this isn't my call, but I'm having a hard time seeing the denormal behaviour as something that's in dire need of touching.
Agreed. That said, I don't think we should diverge from vkd3d without a good reason, so I'd propose to either port vkd3d's 69ecfdfb0 directly, or if we're going to make further changes, change it there first. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40975
`compare_uint` handles multiple cases differently than the current code does, including denormal cases. So just by changing to `compare_uint`, we _are_ touching denormal behavior. The choice then is: do we adjust additional code to keep denormal behavior matching the original, do we _not_ adjust additional code which means we _do_ change denormal behavior, or do we adjust additional code _and_ additional behavior. Door number 2 is... I will make the changes in accordance with vkd3d. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40976
participants (5)
-
Henri Verbeet (＠hverbeet) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Marvin -
Zebediah Figura (＠zfigura)