[RFA,01/22] Change selttest.c to use use std::vector

  This patch changes selftest.c to use std::vector rather than VEC.

I think this is a small net plus.  Because we're using C++03, we
unfortunately don't all the possible benefits here; namely, iterating
over a vector is still a pain.  For reference, in C++11 the loop would
be just:

    for (auto f : tests)
      ...

2016-09-26  Tom Tromey  <tom@tromey.com>

	* selftest.c: Include <vector>, not "vec.h".
	(self_test_function_ptr): Remove.
	(tests): Now a std::vector.
	(register_self_test, run_self_tests): Update.
---
 gdb/ChangeLog  |  7 +++++++
 gdb/selftest.c | 22 +++++++++-------------
 2 files changed, 16 insertions(+), 13 deletions(-)
  

On Mon, Sep 26, 2016 at 10:08:29PM -0600, Tom Tromey wrote:
> This patch changes selftest.c to use std::vector rather than VEC.
> 
> I think this is a small net plus.  Because we're using C++03, we

I'd agree, hopefully we can throw VEC in the trash some day.  Relatedly
we should probably move gcc's hash table stuff to include/ sooner rather
than later so we can similarly get rid of htab.

> unfortunately don't all the possible benefits here; namely, iterating

I think you missed get in that sentence.

> over a vector is still a pain.  For reference, in C++11 the loop would
> be just:
> 
>     for (auto f : tests)

its debatable, but imho its not necessarily obvious what the type of the
local is when you use auto to iterate over vectors especially when they
are members or globals like here.

> +static std::vector<self_test_function *> tests;

should we use a pointer to avoid the static initializer?

> -  for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i)
> +  for (std::vector<self_test_function *>::iterator iter = tests.begin ();
> +       iter != tests.end ();
> +       ++iter)

I believe you can "cheat" here and just use the function pointer type,
because the sane implementation of iterators over vectors is pointers.

Trev
  

>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:

Trevor> I'd agree, hopefully we can throw VEC in the trash some day.  Relatedly
Trevor> we should probably move gcc's hash table stuff to include/ sooner rather
Trevor> than later so we can similarly get rid of htab.

That would be nice; though we could probably use std::set and std::map
in gdb as well.  One wrinkle with hash tables is that they're sometimes
allocated on obstacks; would gcc's handle this?

>> for (auto f : tests)

Trevor> its debatable, but imho its not necessarily obvious what the type of the
Trevor> local is when you use auto to iterate over vectors especially when they
Trevor> are members or globals like here.

Yeah, that's one drawback.
Often one actually wants "auto &f" in there.

>> +static std::vector<self_test_function *> tests;

Trevor> should we use a pointer to avoid the static initializer?

I was on the fence about this one.
On the one hand, static initializers can be very bad.
On the other hand, this one in particular doesn't seem like it could
cause problems.

>> -  for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i)
>> +  for (std::vector<self_test_function *>::iterator iter = tests.begin ();
>> +       iter != tests.end ();
>> +       ++iter)

Trevor> I believe you can "cheat" here and just use the function pointer type,
Trevor> because the sane implementation of iterators over vectors is pointers.

Yeah, or just loop using an int and index into the vector.
I'll see if that looks cleaner.

Tom
  

On Tue, Sep 27, 2016 at 09:55:28AM -0600, Tom Tromey wrote:
> >>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> 
> Trevor> I'd agree, hopefully we can throw VEC in the trash some day.  Relatedly
> Trevor> we should probably move gcc's hash table stuff to include/ sooner rather
> Trevor> than later so we can similarly get rid of htab.
> 
> That would be nice; though we could probably use std::set and std::map
> in gdb as well.  One wrinkle with hash tables is that they're sometimes
> allocated on obstacks; would gcc's handle this?

No, they don't support that at the moment, though I suppose it would
be fine for keys or values to point into obstacks.  Is there a good
reason for this other than using obstacks to provide a sort of automatic
memory management?

> >> for (auto f : tests)
> 
> Trevor> its debatable, but imho its not necessarily obvious what the type of the
> Trevor> local is when you use auto to iterate over vectors especially when they
> Trevor> are members or globals like here.
> 
> Yeah, that's one drawback.
> Often one actually wants "auto &f" in there.

yeah, and not having that can sometimes be bad.

> >> +static std::vector<self_test_function *> tests;
> 
> Trevor> should we use a pointer to avoid the static initializer?
> 
> I was on the fence about this one.
> On the one hand, static initializers can be very bad.
> On the other hand, this one in particular doesn't seem like it could
> cause problems.


yeah, I think its just zeroing some memory and then registering a static
destructor.  So its silly that its necessary at all, but whether to
avoid it or not is just a perf question.

Trev

