[patch] debug mode

[François Dumont]

On 08/18/2010 03:11 PM, Paolo Carlini wrote:
> Hi,
>
> a few comments before my vacation days. In general the new patch looks
> pretty good, some comments below.
>    
>> Hello
>>
>>      Here is my new patch proposition:
>>
>> - I have tested it on x86_64 linux with make check. make debug-check
>> gives me the same failures with or without the patch, the failures
>> looks normal ones to me. For instance the synopsis.cc tests are all
>> failing because of ambiguity between std::vector and
>> std::__debug::vector, other tests are failing because of tests
>> involving the profile mode.
>>      
> Well, before your patch only the synopsis.cc testcases are failing,
> right? Please make sure we don't regress about that, even by way of
> undefs, or whatever other trick, thanks.
>    
No, there were AFAIR 22 failures, the same one with or without the 
tests. Do you mean that I should undef _GLIBCXX_DEBUG when the test 
don't work with it ? If so I could perhaps do so in an other patch 
dedicated to fixing the debug-check target.
>> - 80 columns
>>
>>      Sorry about that I looked for the number of chars and not the
>> number of columns, because of tabulations it is not equal. By the way,
>> are tabulations ok ? Most of projects I work on was forbidding tabs,
>> should I consider that tabs are 8 spaces long ? To fix this problem I
>> replace many __first, __last by respectively __f, __l, I hope it is fine.
>>      
> Certainly tabs are not forbidden, but the indentations must be right, 2
> chars wide each, essentially.
Really, with such a width indentation is not right in files I work on, 
with width 8 it looks better.
>   About __first and __last, I would suggest
> not changing those to __f and __l if at all possible: short names are
> always more dangerous vs BADNAMES on some less widespread machines.
>    
Ok, I will fix it an other way then
>> - Performance gain
>>
>>      This patch contains 4 new performance tests to challenge range
>> constructors of different containers instanciated with int type:
>>      deque_construct: deque instances built from a vector [begin, end[
>> range
>>      list_construct1: list instances built from a vector [begin, end[
>> range
>>      list_construct2: list instances built from a list [begin, end[ range
>>      vector_construct: vector instances built from a vector [begin,
>> end[ range
>>
>>      When run in release mode it gives the following results:
>> deque_construct.cc
>> 11r    9u    1s        0mem    0pf
>> list_construct1.cc                                               343r
>> 341u    0s        0mem    0pf
>> list_construct2.cc                                               346r
>> 346u    0s        0mem    0pf
>> vector_construct.cc
>> 8r    2u    5s        0mem    0pf
>>
>> deque and vector build are much faster because thanks to internal
>> optimizations it relies on memmove.
>>
>>      Once run with _GLIBCXX_DEBUG defined without the patch it gives:
>>
>> deque_construct.cc                                               529r
>> 523u    2s        0mem    0pf
>> list_construct1.cc                                               895r
>> 890u    1s        0mem    0pf
>> list_construct2.cc                                               884r
>> 879u    1s        0mem    0pf
>> vector_construct.cc                                              475r
>> 467u    6s        0mem    0pf
>>
>> All tests are taking much more time. list_construct1 and
>> list_construct2 because we need more time to check safe iterators on
>> each initialization, deque_construct and vector_construct because
>> usage of safe iterators disable completely usage of memmove and so
>> once again we have to check each safe iterators each time we increment
>> the safe iterator.
>>
>>      With the proposed patch we have:
>>
>> deque_construct.cc
>> 11r    7u    2s        0mem    0pf
>> list_construct1.cc                                               341r
>> 339u    0s        0mem    0pf
>> list_construct2.cc                                               865r
>> 861u    1s        0mem    0pf
>> vector_construct.cc
>> 9r    3u    4s        0mem    0pf
>>
>>      As soon as we detect random access safe iterator we check the
>> whole range in a single operation and there is no more overhead on
>> each iterator incrementation. Moreover for deque construct and vector
>> construct we restore usage of memmove. Only list_construct2 is not
>> impacted because we are not able to validate the iterator range in a
>> single operation so we still have the overhead of checking it each
>> time we increment the safe iterator internally.
>>      
> Looks good. But if we are talking about performance, those tests belong
> to testsuite/performance and should use the timing infrastructure
> already in place.
>    
>> - Modifications of functions.h
>>
>>      The only point that I am not fully comfortable with is that I had
>> to introduce a declaration of __check_singular in formatter.h in order
>> to have functions.h to depend on formatter.h and not the opposite as
>> it used to be when formatter.h was including debug.h. If you do not
>> like it I can introduce a new header to define __check_singular and
>> associated __check_singular_aux that will be overloaded in
>> safe_iterator.h. Except that I think modifications are correct and as
>> a matter of fact the test suite did not show any problem.
>>      
> Ok, let's give that a bit more testing (maybe normal make-check too, but
> *without* PCHs? Normally I do that when I change the header dependencies).
>
> Also, I see various testcases defining explicitly _GLIBCXX_DEBUG:
> normally we don't do that: check-debug takes care of testing the
> debug-mode behavior, uniformly. Please see if you can remove those
> explicit defines in the testcases.
>    
Those tests are checking that invalid code is correctly detected thanks 
to the debug mode so I force definition of _GLIBCXX_DEBUG. Is there some 
way to make those tests unsupported when run in normal mode ?
> Finally, the *cont_traits.h headers should also use a correct style, no
> open bracket at the end of a line, I would also recommend a minimum of
> "air" between the lines, and make up your mind, either you uglify (eg,
> _It), or not (eg, first). I would say not, for testcases.
>    
Ok
> Paolo.
>