[PATCH 0/6] A couple of workarounds for warnings emitted by GCC11
GCC11 implements new warnings. This serie of patches: - works around a couple of false positive - fixes a couple of bad handling of corner cases in error handling - last patch should be reviewed by D3D folks. The fix is consistent with function definition, and no regression appear in make test... A+ --- Eric Pouech (6): dlls/msvcrt*: ensure variable sse2_cw is set for all code paths in _control87 (GCC11) In tests, ensure buffers passed as const pointers are always initialized (GCC11) kernelbase: always return NULL in case of error in GetModuleHandleA (GCC11) kernel32/tests: mark pointer as volatile when reading after allocated size Workaround GCC11 complaining about sizeof(a)/sizeof(b) constructs when it's d3dx9_36/tests: fixed order of parameters dlls/d3dx9_36/mesh.c | 2 +- dlls/d3dx9_36/tests/effect.c | 4 ++-- dlls/d3dx9_36/tests/mesh.c | 4 ++-- dlls/kernel32/tests/heap.c | 3 ++- dlls/kernelbase/loader.c | 2 +- dlls/kernelbase/tests/path.c | 1 + dlls/msvcirt/tests/msvcirt.c | 5 +++-- dlls/msvcrt/math.c | 2 +- dlls/shlwapi/tests/clist.c | 1 + 9 files changed, 14 insertions(+), 10 deletions(-)
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/msvcrt/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..ad632e70548 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5643,7 +5643,7 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) { unsigned int flags = 0; #ifdef __i386__ - unsigned int sse2_cw; + unsigned int sse2_cw = 0; __control87_2( newval, mask, &flags, &sse2_cw );
On 9/28/21 11:49, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/msvcrt/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..ad632e70548 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5643,7 +5643,7 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) { unsigned int flags = 0; #ifdef __i386__ - unsigned int sse2_cw; + unsigned int sse2_cw = 0;
__control87_2( newval, mask, &flags, &sse2_cw );
Wouldn't it be better to check for failure from __control87_2()?
Le 28/09/2021 à 20:01, Zebediah Figura (she/her) a écrit :
On 9/28/21 11:49, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/msvcrt/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..ad632e70548 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5643,7 +5643,7 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) { unsigned int flags = 0; #ifdef __i386__ - unsigned int sse2_cw; + unsigned int sse2_cw = 0; __control87_2( newval, mask, &flags, &sse2_cw );
Wouldn't it be better to check for failure from __control87_2()?
unfortunately, gcc11 still complains when checking for failure of _control87_2() gcc doesn't seem to be smart enough to infer that ss2_cw is always when _control87_2() returns 1 A+
On 9/29/21 02:43, Eric Pouech wrote:
Le 28/09/2021 à 20:01, Zebediah Figura (she/her) a écrit :
On 9/28/21 11:49, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/msvcrt/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..ad632e70548 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5643,7 +5643,7 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) { unsigned int flags = 0; #ifdef __i386__ - unsigned int sse2_cw; + unsigned int sse2_cw = 0; __control87_2( newval, mask, &flags, &sse2_cw );
Wouldn't it be better to check for failure from __control87_2()?
unfortunately, gcc11 still complains when checking for failure of _control87_2()
gcc doesn't seem to be smart enough to infer that ss2_cw is always when _control87_2() returns 1
That doesn't match what I have here. With the attached patch gcc 11.1 doesn't complain.
Le 29/09/2021 à 17:57, Zebediah Figura (she/her) a écrit :
On 9/29/21 02:43, Eric Pouech wrote:
Le 28/09/2021 à 20:01, Zebediah Figura (she/her) a écrit :
On 9/28/21 11:49, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/msvcrt/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..ad632e70548 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5643,7 +5643,7 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) { unsigned int flags = 0; #ifdef __i386__ - unsigned int sse2_cw; + unsigned int sse2_cw = 0; __control87_2( newval, mask, &flags, &sse2_cw );
Wouldn't it be better to check for failure from __control87_2()?
unfortunately, gcc11 still complains when checking for failure of _control87_2()
gcc doesn't seem to be smart enough to infer that ss2_cw is always when _control87_2() returns 1
That doesn't match what I have here. With the attached patch gcc 11.1 doesn't complain.
what I tried is: diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..4560040eb9f 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5645,10 +5645,11 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) #ifdef __i386__ unsigned int sse2_cw; - __control87_2( newval, mask, &flags, &sse2_cw ); - - if ((flags ^ sse2_cw) & (_MCW_EM | _MCW_RC)) flags |= _EM_AMBIGUOUS; - flags |= sse2_cw; + if (__control87_2( newval, mask, &flags, &sse2_cw )) + { + if ((flags ^ sse2_cw) & (_MCW_EM | _MCW_RC)) flags |= _EM_AMBIGUOUS; + flags |= sse2_cw; + } #else flags = newval; _setfp(&flags, mask, NULL, 0); which still gives me the warnings, when compiling the 32bit part of a wow64 conf (but not on a pure 32bit conf) your solution doesn't generate warnings on neither of the two so will need further investigation on: - discrepency wrt wow - why the difference between the two patches A+
On 9/29/21 12:59, Eric Pouech wrote:
Le 29/09/2021 à 17:57, Zebediah Figura (she/her) a écrit :
On 9/29/21 02:43, Eric Pouech wrote:
Le 28/09/2021 à 20:01, Zebediah Figura (she/her) a écrit :
On 9/28/21 11:49, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/msvcrt/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..ad632e70548 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5643,7 +5643,7 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) { unsigned int flags = 0; #ifdef __i386__ - unsigned int sse2_cw; + unsigned int sse2_cw = 0; __control87_2( newval, mask, &flags, &sse2_cw );
Wouldn't it be better to check for failure from __control87_2()?
unfortunately, gcc11 still complains when checking for failure of _control87_2()
gcc doesn't seem to be smart enough to infer that ss2_cw is always when _control87_2() returns 1
That doesn't match what I have here. With the attached patch gcc 11.1 doesn't complain.
what I tried is:
diff --git a/dlls/msvcrt/math.c b/dlls/msvcrt/math.c index 7f59a4d20d4..4560040eb9f 100644 --- a/dlls/msvcrt/math.c +++ b/dlls/msvcrt/math.c @@ -5645,10 +5645,11 @@ unsigned int CDECL _control87(unsigned int newval, unsigned int mask) #ifdef __i386__ unsigned int sse2_cw;
- __control87_2( newval, mask, &flags, &sse2_cw ); - - if ((flags ^ sse2_cw) & (_MCW_EM | _MCW_RC)) flags |= _EM_AMBIGUOUS; - flags |= sse2_cw; + if (__control87_2( newval, mask, &flags, &sse2_cw )) + { + if ((flags ^ sse2_cw) & (_MCW_EM | _MCW_RC)) flags |= _EM_AMBIGUOUS; + flags |= sse2_cw; + } #else flags = newval; _setfp(&flags, mask, NULL, 0);
which still gives me the warnings, when compiling the 32bit part of a wow64 conf
(but not on a pure 32bit conf)
your solution doesn't generate warnings on neither of the two
so will need further investigation on:
- discrepency wrt wow
- why the difference between the two patches
I don't get warnings with that diff either. What version of GCC do you have?
which still gives me the warnings, when compiling the 32bit part of a wow64 conf
(but not on a pure 32bit conf)
your solution doesn't generate warnings on neither of the two
so will need further investigation on:
- discrepency wrt wow
- why the difference between the two patches
I don't get warnings with that diff either. What version of GCC do you have?
well, o well.... retesting today, and couldn't get any warnings with the patch I posted yesterday...<grrr> moreover, in this patch, if I change the if (_control... by if (!control... (inverting the test), I get no warnings either which makes absolutely no sense I'm starting to wonder if the -Wmaybe-uninitialized warning could be a little fragile at this point there are a couple of bugs reports on gcc bugzilla reguarding this warning potentially bugs report like may link to the same type of issues https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102381 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101768 I may wait a bit to let things settle down before taking any action on this fix in msvcrt A+
On 9/30/21 12:14 PM, Eric Pouech wrote:
which still gives me the warnings, when compiling the 32bit part of a wow64 conf
(but not on a pure 32bit conf)
your solution doesn't generate warnings on neither of the two
so will need further investigation on:
- discrepency wrt wow
- why the difference between the two patches
I don't get warnings with that diff either. What version of GCC do you have?
well, o well....
retesting today, and couldn't get any warnings with the patch I posted yesterday...<grrr>
moreover, in this patch, if I change the if (_control... by if (!control... (inverting the test), I get no warnings either which makes absolutely no sense
I'm starting to wonder if the -Wmaybe-uninitialized warning could be a little fragile at this point
there are a couple of bugs reports on gcc bugzilla reguarding this warning
potentially bugs report like may link to the same type of issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102381
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101768
I may wait a bit to let things settle down before taking any action on this fix in msvcrt
I mean, -Wmaybe-uninitialized has always been at least somewhat heuristic, throughout the entire history of gcc, and there have been false positives and false negatives. But I've found that it tends to behave nicely for idiomatically structured code. I've found very few cases where "just initialize the variable to zero" seems like the best solution to me.
Le 30/09/2021 à 19:39, Zebediah Figura a écrit :
On 9/30/21 12:14 PM, Eric Pouech wrote:
which still gives me the warnings, when compiling the 32bit part of a wow64 conf
(but not on a pure 32bit conf)
your solution doesn't generate warnings on neither of the two
so will need further investigation on:
- discrepency wrt wow
- why the difference between the two patches
I don't get warnings with that diff either. What version of GCC do you have?
well, o well....
retesting today, and couldn't get any warnings with the patch I posted yesterday...<grrr>
moreover, in this patch, if I change the if (_control... by if (!control... (inverting the test), I get no warnings either which makes absolutely no sense
I'm starting to wonder if the -Wmaybe-uninitialized warning could be a little fragile at this point
there are a couple of bugs reports on gcc bugzilla reguarding this warning
potentially bugs report like may link to the same type of issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102381
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101768
I may wait a bit to let things settle down before taking any action on this fix in msvcrt
I mean, -Wmaybe-uninitialized has always been at least somewhat heuristic, throughout the entire history of gcc, and there have been false positives and false negatives. But I've found that it tends to behave nicely for idiomatically structured code. I've found very few cases where "just initialize the variable to zero" seems like the best solution to me.
I'm not suggesting at this point to disable -Wmaybe-unitialized I'm just facing instability in tests results here: - either because of me - hw instability (i've seen some abnormal crashes) - heisenbug in gcc (listed in decreasing probability) A+
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99091 Your paranoid android. === debiant2 (build log) === error: corrupt patch at line 44 Task: Patch failed to apply === debiant2 (build log) === error: corrupt patch at line 44 Task: Patch failed to apply
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/kernelbase/tests/path.c | 1 + dlls/msvcirt/tests/msvcirt.c | 5 +++-- dlls/shlwapi/tests/clist.c | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/kernelbase/tests/path.c b/dlls/kernelbase/tests/path.c index af511f500f3..b496b75d101 100644 --- a/dlls/kernelbase/tests/path.c +++ b/dlls/kernelbase/tests/path.c @@ -1213,6 +1213,7 @@ static void test_PathCchCanonicalizeEx(void) } path_outW[0] = 0xff; + memset(path_inW, 0xA5, sizeof(path_inW)); hr = pPathCchCanonicalizeEx(path_outW, 0, path_inW, 0); ok(hr == E_INVALIDARG, "expect hr %#x, got %#x\n", E_INVALIDARG, hr); ok(path_outW[0] = 0xff, "expect path_outW unchanged\n"); diff --git a/dlls/msvcirt/tests/msvcirt.c b/dlls/msvcirt/tests/msvcirt.c index 1a72f5ab954..5e27f92585b 100644 --- a/dlls/msvcirt/tests/msvcirt.c +++ b/dlls/msvcirt/tests/msvcirt.c @@ -6816,8 +6816,9 @@ static void test_istrstream(void) strstreambuf *pssb; char buffer[32]; - memset(&is1, 0xab, sizeof(istream)); - memset(&is2, 0xab, sizeof(istream)); + memset(&is1, 0xab, sizeof(istream)); + memset(&is2, 0xab, sizeof(istream)); + memset(buffer, 0xab, sizeof(buffer)); /* constructors/destructors */ pis = call_func4(p_istrstream_buffer_ctor, &is1, buffer, 32, TRUE); diff --git a/dlls/shlwapi/tests/clist.c b/dlls/shlwapi/tests/clist.c index 0c914d608e5..acef37a1a1e 100644 --- a/dlls/shlwapi/tests/clist.c +++ b/dlls/shlwapi/tests/clist.c @@ -574,6 +574,7 @@ static void test_IStream_Write(void) return; InitDummyStream(&streamobj); + memset(buff, 0xA5, sizeof(buff)); hRet = pIStream_Write(&streamobj.IStream_iface, buff, sizeof(buff)); ok(hRet == S_OK, "failed Write()\n");
On 9/28/21 7:49 PM, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/kernelbase/tests/path.c | 1 + dlls/msvcirt/tests/msvcirt.c | 5 +++-- dlls/shlwapi/tests/clist.c | 1 + 3 files changed, 5 insertions(+), 2 deletions(-)
If we're doing this kind of fixes, please split them per-module so it's traceable in case we get new failures accidentally.
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/kernelbase/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index da139e91176..57666d480da 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -334,7 +334,7 @@ done: */ HMODULE WINAPI DECLSPEC_HOTPATCH GetModuleHandleA( LPCSTR module ) { - HMODULE ret; + HMODULE ret = NULL; GetModuleHandleExA( GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, module, &ret ); return ret;
On 9/28/21 11:49, Eric Pouech wrote:
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/kernelbase/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index da139e91176..57666d480da 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -334,7 +334,7 @@ done: */ HMODULE WINAPI DECLSPEC_HOTPATCH GetModuleHandleA( LPCSTR module ) { - HMODULE ret; + HMODULE ret = NULL;
GetModuleHandleExA( GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, module, &ret ); return ret;
That GCC is warning here implies that there is a path where GetModuleHandleExW() fails but doesn't clear its output parameter. Shouldn't that be fixed instead? Or, if not, shouldn't we be explicitly checking for failure from GetModuleHandleExA()?
so that GCC11 no longer complains Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/kernel32/tests/heap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 8558bd7f1b3..38a0a5731c1 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -891,7 +891,8 @@ static void test_HeapQueryInformation(void) static void test_heap_checks( DWORD flags ) { - BYTE old, *p, *p2; + BYTE old, *p2; + BYTE * volatile p; /* as we read past of allocated size, don't let GCC11 complain */ BOOL ret; SIZE_T i, size, large_size = 3000 * 1024 + 37;
not referring to the number of elements of an array Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/d3dx9_36/tests/effect.c | 4 ++-- dlls/d3dx9_36/tests/mesh.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index 4b91425389f..d1954c5cd11 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -1475,7 +1475,7 @@ static void test_effect_parameter_value_GetMatrixPointerArray(const struct test_ HRESULT hr; DWORD cmp = 0xabababab; FLOAT fvalue[EFFECT_PARAMETER_VALUE_ARRAY_SIZE]; - D3DXMATRIX *matrix_pointer_array[sizeof(fvalue)/sizeof(D3DXMATRIX)]; + D3DXMATRIX *matrix_pointer_array[sizeof(fvalue)/(sizeof(D3DXMATRIX))]; UINT l, k, m, element, err = 0; for (element = 0; element <= res_desc->Elements + 1; ++element) @@ -1657,7 +1657,7 @@ static void test_effect_parameter_value_GetMatrixTransposePointerArray(const str HRESULT hr; DWORD cmp = 0xabababab; FLOAT fvalue[EFFECT_PARAMETER_VALUE_ARRAY_SIZE]; - D3DXMATRIX *matrix_pointer_array[sizeof(fvalue)/sizeof(D3DXMATRIX)]; + D3DXMATRIX *matrix_pointer_array[sizeof(fvalue)/(sizeof(D3DXMATRIX))]; UINT l, k, m, element, err = 0; for (element = 0; element <= res_desc->Elements + 1; ++element) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index e981f3623ce..a583a42d30f 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4824,7 +4824,7 @@ static void test_update_semantics(void) hr = mesh->lpVtbl->GetDeclaration(mesh, declaration); ok(hr == D3D_OK, "Couldn't get vertex declaration. Got %#x, expected D3D_OK\n", hr); decl_mem = (int*)declaration; - for (i = sizeof(declaration0)/sizeof(*decl_mem); i < sizeof(declaration)/sizeof(*decl_mem); i++) + for (i = sizeof(declaration0)/(sizeof(*decl_mem)); i < sizeof(declaration)/(sizeof(*decl_mem)); i++) { equal = memcmp(&decl_mem[i], &filler_b, sizeof(filler_b)); ok(equal == 0,
(spotted by GCC11) Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/d3dx9_36/mesh.c | 2 +- dlls/d3dx9_36/tests/mesh.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c index 8f7c926c32a..48abb7ede41 100644 --- a/dlls/d3dx9_36/mesh.c +++ b/dlls/d3dx9_36/mesh.c @@ -5508,7 +5508,7 @@ static inline BOOL is_direction_similar(D3DXVECTOR2 *dir1, D3DXVECTOR2 *dir2, fl static inline D3DXVECTOR2 *unit_vec2(D3DXVECTOR2 *dir, const D3DXVECTOR2 *pt1, const D3DXVECTOR2 *pt2) { - return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); + return D3DXVec2Normalize(dir, D3DXVec2Subtract(dir, pt2, pt1)); } struct cos_table diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index a583a42d30f..8c98f790d1b 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -3611,7 +3611,7 @@ static inline BOOL is_direction_similar(D3DXVECTOR2 *dir1, D3DXVECTOR2 *dir2, fl static inline D3DXVECTOR2 *unit_vec2(D3DXVECTOR2 *dir, const D3DXVECTOR2 *pt1, const D3DXVECTOR2 *pt2) { - return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); + return D3DXVec2Normalize(dir, D3DXVec2Subtract(dir, pt2, pt1)); } static BOOL attempt_line_merge(struct outline *outline,
On Tue, 28 Sept 2021 at 18:50, Eric Pouech <eric.pouech(a)gmail.com> wrote:
static inline D3DXVECTOR2 *unit_vec2(D3DXVECTOR2 *dir, const D3DXVECTOR2 *pt1, const D3DXVECTOR2 *pt2) { - return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); + return D3DXVec2Normalize(dir, D3DXVec2Subtract(dir, pt2, pt1)); }
That's arguably more readable, but note that this doesn't otherwise make much of a difference; D3DXVec2Subtract() returns the output vector, so this effectively does "D3DXVec2Normalize(dir, dir);" either way.
to be complete: gcc11 complains with: In function 'unit_vec2', inlined from 'attempt_line_merge' at /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5542:5: /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5511:12: warning: 'lastdir' may be used uninitialized [-Wmaybe-uninitialized] 5511 | return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /home/eric/work/wine/include/d3dx9.h:31, from /home/eric/work/wine/dlls/d3dx9_36/d3dx9_private.h:32, from /home/eric/work/wine/dlls/d3dx9_36/mesh.c:31: /home/eric/work/wine/dlls/d3dx9_36/mesh.c: In function 'attempt_line_merge': /home/eric/work/wine/include/d3dx9math.h:352:21: note: by argument 2 of type 'const D3DXVECTOR2 *' to 'D3DXVec2Normalize' declared here 352 | D3DXVECTOR2* WINAPI D3DXVec2Normalize(D3DXVECTOR2 *pout, const D3DXVECTOR2 *pv); | ^~~~~~~~~~~~~~~~~ /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5527:25: note: 'lastdir' declared here 5527 | D3DXVECTOR2 curdir, lastdir; | ^~~~~~~ as D3DXV2Normalize isn't symetrical in its arguments definition (first arg is const ptr as input, while second arg is ptr as output) so gcc11 complains about passing as first arg (being a const ptr) an uninitialized object (it has no way of knowing that both args are aliased) A+ On Tue, 28 Sept 2021 at 22:57, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On Tue, 28 Sept 2021 at 18:50, Eric Pouech <eric.pouech(a)gmail.com> wrote:
static inline D3DXVECTOR2 *unit_vec2(D3DXVECTOR2 *dir, const D3DXVECTOR2 *pt1, const D3DXVECTOR2 *pt2) { - return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); + return D3DXVec2Normalize(dir, D3DXVec2Subtract(dir, pt2, pt1)); }
That's arguably more readable, but note that this doesn't otherwise make much of a difference; D3DXVec2Subtract() returns the output vector, so this effectively does "D3DXVec2Normalize(dir, dir);" either way.
-- -- Eric Pouech
On Wed, 29 Sept 2021 at 09:19, Eric Pouech <eric.pouech(a)gmail.com> wrote:
to be complete: gcc11 complains with: In function 'unit_vec2', inlined from 'attempt_line_merge' at /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5542:5: /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5511:12: warning: 'lastdir' may be used uninitialized [-Wmaybe-uninitialized] 5511 | return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /home/eric/work/wine/include/d3dx9.h:31, from /home/eric/work/wine/dlls/d3dx9_36/d3dx9_private.h:32, from /home/eric/work/wine/dlls/d3dx9_36/mesh.c:31: /home/eric/work/wine/dlls/d3dx9_36/mesh.c: In function 'attempt_line_merge': /home/eric/work/wine/include/d3dx9math.h:352:21: note: by argument 2 of type 'const D3DXVECTOR2 *' to 'D3DXVec2Normalize' declared here 352 | D3DXVECTOR2* WINAPI D3DXVec2Normalize(D3DXVECTOR2 *pout, const D3DXVECTOR2 *pv); | ^~~~~~~~~~~~~~~~~ /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5527:25: note: 'lastdir' declared here 5527 | D3DXVECTOR2 curdir, lastdir; | ^~~~~~~
as D3DXV2Normalize isn't symetrical in its arguments definition (first arg is const ptr as input, while second arg is ptr as output) so gcc11 complains about passing as first arg (being a const ptr) an uninitialized object (it has no way of knowing that both args are aliased)
I don't think that's exactly what's happening. In particular, note that the message complains about the second argument to D3DXVec2Normalize(), not the first. My guess is that it doesn't notice that the D3DXVec2Subtract() call initialises "dir" before the call to D3DXVec2Normalize() actually happens. That's admittedly a little tricky; the straightforward translation would first push "dir" to the stack, which points to uninitialised memory at that point, then call D3DXVec2Subtract() which initialises the vector pointed to by "dir", push the return value of D3DXVec2Subtract(), and finally call D3DXVec2Normalize(). In any case, I think the change makes things more readable, so in that regard I think it's fine; I'd quibble a little about the commit message. It's up to Matteo to sign off on this patch though.
On 9/29/21 11:48 AM, Henri Verbeet wrote:
On Wed, 29 Sept 2021 at 09:19, Eric Pouech <eric.pouech(a)gmail.com> wrote:
to be complete: gcc11 complains with: In function 'unit_vec2', inlined from 'attempt_line_merge' at /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5542:5: /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5511:12: warning: 'lastdir' may be used uninitialized [-Wmaybe-uninitialized] 5511 | return D3DXVec2Normalize(D3DXVec2Subtract(dir, pt2, pt1), dir); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /home/eric/work/wine/include/d3dx9.h:31, from /home/eric/work/wine/dlls/d3dx9_36/d3dx9_private.h:32, from /home/eric/work/wine/dlls/d3dx9_36/mesh.c:31: /home/eric/work/wine/dlls/d3dx9_36/mesh.c: In function 'attempt_line_merge': /home/eric/work/wine/include/d3dx9math.h:352:21: note: by argument 2 of type 'const D3DXVECTOR2 *' to 'D3DXVec2Normalize' declared here 352 | D3DXVECTOR2* WINAPI D3DXVec2Normalize(D3DXVECTOR2 *pout, const D3DXVECTOR2 *pv); | ^~~~~~~~~~~~~~~~~ /home/eric/work/wine/dlls/d3dx9_36/mesh.c:5527:25: note: 'lastdir' declared here 5527 | D3DXVECTOR2 curdir, lastdir; | ^~~~~~~
as D3DXV2Normalize isn't symetrical in its arguments definition (first arg is const ptr as input, while second arg is ptr as output) so gcc11 complains about passing as first arg (being a const ptr) an uninitialized object (it has no way of knowing that both args are aliased)
I don't think that's exactly what's happening. In particular, note that the message complains about the second argument to D3DXVec2Normalize(), not the first. My guess is that it doesn't notice that the D3DXVec2Subtract() call initialises "dir" before the call to D3DXVec2Normalize() actually happens. That's admittedly a little tricky; the straightforward translation would first push "dir" to the stack, which points to uninitialised memory at that point, then call D3DXVec2Subtract() which initialises the vector pointed to by "dir", push the return value of D3DXVec2Subtract(), and finally call D3DXVec2Normalize().
The problem actually comes from D3DXVec2Subtract which can return NULL if any of the input pointer is. It's a little bit convoluted, but although the code takes the address of the vector, it's still possible to end up with NULL pointers. For instance if outline->items == NULL and pt_index == 0, pt or prevpt (if pt_index == 1, or wraps around) may be NULL. Then, as pos is the first member, &pt->pos (or &prevpt->pos) may be NULL as well. Of course, outline->items is probably not NULL, but that may be too complicated to infer. Changing pos to be second point2d member solves the warning too. -- Rémi Bernon <rbernon(a)codeweavers.com>
On Wed, 29 Sept 2021 at 12:21, Rémi Bernon <rbernon(a)codeweavers.com> wrote:
The problem actually comes from D3DXVec2Subtract which can return NULL if any of the input pointer is.
It's a little bit convoluted, but although the code takes the address of the vector, it's still possible to end up with NULL pointers.
For instance if outline->items == NULL and pt_index == 0, pt or prevpt (if pt_index == 1, or wraps around) may be NULL. Then, as pos is the first member, &pt->pos (or &prevpt->pos) may be NULL as well.
Of course, outline->items is probably not NULL, but that may be too complicated to infer.
Changing pos to be second point2d member solves the warning too.
Ah right, if D3DXVec2Subtract() would return NULL it would leave "dir" uninitialised. Of course if "outline->items" is in fact NULL, "lastdir" being uninitialised probably isn't the worst thing that's going to happen, but that seems fair enough.
participants (8)
-
Eric Pouech -
Eric Pouech -
Henri Verbeet -
Marvin -
Nikolay Sivov -
Rémi Bernon -
Zebediah Figura -
Zebediah Figura (she/her)