> >> -  for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i)
> >> +  for (std::vector<self_test_function *>::iterator iter = tests.begin ();
> >> +       iter != tests.end ();
> >> +       ++iter)
> 
> Trevor> I believe you can "cheat" here and just use the function pointer type,
> Trevor> because the sane implementation of iterators over vectors is pointers.
> 
> Yeah, or just loop using an int and index into the vector.
> I'll see if that looks cleaner.
> 
> Tom
  

>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:

Tom> That would be nice; though we could probably use std::set and std::map
Tom> in gdb as well.  One wrinkle with hash tables is that they're sometimes
Tom> allocated on obstacks; would gcc's handle this?

Trevor> No, they don't support that at the moment, though I suppose it
Trevor> would be fine for keys or values to point into obstacks.  Is
Trevor> there a good reason for this other than using obstacks to
Trevor> provide a sort of automatic memory management?

Sometimes it's just to avoid more work -- cleaning up an obstack is
easy.  E.g., see dwarf2loc.c:func_verify_no_selftailcall Uses like this
can easily be replaced.

There are also spots that allocate a hash table on the objfile obstack.
Now, this is a bit unfortunate because it means that hash table resizes
will create a kind of "leak" -- not a true leak (the memory is tracked
and will be freed), but basically some chunk of memory will no longer be
useful.

It's possible to address these cases in other ways, though more
difficult.

Tom
  

On 2016-09-27 00:08, Tom Tromey wrote:
> -  printf_filtered (_("Ran %u unit tests, %d failed\n"),
> -		   VEC_length (self_test_function_ptr, tests), failed);
> +  printf_filtered (_("Ran %lu unit tests, %d failed\n"),
> +		   tests.size (), failed);

This doesn't build on 32 bits:

selftest.c: In function ‘void run_self_tests()’:
selftest.c:62:27: error: format ‘%lu’ expects argument of type ‘long 
unsigned int’, but argument 2 has type ‘std::vector<void 
(*)()>::size_type {aka unsigned int}’ [-Werror=format=]
       tests.size (), failed);
                            ^
I think the right format size specifier would be 'z'.
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>> +  printf_filtered (_("Ran %lu unit tests, %d failed\n"),
>> +		   tests.size (), failed);

Simon> This doesn't build on 32 bits:

Thanks for trying that.
I will send it through buildbot before the next round.

Simon> I think the right format size specifier would be 'z'.

I will probably cast to long instead.
I don't see other uses of %zd in gdb.

Tom
  

On 09/27/2016 04:55 PM, Tom Tromey wrote:
> Trevor> I'd agree, hopefully we can throw VEC in the trash some day.  Relatedly
> Trevor> we should probably move gcc's hash table stuff to include/ sooner rather
> Trevor> than later so we can similarly get rid of htab.
> 
> That would be nice; though we could probably use std::set and std::map
> in gdb as well.

std::unordered_map (hash table) would be a more direct replacement.
See gold/system.h -- for C++03 compilers, gold uses unordered_map (and
unordered_set) from std::tr1 if available, otherwise falls back to
std::map (balanced binary tree, usually red/black), with even
older compilers.  The idea being that you'll be able to use gold with
older compilers, though it'll run slower.

Thanks,
Pedro Alves
  

On 09/27/2016 04:55 PM, Tom Tromey wrote:
> Trevor> should we use a pointer to avoid the static initializer?
> 
> I was on the fence about this one.
> On the one hand, static initializers can be very bad.
> On the other hand, this one in particular doesn't seem like it could
> cause problems.

I think it's OK for now.  The patch LGTM.  We may want to revisit later.

I guess with "can be very bad" you're thinking of order of constructors
between compilation units.  Worth keeping in mind is the not making the library
use case harder to get at in the future, considering the potential need for
controlled bring up (static ctors) and teardown (static dtors) independent
of exit() time.   Yet another aspect worth considering is the startup time
overhead impact of global constructors.  VEC has a POD base
class which allows avoiding that:

  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01651.html

See LLVM here considering/doing similar things:

  https://llvm.org/bugs/show_bug.cgi?id=11944

But that feels like premature optimization too me at this point.

Thanks,
Pedro Alves
  

On 09/30/2016 10:16 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>>> +  printf_filtered (_("Ran %lu unit tests, %d failed\n"),
>>> +		   tests.size (), failed);
> 
> Simon> This doesn't build on 32 bits:
> 
> Thanks for trying that.
> I will send it through buildbot before the next round.
> 
> Simon> I think the right format size specifier would be 'z'.
> 
> I will probably cast to long instead.
> I don't see other uses of %zd in gdb.

That's because the %z modifier is a C99 / C++11 feature.

Thanks,
Pedro Alves