MySQL Bugs: #46880: Many compile warnings are reported for BACKUP in PB2

Bug #46880	Many compile warnings are reported for BACKUP in PB2
Submitted:	24 Aug 2009 7:41	Modified:	16 Nov 2009 15:28
Reporter:	Jørgen Løland	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Backup	Severity:	S3 (Non-critical)
Version:		OS:	Any
Assigned to:	Rafal Somla	CPU Architecture:	Any

Description:
In PB2, there are many compile warnings for BACKUP. These should be removed. Examples:

*** At line 66706 of the build log, in ../../sql/backup/backup_aux.h:
"../../sql/backup/backup_aux.h", line 465: Warning: array hides Dynamic_array<backup::Image_info::Ts*>::array.

*** At line 66707 of the build log, in ../../sql/backup/backup_aux.h:
"../../sql/backup/backup_aux.h", line 465: Warning: array hides Dynamic_array<backup::Image_info::Db*>::array.

*** At line 66708 of the build log, in ../../sql/backup/backup_aux.h:
"../../sql/backup/backup_aux.h", line 369: Warning (Anachronism): Formal argument free_element of type extern "C" void(*)(void*) in call to _my_hash_init(st_hash*, unsigned, charset_info_st*, unsigned long, unsigned long, unsigned long, extern "C" unsigned char*(*)(const unsigned char*,unsigned long*,char), extern "C" void(*)(void*), unsigned) is being passed void(*)(void*).

How to repeat:
See compile warnings in PB2

Suggested fix:
Fix all warnings coming from BACKUP related files.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/83641

2870 Rafal Somla	2009-09-17
      BUG#46880 - Many compile warnings are reported for BACKUP in PB2
      
      This patch fixes problems found by compilers on various platforms used
      in PB2.
     @ client/backup_stream.c
        Use explicit cast to avoid compile warnings.
     @ client/mysqlbackup.cc
        Use casts to avoid "dereferencing type-punned pointer will break 
        strict-aliasing rules" warnings.
     @ sql/backup/backup_aux.h
        Function my_hash_init needs extern "C" callback functions. Template 
        members can not be extern "C". Reorganize code to have plain callback
        function and use virtual destructor for type-safe deleting of pointers.
     @ sql/backup/backup_info.cc
        - Declare HASH callback functions as extern "C".
        - Add missing template instantiation.
     @ sql/backup/image_info.cc
        Add missing template instantiations.
     @ sql/backup/image_info.h
        Change to compatible type.
     @ sql/backup/kernel.cc
        Change name of local variable as it conflicts with class member.
     @ sql/backup/stream.cc
        Change name of local variable as it conflicts with class member.
     @ sql/backup/stream_v1.c
        Use explicit casts to avoid warnings about size_t -> unsigned long 
        conversion.
     @ sql/backup/stream_v1_transport.c
        - Remove re-typing trick to avoid "dereferencing type-punned pointer 
        will break strict-aliasing rules" warning.
        - Use explicit casts to avoid warnings about size_t -> unsigned long 
        conversion.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/83708

2870 Rafal Somla	2009-09-18
      BUG#46880 - Many compile warnings are reported for BACKUP in PB2
      
      This patch fixes problems found by compilers on various platforms used
      in PB2.
     @ client/backup_stream.c
        Use explicit cast to avoid compile warnings.
     @ client/mysqlbackup.cc
        Use casts to avoid "dereferencing type-punned pointer will break 
        strict-aliasing rules" warnings.
     @ sql/backup/backup_aux.h
        - Refactored Map<A,B> template to use MEM_ROOT for HASH node allocation. This removes the need to pass a free_node callback when initializing HASH. It was a problem, because callbacks had to be generated from the template but they should be plain C functions (with "C" linkage). This triggered warnings on some platforms.
        
        - Renamed local variables with the same names as class members.
     @ sql/backup/backup_info.cc
        - Declare HASH callback functions as extern "C".
        - Add missing template instantiation.
     @ sql/backup/error.h
        - Add explicit format string as otherwise some compilers warn that 
        "format not a string literal and no format arguments" which is 
        considered a security hole.
     @ sql/backup/image_info.cc
        Add missing template instantiations.
     @ sql/backup/image_info.h
        Change to compatible type.
     @ sql/backup/kernel.cc
        - Use explicit format string in my_printf_error.
        - Change name of local variable as it conflicts with class member.
     @ sql/backup/stream.cc
        Change name of local variable as it conflicts with class member.
     @ sql/backup/stream_v1.c
        Use explicit casts to avoid warnings about size_t -> unsigned long 
        conversion.
     @ sql/backup/stream_v1_transport.c
        - Remove re-typing trick to avoid "dereferencing type-punned pointer 
        will break strict-aliasing rules" warning.
        - Use explicit casts to avoid warnings about size_t -> unsigned long 
        conversion.

