Debug algorithms

[François Dumont]

On 14/09/2016 12:33, Jonathan Wakely wrote:
> On 06/09/16 22:26 +0200, François Dumont wrote:
>> Hi
>>
>>    Any final decision regarding this patch ?
>>
>>    Note that __std_a namespace is optional, I can remove this change 
>> from the patch if you want.
>
> Can you describe what difference that would make?

Then library will be internally exposed to the alternative mode. At the 
moment when any mode is activated it also impact usage of algos used for 
example in implementation of containers. When it is parallel mode I 
think it is on purpose. When it is debug mode it is useless because 
containers already have a debug doing all the checks. But those checks 
are not expensive so it is not a problem to use std:: directly.

> Would it avoid all the changes to Parallel Mode?

No, changes to parallel mode comes from the fact that with this patch 
algos now have 2 available modes. If an algo is specialized in one mode 
it has to be in the other too to avoid complex namespace management 
depending on each algo. So I had to add a number of algos to parallel 
mode even if they do not have any parallel implementation.

> Given that Parallel Mode is hardly used
> by anyone, and will be supersedeed by the C++17 parallel algorithms, I
> don't think we should be spending much time making huge changes to it.

I even had another patch to make some cleanup to parallel mode after 
this one. I might keep it for myself then. There won't be anything 
reusable for the C++17 version ?

>
> The patch is very large, but a lot of it seems to have nothing to do
> with the actual change e.g.
>
> -  template<typename _RAIter, typename _Compare>
> +  template<typename _RAIter, typename _Comp>
>     void
> -    sort(_RAIter, _RAIter, _Compare);
> +    sort(_RAIter, _RAIter, _Comp);
>
> This seems to be some unrelated cleanup that just makes the patch
> larger.

Yes, as I spent a lot of time working on it I get tired of the 
inconsistency of algos declaration in each mode or in algorithmfwd.h and 
decided to generalize one version. I can try to revert some of those 
changes.

But most of changes are rather reorganization of algos in headers to 
avoid playing with _GLIBCXX_BEGIN_XXX_NAMESPACE and 
_GLIBCXX_END_XXX_NAMESPACE too often. I even wanted to move non std 
algos, the helpers ones, in __details namespace.

There are also a number of std:: added to iterator_traits usages as we 
are not always using it from std namespace directly but sometimes from 
std::__cx1998.

>
> The __std_a namespace is just used for indirection to refer to either
> std::foo or std::_GLIBCXX_STD_A::foo, is that right?

Yes, I took this approach from the parallel mode __gnu_sequential.

> So no function
> templates in normal mode change mangled name, because they're still in
> namespace std?
Sorry, I don't get your point here.
>
> This is a pre-existing problem, but I hate the fact that depending on
> the mode, _GLIBCXX_STD_A is either the top-level namespace, or a
> nested one. That means it can't be used to form a fully-qualified
> name. e.g. _GLIBCXX_STD_A::copy is either std::copy or
> __cxx1998::copy. Why isn't the latter std::__cxx1998::copy instead?
> It would be cleaner if ::_GLIBCXX_STD_A::copy was valid, i.e. the
> macro expands to std or std::__cxx1998. For comparison,
> <tr1/gamma.tcc> does this:
>
>  #if _GLIBCXX_USE_STD_SPEC_FUNCS
>  # define _GLIBCXX_MATH_NS ::std
>  #elif defined(_GLIBCXX_TR1_CMATH)
>  namespace tr1
>  {
>  # define _GLIBCXX_MATH_NS ::std::tr1
>
> This means _GLIBCXX_MATH_NS::foo is always valid in any scope.

Yes I agree. I even wondered why we were not using ::std but I see that 
it is done with _GLIBCXX_MATH_NS so we could do the same. With this 
patch _GLIBCXX_STD_A is now only used to explicit the usage of the 
normal algo. The parallel mode cleanup I had prepare also remove a 
number of usage of _GLIBCXX_STD_A replaced by __gnu_sequential.

>
>
> It's not clear to me from reading the patch when I should use __std_a
> and when I should use std. If I forget to use __std_a (and I *will*
> forget, and other people probably will too) what happens? We get
> slightly slower code in Debug Mode? Anything else?
Nothing else.
>
> But if you get rid of __std_a then maybe the questions above don't
> need to be answered anyway.
>
>
> Some other feedback:
>
> I don't like the name __cxx1998_a. The __cxx1998 name is historical,
> but it contains C++11 features too. For a new namespace I'd prefer a
> name that isn't misleading. The "cxx1998" part is untrue, and the
> important part is "a" which doesn't tell you anything.
I wanted to rename __cxx1998 into __normal and so have a __normal_a.
>
> Does the earlier unwrapping of debug iterators lose any safety, or
> will all the same checks always get run?  e.g. if you call an algo in
> debug mode today and it increments past the end then the iterator
> aborts. If we unwrap the debug iterator and invoke a normal algo,
> will we lose that check?
No, we unwrap debug iterators only if we have been able to check that we 
won't reach past-the-end. We could re-wrap the iterator in a proxy 
checking only for past-the-end problem but it would be overkill.

> Assuming the debug algo uses
> __glibcc_require_valid_range() we should be OK, but I'd like to be
> sure.
>
> How much performance does this actually buy us? I don't really mind if
> we can't make Debug Mode faster than it is now. We have the
> lightweight assertions, which don't change algorithmic complexity and
> don't change ABI. IMHO it's OK if Debug Mode is heavier and slower.
Big changes will be for instance when using deque iterators. We have 
some algos specialized for deque::iterator which are not used anymore 
when debug mode is active as algos are not specialized for 
_Safe_iterator<deque::iterator>. This is true also for vector::iterator 
as some algos end in call to memcpy/memmove, it will be true in debug 
mode too.
>
> We need to get better at documenting things. The interaction between
> the different namespaces in Debug Mode, Parallel Mode etc is already
> confusing, and I think this patch would make it worse. The namespaces
> used for the math special functions should be documented too (but it's
> simpler there, we just have std and std::tr1, with nested __detail).
> Is all the information in the manual still correct at
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html ?
I will check. As much as possible we try to be transparent for users so 
they shouldn't bother even if we still need to document it properly.

François