diff mbox series

suppression: Better handle soname/filename properties evaluation

Message ID 865zfzzzr4.fsf@seketeli.org
State Committed
Headers
Series suppression: Better handle soname/filename properties evaluation |

Commit Message

Dodji Seketeli Jan. 1, 2020, midnight UTC 
  At the moment, we don't make any difference between these two cases:

  1/ A suppression specification has no soname or file name related
  property.

  2/ A suppression specification has a soname or file name related
  property that doesn't match the soname or file name of a given
  corpus.

In both cases 1/ and 2/ libabigail currently assumes that the
suppression specification does not match the given corpus we are
looking at.  This can be wrong, especially if we are in the case 1/
and if the suppression does have other properties that should make it
match the corpus.

This patch fixes this issue.

	* include/abg-suppression.h
	(suppression_base::has_{soname,file_name}_related_property): Add
	new member functions.
	* src/abg-dwarf-reader.cc (read_context::suppression_can_match):
	Fix the logic to make a difference between the case where the
	suppression doesn't have any soname/filename property and the case
	where the suppression does have a soname/filename property that
	does not match the current binary.
	* src/abg-reader.cc (read_context::suppression_can_match):
	Likewise.
	* src/abg-suppression-priv.h
	(suppression_base::priv::matches_soname): If the suppression does
	not have any soname related property then it doesn't match the
	soname we are looking at.
	(suppression_base::priv::matches_binary_name): If the suppression
	does not have any filename related property then it doesn't match
	the filename we are looking at.
	* src/abg-suppression.cc
	(suppression_base::has_{soname,file_name}_related_property):
	Define new member functions.
	(sonames_of_binaries_match): If the suppression does not have any
	soname related property then it doesn't match the corpora of the
	diff we are looking at.
	(names_of_binaries_match): If the suppression does not have any
	filename related property then it doesn't match the corpora of the
	diff we are looking at.
	(type_suppression::suppresses_type): Fix the logic to make a
	difference between the case where the suppression doesn't have any
	soname/filename property and the case where the suppression does
	have a soname/filename property that does not match the current
	binary.
	(function_suppression::suppresses_{function, function_symbol}):
	Likewise.
	(variable_suppression::suppresses_{variable, variable_symbol}):
	Likewise.
	(file_suppression::suppresses_file): Likewise.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 include/abg-suppression.h  |   6 +++
 src/abg-dwarf-reader.cc    |  30 +++++++++---
 src/abg-reader.cc          |  20 ++++++--
 src/abg-suppression-priv.h |  58 +++++++++++++++++++----
 src/abg-suppression.cc     | 113 ++++++++++++++++++++++++++++++++++-----------
 5 files changed, 180 insertions(+), 47 deletions(-)
diff mbox series

Patch

diff --git a/include/abg-suppression.h b/include/abg-suppression.h
index 02f3fc4..05709d6 100644
--- a/include/abg-suppression.h
+++ b/include/abg-suppression.h
@@ -97,6 +97,9 @@  public:
   const string&
   get_file_name_not_regex_str() const;
 
+  bool
+  has_file_name_related_property() const;
+
   void
   set_soname_regex_str(const string& regexp);
 
@@ -109,6 +112,9 @@  public:
   const string&
   get_soname_not_regex_str() const;
 
+  bool
+  has_soname_related_property() const;
+
   virtual bool
   suppresses_diff(const diff*) const = 0;
 
diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 436f610..0697c7a 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -8820,17 +8820,35 @@  public:
   /// Tests if a suppression specification can match ABI artifacts
   /// coming from the binary being analyzed.
   ///
-  /// This tests if the suppression matches the soname of and binary
-  /// name of the ELF binary being analyzed.
+  /// This tests if the suppression can match the soname of and binary
+  /// name of the ELF binary being analyzed.  More precisely, if there
+  /// are any soname or file name property in the suppression and if
+  /// those do *NOT* match the current binary, then the function
+  /// returns false.
   ///
   /// @param s the suppression specification to consider.
+  ///
+  /// @return true iff either there are no soname/filename related
+  /// property on the suppression, or if none of the soname/filename
+  /// properties of the suppression match the current binary.
   bool
   suppression_can_match(const suppr::suppression_base& s) const
   {
-    if (s.priv_->matches_soname(dt_soname())
-	&& s.priv_->matches_binary_name(elf_path()))
-      return true;
-    return false;
+    if (!s.priv_->matches_soname(dt_soname()))
+      if (s.has_soname_related_property())
+	// The suppression has some SONAME related properties, but
+	// none of them match the SONAME of the current binary.  So
+	// the suppression cannot match the current binary.
+	return false;
+
+    if (!s.priv_->matches_binary_name(elf_path()))
+      if (s.has_file_name_related_property())
+	// The suppression has some file_name related properties, but
+	// none of them match the file name of the current binary.  So
+	// the suppression cannot match the current binary.
+	return false;
+
+    return true;
   }
 
   /// Test whether if a given function suppression matches a function
diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 30b9abc..7c8a56a 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -948,10 +948,22 @@  public:
   suppression_can_match(const suppr::suppression_base& s) const
   {
     corpus_sptr corp = get_corpus();
-    if (s.priv_->matches_soname(corp->get_soname())
-	&& s.priv_->matches_binary_name(corp->get_path()))
-      return true;
-    return false;
+
+    if (!s.priv_->matches_soname(corp->get_soname()))
+      if (s.has_soname_related_property())
+	// The suppression has some SONAME related properties, but
+	// none of them match the SONAME of the current binary.  So
+	// the suppression cannot match the current binary.
+	return false;
+
+    if (!s.priv_->matches_binary_name(corp->get_path()))
+      if (s.has_file_name_related_property())
+	// The suppression has some file_name related properties, but
+	// none of them match the file name of the current binary.  So
+	// the suppression cannot match the current binary.
+	return false;
+
+    return true;
   }
 
   /// Test whether if a given function suppression matches a function
diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h
index 449814c..d872168 100644
--- a/src/abg-suppression-priv.h
+++ b/src/abg-suppression-priv.h
@@ -179,32 +179,72 @@  public:
     return soname_not_regex_;
   }
 
+  /// Test if the current suppression matches a given SONAME.
+  ///
+  /// @param soname the SONAME to consider.
+  ///
+  /// @return true iff the suppression matches the SONAME denoted by
+  /// @p soname.
+  ///
+  /// Note that if the suppression contains no property that is
+  /// related to SONAMEs, the function returns false.
   bool
   matches_soname(const string& soname) const
   {
+    bool has_regexp = false;
     if (sptr_utils::regex_t_sptr regexp = get_soname_regex())
-      if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) != 0)
-	return false;
+      {
+	has_regexp = true;
+	if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) != 0)
+	  return false;
+      }
 
     if (sptr_utils::regex_t_sptr regexp = get_soname_not_regex())
-      if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) == 0)
+      {
+	has_regexp = true;
+	if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) == 0)
+	  return false;
+      }
+
+      if (!has_regexp)
 	return false;
 
     return true;
   }
 
+  /// Test if the current suppression matches the full file path to a
+  /// given binary.
+  ///
+  /// @param binary_name the full path to the binary.
+  ///
+  /// @return true iff the suppression matches the path denoted by @p
+  /// binary_name.
+  ///
+  /// Note that if the suppression contains no property that is
+  /// related to file name, the function returns false.
   bool
   matches_binary_name(const string& binary_name) const
   {
+    bool has_regexp = false;
+
     if (sptr_utils::regex_t_sptr regexp = get_file_name_regex())
-      if (regexec(regexp.get(), binary_name.c_str(),
-		  0, NULL, 0) != 0)
-	return false;
+      {
+	has_regexp = true;
+	if (regexec(regexp.get(), binary_name.c_str(),
+	  0, NULL, 0) != 0)
+	  return false;
+      }
 
     if (sptr_utils::regex_t_sptr regexp = get_file_name_not_regex())
-      if (regexec(regexp.get(), binary_name.c_str(),
-		  0, NULL, 0) == 0)
-	return false;
+      {
+	has_regexp = true;
+	if (regexec(regexp.get(), binary_name.c_str(),
+	  0, NULL, 0) == 0)
+	  return false;
+      }
+
+    if (!has_regexp)
+      return false;
 
     return true;
   }
diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index 05d3858..663749d 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -184,6 +184,18 @@  const string&
 suppression_base::get_file_name_not_regex_str() const
 {return priv_->file_name_not_regex_str_;}
 
+/// Test if the current suppression has a property related to file
+/// name.
+///
+/// @return true iff the current suppression has either a
+/// file_name_regex or a file_name_not_regex property.
+bool
+suppression_base::has_file_name_related_property() const
+{
+  return (!(get_file_name_regex_str().empty()
+	    && get_file_name_not_regex_str().empty()));
+}
+
 /// Setter of the "soname_regex_str property of the current instance
 /// of @ref suppression_base.
 ///
@@ -234,6 +246,17 @@  const string&
 suppression_base::get_soname_not_regex_str() const
 {return priv_->soname_not_regex_str_;}
 
+/// Test if the current suppression has a property related to SONAMEs.
+///
+/// @return true iff the current suppression has either a soname_regex
+/// or a soname_not_regex property.
+bool
+suppression_base::has_soname_related_property() const
+{
+  return (!(get_soname_regex_str().empty()
+	    && get_soname_not_regex_str().empty()));
+}
+
 /// Check if the SONAMEs of the two binaries being compared match the
 /// content of the properties "soname_regexp" and "soname_not_regexp"
 /// of the current suppression specification.
