Re: Make the 2 versions of delete more similar
On 2 October 2013 13:28, Marc Glisse wrote:
> Hello,
>
> I don't understand why those 2 files differ by more than 1 extra argument,
> so I am changing that.
>
> Bootstrap and testsuite on x86_64.
>
> 2013-10-03 Marc Glisse
>
> * libsupc++/del_op.cc (operator delete): Don't test for 0 before
> free.
Just checking, for the nervous:
Is the plan that this change will not effect any code behaviour (as
correct implementations of free are happy to take a NULL pointer, and
not do anything)?
Chris
> _GLIBCXX_WEAK_DEFINITION void
> operator delete(void* ptr) _GLIBCXX_USE_NOEXCEPT
> {
> - if (ptr)
> -std::free(ptr);
> + std::free(ptr);
> }
nth_element fix [58800]
A previous fix to a performance problem in std::sort unfortunately has lead to a crashing bug in std::nth_element. The following minimal patch fixes the problem (there are a couple of different ways of fixing it, this is the shortest and safest). 2013-09-19 Chris Jefferson PR libstdc++/58800 * include/bits/stl_algo.h (__unguarded_partition_pivot) : Change __last - 2 to __last - 1, so we correctly work on arrays of length 4. * testsuite/25_algorithms/nth_element/58800.cc : New nth_element1.patch Description: Binary data
libstdc++ Testsuite extension for sort, partial_sort, partial_sort_copy, nth_element
The following patch (related to my 58800 patch) adds many more tests for several important functions. While 58800 is my fault, the reason it was not caught earlier is that many functions in libstdc++ have almost no testing. This works toward fixing that. These tests are kept to an acceptable amount of time (~2 seconds/ function) at the moment (and automatic reduction for simulators), although I would like to take much more time and do much more testing, but I realise that might annoy some people so it might not be on by default. I plan to write my tests if these are accepted (although writing this reminded me why gcc is the worst open source project for submitting code to I have ever worked with). 2013-10-16 Edward Smith-Rowland <3dw...@verizon.net> * testsuite/util/testsuite_iterators.h : Add 'val' method to read testsuite container. * testsuite/25_algorithms/partial_sort/nth_element.cc : New * testsuite/25_algorithms/partial_sort/random_test.cc : New * testsuite/25_algorithms/partial_sort_copy/random_test.cc : New * testsuite/25_algorithms/sort/random_test.cc : New * testsuite/util/testsuite_containergen.h : New header for automatically testing functions on a large set of autogenerated headers sorting_test.patch Description: Binary data
Re: Rb tree node recycling patch
Personally, I consider the testing insufficent, although the testing
was already insufficient! Note that this is my opinion, don't assume I
talk for all of libstdc++!
After I broke std::nth_element in 4.8.2 (woops), I am of the view that
the libstdc++ testsuite is defficient -- many code paths are never
tested. The nth_element breakage should really have been picked up,
about 1/3 of all random invocations of nth_element failed!
You could be "inspired" by the code I wrote for algorithms, it is in
testsuite/util/testsuite_containergen.h. The basic idea is I make sure
to generate special cases (size 0 containers, containers containing
only a single value repeated), and then a range of containers of
various different sizes.
I realise suggesting this is probably as much work as your patch, and
you shouldn't assume this is required, but I think it would improve
the testsuite. If you do go down this route, then be sure to reduce
your test's size for simulators. You can look at for example at my new
tests which feature:
// { dg-options "-std=gnu++11" }
// { dg-options "-std=gnu++11 -DSIMULATOR_TEST" { target simulator } }
Which enables the macro SIMULATOR_TEST on simulators, where I do much
less testing (else the tester takes too long.
On 27 December 2013 18:30, François Dumont wrote:
> Hi
>
> Here is a patch to add recycling of Rb tree nodes when possible.
>
> I replaced the _Rb_tree::_M_move_assign with a move assignment operator
> to benefit from:
> - move of elements when the whole data structure cannot be moved
> - faster data structure cloning rather than full regeneration of the tree
> when _M_move_assign was failing
>
> Note that this patch contains also a cleanup of a useless template
> parameter _Is_pod_comparator on _Rb_tree_impl. If you want to apply it
> quickly for 4.9 do not hesitate.
>
> I haven't done any specific test for this feature, existing ones looks
> good enough to me. If you want me to add some I will when back from
> vacation. I am mostly submitting this patch to show you that I worked on it
> and you do not need to do it yourself.
>
> 2013-12-27 François Dumont
>
> * include/bits/stl_tree.h (_Rb_tree_reuse_or_alloc_node<>): New.
> (_Rb_tree_alloc_node<>): Likewise.
> (_Rb_tree<>::_M_clone_node): Made template to take a node
> generator.
> (_Rb_tree_impl<>): Remove unused _Is_pod_comparator template
> value.
> (_Rb_tree<>::_M_move_assign): Replace by...
> (_Rb_tree<>::operator(_Rb_tree&&)): ...this.
> (_Rb_tree_impl<>::_M_reset): New.
> (_Rb_tree<>::_M_insert_): Add node generator parameter.
> (_Rb_tree<>::_M_copy): Add overload taking a node generator.
> (_Rb_tree<>::_M_insert_unique_): Add node generator parameter.
> (_Rb_tree<>::_M_insert_equal_): Add node generator parameter.
> (_Rb_tree<>::_M_assign_unique): New.
> (_Rb_tree<>::_M_assign_equal): New.
> (_Rb_tree<>): Adapt to use _Rb_tree_impl<>::_M_reset and reuse
> nodes as much as possible.
> * include/bits/stl_set.h (set<>::operator=(set<>&&): Adapt to use
> _Rb_tree move assignment operator.
> (set<>::operator=(initializer_list<>)): Adapt to use
> _Rb_tree<>::_M_assign_unique.
> * include/bits/stl_multiset.h
> (multiset<>::operator=(multiset<>&&)): Adapt to use
> _Rb_tree move assignment operator.
> (multiset<>::operator=(initializer_list<>)): Adapt to use
> _Rb_tree<>::_M_assign_equal.
> * include/bits/stl_map.h (map<>::operator=(map<>&&): Adapt to use
> _Rb_tree move assignment operator.
> (map<>::operator=(initializer_list<>)): Adapt to use
> _Rb_tree<>::_M_assign_unique.
> * include/bits/stl_multimap.h
> (multimap<>::operator=(multimap<>&&)): Adapt to use
> _Rb_tree move assignment operator.
> (multimap<>::operator=(initializer_list<>)): Adapt to use
> _Rb_tree<>::_M_assign_equal.
>
> Tested under Linux x86_64.
>
> Happy new year.
>
> François
>
Fwd: vector lightweight debug mode
-- Forwarded message --
From: Christopher Jefferson
Date: 17 September 2015 at 18:59
Subject: Re: vector lightweight debug mode
To: Jonathan Wakely
On 16 September 2015 at 21:29, Jonathan Wakely wrote:
> On 16/09/15 21:37 +0200, François Dumont wrote:
>
>>>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> iterator
>>>> insert(const_iterator __position, size_type __n, const
>>>> value_type& __x)
>>>> {
>>>> +__glibcxx_assert(__position >= cbegin() && __position <= cend());
>>>> difference_type __offset = __position - cbegin();
>>>> _M_fill_insert(begin() + __offset, __n, __x);
>>>> return begin() + __offset;
>>>
>>>
>>> This is undefined behaviour, so I'd rather not add this check (I know
>>> it's on the google branch, but it's still undefined behaviour).
>>
>>
>> Why ? Because of the >= operator usage ? Is the attached patch better ?
>> < and == operators are well defined for a random access iterator, no ?
>
>
> No, because it is undefined to compare iterators that belong to
> different containers, or to compare pointers that point to different
> arrays.
While that's true, on the other hand it's defined behaviour when the
assert passes, and in the case where the thing it's trying to check
fails, we are off into undefined-land anyway.
A defined check would be to check if __offset is < 0 or > size(). Once
again if it's false we are undefined, but the assert line itself is
then defined behaviour.
Re: Remove unordered containers iterators default initialization
On 25 November 2013 21:02, François Dumont wrote: > > Hi > > Following N3644 discussion thread here is a patch proposal to remove > default zero-initialization of unordered containers iterator. I also took the > time to remove default zero-init of nodes _M_nxt pointer. > > 2013-11-25 François Dumont > > * include/bits/hashtable_policy.h (_Hash_node_base): Default > default constructor. > (_Node_iterator): Likewise. > (_Node_const_iterator): Likewise. > * include/bits/hashtable.h: Adapt. > > Tested under Linux x86_64. > > Ok to commit ? > I just want to check exactly what is going on here, please correct me if I am wrong. Before N3644 there was (I believe) no guarantee we either default or value constructing iterators lead to a default state. Therefore, we were previously providing more functionality than the standard required, which you are now removing. Are we sure we want to remove that extra functionality? Chris
Re: Enhance std::hash for pointers
My concern with accepting this patch is that many of libstdc++'s hash
functions are awful from a mixing point of view -- you would get
exactly the same problem from users who have integers which are always
a multiple of a power of 2 (which is in practice not uncommon). This
would give exactly the same problem.
Rather than try to "fix" one hash function like this, we should just
accept our hash functions might have low quality lower order bits.
On 8 May 2015 at 21:18, François Dumont wrote:
> On 08/05/2015 10:02, Richard Biener wrote:
>>
>> On Wed, May 6, 2015 at 10:10 PM, François Dumont
>> wrote:
>>>
>>> Hi
>>>
>>> Following Marc Glisse comment #4
>>> on:https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65641 I would like to
>>> propose this enhancement to the hash functor for pointers. It simply gets
>>> rid of the irrelevant bits on pointers hash code based on memory
>>> alignment
>>> of the pointed type. The only drawback I can think of is that the type
>>> needs
>>> to be complete at std::hash instantiation time but is it really an issue
>>> ?
>>>
>>> IMO it is quite obvious that the resulting hash code will be better
>>> but
>>
>> If you use a real hashing function that's not true. That is, something
>> else than GCCs pointer_hash (void *p) { return (uintptr_t)p >>3; }.
>
>
> Sorry, I don't understand your remark. Do you mean that if someone is not
> using std::hash to hash its pointers he won't benefit from the enhancement ?
>
> It is a good point however to see that gcc is using something similar
> internally.
>
> François
>
Re: [patch] New std::string implementation
Some (very small) questions / cleanups 1) Do you plan on supporting CXX11 ABI on C++03? There is some #if __cplusplus < 201103L inside the new basic_string. 2) Is there a need for the #if 0 _M_mutate? I tried bootstrapping on Mac OS X 10.10, and got lots of linking issues, the relevant part is:: /bin/sh ../libtool --tag CXX --mode=link /Users/caj/progs/gcc/gcc-bin/./gcc/xgcc -shared-libgcc -B/Users/caj/progs/gcc/gcc-bin/./gcc -nostdinc++ -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/src -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/src/.libs -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/libsupc++/.libs -B/gccbin/x86_64-apple-darwin14.0.0/bin/ -B/gccbin/x86_64-apple-darwin14.0.0/lib/ -isystem /gccbin/x86_64-apple-darwin14.0.0/include -isystem /gccbin/x86_64-apple-darwin14.0.0/sys-include -Wl,-single_module -fno-common -DPIC -fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual -Wabi -fdiagnostics-show-location=once -fvisibility-inlines-hidden -ffunction-sections -fdata-sections -frandom-seed=libstdc++.la -o libstdc++.la -version-info 6:21:0 -Wl,-exported_symbols_list,libstdc++-symbols.explist -lm -rpath /gccbin/lib compatibility.lo compatibility-debug_list.lo compatibility-debug_list-2.lo compatibility-c++0x.lo compatibility-atomic-c++0x.lo compatibility-thread-c++0x.lo compatibility-chrono.lo compatibility-condvar.lo ../libsupc++/libsupc++convenience.la ../src/c++98/libc++98convenience.la ../src/c++11/libc++11convenience.la libtool: link: /Users/caj/progs/gcc/gcc-bin/./gcc/xgcc -shared-libgcc -B/Users/caj/progs/gcc/gcc-bin/./gcc -nostdinc++ -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/src -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/src/.libs -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/libsupc++/.libs -B/gccbin/x86_64-apple-darwin14.0.0/bin/ -B/gccbin/x86_64-apple-darwin14.0.0/lib/ -isystem /gccbin/x86_64-apple-darwin14.0.0/include -isystem /gccbin/x86_64-apple-darwin14.0.0/sys-include-dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/libstdc++.6.dylib .libs/compatibility.o .libs/compatibility-debug_list.o .libs/compatibility-debug_list-2.o .libs/compatibility-c++0x.o .libs/compatibility-atomic-c++0x.o .libs/compatibility-thread-c++0x.o .libs/compatibility-chrono.o .libs/compatibility-condvar.o -Wl,-force_load,../libsupc++/.libs/libsupc++convenience.a -Wl,-force_load,../src/c++98/.libs/libc++98convenience.a -Wl,-force_load,../src/c++11/.libs/libc++11convenience.a -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/libsupc++/.libs -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/src -L/Users/caj/progs/gcc/gcc-bin/x86_64-apple-darwin14.0.0/libstdc++-v3/src/.libs -lm -Wl,-single_module -Wl,-exported_symbols_list -Wl,libstdc++-symbols.explist -install_name /gccbin/lib/libstdc++.6.dylib -compatibility_version 7 -current_version 7.21 -Wl,-single_module ld: warning: cannot export hidden symbol __cxxabiv1::__pbase_type_info::__pointer_catch(__cxxabiv1::__pbase_type_info const*, void**, unsigned int) const from ../libsupc++/.libs/libsupc++convenience.a(pbase_type_info.o) ld: warning: cannot export hidden symbol std::basic_stringbuf[abi:cxx11], std::allocator >::~cxx11() from ../src/c++98/.libs/libc++98convenience.a(complex_io.o) ld: warning: cannot export hidden symbol std::basic_stringbuf[abi:cxx11], std::allocator >::~cxx11() from ../src/c++98/.libs/libc++98convenience.a(complex_io.o) ld: warning: cannot export hidden symbol construction vtable for std::basic_istream >-in-std::istrstream from ../src/c++98/.libs/libc++98convenience.a(strstream.o) ld: warning: cannot export hidden symbol construction vtable for std::basic_ostream >-in-std::ostrstream from ../src/c++98/.libs/libc++98convenience.a(strstream.o) duplicate symbol std::basic_stringbuf, std::allocator >::showmanyc() in: ../src/c++11/.libs/libc++11convenience.a(cow-sstream-inst.o) ../src/c++11/.libs/libc++11convenience.a(cow-string-inst.o) duplicate symbol std::basic_stringbuf, std::allocator >::underflow() in: ../src/c++11/.libs/libc++11convenience.a(cow-sstream-inst.o) ../src/c++11/.libs/libc++11convenience.a(cow-string-inst.o) duplicate symbol std::basic_stringbuf, std::allocator >::pbackfail(int) in: ../src/c++11/.libs/libc++11convenience.a(cow-sstream-inst.o) ../src/c++11/.libs/libc++11convenience.a(cow-string-inst.o) duplicate symbol std::basic_stringbuf, std::allocator >::seekoff(long long, std::_Ios_Seekdir, std::_Ios_Openmode) in: ../src/c++11/.libs/libc++11convenience.a(cow-sstream-inst.o) ../src/c++11/.libs/libc++11convenience.a(cow-string-inst.o) duplicate symbol std::basic_stringbuf, std::allocator >::seekpos(std::fpos<__mbstate_t>, std::_Ios_Openmode) in: ../src/c++11/.libs/libc++11convenience.a(cow-sstream-inst.o) ../src/c++11/.libs/libc++11convenience.a(cow-string-inst.o) O
Re: [Bug libstdc++/61107] stl_algo.h: std::__inplace_stable_partition() doesn't process the whole data range
I did suggest this change, so I feel I should defend it! Our testing of many algorithms is woefully slim, that is how (for example) the segfaulting bug in std::nth_element got through into a release -- the tests for that algorithm were terrible, and basically didn't test the functionality on enough possible inputs. I consider a series of random inputs to be a good practical way of getting decent code coverage and perform a basic sanity test, without the need for an excessive amount of coding. While these tests aren't showing anything yet, a) We didn't know that until after they were written and executed, and: b) They might help catch problems in future, particular in other algorithms changing underlying functionality. I recently added a set of similar tests for a number of algorithms. If you have an alternative suggestion for better testing I'd be happy to hear it, but I think the algorithms need something beyond just one or two hardwired inputs. Chris On 10 November 2014 22:20, Jonathan Wakely wrote: > On 10/11/14 23:14 +0100, François Dumont wrote: >> >>I introduced the random tests after Christopher Jefferson request to >> have more intensive tests on those algos. Is it the whole stuff of tests >> using random numbers that you don't like or just the usage of mt19937 ? > > > The use of random number in general. > >> If second is this new version using the usual random_device I used so far >> better ? > > > That would be much worse because failures would not be reproducible! > >> If it is the whole usage of random numbers that you don't like I will >> simply get rid of the new tests files. > > > Did the new tests fail before your fix to stl_algo.h? > > If yes, you could extract the values generated in the case that fails > and add a test using those values (this is what I should have done for > the leaking set tests) > > If no, they aren't really testing anything useful. >
Re: [patch][google/integration] Add lightweight checks to vector::begin et. al. (issue4973065)
On 6 Sep 2011, at 21:19, Paul Pluzhnikov wrote: > On Tue, Sep 6, 2011 at 12:44 PM, Jonathan Wakely > wrote: >> On 6 September 2011 20:23, Jonathan Wakely wrote: >>> >>> What's a dangling vector anyway? One that has been moved from? >> >> Apparently not, since a moved-from vector would pass __valid() (as >> indeed it should) >> >> So I'm quite curious what bugs this catches. > > The garden variety "use after free": > > vector<> *v = new vector<>; > ... > delete v; > ... > > for (it = v->begin(); it != v->end(); ++it) // Oops! > >> The existing debug mode >> catches some fairly subtle cases of user error such as comparing >> iterators from different containers, or dereferencing past-the-end >> iterators. This patch seems to catch user errors related to ... what? >> Heap corruption? Using a vector after its destructor runs? Those >> are pretty serious, un-subtle errors and not specific to vector, so >> why add code to vector (code which isn't 100% reliable if it only >> catches corruption after the memory is reused and new data written to >> it) > > It is 100% percent reliable for us, due to use of a debugging allocator > which scribbles on all free()d memory. > > But yes, that's one more reason why this patch is not really good for > upstream. > >> to detect more general problems that can happen to any type? > > We can't (easily) catch the general problem. This patch allows us to easily > catch this particular instance of it. > >> The fact the patch did catch real bugs doesn't mean it's a good idea, >> as you say, those bugs probably could have been caught in other ways. > > Sure -- we have other ways to catch the these bugs. They are not very > practical at the moment due to their runtime overhead. > > As for your other suggestion: enabling _GLIBCXX_DEBUG just for vector, > that didn't occur to me and is something I'd like to explore. It might be interesting to profile your app under _GLIBCXX_DEBUG, and see where the high costs are. I previously found that stable_sort had a stupidly high cost, and it turned out with some tweaking we could get just as much protection with a lower cost. Chris
Re: [v3] constexpr tuple
On 8 Sep 2011, at 18:34, Paolo Carlini wrote:
> On 09/07/2011 07:44 AM, Daniel Krügler wrote:
>> Is tuple_cat now considered conforming?
>> No, see:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50159
> By the way, Daniel, I was considering giving that issue a try, if you have
> tips (or even more ;) about the implementation of the C++11 conforming
> tuple_cat, I'm all ears…
This might be totally insane, but I believe that:
tuple_cat(tuple_cat(A,B), C) always equivalent to tuple_cat(A,B,C);
Therefore, how close would something like (warning, not even compiled)
template
auto
tuple_cat(_Tuple1&& __t1, _Tuple2&& __t2, _Tuples&&… __tuples)
-> tuple_cat(tuple_cat(std::forward<_Tuple1>(__t1),
std::forward<_Tuple2>(__t2)),
std::forward<_Tuples&&>(__tuples)…)
{ tuple_cat(tuple_cat(std::forward<_Tuple1>(__t1),
std::forward<_Tuple2>(__t2)),
std::forward<_Tuples&&>(__tuples)…); }
I imagine that first return type unfortunately isn't valid, but it shouldn't be
hard to glue together the list of template arguments.
Chris
Re: RFA (libstdc++): C++/v3 PATCH for c++/24163 (lookup in dependent bases) and c++/29131
On 20 May 2011, at 20:30, Joe Buck wrote: > On Fri, May 20, 2011 at 09:32:16AM -0700, Jason Merrill wrote: >> G++ has had a long-standing bug with unqualified name resolution in >> templates: if we didn't find any declaration when looking up a name in >> the template definition, we would do an additional unqualified lookup at >> the point of instantiation. This led to incorrectly finding >> namespace-scope functions declared later (29131) and member functions of >> dependent bases (24163). This patch fixes that bug. > > I get the impression that most competing C++ compilers (other than the > old HP compiler) were (or are) very loose about that rule. > >> To be friendly to users, the patch also allows affected code to compile >> with -fpermissive and provides suggestions about how to fix the code: >> either declaring the desired function earlier (29131) or explicitly >> qualifying the name with this-> or Class:: (24163). > > I think that it's quite likely that there is a lot of C++ code out there > that depends on this bug to compile. So I'm glad that you've included > user guidance in the error messages, and it would be interesting to see > how much code is affected when, say, compiling a distro. Fortunately clang already implements this rule correctly, so many code bases are already getting fixed. I personally (with a couple of other people) fixed dozens of places in boost where this was breaking code. I expect this to break huge amounts of code in g++-only code bases, but will also help improve compatibility with other compilers. I could see the temptation to introduce this as a mandatory warning for a while, and only add it under -pedantic. However, it might be easier to just force people to fix their code. Chris