Ok to push by me.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/83901

2870 Rafal Somla	2009-09-21
      BUG#46880 - Many compile warnings are reported for BACKUP in PB2
      
      This patch fixes problems found by compilers on various platforms used
      in PB2.
     @ client/backup_stream.c
        Use explicit cast to avoid compile warnings.
     @ client/mysqlbackup.cc
        Use casts to avoid "dereferencing type-punned pointer will break 
        strict-aliasing rules" warnings.
     @ sql/backup/backup_aux.h
        - Refactored Map<A,B> template to use MEM_ROOT for HASH node allocation. This removes the need to pass a free_node callback when initializing HASH. It was a problem, because callbacks had to be generated from the template but they should be plain C functions (with "C" linkage). This triggered warnings on some platforms.
        
        - Renamed local variables with the same names as class members.
     @ sql/backup/backup_info.cc
        - Declare HASH callback functions as extern "C".
        - Add missing template instantiation.
     @ sql/backup/error.h
        - Add explicit format string as otherwise some compilers warn that 
        "format not a string literal and no format arguments" which is 
        considered a security hole.
     @ sql/backup/image_info.cc
        Add missing template instantiations.
     @ sql/backup/image_info.h
        Change to compatible type.
     @ sql/backup/kernel.cc
        - Use explicit format string in my_printf_error.
        - Change name of local variable as it conflicts with class member.
     @ sql/backup/logger.cc
        - Add explicit format string as otherwise some compilers warn that 
        "format not a string literal and no format arguments" which is 
        considered a security hole.
     @ sql/backup/stream.cc
        Change name of local variable as it conflicts with class member.
     @ sql/backup/stream_v1.c
        Use explicit casts to avoid warnings about size_t -> unsigned long 
        conversion.
     @ sql/backup/stream_v1_transport.c
        - Remove re-typing trick to avoid "dereferencing type-punned pointer 
        will break strict-aliasing rules" warning.
        - Use explicit casts to avoid warnings about size_t -> unsigned long 
        conversion.

Good to push. Please verify that all backup related warnings are removed from PB after pushing this patch.

Pushed to team tree 6.0-backup.
revision-id: rafal.somla@sun.com-20090921113902-oznjb0bo0ywqr00d

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/84239

2872 Rafal Somla	2009-09-23
      BUG#46880 - Many compile warnings are reported for BACKUP in PB2
      
      A follow-up patch which fixes two additional warnings reported in PB2.
     @ sql/backup/backup_aux.h
        - Add custom operator delete corresponding to custom operator new.
        - Add missing cast to ulong.

Follow-up patch pushed to team 6.0-backup tree.
revision-id:rafal.somla@sun.com-20090923060148-3x7x28objlrl9fwz

Pushed into 6.0.14-alpha (revid:alik@sun.com-20091025133616-ca4inerav4vpdnaz) (version source revid:ingo.struewing@sun.com-20090928125502-9t9uqhzsp87vmgnx) (merge vers: 6.0.14-alpha) (pib:13)

No changelog entry needed.

I believe this patch broke a couple of unit tests:

backup/bstr_data_chunks-t .................... ok     
backup/bstr_eoc-t ............................ ok     
backup/bstr_callback_errors-t ................ Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/32 subtests 
backup/bug45737-t ............................ ok   
backup/bstr_random_chunks-t .................. ok     
backup/bstr_eos-t ............................ bstr_eos-t: stream_v1_transport.c:1993: bstream_next_chunk: Assertion `s->buf.header > s->buf.pos' failed.
backup/bstr_eos-t ............................ All 13 subtests passed 
backup/bstr_extra1-t ......................... ok   
backup/bstr_extra-t .......................... ok   
backup/bstr_io_errors-t ...................... ok

REFINED PROBLEM DESCRIPTION
---------------------------
The problem in bstr_eos-t is caused by assertion added at stream_v1_transprt.cc:2029:

      if (s->buf.header < s->buf.pos)
        s->buf.begin= s->buf.header;
      else
      {
        ASSERT(s->buf.header > s->buf.pos);    <=============== HERE
        howmuch= (unsigned long)(s->buf.header - s->buf.pos);

Failing assertion indicates error in the logic. If s->buf.headr == s->buf.pos then the else branch should not be entered but it is enough to move begin to header position. Thus if condition should be "s->buf.header <= s->buf.pos" and then assertion is redundant.

Note: Other reported problems in unit tests will be fixed in a patch for BUG#47540.

All test failures are fixed now by the patch for BUG#47540 (revid:ritheesh.vedire@sun.com-20091116073642-nmlctlxhiq193xsy).

I've checked it in mysql-6.0-backup tree. At the moment the fix is not yet pushed to the main tree.

No changelog entry needed.