From e5cf9d1f600f3314ae915a6ba3e60529ba647589 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Fri, 24 Jul 2015 14:04:11 +0200 Subject: [PATCH] Consider default symbol versions when computing added/removed fns/vars When computing the set of added function or variable symbols, if a symbol S with no version symbol was present in a given corpus and that symbol gained a *DEFAULT* version V in the second corpus, we should not consider that a new symbol S was added (and that the former S was removed) because: 1/ S was already present in the first corpus 2/ applications linked to the first corpus and that were using S (with no version) there, will automatically use the S with version V in the second corpus, without needing any re-linking; the power of symbol versioning! Rather, it's just that S gained a default symbol version. This patch implements that. * include/abg-corpus.h (corpus::{lookup_function_symbol, lookup_variable_symbol}): Take a elf_symbol::version object, rather than a string representing the version. Add an overload that takes an elf_symbol. * src/abg-corpus.cc (find_symbol_by_version): New static function. (corpus::{lookup_function_symbol, lookup_variable_symbol}): Take a elf_symbol::version object, rather than a string representing the version. Add an overload that takes an elf_symbol. If the looked up symbol has no version and if the corpus contains a symbol with the same name and with a default version, then return that latter symbol if the corpus doesn't contain a symbol with the same name and empty version. * src/abg-comparison.cc (class_diff::ensure_lookup_tables_populated): Adjust. (corpus_diff::priv::ensure_lookup_tables_populated): Before deciding that a symbol has been added, if the symbol has a default version, make sure no symbol with the same name and without version was present in the former corpus. Similarly, before deciding that a symbol has been removed, if the symbol has no version, make sure the latter corpus has no symbol with the same name and with a default version. * tests/data/test-diff-dwarf/test12-report.txt: Adjust. The function should not be considered as added, because its symbol (and version) was already present in the former DSO. Signed-off-by: Dodji Seketeli --- include/abg-corpus.h | 10 +- src/abg-comparison.cc | 104 ++++++++++++++----- src/abg-corpus.cc | 93 +++++++++++++---- tests/data/test-diff-dwarf/test12-report.txt | 7 -- 4 files changed, 161 insertions(+), 53 deletions(-) diff --git a/include/abg-corpus.h b/include/abg-corpus.h index 82ad8d6f..ee0dcf1a 100644 --- a/include/abg-corpus.h +++ b/include/abg-corpus.h @@ -178,14 +178,20 @@ public: const elf_symbol_sptr lookup_function_symbol(const string& symbol_name, - const string& symbol_version) const; + const elf_symbol::version& version) const; + + const elf_symbol_sptr + lookup_function_symbol(const elf_symbol& symbol) const; const elf_symbol_sptr lookup_variable_symbol(const string& n) const; const elf_symbol_sptr lookup_variable_symbol(const string& symbol_name, - const string& symbol_version) const; + const elf_symbol::version& version) const; + + const elf_symbol_sptr + lookup_variable_symbol(const elf_symbol& symbol) const; const functions& get_functions() const; diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 47557c37..358dd80f 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -8283,8 +8283,7 @@ class_diff::ensure_lookup_tables_populated(void) const // symbols. if (!i->second->get_symbol() || (i->second->get_symbol() - && s->lookup_function_symbol(i->second->get_symbol()->get_name(), - i->second->get_symbol()->get_version().str()))) + && s->lookup_function_symbol(*i->second->get_symbol()))) to_delete.push_back(i->first); @@ -8301,8 +8300,7 @@ class_diff::ensure_lookup_tables_populated(void) const i != inserted_member_fns().end(); ++i) if (!i->second->get_symbol() - || f->lookup_function_symbol(i->second->get_symbol()->get_name(), - i->second->get_symbol()->get_version().str())) + || f->lookup_function_symbol(*i->second->get_symbol())) to_delete.push_back(i->first); for (vector::const_iterator i = to_delete.begin(); @@ -12779,8 +12777,7 @@ corpus_diff::priv::ensure_lookup_tables_populated() for (string_function_ptr_map::const_iterator i = deleted_fns_.begin(); i != deleted_fns_.end(); ++i) - if (second_->lookup_function_symbol(i->second->get_symbol()->get_name(), - i->second->get_symbol()->get_version().str())) + if (second_->lookup_function_symbol(*i->second->get_symbol())) to_delete.push_back(i->first); for (vector::const_iterator i = to_delete.begin(); @@ -12794,9 +12791,23 @@ corpus_diff::priv::ensure_lookup_tables_populated() for (string_function_ptr_map::const_iterator i = added_fns_.begin(); i != added_fns_.end(); ++i) - if (first_->lookup_function_symbol(i->second->get_symbol()->get_name(), - i->second->get_symbol()->get_version().str())) - to_delete.push_back(i->first); + { + if (first_->lookup_function_symbol(*i->second->get_symbol())) + to_delete.push_back(i->first); + else if (! i->second->get_symbol()->get_version().is_empty() + && i->second->get_symbol()->get_version().is_default()) + // We are looking for a symbol that has a default version, + // and which seems to be newly added. Let's see if the same + // symbol with *no* version was already present in the + // former corpus. If yes, then the symbol shouldn't be + // considered as 'added'. + { + elf_symbol::version empty_version; + if (first_->lookup_function_symbol(i->second->get_symbol()->get_name(), + empty_version)) + to_delete.push_back(i->first); + } + } for (vector::const_iterator i = to_delete.begin(); i != to_delete.end(); @@ -12870,8 +12881,7 @@ corpus_diff::priv::ensure_lookup_tables_populated() for (string_var_ptr_map::const_iterator i = deleted_vars_.begin(); i != deleted_vars_.end(); ++i) - if (second_->lookup_variable_symbol(i->second->get_symbol()->get_name(), - i->second->get_symbol()->get_version().str())) + if (second_->lookup_variable_symbol(*i->second->get_symbol())) to_delete.push_back(i->first); for (vector::const_iterator i = to_delete.begin(); @@ -12885,9 +12895,21 @@ corpus_diff::priv::ensure_lookup_tables_populated() for (string_var_ptr_map::const_iterator i = added_vars_.begin(); i != added_vars_.end(); ++i) - if (first_->lookup_variable_symbol(i->second->get_symbol()->get_name(), - i->second->get_symbol()->get_version().str())) + if (first_->lookup_variable_symbol(*i->second->get_symbol())) to_delete.push_back(i->first); + else if (! i->second->get_symbol()->get_version().is_empty() + && i->second->get_symbol()->get_version().is_default()) + // We are looking for a symbol that has a default version, + // and which seems to be newly added. Let's see if the same + // symbol with *no* version was already present in the + // former corpus. If yes, then the symbol shouldn't be + // considered as 'added'. + { + elf_symbol::version empty_version; + if (first_->lookup_variable_symbol(i->second->get_symbol()->get_name(), + empty_version)) + to_delete.push_back(i->first); + } for (vector::const_iterator i = to_delete.begin(); i != to_delete.end(); @@ -12908,8 +12930,7 @@ corpus_diff::priv::ensure_lookup_tables_populated() assert(i < first_->get_unreferenced_function_symbols().size()); elf_symbol_sptr deleted_sym = first_->get_unreferenced_function_symbols()[i]; - if (!second_->lookup_function_symbol(deleted_sym->get_name(), - deleted_sym->get_version())) + if (!second_->lookup_function_symbol(*deleted_sym)) deleted_unrefed_fn_syms_[deleted_sym->get_id_string()] = deleted_sym; } @@ -12929,10 +12950,27 @@ corpus_diff::priv::ensure_lookup_tables_populated() if ((deleted_unrefed_fn_syms_.find(added_sym->get_id_string()) == deleted_unrefed_fn_syms_.end())) { - if (!first_->lookup_function_symbol(added_sym->get_name(), - added_sym->get_version())) - added_unrefed_fn_syms_[added_sym->get_id_string()] = - added_sym; + if (!first_->lookup_function_symbol(*added_sym)) + { + bool do_add = true; + if (! added_sym->get_version().is_empty() + && added_sym->get_version().is_default()) + { + // So added_seem has a default version. If + // the former corpus had a symbol with the + // same name as added_sym but with *no* + // version, then added_sym shouldn't be + // considered as a newly added symbol. + elf_symbol::version empty_version; + if(first_->lookup_function_symbol(added_sym->get_name(), + empty_version)) + do_add = false; + } + + if (do_add) + added_unrefed_fn_syms_[added_sym->get_id_string()] = + added_sym; + } } else deleted_unrefed_fn_syms_.erase(added_sym->get_id_string()); @@ -12953,8 +12991,7 @@ corpus_diff::priv::ensure_lookup_tables_populated() assert(i < first_->get_unreferenced_variable_symbols().size()); elf_symbol_sptr deleted_sym = first_->get_unreferenced_variable_symbols()[i]; - if (!second_->lookup_variable_symbol(deleted_sym->get_name(), - deleted_sym->get_version())) + if (!second_->lookup_variable_symbol(*deleted_sym)) deleted_unrefed_var_syms_[deleted_sym->get_id_string()] = deleted_sym; } @@ -12974,10 +13011,27 @@ corpus_diff::priv::ensure_lookup_tables_populated() if (deleted_unrefed_var_syms_.find(added_sym->get_id_string()) == deleted_unrefed_var_syms_.end()) { - if (!first_->lookup_variable_symbol(added_sym->get_name(), - added_sym->get_version())) - added_unrefed_var_syms_[added_sym->get_id_string()] = - added_sym; + if (!first_->lookup_variable_symbol(*added_sym)) + { + bool do_add = true; + if (! added_sym->get_version().is_empty() + && added_sym->get_version().is_default()) + { + // So added_seem has a default version. If + // the former corpus had a symbol with the + // same name as added_sym but with *no* + // version, then added_sym shouldn't be + // considered as a newly added symbol. + elf_symbol::version empty_version; + if(first_->lookup_variable_symbol(added_sym->get_name(), + empty_version)) + do_add = false; + } + + if (do_add) + added_unrefed_var_syms_[added_sym->get_id_string()] = + added_sym; + } } else deleted_unrefed_var_syms_.erase(added_sym->get_id_string()); diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc index 394987d6..f5b34310 100644 --- a/src/abg-corpus.cc +++ b/src/abg-corpus.cc @@ -1485,18 +1485,62 @@ corpus::lookup_function_symbol(const string& n) const return it->second[0]; } +/// Look into a set of symbols and look for a symbol that has a given +/// version. +/// +/// This is a sub-routine for corpus::lookup_function_symbol() and +/// corpus::lookup_variable_symbol(). +/// +/// @param version the version of the symbol to look for. +/// +/// @param symbols the set of symbols to consider. +/// +/// @return the symbol found, or nil if none was found. +static const elf_symbol_sptr +find_symbol_by_version(const elf_symbol::version& version, + const vector& symbols) +{ + if (version.is_empty()) + { + // We are looing for a symbol with no version. + + // So first look for possible aliases with no version + for (elf_symbols::const_iterator s = symbols.begin(); + s != symbols.end(); + ++s) + if ((*s)->get_version().is_empty()) + return *s; + + // Or, look for a version that is a default one! + for (elf_symbols::const_iterator s = symbols.begin(); + s != symbols.end(); + ++s) + if ((*s)->get_version().is_default()) + return *s; + } + else + // We are looking for a symbol with a particular defined version. + for (elf_symbols::const_iterator s = symbols.begin(); + s != symbols.end(); + ++s) + if ((*s)->get_version().str() == version.str()) + return *s; + + return elf_symbol_sptr(); +} + /// Look in the function symbols map for a symbol with a given name. /// /// @param symbol_name the name of the symbol to look for. /// -/// @param symbol_version the version of the symbol to look for. +/// @param version the version of the symbol to look for. /// -/// return the symbol with the name @p symbol_name and with the name -/// @p symbol_version, or nil if no symbol has been found with that -/// name and version. +/// return the symbol with name @p symbol_name and with version @p +/// version, or nil if no symbol has been found with that name and +/// version. const elf_symbol_sptr corpus::lookup_function_symbol(const string& symbol_name, - const string& symbol_version) const + const elf_symbol::version& version) const { if (!get_fun_symbol_map_sptr()) return elf_symbol_sptr(); @@ -1506,14 +1550,19 @@ corpus::lookup_function_symbol(const string& symbol_name, if ( it == get_fun_symbol_map_sptr()->end()) return elf_symbol_sptr(); - for (elf_symbols::const_iterator s = it->second.begin(); - s != it->second.end(); - ++s) - if ((*s)->get_version().str() == symbol_version) - return *s; - return elf_symbol_sptr(); + return find_symbol_by_version(version, it->second); } +/// Look in the function symbols map for a symbol with the same name +/// and version as a given symbol. +/// +/// @param symbol the symbol to look for. +/// +/// return the symbol with the same name and version as @p symbol. +const elf_symbol_sptr +corpus::lookup_function_symbol(const elf_symbol& symbol) const +{return lookup_function_symbol(symbol.get_name(), symbol.get_version());} + /// Look in the variable symbols map for a symbol with a given name. /// /// @param n the name of the symbol to look for. @@ -1538,10 +1587,11 @@ corpus::lookup_variable_symbol(const string& n) const /// /// @param symbol_version the version of the symbol to look for. /// -/// return the first symbol with the name @p n. +/// return the first symbol with the name @p symbol_name and with +/// version @p version. const elf_symbol_sptr corpus::lookup_variable_symbol(const string& symbol_name, - const string& symbol_version) const + const elf_symbol::version& version) const { if (!get_var_symbol_map_sptr()) return elf_symbol_sptr(); @@ -1551,14 +1601,19 @@ corpus::lookup_variable_symbol(const string& symbol_name, if ( it == get_var_symbol_map_sptr()->end()) return elf_symbol_sptr(); - for (elf_symbols::const_iterator s = it->second.begin(); - s != it->second.end(); - ++s) - if ((*s)->get_version().str() == symbol_version) - return *s; - return elf_symbol_sptr(); + return find_symbol_by_version(version, it->second); } +/// Look in the variable symbols map for a symbol with the same name +/// and version as a given symbol. +/// +/// @param symbol the symbol to look for. +/// +/// return the symbol with the same name and version as @p symbol. +const elf_symbol_sptr +corpus::lookup_variable_symbol(const elf_symbol& symbol) const +{return lookup_variable_symbol(symbol.get_name(), symbol.get_version());} + /// Return the functions public decl table of the current corpus. /// /// The function public decl tables is a vector of all the functions diff --git a/tests/data/test-diff-dwarf/test12-report.txt b/tests/data/test-diff-dwarf/test12-report.txt index 31ae2196..e69de29b 100644 --- a/tests/data/test-diff-dwarf/test12-report.txt +++ b/tests/data/test-diff-dwarf/test12-report.txt @@ -1,7 +0,0 @@ -Functions changes summary: 0 Removed, 0 Changed, 1 Added function -Variables changes summary: 0 Removed, 0 Changed, 0 Added variable - -1 Added function: - - 'function int _foo3(int, int)' - -- 2.43.5