Compiler warnings in XtraDB

[Alexey Kopytov]

I get the following compiler warnings in XtraDB code in lp:percona-server:

buf/buf0buddy.c: In function ‘buf_buddy_relocate’:
buf/buf0buddy.c:493: warning: ISO C90 forbids mixed declarations and code
In file included from buf/buf0buf.c:36:
./include/buf0buf.ic:1078: warning: unused parameter ‘mtr’

buf/buf0buf.c: In function ‘_increment_page_get_statistics’:
buf/buf0buf.c:84: warning: comparison of unsigned expression < 0 is always false
buf/buf0buf.c:86: warning: comparison is always false due to limited range of data type

buf/buf0flu.c: In function ‘buf_flush_buffered_writes’:
buf/buf0flu.c:723: warning: comparison between signed and unsigned
buf/buf0flu.c: In function ‘buf_flush_post_to_doublewrite_buf’:
buf/buf0flu.c:850: warning: comparison between signed and unsigned
buf/buf0flu.c:884: warning: comparison between signed and unsigned

dict/dict0load.c: In function ‘dict_check_tablespaces_and_store_max_id’:
dict/dict0load.c:399: warning: implicit declaration of function ‘trx_sys_sys_space’

fil/fil0fil.c: In function ‘fil_node_open_file’:
fil/fil0fil.c:693: warning: comparison between signed and unsigned
mv -f .deps/libinnobase_a-eval0eval.Tpo .deps/libinnobase_a-eval0eval.Po
fsp/fsp0fsp.c: In function ‘xdes_find_bit’:
fsp/fsp0fsp.c:475: warning: comparison between signed and unsigned

fsp/fsp0fsp.c: In function ‘xdes_get_n_used’:
fsp/fsp0fsp.c:544: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘xdes_is_full’:
fsp/fsp0fsp.c:581: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘xdes_init’:
fsp/fsp0fsp.c:642: warning: comparison between signed and unsigned

fsp/fsp0fsp.c: In function ‘fsp_alloc_free_page’:
fsp/fsp0fsp.c:1640: warning: comparison between signed and unsigned

fsp/fsp0fsp.c: In function ‘fseg_find_free_frag_page_slot’:
fsp/fsp0fsp.c:2189: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fseg_find_last_used_frag_page_slot’:
fsp/fsp0fsp.c:2216: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fseg_get_n_frag_pages’:
fsp/fsp0fsp.c:2244: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fseg_create_general’:
fsp/fsp0fsp.c:2348: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fseg_fill_free_list’:
fsp/fsp0fsp.c:2500: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fseg_alloc_free_page_low’:
fsp/fsp0fsp.c:2662: warning: comparison between signed and unsigned
fsp/fsp0fsp.c:2684: warning: comparison between signed and unsigned
fsp/fsp0fsp.c:2738: warning: comparison between signed and unsigned
fsp/fsp0fsp.c:2782: warning: comparison between signed and unsigned

fsp/fsp0fsp.c: In function ‘fsp_reserve_free_pages’:
fsp/fsp0fsp.c:2954: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fsp_reserve_free_extents’:
fsp/fsp0fsp.c:3037: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fsp_get_available_space_in_free_extents’:
fsp/fsp0fsp.c:3200: warning: comparison between signed and unsigned
fil/fil0fil.c: In function ‘fil_load_single_table_tablespace’:
fil/fil0fil.c:3781: warning: comparison between signed and unsigned
fil/fil0fil.c:3801: warning: comparison between signed and unsigned

fsp/fsp0fsp.c: In function ‘fseg_validate_low’:
fsp/fsp0fsp.c:3839: warning: comparison between signed and unsigned
fsp/fsp0fsp.c:3865: warning: comparison between signed and unsigned
fsp/fsp0fsp.c: In function ‘fsp_validate’:
fsp/fsp0fsp.c:4047: warning: comparison between signed and unsigned
fsp/fsp0fsp.c:4098: warning: comparison between signed and unsigned
fsp/fsp0fsp.c:4124: warning: comparison between signed and unsigned
fil/fil0fil.c: At top level:
fil/fil0fil.c:4972: warning: unused parameter ‘zip_size’
fil/fil0fil.c:4975: warning: unused parameter ‘byte_offset’
fil/fil0fil.c:4978: warning: unused parameter ‘len’

mtr/mtr0mtr.c: In function ‘mtr_memo_note_modification_all’:
mtr/mtr0mtr.c:134: warning: implicit declaration of function ‘buf_flush_note_modification’

