Re: Support for function pointers and references

Re: Support for function pointers and references

[Dodji Seketeli]

Hey Ondrej,

Ondrej Oprala <ooprala@redhat.com> a écrit:

> I recently pushed my proposed changes to the ondrej/fnptrs branch if
> anyone wants to try/comment/review them. Any and all comments are
> welcome.

I'll review the branch this week.

Thank you very much for working on this important topic.  It's very
appreciated.

Cheers,

-- 
		Dodji

Re: Support for function pointers and references

[Dodji Seketeli]

Hey,

> I've followed your renames, but I've made the condition
>
> if (!ctxt.fntype_is_referenced(*i) || ctxt.type_is_emitted(*i))
>
> ...and amended the comment accordingly...as otherwise it would let
> unreferenced fntypes slip through.

And you are right.  My bad.

> I also created a ctxt.clear_referenced_fntypes_map, since it needs to be cleared
> along with the emitted_types_map, otherwise we'd get duplicates across
> TUs.

Great, thanks.

> The changes are in the usual place :)

Thanks, I have looked at them and I just have one remaining nit comment:

diff --git a/src/abg-writer.cc b/src/abg-writer.cc
[...]

+  /// Record a given function type as being scheduled for emitting
+  /// to the XML output.
+  ///
+  /// @param f a shared pointer to a function type
+  void
+  record_fntype_as_referenced(const function_type_sptr& f)

This comment should be amended to say that the function records a
function type as being referenced by another type that has been
emitted (e.g, a pointer or reference type which pointed-to type is the
function type passed in argument).

+  /// Test if a given function type has been written out to the
+  /// XML output.
+  ///
+  /// @param f a shared pointer to a function type
+  ///
+  /// @return true if the type has already been emitted, false
+  /// otherwise.
+  bool
+  fntype_is_referenced(const function_type_sptr& f)

Likewise.

The patch is OK to commit with the changes above.  Again, you can commit
it to master or I can do it, as you prefer.

Thank you very much for your dedication and your patience on this topic.
The patch-set is in, and I like it very much.  This is a great new
capability for libabigail!  Woot!

Cheers,

-- 
		Dodji

Re: Support for function pointers and references

[Ondrej Oprala]

On 30.09.2015 20:05, Dodji Seketeli wrote:
> Hey,
>
>> I've followed your renames, but I've made the condition
>>
>> if (!ctxt.fntype_is_referenced(*i) || ctxt.type_is_emitted(*i))
>>
>> ...and amended the comment accordingly...as otherwise it would let
>> unreferenced fntypes slip through.
> And you are right.  My bad.
>
>> I also created a ctxt.clear_referenced_fntypes_map, since it needs to be cleared
>> along with the emitted_types_map, otherwise we'd get duplicates across
>> TUs.
> Great, thanks.
>
>> The changes are in the usual place :)
> Thanks, I have looked at them and I just have one remaining nit comment:
>
> diff --git a/src/abg-writer.cc b/src/abg-writer.cc
> [...]
>
> +  /// Record a given function type as being scheduled for emitting
> +  /// to the XML output.
> +  ///
> +  /// @param f a shared pointer to a function type
> +  void
> +  record_fntype_as_referenced(const function_type_sptr& f)
>
> This comment should be amended to say that the function records a
> function type as being referenced by another type that has been
> emitted (e.g, a pointer or reference type which pointed-to type is the
> function type passed in argument).
>
> +  /// Test if a given function type has been written out to the
> +  /// XML output.
> +  ///
> +  /// @param f a shared pointer to a function type
> +  ///
> +  /// @return true if the type has already been emitted, false
> +  /// otherwise.
> +  bool
> +  fntype_is_referenced(const function_type_sptr& f)
>
> Likewise.
>
> The patch is OK to commit with the changes above.  Again, you can commit
> it to master or I can do it, as you prefer.
Changed and committed to master
> Thank you very much for your dedication and your patience on this topic.
> The patch-set is in, and I like it very much.  This is a great new
> capability for libabigail!  Woot!
>
> Cheers,
>
Thanks for the numerous IRC brainstorming sessions!
  Cheers,
     Ondrej!

Re: Support for function pointers and references

[Dodji Seketeli]

Hello Ondrej,

So I have been looking at this patch:

    commit 5e9187ac43824a79c3e3d55ff71503aeb6718c41
    Author: Ondrej Oprala <ooprala@redhat.com>
    Date:   Wed Sep 9 10:12:03 2015 +0200

	Generalize some dwarf-reader functions to generate and return
	instances of type_or_decl_base_stpr to be able to propagate
	types occurring without an accompanying declaration.

	    * src/abg-dwarf-reader.cc (build_ir_node_from_die): Return
	    a type_or_decl_base_sptr instead.
	    (get_scope_for_die): Likewise.
	    (build_class_type_and_add_to_ir): Typecast the assignment from
	    build_ir_node_from_die properly.
	    (build_{qualified,reference,array,typedef}_type): Likewise.
	    (build_pointer_type_def): Likewise.
	    (build_{var,function}_decl): Likewise.

	Signed-off-by: Ondrej Oprala <ooprala@redhat.com>


