Fix nontrivial-memcall warnings in cute_graphics_sdlgpu.cpp - #573
Open
pusewicz wants to merge 2 commits into
Open
Fix nontrivial-memcall warnings in cute_graphics_sdlgpu.cpp#573pusewicz wants to merge 2 commits into
pusewicz wants to merge 2 commits into
Conversation
Two different situations here, treated differently: - cf_sdlgpu_make_shader_from_bytecode / cf_sdlgpu_make_compute_shader_from_bytecode used CF_NEW (default-initialization) followed by a memset to zero the plain C-array/scalar members that default-init leaves uninitialized. That memset also stomped the Cute::Array members, which were already properly constructed. Switched to CF_PLACEMENT_NEW with an explicit T() (value-initialization), which zero-initializes every member in one well-defined step and removes the memset entirely. Verified with a temporary 0xCD poison-and-assert pass that no member was left uninitialized (removed after confirming clean). - cf_sdlgpu_shader_swap_contents / cf_sdlgpu_compute_shader_swap_contents do a deliberate raw byte-swap of two live objects for hot-reload (both types are pointer-only handles with no self-references, per the existing comment). This is intentional, not a bug, so just cast the destination pointers to void* to silence the warning, as the compiler itself suggests.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets -Wnontrivial-memcall warnings in the SDL_gpu backend by removing memset on already-constructed shader internal objects and adjusting shader hot-reload swapping code paths.
Changes:
- Replaced
CF_NEW + CF_MEMSETwith value-initialized placement-new allocations forCF_ShaderInternalandCF_ComputeShaderInternal. - Updated the shader/compute-shader “swap contents” hot-reload routines to cast destinations to
void*forCF_MEMCPYcalls.
Suppressed comments (1)
src/cute_graphics_sdlgpu.cpp:2651
cf_sdlgpu_compute_shader_swap_contentshas the same issue as the graphics shader swap:CF_ComputeShaderInternalcontains non-trivialCute::Arraymembers, so thememcpyswap is UB even if cast tovoid*. Also,cf_sdlgpu_destroy_compute_shadercurrentlyCF_FREEs the object without running its destructor, which leaks the reflection/samplerCute::Arrayallocations. Use a move-based swap and explicitly run the destructor before freeing (mirrorscf_sdlgpu_destroy_shader_internal).
uint8_t tmp[sizeof(CF_ComputeShaderInternal)];
CF_MEMCPY(tmp, pa, sizeof(tmp));
CF_MEMCPY((void*)pa, pb, sizeof(tmp));
CF_MEMCPY((void*)pb, tmp, sizeof(tmp));
Comment on lines
+1322
to
+1325
| uint8_t tmp[sizeof(CF_ShaderInternal)]; | ||
| CF_MEMCPY(tmp, pa, sizeof(tmp)); | ||
| CF_MEMCPY(pa, pb, sizeof(tmp)); | ||
| CF_MEMCPY(pb, tmp, sizeof(tmp)); | ||
| CF_MEMCPY((void*)pa, pb, sizeof(tmp)); | ||
| CF_MEMCPY((void*)pb, tmp, sizeof(tmp)); |
Contributor
Author
There was a problem hiding this comment.
Addressed in 7822c21 — swapped to Cute::swap (already exists in the codebase, does a proper move-based 3-step swap).
The raw memcpy-based 3-step swap was UB for a non-trivially-copyable type even with the destination pointers cast to void* to silence the compiler -- the cast only hides the warning, it doesn't make copying a live object's bytes past its own constructor/destructor well-defined. Cute::swap already exists (a proper move-based 3-step swap) and is in scope via the file's `using namespace Cute`, so swap the raw memcpy dance for a single call to it. Verified with a standalone struct mirroring CF_ShaderInternal's member mix (scalar, plain C-array, Cute::Array) that all three kinds swap correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two different situations here, so two different treatments:
cf_sdlgpu_make_shader_from_bytecode/cf_sdlgpu_make_compute_shader_from_bytecodeusedCF_NEW(default-initialization) followed by amemsetto zero the plain C-array/scalar members that default-init leaves uninitialized — verified empirically thatCF_NEW'snew(args) (Type)form really does leave those as garbage. The memset also stomped theCute::Arraymembers, which were already properly constructed (harmlessly, since their zero-state matches empty, but still unspecified behavior on a live non-trivial object). Switched toCF_PLACEMENT_NEWwith an explicitT()(value-initialization), which zero-initializes every member — plain arrays included — in one well-defined step, so the memset goes away entirely rather than being merely silenced.Verified with a temporary
0xCD-poison +CF_ASSERTpass over every member (removed once confirmed clean) that nothing is left uninitialized after the change.cf_sdlgpu_shader_swap_contents/cf_sdlgpu_compute_shader_swap_contentsdo a deliberate raw byte-swap of two live objects for hot-reload (both types are pointer-only handles with no self-references, per the existing comment) — that's intentional, not a bug. Left the swap as-is and just cast the destination pointers tovoid*, per the compiler's own suggested fix.Verified: framework + test suite builds warning-clean, and
./testspasses the same 313/333 asmaster(remaining 20 are the pre-existing headless GPU rendering failures from #572, unrelated to this change).