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
Hi marko, ptal thanks (based on 10.5 and fixing at sql layer / innodb)
bb-10.5-mdev-33220 ef39e155244bbe01da6762005ecf441303a7ef54
|
MDEV-33220 Fix -Wmaybe-uninitialized warnings
|
> Is this task really for 10.5 or for 11.5?
It's a technical task to fix compiler complaints when run with a specific flag. It is likely to be a recurring issue across different versions because people don't always compile with -Og and gcc 13 when the problem would resurface.
> Fixing build errors should be a bug, not a task.
While I agree in general build errors should be bugs, this ticket is about making the code compile when the compiler is run with a specific flag and specific compiler versions, and the complaints are all false positives. So I think it's more a technical task than bug. Also it will resurface from time to time as people don't always compile under these conditions.
Got some time to try again today. Here's a 10.4 patch
8c78ebdb574 upstream/bb-10.4-mdev-33220 MDEV-33220 Fix -wmaybe-uninitialized warnings
|
There's probably gonna be conflicts or insufficient changes that leave more warnings when merged to higher versions.
For other versions:
- a547bfb9ef7 upstream/bb-10.5-mdev-33220
MDEV-33220Fix -wmaybe-uninitialized warnings
Thank you, this does fix the -Wmaybe-uninitialized warnings for me. However, I notice some added code duplication, like the following:
diff --git a/storage/spider/spd_copy_tables.cc b/storage/spider/spd_copy_tables.cc
|
index 6e9d1f2bcc6..dbe74ddaa8d 100644
|
--- a/storage/spider/spd_copy_tables.cc
|
+++ b/storage/spider/spd_copy_tables.cc
|
@@ -1002,7 +1002,12 @@ long long spider_copy_tables_body(
|
all_link_cnt =
|
copy_tables->link_idx_count[0] + copy_tables->link_idx_count[1];
|
if (
|
- !(tmp_sql = new spider_string[all_link_cnt]) ||
|
+ !(tmp_sql = new spider_string[all_link_cnt])
|
+ ) {
|
+ my_error(ER_OUT_OF_RESOURCES, MYF(0), HA_ERR_OUT_OF_MEM);
|
+ goto error;
|
+ }
|
+ if (
|
!(spider = new ha_spider[all_link_cnt])
|
) {
|
my_error(ER_OUT_OF_RESOURCES, MYF(0), HA_ERR_OUT_OF_MEM); |
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.
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).
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.
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:
if (tmp_sql) |
{
|
delete [] tmp_sql; |
}
|
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.
The warnings are issued by GCC when building with -Og (optimize while trying to keep the executable debugger-friendly).