On 07/10/2015 22:09, Jonathan Wakely wrote:
> On 07/10/15 21:38 +0200, François Dumont wrote:
>> Hi
>>
>>    I completed vector assertion mode. Here is the result of the new
>> test you will find in the attached patch.
>>
>> With debug mode:
>> /home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:375:
>>
>> Error: attempt to advance a dereferenceable (start-of-sequence)
>> iterator 2
>> steps, which falls outside its valid range.
>>
>> Objects involved in the operation:
>>    iterator @ 0x0x7fff1c346760 {
>>      type =
>> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*,
>> std::__cxx1998::vector<int, std::allocator<int> > >,
>> std::__debug::vector<int, std::allocator<int> > > (mutable iterator);
>>      state = dereferenceable (start-of-sequence);
>>      references sequence with type 'std::__debug::vector<int,
>> std::allocator<int> >' @ 0x0x7fff1c3469a0
>>    }
>> XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>>
>>
>> With assertion mode:
>> /home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:1124:
>>
>> Error: invalid insert position outside container [begin, end) range.
>>
>> Objects involved in the operation:
>>    sequence "this" @ 0x0x7fff60b1f870 {
>>      type = std::vector<int, std::allocator<int> >;
>>    }
>>    iterator "__position" @ 0x0x7fff60b1f860 {
>>      type = __gnu_cxx::__normal_iterator<int const*, std::vector<int,
>> std::allocator<int> > >;
>>    }
>> XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>
> I still don't like the formatted output for the lightweight mode, it
> adds a dependency on I/O support in libc, which is a problem for
> embedded systems.

I thought you just meant I/O dependency in terms of included headers.
The __glibcxx_assert also has some I/O as in case of failure it calls:

  inline void
  __replacement_assert(const char* __file, int __line,
               const char* __function, const char* __condition)
  {
    __builtin_printf("%s:%d: %s: Assertion '%s' failed.\n", __file, __line,
             __function, __condition);
    __builtin_abort();
  }

but it is much more limited than the _GLIBCXX_DEBUG_VERIFY counterpart
which is calling fprintf to send to stderr.

So ok let's limit this mode to glibcxx_assert.

>
> The idea was to just add really cheap checks and abort  :-(
>
> Have you compared codegen with and without assertion mode? How much
> more code is added to member functions like operator[] that must be
> inlined for good performance?  Is it likely to affect inlining
> decisions?
>
> I suspect it will have a much bigger impact than if we just use
> __builtin_abort() as I made it do originally.

I think that impact on compiled code depends more on the assert
condition than on the code executed when this assertion happens to be
false. But I haven't check it and will try.

In the attached patch I eventually:
- Move assertion macros in debug/assertions.h, it sounds like the right
place for those.
- Complete implementation of assertion checks by using __valid_range
function. All checks I can think of are now in place. I still need to
compare with google branch.

Note that for the latter, condition is still evaluated in O(1).
__valid_range detects iterator issues without looping through them.
__valid_range, by considering iterator category, also make those macros
usable in any container.

François