Re: Support for function pointers and references

[Ondrej Oprala <ooprala@redhat.com>]

On 29.09.2015 10:58, Dodji Seketeli wrote:
> Hello Ondrej,
>
> I have been looking at the second and last patch of the series:
>
>      commit 18a700ff0751a0c578a8c5c59abc5c87c8143335
>      Author: Ondrej Oprala <ooprala@redhat.com>
>      Date:   Wed Sep 23 08:44:00 2015 +0200
>
> 	Support pointers and references to functions
>
> Again, great patch.  Thanks for your hard work.
>
> I have a few comments that you'll find below.
>
> diff --git a/include/abg-ir.h b/include/abg-ir.h
>
> [...]
>
> @@ -1042,6 +1045,7 @@ public:
>   
>     /// Convenience typedef for a vector of @ref decl_base_sptr.
>     typedef std::vector<decl_base_sptr >	declarations;
> +  typedef std::vector<function_type_sptr >	function_types;
>
> Please add a comment for this typedef (starting with "///") so that
> the apidoc is updated, just like what we do for the other typedefs.  I
> agree this commenting business feels cumbersome but then later we're
> happy when we enjoy the completeness of the apidoc :-)
>
>
> [...]
>
> @@ -5986,7 +5986,6 @@ compute_diff_for_types(const decl_base_sptr first,
>      ||(d = try_to_diff<array_type_def>(f, s, ctxt))
>      ||(d = try_to_diff<qualified_type_def>(f, s, ctxt))
>      ||(d = try_to_diff<typedef_decl>(f, s, ctxt))
> -   ||(d = try_to_diff<function_type>(f, s, ctxt))
>      ||(d = try_to_diff_distinct_kinds(f, s, ctxt)));
>
> Why avoid handling function types here, to just handle them ...
>
>      @@ -7138,9 +7137,21 @@ compute_diff(pointer_type_def_sptr	first,
>         if (first && second)
>           assert(first->get_environment() == second->get_environment());
>
>      -  diff_sptr d = compute_diff_for_types(first->get_pointed_to_type(),
>      -				       second->get_pointed_to_type(),
>      -				       ctxt);
>      +  diff_sptr d;
>      +
>      +  function_type_sptr f = dynamic_pointer_cast<function_type>(first->get_pointed_to_type()),
>      +		     g = dynamic_pointer_cast<function_type>(second->get_pointed_to_type());
>      +  if (f && g)
>      +    d = compute_diff(f, g, ctxt);
>      +  else if (f || g)
>      +    d = try_to_diff_distinct_kinds(first->get_pointed_to_type(),
>      +				   second->get_pointed_to_type(),
>      +				   ctxt);
>      +  else
>      +    d = compute_diff_for_types(first->get_pointed_to_type(),
>      +			       second->get_pointed_to_type(),
>      +			       ctxt);
>      +
>
> ... here ?
>
> I would think that compute_diff_for_type() would handle the two
> pointer types fine; then the overload of compute_diff() for pointer
> types would call compute_diff_for_type() on the pointed-to types,
> again; then compute_diff_for_type() *should* handle all the cases of
> pointed-to types; if not, it should extended to do so.  What am I
> missing?
You're right, fixed now.
> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
>
> [...]
>
> +	      /* For now, we skip the hidden vtable pointer */
> +	      if (n.substr(0, 5) == "_vptr"
> +		  && !std::isalnum(n.at(5))
> +		  && n.at(5) != '_')
> +                continue;
>
> Please add a comment explaining the (name) pattern you are looking for
> to recognize the vtable pointer member here.  In these matters, I
> believe redundancy of information is helpful to catch issues later.
>
> -    utype_decl =
> -      dynamic_pointer_cast<decl_base>(build_ir_node_for_void_type(ctxt));
>
> Hehe, see, I was using the dynamic_pointer_cast, rather than using the
> 'is_decl()' function I told you about earlier.  Bad bad me.  :-)
>
> It's cool how your changes removed those, by the way.
>
> Initially the code base had a lot of these; but then after I have
> started to debug the code base a little bit more, it appeared that using
> the is_*() function was much more practical.  So I introduced them in
> new code, updated some old code, but obviously, some old code still
> needs to be updated.  I think we'll need to provide some patches later
> to update the code towards that end.
>
> [...]
>
> +static function_type_sptr
> +build_function_type(read_context&	ctxt,
> +		    Dwarf_Die*		die,
> +		    bool		die_is_from_alt_di,
> +		    class_decl_sptr	is_method,
> +		    size_t		where_offset)
> +{
>
> [...]
>
> +    return_type_decl = dynamic_pointer_cast<decl_base>(
> +      build_ir_node_from_die(ctxt, &ret_type_die,
> +			     ret_type_die_is_in_alternate_debug_info,
> +			     /*called_from_public_decl=*/true,
> +			     where_offset));
>
> Please use is_decl() here.
>
> [...]
>
> +  if (!result && dwarf_child(die, &child) == 0)
> +    do
> +      {
>
> It seems to me the !result clause is useless here; is it not?
>
> I understand that you took this code from the former
> build_function_decl() code; in that former context, the !result clause
> was not useless because the 'result' could have been initialized to a
> non-nil declaration.  But here, I don't see that.
Oops, you're right
> [...]
>
> +	      parm_type_decl = dynamic_pointer_cast<decl_base>(
> +		build_ir_node_from_die(ctxt, &parm_type_die,
> +				       parm_type_die_is_in_alt_di,
> +				       /*called_from_public_decl=*/true,
> +				       where_offset));
>
> Please use is_decl() rather than the dynamic_pointer_cast.
>
> [...]
>
> +      }
> +  while (!result && dwarf_siblingof(&child, &child) == 0);
>
> Here, I think the !result clause is useless as well.
>
> By the way, build_function_type() is cool, I like it :-)
>
> and using it here ...
>
>      @@ -7315,75 +7447,10 @@ build_function_decl(read_context&	ctxt,
>      [...]
>
>      +      function_type_sptr fn_type(build_function_type(ctxt,
>      +						     die,
>      +						     is_in_alt_di,
>      +						     is_method,
>      +						     where_offset));
>
> looks very natural much more than what I was doing.  That is super cool.
> thank you for that!
>
> [...]
>
> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> [...]
>
> +/// Getter of the the function types of the current @ref translation
> +/// unit.
>
> There is a "the the" repetition there.  Also, it should be @ref translation_unit.
>
> [...]
>
> @@ -6541,16 +6549,16 @@ pointer_type_def::pointer_type_def(const type_base_sptr&
> [...]
>
> +      if (pto)
> +        set_visibility(pto->get_visibility());
> +      pointed_to_type_ = weak_ptr<type_base>(type_or_void(pointed_to, 0));
>
> Please use the type_base_wptr typedef here, rather than the longer
> weak_ptr<type_base> notation.
>
> -	name = "void";
> +        name = get_type_name(get_pointed_to_type(), /*qualified_name=*/true);
>
> It looks like there is a white space issue here.  Normally, before the
> "name" token, there should be one tab, not 8 white spaces.
>
> [...]
>
> +      else
> +        name = get_type_name(dynamic_pointer_cast<function_type>(pointed_to),
> +			     /*qualified_name=*/true) + "&";
>
> Please use is_function_type() here, rather than
> dynamic_pointer_cast().
>
> [...]
>
> +
> +      pointed_to_type_ = weak_ptr<type_base>(type_or_void(pointed_to, 0));
>
> Please use type_base_wptr.
>
> [...]
>
> @@ -6808,7 +6825,10 @@ reference_type_def::get_qualified_name() const
>   	get_type_declaration(type_or_void(get_pointed_to_type(),
>   					  get_environment()));
>         string name;
> -      td->get_qualified_name(name);
> +      if (!td)
> +	name = get_type_name(get_pointed_to_type(), /*qualified_name=*/true);
> +      else
> +	td->get_qualified_name(name);
>
> I think we could do away with 'td' completely. I mean Let's not even
> try to get the type declaration here.  Let's just use get_type_name()
> as it knows how to handle types that don't have declarations.  It'll
> make the logic simpler and thus easier to maintain later.
>
> [...]
>
> diff --git a/src/abg-writer.cc b/src/abg-writer.cc
> [...]
> @@ -1232,6 +1266,9 @@ write_pointer_type_def(const pointer_type_def_sptr	decl
> [...]
>
> +  if (dynamic_pointer_cast<function_type>(decl->get_pointed_to_type()))
> +    ctxt.record_type_as_emitted(decl->get_pointed_to_type());
> +
>
> I think that as the pointed to type is *not* written here, it should
> not be marked as being written.
>
> My understanding is that the reason why you are doing this hack is that
> only function types that are referenced via function pointers should be
> emitted.  Other functions types that are not referenced by function
> pointers should not be emitted.  Is that the case?
>
> If so, what you could do instead, is to keep a (well documented) map of
> "function types to emit", on the side.  Note that the key of the map
> would be the pointer *value* of the canonical type of the function type.
> So when you are here:
>
>      @@ -971,6 +991,20 @@ write_translation_unit(const translation_unit&	tu,
>      [...]
>      +  typedef scope_decl::function_types function_types;
>      +  typedef function_types::const_iterator const_fn_iterator;
>      +  const function_types& t = tu.get_function_types();
>      +
>      +  for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
>      +    {
>      +      if (!ctxt.type_is_emitted(dynamic_pointer_cast<type_base>(*i)))
>      +	// This type has already been written out to the current
>      +	// translation unit, so do not emit it again.
>      +	continue;
>      +      o << "\n";
>      +      write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
>      +    }
>      +
>
> , you can then check that the function type you are about to emit is
> part of the map of function types to emit.  If it is, then emit it
> (that would mark the function type as having been emitted) and remove
> it from the map of function pointers to emit.  You can thus assert
> that a function type that is in the map of the function types to emit
> has *not* been emitted yet.
>
> Then you can remove this:
>
>      +  void
>      +  record_type_id_as_not_emitted(const string& id)
>      +  {m_emitted_type_id_map.erase(id);}
>
> , this,
>
>      +  void
>      +  record_type_as_not_emitted(const type_base_sptr& t)
>
> and this (in the new write_function_type()):
>
>      +  ctxt.record_type_as_not_emitted(decl);

Yeah, I tried to hack around it using existing code...should be much 
better now.
>
> Cheers,
>
I've rebased, fixed and amended both patches and they're in 
ondrej/fnptrs again. Thank you for your review!
Cheers,
   Ondrej