@@ -254,6 +277,9 @@  sonames_of_binaries_match(const suppression_base& suppr,
   string first_soname = ctxt.get_corpus_diff()->first_corpus()->get_soname(),
     second_soname = ctxt.get_corpus_diff()->second_corpus()->get_soname();
 
+  if (!suppr.has_soname_related_property())
+    return false;
+
   if (!suppr.priv_->matches_soname(first_soname)
       && !suppr.priv_->matches_soname(second_soname))
     return false;
@@ -281,6 +307,9 @@  names_of_binaries_match(const suppression_base& suppr,
   string first_binary_path = ctxt.get_corpus_diff()->first_corpus()->get_path(),
     second_binary_path = ctxt.get_corpus_diff()->second_corpus()->get_path();
 
+  if (!suppr.has_file_name_related_property())
+    return false;
+
   if (!suppr.priv_->matches_binary_name(first_binary_path)
       && !suppr.priv_->matches_binary_name(second_binary_path))
     return false;
@@ -850,17 +879,18 @@  type_suppression::suppresses_type(const type_base_sptr& type,
 {
   if (ctxt)
     {
-      // Check if the names of the binaries match
+      // Check if the names of the binaries match the suppression
       if (!names_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_file_name_related_property())
+	  return false;
 
-      // Check if the sonames of the binaries match
+      // Check if the sonames of the binaries match the suppression
       if (!sonames_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_soname_related_property())
+	  return false;
     }
 
   return suppresses_type(type);
-
 }
 
 /// Test if an instance of @ref type_suppression matches a given type.
@@ -2419,16 +2449,19 @@  function_suppression::suppresses_function(const function_decl* fn,
   if (!(get_change_kind() & k))
     return false;
 
-  // Check if the name and soname of the binaries match
+  // Check if the name and soname of the binaries match the current
+  // suppr spec
   if (ctxt)
     {
-      // Check if the name of the binaries match
+      // Check if the name of the binaries match the current suppr spec
       if (!names_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_file_name_related_property())
+	  return false;
 
-      // Check if the soname of the binaries match
+      // Check if the soname of the binaries match the current suppr spec
       if (!sonames_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_soname_related_property())
+	  return false;
     }
 
   string fname = fn->get_qualified_name();
@@ -2752,16 +2785,21 @@  function_suppression::suppresses_function_symbol(const elf_symbol* sym,
   ABG_ASSERT(k & function_suppression::ADDED_FUNCTION_CHANGE_KIND
 	 || k & function_suppression::DELETED_FUNCTION_CHANGE_KIND);
 
-  // Check if the name and soname of the binaries match
+  // Check if the name and soname of the binaries match the current
+  // suppr spect
   if (ctxt)
     {
-      // Check if the name of the binaries match
+      // Check if the name of the binaries match the current
+      // suppr spect
       if (!names_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_file_name_related_property())
+	  return false;
 
-      // Check if the soname of the binaries match
+      // Check if the soname of the binaries match the current
+      // suppr spect
       if (!sonames_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_soname_related_property())
+	  return false;
     }
 
   string sym_name = sym->get_name(), sym_version = sym->get_version().str();
@@ -3715,13 +3753,17 @@  variable_suppression::suppresses_variable(const var_decl* var,
   // Check if the name and soname of the binaries match
   if (ctxt)
     {
-      // Check if the name of the binaries match
+      // Check if the name of the binaries match the current
+      // suppr spec
       if (!names_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_file_name_related_property())
+	  return false;
 
-      // Check if the soname of the binaries match
+      // Check if the soname of the binaries match the current suppr
+      // spec
       if (!sonames_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_soname_related_property())
+	  return false;
     }
 
   string var_name = var->get_qualified_name();
@@ -3871,16 +3913,20 @@  variable_suppression::suppresses_variable_symbol(const elf_symbol* sym,
   ABG_ASSERT(k & ADDED_VARIABLE_CHANGE_KIND
 	 || k & DELETED_VARIABLE_CHANGE_KIND);
 
-  // Check if the name and soname of the binaries match
+  // Check if the name and soname of the binaries match the current
+  // suppr spec.
   if (ctxt)
     {
-      // Check if the name of the binaries match
+      // Check if the name of the binaries match the current suppr
+      // spec
       if (!names_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_file_name_related_property())
+	  return false;
 
-      // Check if the soname of the binaries match
+      // Check if the soname of the binaries match the current suppr spec
       if (!sonames_of_binaries_match(*this, *ctxt))
-	return false;
+	if (has_soname_related_property())
+	  return false;
     }
 
   string sym_name = sym->get_name(), sym_version = sym->get_version().str();
@@ -4227,15 +4273,26 @@  file_suppression::suppresses_file(const string& file_path)
   string fname;
   tools_utils::base_name(file_path, fname);
 
+  bool has_regexp = false;
+
   if (sptr_utils::regex_t_sptr regexp =
       suppression_base::priv_->get_file_name_regex())
-    if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) != 0)
-      return false;
+    {
+      has_regexp = true;
+      if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) != 0)
+	return false;
+    }
 
   if (sptr_utils::regex_t_sptr regexp =
       suppression_base::priv_->get_file_name_not_regex())
-    if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) == 0)
-      return false;
+    {
+      has_regexp = true;
+      if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) == 0)
+	return false;
+    }
+
+  if (!has_regexp)
+    return false;
 
   return true;
 }