Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various -Wmaybe-uninitialized warnings from GCC 12.2.1 #66248

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Sep 22, 2022

Not sure why I didn't get those before, it may be due to upstream changes (12.2.1 is a moving target, it's basically 12.3-dev), or simply rebuilding Godot from scratch with different options.

Edit: I got those while testing #66242, I think the difference is that dev builds now default to -Og optimization on Linux (debugging) instead of -O0 (no optimization). GCC finds more/different warnings with optimizations enabled.

Comment on lines +850 to +855
if (size_accum != nullptr) {
r_offsets[i] = (*size_accum);
(*size_accum) += elem_size;
} else {
r_offsets[i] = 0;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review @godotengine/rendering @TokageItLab.

I don't know if that's the right fix but reading the code it does look like we're potentially dereferencing an uninitialized pointer here since it's only initialized for RS::ARRAY_VERTEX, RS::ARRAY_COLORS and RS::ARRAY_BONES but this loop goes through all of RS::ARRAY_MAX.

Not sure why I didn't get those before, it may be due to upstream
changes (12.2.1 is a moving target, it's basically 12.3-dev), or simply
rebuilding Godot from scratch with different options.
@akien-mga akien-mga force-pushed the warnings-gcc-Wmaybe-uninitialized branch from 8be0aad to d1a155e Compare September 22, 2022 09:30
@akien-mga akien-mga requested a review from a team as a code owner September 22, 2022 09:30
@akien-mga
Copy link
Member Author

Added a couple warning fixes to be able to build with tools=no target=release. I've been seeing those on and off for a while but always started overengineering the fix, now I've just done the minimum.

There's also a series of warning in internal BVH code that need to be fixed to build release templates without warnings, @lawnjelly is looking into it. This was my workaround:

diff --git a/core/math/bvh_structs.inc b/core/math/bvh_structs.inc
index 58c8f0479a..82e5bde870 100644
--- a/core/math/bvh_structs.inc
+++ b/core/math/bvh_structs.inc
@@ -61,20 +61,20 @@ private:
 public:
 	// accessors
 	BVHABB_CLASS &get_aabb(uint32_t p_id) {
-		BVH_ASSERT(p_id < MAX_ITEMS);
+		ERR_FAIL_UNSIGNED_INDEX_V(p_id, MAX_ITEMS, aabbs[0]);
 		return aabbs[p_id];
 	}
 	const BVHABB_CLASS &get_aabb(uint32_t p_id) const {
-		BVH_ASSERT(p_id < MAX_ITEMS);
+		ERR_FAIL_UNSIGNED_INDEX_V(p_id, MAX_ITEMS, aabbs[0]);
 		return aabbs[p_id];
 	}
 
 	uint32_t &get_item_ref_id(uint32_t p_id) {
-		BVH_ASSERT(p_id < MAX_ITEMS);
+		ERR_FAIL_UNSIGNED_INDEX_V(p_id, MAX_ITEMS, item_ref_ids[0]);
 		return item_ref_ids[p_id];
 	}
 	const uint32_t &get_item_ref_id(uint32_t p_id) const {
-		BVH_ASSERT(p_id < MAX_ITEMS);
+		ERR_FAIL_UNSIGNED_INDEX_V(p_id, MAX_ITEMS, item_ref_ids[0]);
 		return item_ref_ids[p_id];
 	}
 

Comment on lines +798 to +799
default:
ERR_FAIL(); // Shouldn't happen, silence warnings.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is to initialize this variable before the switch even though it should be properly initialized in all switch branches.

The problem as I understand it is that enums suck :) So even if we make sure only those three values are passed as input, in theory something could call with a garbage int and it would work just fine, using uninitialized texture.rd_type.

@akien-mga akien-mga merged commit ca88b23 into godotengine:master Sep 23, 2022
@akien-mga akien-mga deleted the warnings-gcc-Wmaybe-uninitialized branch September 23, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant