Details
-
Task
-
Status: Closed (View Workflow)
-
Critical
-
Resolution: Fixed
-
None
Description
Reported by marko, happens when building with -Og and in various components, and likely to pop up again from time to time, so I suggest we keep this ticket open and ongoing as more patches get reviewed and pushed for this issue.
For example, instead of
if ( |
!(db_name = get_field(mem_root, table->field[0])) ||
|
!(table_name = get_field(mem_root, table->field[1])) ||
|
!(link_id = get_field(mem_root, table->field[2]))
|
)
|
DBUG_RETURN(HA_ERR_OUT_OF_MEM);
|
fix with
if (!(db_name = get_field(...)) |
DBUG_RETURN(...);
|
if (!(table_name = get_field(...)) |
DBUG_RETURN(...);
|
if (!(link_id = get_field(...)) |
DBUG_RETURN(...);
|
Attachments
Issue Links
- relates to
-
MDEV-35073 -Wmaybe-uninitialized in result of spider_conn_first_link_idx
-
- Closed
-
Activity
Thank you, the fixes look good, but I think that we should be more specific that this is about fixing g++-13 -Wmaybe-uninitialized. I tried compiling the 10.4 branch with GCC 14.0.1, using both -Og and -O3. No warnings were issued before or after your changes. I also tried compiling with GCC 13.2.0. Your fixes make a difference for both settings.
Some other warnings were issued, and I took the liberty of fixing them.
I would like to point out that just free(NULL) is a valid operation in C, also in C++ it is redundant to check if a pointer is null before invoking delete. Fixing the memory leaks is the subject of another task.
Thanks marko, I've updated the commit message and the delete [] to be unconditional. ptal thanks
bb-10.4-ycp bb-10.4-mdev-33220 42439ff66ab75249f56eee70db5f83329ff21393
|
MDEV-33220 Fix -wmaybe-uninitialized warnings for g++-13
|
Thank you, I tested that branch merged on the current 10.4 head, using GCC 13.2.0, and there were no -Wmaybe-uninitialized warnings.
At -O3 (not -Og or -O2) there were some -Warray-bounds, but that is a separate matter.
Thanks for the review.
Pushed ef9cdacf51320fbb2935c03d406b22a3688bf295 to 10.4
Other versions with conflicts:
- 10.5 733ec188d1e982fbdbf9925ac65a5bfb4ea6bc6a
- 10.11 58971b7e1ba4711f8f25d7264310427154317eeb
no non-trivial conflict resolutions, so higher versions should be similarly easily resolved.
Thanks for the comments. ptal at the updated patch:
b1fe542c59b upstream/bb-10.4-mdev-33220 MDEV-33220 Fix -wmaybe-uninitialized warnings> I think that we should make use of an additional goto label oom: in order to avoid duplicated function calls. I see that this particular function is duplicating a lot of such calls already. Last time I checked, even at -O3 such calls are not being optimized away.
Done.
> Also, it may be a matter of taste, but I would put the assignment outside the if condition. It should make no difference to the generated code: the value should be cached in the "function return value" register (rax on AMD64).
Ack. I prefer the assignment in the condition because it is shorter.
> A side finding in the above code snippet is that if we failed to allocate spider, then we would fail to free the memory that was allocated for tmp_sql.
This is prevented by the following lines under the error label:
{}