I just have some nitpicking comments about it :-)

[...]

@@ -6570,11 +6570,11 @@ build_class_type_and_add_to_ir(read_context&	ctxt,
 				     type_die_is_in_alternate_debug_info))
 		continue;
 
-	      decl_base_sptr base_type =
+	      decl_base_sptr base_type = dynamic_pointer_cast<decl_base>(

Just as a matter of style, I'd prefer that you use the is_decl()
function (declared in include/abg-fwd.h), which basically does the
dynamic_pointer_cast here, but which is arguably more legible, easier to
call from the debugger (should the need arise) and more importantly, is
what is used throughout the code base for this purpose.

The same comment applies to the other similar calls to the
dynamic_pointer_cast operator to cast type_or_decl_base_sptr to
decl_base_sptr.

 		build_ir_node_from_die(ctxt, &type_die,
 				       type_die_is_in_alternate_debug_info,
 				       called_from_public_decl,
-				       where_offset);
+				       where_offset));

[...]

@@ -6892,8 +6895,8 @@ build_pointer_type_def(read_context&	ctxt,
       assert(result);
       return result;
     }
-
   type_base_sptr utype = is_type(utype_decl);
+
   assert(utype);

I think this is a useless white space change.

This patch looks good to me with those changes.  I can commit it to
master once you've amended it, or you can do it yourself, as you wish
:-)

Thank you again for working on this.

Cheers,

-- 
		Dodji

Re: Support for function pointers and references

[Ondrej Oprala]

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

Support for function pointers and references

[Ondrej Oprala]

Hi everyone,
I recently pushed my proposed changes to the ondrej/fnptrs branch if 
anyone wants to try/comment/review them. Any and all comments are welcome.

There is one known issue/non-issue and that is that some type-id's are 
wasted in the output of abidw, meaning a function-type is assigned a 
type-id but is not emitted in the end. The coherence of id='X' and 
type-id='X' across the abixml is of course maintained.

Thanks,
  Ondrej

Re: Support for function pointers and references

[Ondrej Oprala]

Hi Dodji,
thank you for a lightning fast review!
I'll amend it and rebase against master.
I'll ping you on IRC when it's done.

Thanks again!
  Ondrej

On 28.09.2015 16:44, Dodji Seketeli wrote:
> Hello Ondrej,
>
> So I have been looking at this patch:
>
>      commit 5e9187ac43824a79c3e3d55ff71503aeb6718c41
>      Author: Ondrej Oprala <ooprala@redhat.com>
>      Date:   Wed Sep 9 10:12:03 2015 +0200
>
> 	Generalize some dwarf-reader functions to generate and return
> 	instances of type_or_decl_base_stpr to be able to propagate
> 	types occurring without an accompanying declaration.
>
> 	    * src/abg-dwarf-reader.cc (build_ir_node_from_die): Return
> 	    a type_or_decl_base_sptr instead.
> 	    (get_scope_for_die): Likewise.
> 	    (build_class_type_and_add_to_ir): Typecast the assignment from
> 	    build_ir_node_from_die properly.
> 	    (build_{qualified,reference,array,typedef}_type): Likewise.
> 	    (build_pointer_type_def): Likewise.
> 	    (build_{var,function}_decl): Likewise.
>
> 	Signed-off-by: Ondrej Oprala <ooprala@redhat.com>
>
>
> I just have some nitpicking comments about it :-)
>
> [...]
>
> @@ -6570,11 +6570,11 @@ build_class_type_and_add_to_ir(read_context&	ctxt,
>   				     type_die_is_in_alternate_debug_info))
>   		continue;
>   
> -	      decl_base_sptr base_type =
> +	      decl_base_sptr base_type = dynamic_pointer_cast<decl_base>(
>
> Just as a matter of style, I'd prefer that you use the is_decl()
> function (declared in include/abg-fwd.h), which basically does the
> dynamic_pointer_cast here, but which is arguably more legible, easier to
> call from the debugger (should the need arise) and more importantly, is
> what is used throughout the code base for this purpose.
>
> The same comment applies to the other similar calls to the
> dynamic_pointer_cast operator to cast type_or_decl_base_sptr to
> decl_base_sptr.
>
>   		build_ir_node_from_die(ctxt, &type_die,
>   				       type_die_is_in_alternate_debug_info,
>   				       called_from_public_decl,
> -				       where_offset);
> +				       where_offset));
>
> [...]
>
> @@ -6892,8 +6895,8 @@ build_pointer_type_def(read_context&	ctxt,
>         assert(result);
>         return result;
>       }
> -
>     type_base_sptr utype = is_type(utype_decl);
> +
>     assert(utype);
>
> I think this is a useless white space change.
>
> This patch looks good to me with those changes.  I can commit it to
> master once you've amended it, or you can do it yourself, as you wish
> :-)
>
> Thank you again for working on this.
>
> Cheers,
>

