VideoCommon: Don't add garbage to shader uids in debug builds

[TellowKrinkle]

Prevents debug builds from leaving stack data in UIDs, making them no longer compare equal

There wasn't anything stopping compilers from doing it in release builds either, but most optimizations would make the padding zero

[iwubcode]

This is fine but I don't get why it's needed? Aren't the shader uid structs (ex geometry_shader_uid_data) containing bit-fields and thus default initialized by our brace initialization?

[Pokechu22]

A similar memset call also already exists:

dolphin/Source/Core/VideoCommon/GXPipelineTypes.h

Lines 34 to 37 in 431d757

	// We use memcmp() for comparing pipelines as std::tie generates a large number of instructions,
	// and this map lookup can happen every draw call. However, as using memcmp() will also compare
	// any padding bytes, we have to ensure these are zeroed out.
	GXPipelineUid() { std::memset(static_cast<void*>(this), 0, sizeof(*this)); }

dolphin/Source/Core/VideoCommon/GXPipelineTypes.h

Line 58 in 431d757

GXUberPipelineUid() { std::memset(static_cast<void*>(this), 0, sizeof(*this)); }

[TellowKrinkle]

This is fine but I don't get why it's needed? Aren't the shader uid structs (ex geometry_shader_uid_data) containing bit-fields and thus default initialized by our brace initialization?

The active fields are initialized, but Clang doesn't think it needs to initialize the padding: https://gcc.godbolt.org/z/cf7KxdEcs

A similar memset call also already exists:

dolphin/Source/Core/VideoCommon/GXPipelineTypes.h

Lines 34 to 37 in 431d757

	// We use memcmp() for comparing pipelines as std::tie generates a large number of instructions,
	// and this map lookup can happen every draw call. However, as using memcmp() will also compare
	// any padding bytes, we have to ensure these are zeroed out.
	GXPipelineUid() { std::memset(static_cast<void*>(this), 0, sizeof(*this)); }

dolphin/Source/Core/VideoCommon/GXPipelineTypes.h

Line 58 in 431d757

GXUberPipelineUid() { std::memset(static_cast<void*>(this), 0, sizeof(*this)); }

Indeed, we zero pipeline uids there. But then this happens:

1. UpdatePipelineConfig calls UberShader::GetVertexShaderUid

   dolphin/Source/Core/VideoCommon/VertexManagerBase.cpp

   Line 639 in 107a928

   m_current_uber_pipeline_config.vs_uid = UberShader::GetVertexShaderUid();

2. UberShader::GetVertexShaderUid creates a VertexShaderUid on the stack, populates it, and returns it. Padding bytes on the new VertexShaderUid are garbage.

   dolphin/Source/Core/VideoCommon/UberShaderVertex.cpp

   Lines 17 to 22 in 107a928

   	VertexShaderUid out;
   	vertex_ubershader_uid_data* const uid_data = out.GetUidData();
   	uid_data->num_texgens = xfmem.numTexGen.numTexGens;
   	return out;

3. The VertexShaderUid is copied into the pipeline uid, garbage padding included.

[shuffle2]

1. Can we enable some init-all compiler flag to just force all padding initialized? At least msvc and clang has this.
2. Can this be solved instead by defining yet another constructor type to also do the memset? (The intent was that this was the case)
3. Can it be solved by packing the structs?

Edit: I see you’re doing 2. Nvm then :p maybe the other topics are interesting though.

[delroth]

I was going to suggest to move this memset to individual uid_data constructors instead -- but since we're already checking that the types are POD / trivially copiable I think this is good enough.

I'm -0.5 on doing this via compiler flags, seems like a really big hammer for a very localized problem.