os/os0file.c:3394: warning: unused parameter ‘trx’

handler/ha_innodb.cc:6088: warning: comparison of unsigned expression >= 0 is always true

row/row0mysql.c: In function ‘row_insert_for_mysql’:
row/row0mysql.c:1141: warning: implicit declaration of function ‘innobase_mysql_print_thd’

sync/sync0rw.c:236: warning: unused parameter ‘cfile_name’
sync/sync0rw.c:237: warning: unused parameter ‘cline’
srv/srv0srv.c: In function ‘srv_master_thread’:
srv/srv0srv.c:3087: warning: comparison between signed and unsigned
sync/sync0sync.c:247: warning: unused parameter ‘cfile_name’
sync/sync0sync.c:248: warning: unused parameter ‘cline’
srv/srv0start.c: In function ‘open_or_create_data_files’:
srv/srv0start.c:1037: warning: comparison between signed and unsigned

trx/trx0sys.c: In function ‘trx_sys_create_doublewrite_buf’:
trx/trx0sys.c:328: warning: comparison between signed and unsigned
trx/trx0sys.c:361: warning: comparison between signed and unsigned
trx/trx0sys.c:362: warning: comparison between signed and unsigned
trx/trx0sys.c:371: warning: comparison between signed and unsigned
trx/trx0sys.c:372: warning: comparison between signed and unsigned
trx/trx0sys.c:380: warning: comparison between signed and unsigned
trx/trx0sys.c:475: warning: comparison between signed and unsigned
trx/trx0sys.c:508: warning: comparison between signed and unsigned
trx/trx0sys.c:509: warning: comparison between signed and unsigned
trx/trx0sys.c:518: warning: comparison between signed and unsigned
trx/trx0sys.c:519: warning: comparison between signed and unsigned
trx/trx0sys.c:527: warning: comparison between signed and unsigned

trx/trx0sys.c: In function ‘trx_sys_doublewrite_init_or_restore_pages’:
trx/trx0sys.c:650: warning: comparison between signed and unsigned
trx/trx0sys.c:663: warning: comparison between signed and unsigned

[Yasufumi Kinoshita (yasufumi-kinoshita)]

What I don't understand are....

Which option of gcc outputs the warnings "comparison between signed and unsigned"?
I have not met the warnings at least for my options.

And why do you want to fix them?
"comparison between signed and unsigned"
If compared correctly, no problem.

And,
"comparison of unsigned expression < 0 is always false"
is for the future definition change. Even if the sign of the type was changed in mysqld, it works fine.
It was remained intentionally, even if the pointed at the similar past bug report.

Why do you assure all variables' sign are not changed forever? (5.5, 6.x, 7.x....)

I don't agree with your too much "dainty" about coding.
And you seemed to ignore the intention of the code for the future.
It seems to increase possibility to cause bug in the future.

Such dainty is only for perfect code as it is.
Our patching activity will be never perfect, because the base code is changing always.
We should prepare to the changes of the base, always.

I'd like to take "safeness for future porting of the patches" rather than "dainty".

Though, I will check the other warnings.

[Alexey Kopytov (akopytov)]

Yasufumi,

>Which option of gcc outputs the warnings "comparison between signed and unsigned"?

This is what MySQL maintainer mode uses: "-Wall -Wextra -Wunused -Wwrite-strings -Wno-strict-aliasing -Werror -Wno-unused-parameter" for C++ and "-Wall -Wextra -Wunused -Wwrite-strings -Wno-strict-aliasing -Werror -Wdeclaration-after-statement"

They are defined in config/ac-macros/maintainer.m4

>And why do you want to fix them?

I want to fix them, because I want to make sure every _new_ warning is addressed by developers. MySQL maintainer mode was specifically implemented for that purpose. As of 5.1.52 it is automatically turned on when you build --with-debug=full which is what I use for my development builds.

>"comparison between signed and unsigned"
>If compared correctly, no problem.

Sure, "correctly" is the keyword. Such comparisons is a frequent source of bugs. Consider:

  int a = -1;
  unsigned int b = 1;

  if (a > b)
    printf("Oops!\n");

And such bugs are typically hard to find too. Hence the compiler warning. It is just supposed to notify the developer that there _may_ be a problem with the code. You then have to take a look and decide whether the problem is real or not. But, as I wrote previously, if you have hundreds of such warnings in your build log, how will you notice the new one?

> And,
> "comparison of unsigned expression < 0 is always false"
> is for the future definition change. Even if the sign of the type was changed in mysqld, it works fine.
> It was remained intentionally, even if the pointed at the similar past bug report.