Re: Support for function pointers and references

[Dodji Seketeli]

Hello Ondrej,

I have looked at the new revisions of your patches, please find my
comments below.

The patch:

    commit 116a65c65335d731cdae1873cbce9bd338906a90
    Author: Ondrej Oprala <ooprala@redhat.com>
    Date:   Wed Sep 9 10:12:03 2015 +0200

        Generalize some dwarf-reader functions to generate and return
        instances of type_or_decl_base_stpr to be able to propagate
        types occurring without an accompanying declaration.

            * src/abg-dwarf-reader.cc (build_ir_node_from_die): Return
            a type_or_decl_base_sptr instead.
            (get_scope_for_die): Likewise.
            (build_class_type_and_add_to_ir): Typecast the assignment from
            build_ir_node_from_die properly.
            (build_{qualified,reference,array,typedef}_type): Likewise.
            (build_pointer_type_def): Likewise.
            (build_{var,function}_decl): Likewise.

        Signed-off-by: Ondrej Oprala <ooprala@redhat.com>

Looks good to me.  It's OK to go in.

For the patch:

    commit 18a700ff0751a0c578a8c5c59abc5c87c8143335
    Author: Ondrej Oprala <ooprala@redhat.com>
    Date:   Wed Sep 23 08:44:00 2015 +0200

        Support pointers and references to functions

I still have a few comments.  Please find them below.

diff --git a/src/abg-writer.cc b/src/abg-writer.cc

[...]

+/// A convenience typedef for a map that associates a shared pointer to a type
+/// to a bool.

There is a little repetition in "to a type to a bool" here.

[...]

@@ -132,6 +138,9 @@ class write_context
   ostream&				m_ostream;
   type_ptr_map				m_type_id_map;
   unordered_map<string, bool>		m_emitted_type_id_map;

[...]

+  /// A vector of canonical types of function types that
+  /// pointed or referenced to in the current TU
+  fn_shared_ptr_map			m_fntype_to_emit_map;

I would call this member m_referenced_fntypes_map.  Also, I would
rather use the comment:

    /// A vector of function types that are referenced by emitted pointers
    /// or reference, i.e, that are pointed-to types of pointers or references
    /// that are emitted.

Then I would rename:

    +  void
    +  record_fntype_to_emit(const function_type_sptr& f)

into:

    record_fntype_as_referenced(const function_type_sptr& f)


I'd also rename:
    +  bool
    +  fntype_to_emit(const function_type_sptr& f)

into:

    bool
    fntype_is_referenced(const function_type_sptr& f)

So, in write_translation_unit, I'd replace the new:

    +  for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
    +    {
    +      if (!ctxt.fntype_to_emit(*i))
    +	// This function type wasn't marked as one to be emitted
    +	// so skip it.
    +	continue;
    +      o << "\n";
    +      write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
    +    }
+

