New valgrind warnings in wined3d/arb_program_shader.c

[Dan Kegel]

Hi Stefan,
your recent round of changes (circa
http://www.winehq.org/pipermail/wine-cvs/2008-July/045946.html)
seems to have triggered a whole bunch of valgrind warnings, e.g.

http://kegel.com/wine/valgrind/logs/2008-07-31-07.46/vg-d3d9_visual-diff.txt
+ Use of uninitialised value of size 4
+    at  hash_table_get_by_idx (utils.c:1600)
+    by  hash_table_get (utils.c:1750)
+    by  find_ffp_shader (utils.c:1961)
+    by  fragment_prog_arbfp (arb_program_shader.c:2761)
+    by  ActivateContext (context.c:1169)
+    by  drawPrimitive (drawprim.c:950)
+    by  IWineD3DDeviceImpl_DrawIndexedPrimitiveUP (device.c:5350)
+    by  IDirect3DDevice9Impl_DrawIndexedPrimitiveUP (device.c:1227)
+    by  lighting_test (visual.c:300)
+    by  func_visual (visual.c:9234)
+    by  run_test (test.h:488)
+    by  main (test.h:537)
+  Uninitialised value was created by a stack allocation
+    at  fragment_prog_arbfp (arb_program_shader.c:2745)

http://kegel.com/wine/valgrind/logs/2008-07-31-07.46/vg-ddraw_visual-diff.tx...
+ Use of uninitialised value of size 4
+    at  hash_table_get_by_idx (utils.c:1600)
+    by  hash_table_get (utils.c:1750)
+    by  find_ffp_shader (utils.c:1961)
+    by  fragment_prog_arbfp (arb_program_shader.c:2761)
+    by  ActivateContext (context.c:1169)
+    by  drawPrimitive (drawprim.c:950)
+    by  IWineD3DDeviceImpl_DrawIndexedPrimitiveUP (device.c:5350)
+    by  IDirect3DDeviceImpl_7_DrawIndexedPrimitive (device.c:3731)
+    by  IDirect3DDeviceImpl_7_DrawIndexedPrimitive_FPUSetup (device.c:3754)
+    by  lighting_test (visual.c:276)
+    by  func_visual (visual.c:2649)
+    by  run_test (test.h:488)
+    by  main (test.h:537)
+  Uninitialised value was created by a stack allocation
+    at  fragment_prog_arbfp (arb_program_shader.c:2745)

Could you have a look?

Thanks,
Dan

[Stefan Dösinger]

Does the attached patch fix the warnings?
> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On
> Behalf Of Dan Kegel
> Sent: Thursday, July 31, 2008 12:22 PM
> To: wine-devel
> Cc: Stefan Dösinger
> Subject: New valgrind warnings in wined3d/arb_program_shader.c
>
> Hi Stefan,
> your recent round of changes (circa
> http://www.winehq.org/pipermail/wine-cvs/2008-July/045946.html)
> seems to have triggered a whole bunch of valgrind warnings, e.g.
>
> http://kegel.com/wine/valgrind/logs/2008-07-31-07.46/vg-d3d9_visual-
> diff.txt
> - Use of uninitialised value of size 4
> - at  hash_table_get_by_idx (utils.c:1600)
> - by  hash_table_get (utils.c:1750)
> - by  find_ffp_shader (utils.c:1961)
> - by  fragment_prog_arbfp (arb_program_shader.c:2761)
> - by  ActivateContext (context.c:1169)
> - by  drawPrimitive (drawprim.c:950)
> - by  IWineD3DDeviceImpl_DrawIndexedPrimitiveUP (device.c:5350)
> - by  IDirect3DDevice9Impl_DrawIndexedPrimitiveUP (device.c:1227)
> - by  lighting_test (visual.c:300)
> - by  func_visual (visual.c:9234)
> - by  run_test (test.h:488)
> - by  main (test.h:537)
> - Uninitialised value was created by a stack allocation
> - at  fragment_prog_arbfp (arb_program_shader.c:2745)
>
> http://kegel.com/wine/valgrind/logs/2008-07-31-07.46/vg-ddraw_visual-
> diff.txt
> - Use of uninitialised value of size 4
> - at  hash_table_get_by_idx (utils.c:1600)
> - by  hash_table_get (utils.c:1750)
> - by  find_ffp_shader (utils.c:1961)
> - by  fragment_prog_arbfp (arb_program_shader.c:2761)
> - by  ActivateContext (context.c:1169)
> - by  drawPrimitive (drawprim.c:950)
> - by  IWineD3DDeviceImpl_DrawIndexedPrimitiveUP (device.c:5350)
> - by  IDirect3DDeviceImpl_7_DrawIndexedPrimitive (device.c:3731)
> - by  IDirect3DDeviceImpl_7_DrawIndexedPrimitive_FPUSetup
>
> (device.c:3754)
> - by  lighting_test (visual.c:276)
> - by  func_visual (visual.c:2649)
> - by  run_test (test.h:488)
> - by  main (test.h:537)
> - Uninitialised value was created by a stack allocation
> - at  fragment_prog_arbfp (arb_program_shader.c:2745)
>
> Could you have a look?
>
> Thanks,
> Dan

[Dan Kegel]

Stefan Dösinger  wrote:
> Does the attached patch fix the warnings?

It had a level of indirection mistake, but yes, the
following patch fixes the new warnings:

diff --git a/dlls/wined3d/utils.c b/dlls/wined3d/utils.c
index a9fc780..6e1e0ac 100644
--- a/dlls/wined3d/utils.c
+++ b/dlls/wined3d/utils.c
@@ -1790,6 +1790,7 @@ void gen_ffp_op(IWineD3DStateBlockImpl
*stateblock, struct ffp_settings *setting
     DWORD ttff;
     DWORD cop, aop, carg0, carg1, carg2, aarg0, aarg1, aarg2;

+    memset(settings, 0, sizeof(*settings));
     for(i = 0; i < GL_LIMITS(texture_stages); i++) {
         IWineD3DBaseTextureImpl *texture;
         if(stateblock->textureState[i][WINED3DTSS_COLOROP] ==
WINED3DTOP_DISABLE) {

[Stefan Dösinger]

> It had a level of indirection mistake, but yes, the
> following patch fixes the new warnings:

I think it's gcc's fault, or the code relying on something that the C spec
doesn't guarantee. It seems to me that gcc doesn't pack the bitfields
properly as we hoped, and leaves padding bytes in between that are left
uninitialized. The hash function then treats the structure as a binary blob.

Since the bitfields did not do what we hoped they'd do(avoid hashing over
empty data), I'll dig my old manual packing patch out again.

Meanwhile, the problems should not cause any crashes or incorrect rendering,
but it has negative effects on performance.

I tried replacing the memset with this:

for(i = 0; i < 8; i++) {
        settings->op[i].cop = 0;
        settings->op[i].aop = 0;
        /* 0x3F: set all 6 bits of the args to 1 */
        settings->op[i].carg0 = 0;
        settings->op[i].carg1 = 0;
        settings->op[i].carg2 = 0;
        settings->op[i].aarg0 = 0;
        settings->op[i].aarg1 = 0;
        settings->op[i].aarg2 = 0;
        settings->op[i].dst = 0;
        settings->op[i].tex_type = 0;
        settings->op[i].projected = 0;
        settings->op[i].color_correction = 0;
    }
    settings->fog = 0;

The code manually initializes all members of the structure, but the valgrind
warnings still remain

[Alexandre Julliard]

Stefan Dösinger  writes:
> > It had a level of indirection mistake, but yes, the
> > following patch fixes the new warnings:
>
> I think it's gcc's fault, or the code relying on something that the C spec
> doesn't guarantee. It seems to me that gcc doesn't pack the bitfields
> properly as we hoped, and leaves padding bytes in between that are left
> uninitialized. The hash function then treats the structure as a binary blob.

gcc is perfectly correct, if you don't want padding space then your
bitfields have to add up to the size of an integer.

[Stefan Dösinger]

> gcc is perfectly correct, if you don't want padding space then your
> bitfields have to add up to the size of an integer.

Figured that out by now :-)

I thought it left paddings between my flags, but actually there are 12 unused bits at the end, and one uninitialized, because unused, but still hashed member adding troubles.