So if your code is correct about variables signedness (i.e. does not produce warnings), you _will_ get a warning whenever the signedness for some variable will change, right?

Let's look at the actual code producing those warnings:

---
buf/buf0buf.c: In function ‘_increment_page_get_statistics’:
buf/buf0buf.c:84: warning: comparison of unsigned expression < 0 is always false
---

This one is about variable introduced by _our_ patch. How can it change its signedness in the upstream development, if the upstream MySQL does not have it?

---
buf/buf0buf.c:86: warning: comparison is always false due to limited range of data type
---

Again, it's about the variable from our patch.

So what "future definition change" are you talking about?

[Mark Callaghan (mdcallag)]

I encountered this in the 5.1.54 release branch

Maybe this is a gcc version thing. I use 4.4 in this case. MySQL added -Werror when --with-debug is used. That broke the official build (the problem is in the extra subdir). It also broke changes from Percona

My configure line:
./configure --enable-thread-safe-client --with-plugins=partition,csv,blackhole,myisam,heap,innodb_plugin --without-plugin-innobase --with-fast-mutexes --with-extra-charsets=all --with-debug C_EXTRA_FLAGS="-fno-omit-frame-pointer -fno-strict-aliasing -DHAVE_purify -Wall" CFLAGS="-O0 -g" CXXFLAGS="-g -O0"

cc1: warnings being treated as errors
In file included from ./include/buf0buf.h:1571,
                 from ./include/page0page.h:33,
                 from ./include/page0cur.h:32,
                 from ./include/btr0btr.h:33,
                 from btr/btr0btr.c:26:
./include/buf0buf.ic: In function ‘buf_page_release’:
./include/buf0buf.ic:1078: error: unused parameter ‘mtr’
cc1: warnings being treated as errors
In file included from ./include/buf0buf.h:1571,
                 from ./include/page0page.h:33,
                 from ./include/page0cur.h:32,
                 from ./include/btr0cur.h:31,
                 from btr/btr0cur.c:44:
./include/buf0buf.ic: In function ‘buf_page_release’:
./include/buf0buf.ic:1078: error: unused parameter ‘mtr’
make[2]: *** [libinnobase_a-btr0btr.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [libinnobase_a-btr0cur.o] Error 1

and another error, I haven't seen this before

cc1: warnings being treated as errors
buf/buf0buddy.c: In function ‘buf_buddy_relocate’:
buf/buf0buddy.c:493: error: ISO C90 forbids mixed declarations and code

the problem is that code was added before declarations

        /* We look inside the allocated objects returned by
        buf_buddy_alloc() and assume that anything of
        PAGE_ZIP_MIN_SIZE or larger is a compressed page that contains
        a valid space_id and page_no in the page header. Should the
        fields be invalid, we will be unable to relocate the block.
        We also assume that anything that fits sizeof(buf_page_t)
        actually is a properly initialized buf_page_t object. */

        if (size >= PAGE_ZIP_MIN_SIZE) {
                /* This is a compressed page. */
                mutex_t* mutex;

                if (!have_page_hash_mutex) {
                        mutex_exit(&zip_free_mutex);
                        mutex_enter(&LRU_list_mutex);
                        rw_lock_x_lock(&page_hash_latch);
                }

                /* The src block may be split into smaller blocks,
                some of which may be free. Thus, the
                mach_read_from_4() calls below may attempt to read
                from free memory. The memory is "owned" by the buddy
                allocator (and it has been allocated from the buffer
                pool), so there is nothing wrong about this. The
                mach_read_from_4() calls here will only trigger bogus
                Valgrind memcheck warnings in UNIV_DEBUG_VALGRIND builds. */
                ulint space = mach_read_from_4(
                        (const byte*) src ...

[Yasufumi Kinoshita (yasufumi-kinoshita)]

Alexey,

Thank you. OK.
I understand your intention well.

About "comparison between signed and unsigned":

I will do limiting range and casting the sign by my intention as can as possible.

About "comparison of unsigned expression < 0 is always false"

I might misunderstand for something another bug of the past, sorry.
I will look and fix.

Mark,

Anyway, I will fix the warnings caused by our patches at least, for now.
I will check by making difference of build log from normal MySQL.

[Yasufumi Kinoshita (yasufumi-kinoshita)]

I will fix also for 5.5.x soon

[Shahriyar Rzayev (rzayev-sehriyar)]

Percona now uses JIRA for bug reports so this bug report is migrated to: https://jira.percona.com/browse/PS-2601