* Support for function pointers and references @ 2015-01-01 0:00 Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Ondrej Oprala @ 2015-01-01 0:00 UTC (permalink / raw) To: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 Support for function pointers and references Ondrej Oprala @ 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Dodji Seketeli 2 siblings, 1 reply; 10+ messages in thread From: Dodji Seketeli @ 2015-01-01 0:00 UTC (permalink / raw) To: Ondrej Oprala; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 ` Dodji Seketeli @ 2015-01-01 0:00 ` Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 0 siblings, 1 reply; 10+ messages in thread From: Ondrej Oprala @ 2015-01-01 0:00 UTC (permalink / raw) To: Dodji Seketeli; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 ` Ondrej Oprala @ 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 0 siblings, 1 reply; 10+ messages in thread From: Dodji Seketeli @ 2015-01-01 0:00 UTC (permalink / raw) To: Ondrej Oprala; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 ` Dodji Seketeli @ 2015-01-01 0:00 ` Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 0 siblings, 1 reply; 10+ messages in thread From: Ondrej Oprala @ 2015-01-01 0:00 UTC (permalink / raw) To: Dodji Seketeli; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 ` Ondrej Oprala @ 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 0 siblings, 1 reply; 10+ messages in thread From: Dodji Seketeli @ 2015-01-01 0:00 UTC (permalink / raw) To: Ondrej Oprala; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 ` Dodji Seketeli @ 2015-01-01 0:00 ` Ondrej Oprala 0 siblings, 0 replies; 10+ messages in thread From: Ondrej Oprala @ 2015-01-01 0:00 UTC (permalink / raw) To: Dodji Seketeli; +Cc: libabigail 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! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 Support for function pointers and references Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli @ 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Dodji Seketeli 2 siblings, 0 replies; 10+ messages in thread From: Dodji Seketeli @ 2015-01-01 0:00 UTC (permalink / raw) To: Ondrej Oprala; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 Support for function pointers and references Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Dodji Seketeli @ 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 2 siblings, 1 reply; 10+ messages in thread From: Dodji Seketeli @ 2015-01-01 0:00 UTC (permalink / raw) To: Ondrej Oprala; +Cc: libabigail 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for function pointers and references 2015-01-01 0:00 ` Dodji Seketeli @ 2015-01-01 0:00 ` Ondrej Oprala 0 siblings, 0 replies; 10+ messages in thread From: Ondrej Oprala @ 2015-01-01 0:00 UTC (permalink / raw) To: Dodji Seketeli; +Cc: libabigail 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, > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-09-30 19:29 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-01 0:00 Support for function pointers and references Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Dodji Seketeli 2015-01-01 0:00 ` Ondrej Oprala
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).