with:

    for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
      {
	if ((ctxt.fntype_is_referenced(*i) && ctxt.type_is_emitted(*i))
	  // This function type, referenced by an emitted pointer or
	  // reference type, has already been emitted, so skip it.
	  continue;
	o << "\n";
	write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
      }

The idea is to make the code reflect exactly what we are thinking
about doing, which is, we want function types that are referenced by
pointer (or reference) types to be emitted; other function types
should not be emitted.

[...]

+static bool
+write_function_type(const shared_ptr<function_type> decl, write_context& ctxt,
+		    unsigned indent)
+{
[...]

+  ctxt.record_fntype_as_emitted(decl);

I'd replace this call by a call to the usual
ctxt.record_type_as_emitted() that is used throughout the code:

    ctxt.record_type_as_emitted(decl)

I'd then remove the new member write_context::record_fntype_as_emitted()
that you've added.

Also, the new test material (you now, the new test files) need to be
added to tests/data/Makefile.am, otherwise they are not added to the
tarball by "make dist"; and "make distcheck" would fail anyhow.  As a
general rule, all patches should pass "make distcheck".

Thanks!

-- 
		Dodji

Re: Support for function pointers and references

[Dodji Seketeli]

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?

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.

[...]

+	      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);

Cheers,

-- 
		Dodji

Re: Support for function pointers and references

[Ondrej Oprala]

On 30.09.2015 09:10, Dodji Seketeli wrote:
> Hello Ondrej,
>
> I have looked at the new revisions of your patches, please find my
> comments below.
>
> The patch:
>
>      commit 116a65c65335d731cdae1873cbce9bd338906a90
>      Author: Ondrej Oprala <ooprala@redhat.com>
>      Date:   Wed Sep 9 10:12:03 2015 +0200
>
>          Generalize some dwarf-reader functions to generate and return
>          instances of type_or_decl_base_stpr to be able to propagate
>          types occurring without an accompanying declaration.
>
>              * src/abg-dwarf-reader.cc (build_ir_node_from_die): Return
>              a type_or_decl_base_sptr instead.
>              (get_scope_for_die): Likewise.
>              (build_class_type_and_add_to_ir): Typecast the assignment from
>              build_ir_node_from_die properly.
>              (build_{qualified,reference,array,typedef}_type): Likewise.
>              (build_pointer_type_def): Likewise.
>              (build_{var,function}_decl): Likewise.
>
>          Signed-off-by: Ondrej Oprala <ooprala@redhat.com>
>
> Looks good to me.  It's OK to go in.
Cool, thanks!
> For the patch:
>
>      commit 18a700ff0751a0c578a8c5c59abc5c87c8143335
>      Author: Ondrej Oprala <ooprala@redhat.com>
>      Date:   Wed Sep 23 08:44:00 2015 +0200
>
>          Support pointers and references to functions
>
> I still have a few comments.  Please find them below.
>
> diff --git a/src/abg-writer.cc b/src/abg-writer.cc
>
> [...]
>
> +/// A convenience typedef for a map that associates a shared pointer to a type
> +/// to a bool.
>
> There is a little repetition in "to a type to a bool" here.
>
> [...]
>
> @@ -132,6 +138,9 @@ class write_context
>     ostream&				m_ostream;
>     type_ptr_map				m_type_id_map;
>     unordered_map<string, bool>		m_emitted_type_id_map;
>
> [...]
>
> +  /// A vector of canonical types of function types that
> +  /// pointed or referenced to in the current TU
> +  fn_shared_ptr_map			m_fntype_to_emit_map;
>
> I would call this member m_referenced_fntypes_map.  Also, I would
> rather use the comment:
>
>      /// A vector of function types that are referenced by emitted pointers
>      /// or reference, i.e, that are pointed-to types of pointers or references
>      /// that are emitted.
>
> Then I would rename:
>
>      +  void
>      +  record_fntype_to_emit(const function_type_sptr& f)
>
> into:
>
>      record_fntype_as_referenced(const function_type_sptr& f)
>
>
> I'd also rename:
>      +  bool
>      +  fntype_to_emit(const function_type_sptr& f)
>
> into:
>
>      bool
>      fntype_is_referenced(const function_type_sptr& f)
>
> So, in write_translation_unit, I'd replace the new:
>
>      +  for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
>      +    {
>      +      if (!ctxt.fntype_to_emit(*i))
>      +	// This function type wasn't marked as one to be emitted
>      +	// so skip it.
>      +	continue;
>      +      o << "\n";
>      +      write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
>      +    }
> +
>
> with:
>
>      for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
>        {
> 	if ((ctxt.fntype_is_referenced(*i) && ctxt.type_is_emitted(*i))
> 	  // This function type, referenced by an emitted pointer or
> 	  // reference type, has already been emitted, so skip it.
> 	  continue;
> 	o << "\n";
> 	write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
>        }
I've followed your renames, but I've made the condition

if (!ctxt.fntype_is_referenced(*i) || ctxt.type_is_emitted(*i))

...and amended the comment accordingly...as otherwise it would let unreferenced fntypes slip through.
I also created a ctxt.clear_referenced_fntypes_map, since it needs to be cleared
along with the emitted_types_map, otherwise we'd get duplicates across TUs.


> The idea is to make the code reflect exactly what we are thinking
> about doing, which is, we want function types that are referenced by
> pointer (or reference) types to be emitted; other function types
> should not be emitted.
>
> [...]
>
> +static bool
> +write_function_type(const shared_ptr<function_type> decl, write_context& ctxt,
> +		    unsigned indent)
> +{
> [...]
>
> +  ctxt.record_fntype_as_emitted(decl);
>
> I'd replace this call by a call to the usual
> ctxt.record_type_as_emitted() that is used throughout the code:
>
>      ctxt.record_type_as_emitted(decl)
>
> I'd then remove the new member write_context::record_fntype_as_emitted()
> that you've added.
>
> Also, the new test material (you now, the new test files) need to be
> added to tests/data/Makefile.am, otherwise they are not added to the
> tarball by "make dist"; and "make distcheck" would fail anyhow.  As a
> general rule, all patches should pass "make distcheck".
My bad, sorry.
> Thanks!
>
The changes are in the usual place :)
Thanks for your patience!
  Ondrej