Horrible heap error when running Psychophbia

[Edward-san]

It's the padding bits in GLSkyInfo struct. If I change the order of its members like this ('pad' is required, it seems):

Code: Select all

	FMaterial *texture[2];
	FTextureID skytexno1;
	PalEntry fadecolor;	// if this isn't made part of the dome things will become more complicated when sky fog is used.
	float x_offset[2];
	float y_offset;		// doubleskies don't have a y-offset
	unsigned mirrored : 1;
	unsigned doublesky : 1;
	unsigned sky2 : 1;
	unsigned pad : 1;

the valgrind error disappears in doom2 maps (and also reduced the size of the struct by 8 bytes in my system).

[Graf Zahl]

What? How is that possible, does 'sizeof' give a bad value?
Normally it should include padding bytes to ensure that arrays are properly offset.
Can it be that there's an alignment mismatch between files? That could explain a crash.

[Edward-san]

Hm, I still get some errors with psychophobia. See attached text.

[Graf Zahl]

Looks like another manifestation of the same thing to me.
Can you check the size of the affected structures with a static_assert? As I said, my suspicion is that some file is doing some bad alignment voodoo.

[Edward-san]

I added this code in gl_portal.h:

Code: Select all

static_assert( sizeof(GLHorizonInfo) == 96, "bad GLHorizonInfo size!" );
static_assert( sizeof(GLSkyInfo) == 48, "bad GLSkyInfo size!" );

no error came out.

[Graf Zahl]

That's really odd...
I have no idea what happens here.

[milasudril]

Now I have tested for the exit bug in vanilla ZDoom, and yes, it has the same problem. I think this part of the story should be forwarded to Randy. And the software renderer is totally broken in the ZDoom repo. I wonder if the source really is clean from pointer size assumptions, such as

Bad assumptions

sizeof(int)==sizeof(void*) //True on 32-bit Linux and Windows systems
sizeof(int)==sizeof(long) //True on 32-bit Linux systems, 32- and 64-bit Windows systems, but not on 64-bit Linux systems
sizeof(long)==sizeof(void*) //True on Linux and 32-bit Windows but not on 64-bit Windows
sizeof(long long)==sizeof(void*) //True on 64 bit Linux and Windows [This is an unlikely assumption]

Better assumptions

sizeof(size_t)==sizeof(void*)
sizeof(intptr_t)==sizeof(void*)

I have a 32-bit Linux box with OpenGL 3, so it is straight forward to test things on that machine, a Pentium 4 (Prescott) with 1 G ram, NVIDIA GeForce 9500 GT.

[milasudril]

Though what I am discussing now is really a ZDoom issue, I want to complete with valgrind output from the 32-bit system running ZDoom. I should have removed -fomit-frame-pointer on the 64-bit compile. I guess that is why I cannot get good output from valgrind on that machine. However, this output looks much nicer on the 32-bit system. I can also confirm that the software renderer has fewer issues on 32-bit than on 64-bit. There is only an init issue that does not appear more than once:

==3762== Conditional jump or move depends on uninitialised value(s)
==3762== at 0x8148511: R_DrawVisSprite(vissprite_t*) (r_things.cpp:409)
==3762== by 0x814AEB9: R_DrawPSprite(DPSprite*, AActor*, float, float, double, double, double) (r_things.cpp:1547)
==3762== by 0x814B6B6: R_DrawPlayerSprites() (r_things.cpp:1663)
==3762== by 0x813B8A6: R_RenderActorView(AActor*, bool) (r_main.cpp:917)
==3762== by 0x8130E8F: FSoftwareRenderer::RenderView(player_t*) (r_swrenderer.cpp:158)
==3762== by 0x81E55CA: D_Display() (d_main.cpp:770)
==3762== by 0x81E6432: D_DoomLoop() (d_main.cpp:1013)
==3762== by 0x81E8E1C: D_DoomMain() (d_main.cpp:2642)
==3762== by 0x811787E: main (i_main.cpp:317)

... And there are no distorted walls.

I will return to the OpenGL renderer after I have compiled GZDoom also

[milasudril]

None of this could be cause of a bad crash, though.

Sure, the name stuff could cause a crash. The only time you know that the heap has been corrupted is when you call any of the heap functions again, and that happens usually when the program exits. Thus the crash happens at that moment. Glibc detects that the heap was corrupted and kills the application.

[Edward-san]

Do you mean you can reproduce the heap buffer crash in zdoom, too?

[milasudril]

Do you mean you can reproduce the heap buffer crash in zdoom, too?

Yes, I can. Both on 32-bit and 64-bit.

[milasudril]

So this is what happens in GZDoom on the 32-bit system. Notice that on this system, there is no error in the sky code. However, there are some issues with the static splash screen. What I have seen so far:

1. Both ZDoom and GZDoom are affected by an out-of-bounds issue that appears to be in the name manager. 32- or 64-bit does not matter. => Report to ZDoom
2. The ZDoom software renderer does not work on the 64-bit system, but it works on the 32-bit system => Report to ZDoom
3. The sky rendering init issue is only present on the 64-bit system.
4. Some other uninitializations bugs are present when running Psychophobia (gl_walls.cpp:527, gl_drawinfo.cpp:612). I cannot test this on the 32-bit machine, since it is to slow. See attached file.

Fix #1 first. #2 and #3 may appear due to some misuse of data types. Most likely is that something is declared directly or indirectly as long, when it should have been int, or vice versa. #3 may be a consequence of #1, due to the latent nature of heap errors.

[Edward-san]

2. The ZDoom software renderer does not work on the 64-bit system, but it works on the 32-bit system => Report to ZDoom

Keep in mind that when you use valgrind you should compile zdoom with -DVALGRIND=ON in cmake options, so that the renderer should look okay.

[milasudril]

2. The ZDoom software renderer does not work on the 64-bit system, but it works on the 32-bit system => Report to ZDoom

Keep in mind that when you use valgrind you should compile zdoom with -DVALGRIND=ON in cmake options, so that the renderer should look okay.

Which by itself is a bug, since that defeats the purpose of valgrind. Also, self-modifying code, seriously . Also this switch does not help, but I notice that the renderer works outside valgrind.

[Graf Zahl]

If this is the same name related crash I am thinking about, it should be already fixed