This example will demonstrate 2 problems in mt_allocator.cc: ---------------- typedef __gnu_cxx::__mt_alloc<char> allocator_type; typedef __gnu_cxx::__pool_base::_Tune tune_type; allocator_type mt_char; tune_type t(8, 40000, 8, (50000 - 4 * sizeof(void*)), 4096, 10, false); mt_char._M_set_options(t); allocator_type::pointer pc = mt_char.allocate(40000); ---------------- First bug in __pool<..>::_M_initialize(): Binmap_type __bin_max = _M_options._M_min_bin; // not correct. size_t __bin_max = _M_options._M_min_bin; // correct. Second bug in __pool<..>::_M_reserve_block(): while (--__block_count > 0) // not correct because __block_count may be equal 0(and size_t is unsigned) { __c += __bin_size; __block->_M_next = reinterpret_cast<_Block_record*>(__c); __block = __block->_M_next; } while (__block_count > 0) // correct { __c += __bin_size; __block->_M_next = reinterpret_cast<_Block_record*>(__c); __block = __block->_M_next; --__block_count; }
The first "bug" simply doesn't exist given the comment at the beginning of __pool_base. The second one is at most a documentation issue: _M_chunk_size shall be always much bigger than _M_max_bytes, thus __block_count always > 0.
(In reply to comment #1) > The first "bug" simply doesn't exist given the comment at the beginning of > __pool_base In the beginning of __pool_base we see: // Using short int as type for the binmap implies we are never // caching blocks larger than 65535 with this allocator. So, it says that I can cache blocks of up to 65535 bytes, while in reality limit is 32768. Code below will generate sigfault: // int main() { typedef __gnu_cxx::__mt_alloc<char> allocator_type; typedef __gnu_cxx::__pool_base::_Tune tune_type; //3.4: typedef __gnu_cxx::__mt_alloc<char>::_Tune tune_type; allocator_type mt_char; tune_type t(8, 50000, 8, (200000 - 4 * sizeof(void*)), 4096, 10, false); mt_char._M_set_options(t); allocator_type::pointer pc = mt_char.allocate(40000); return 0; } _Binmap_type* __bp = _M_binmap; _Binmap_type __bin_max = _M_options._M_min_bin; // not correct since you cast size_t into u_short //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ _Binmap_type __bint = 0; for (_Binmap_type __ct = 0; __ct <= _M_options._M_max_bytes; ++__ct) { if (__ct > __bin_max) { __bin_max <<= 1; ++__bint; } printf("__ct %d __bint %d __bin_max %d\n", __ct, __bint, __bin_max); *__bp++ = __bint; } __ct 32757 __bint 12 __bin_max 32768 __ct 32758 __bint 12 __bin_max 32768 __ct 32759 __bint 12 __bin_max 32768 __ct 32760 __bint 12 __bin_max 32768 __ct 32761 __bint 12 __bin_max 32768 __ct 32762 __bint 12 __bin_max 32768 __ct 32763 __bint 12 __bin_max 32768 __ct 32764 __bint 12 __bin_max 32768 __ct 32765 __bint 12 __bin_max 32768 __ct 32766 __bint 12 __bin_max 32768 __ct 32767 __bint 12 __bin_max 32768 __ct 32768 __bint 12 __bin_max 32768 __ct 32769 __bint 13 __bin_max 0 // incorrect values start here __ct 32770 __bint 14 __bin_max 0 __ct 32771 __bint 15 __bin_max 0 __ct 32772 __bint 16 __bin_max 0 __ct 32773 __bint 17 __bin_max 0 __ct 32774 __bint 18 __bin_max 0 __ct 32775 __bint 19 __bin_max 0 __ct 32776 __bint 20 __bin_max 0 __ct 32777 __bint 21 __bin_max 0 __ct 32778 __bint 22 __bin_max 0 __ct 32779 __bint 22 __bin_max 0 so we have incorrect binmap array. > The second one is at most a documentation issue: _M_chunk_size > shall be always much bigger than _M_max_bytes, thus __block_count always > 0. would it not be easier to do a post increment and not have a problem with people never reading documentation? especially considering that it's so easy to fix?
(In reply to comment #2) > would it not be easier to do a post increment and not have a problem with > people never reading documentation? especially considering that it's so easy to > fix? No, for the simple reason that the allocator does not work is __block_count turns out to be zero. The problem with your PR is that you are doing sort of syntactical analysis of the code without understandind the functioning of the allocator.
(In reply to comment #3) > No, for the simple reason that the allocator does not work is __block_count > turns out to be zero. The problem with your PR is that you are doing sort of > syntactical analysis of the code without understandind the functioning of the > allocator. ok, perhaps this is not really a bug, however segfault is not very user friendly. could ASSERT solve it? about the first one, I take it you agree that it is a real bug?
(In reply to comment #4) > ok, perhaps this is not really a bug, however segfault is not very user > friendly. could ASSERT solve it? No, we don't have asserts anywhere, for various reasons. Really, the documentation must be asjusted and then the user *must* carefully follow it. This piece of code is only for espert usage, not for novices, this should be clear. > about the first one, I take it you agree that it is a real bug? Yes, the documentation must be slightly adjusted to 32767.
Subject: Bug 29179 Author: paolo Date: Mon Sep 25 10:05:27 2006 New Revision: 117193 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=117193 Log: 2006-09-25 Paolo Carlini <pcarlini@suse.de> PR libstdc++/29179 * include/ext/mt_allocator.h (__pool_base): Adjust/extend documentation in comments. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/ext/mt_allocator.h
Subject: Bug 29179 Author: paolo Date: Mon Sep 25 10:05:43 2006 New Revision: 117194 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=117194 Log: 2006-09-25 Paolo Carlini <pcarlini@suse.de> PR libstdc++/29179 * include/ext/mt_allocator.h (__pool_base): Adjust/extend documentation in comments. Modified: branches/gcc-4_1-branch/libstdc++-v3/ChangeLog branches/gcc-4_1-branch/libstdc++-v3/include/ext/mt_allocator.h
Fixed for 4.1.2.