[applied] Bug rhbz#2182807 -- abipkgdiff crashes on missing debuginfo package

Message ID 87ttxul3hu.fsf@redhat.com
State New
Headers
Series [applied] Bug rhbz#2182807 -- abipkgdiff crashes on missing debuginfo package |

Commit Message

Dodji Seketeli April 5, 2023, 4:04 p.m. UTC
  Hello,

When abipkgdiff is called with a debug info package that references an
alternate debug info file that is not found -- because debug info
package is missing from the command line -- the program aborts.  This
is because the libabigail library is further invoked by the tool with
debuginfo in an inconsistent state (missing alternate debug info).

Note however that abipkgdiff only emits an explanatory message when
invoked with the --verbose option.

This patch teaches abipkgdiff to emit explanatory messages when an
alternate debug info file is not found.  The message suggests that the
user adds the missing RPM package (which contains the alternate
missing debuginfo file) to the command line using the --d1/--d2
switches.

	* src/abg-fe-iface.cc (status_to_diagnostic_string): Remove the
	newline from the end of the returned diagnostic string.
	* tools/abipkgdiff.cc (get_pretty_printed_list_of_packages)
	(emit_alt_debug_info_not_found_error): Define new static
	functions.
	(compare, compare_to_self): Add an ostream& parameter as a
	destination of diagnostic messages.  If the
	abigail::fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND bit is set in
	the status code, emit the explanatory message to output stream
	associated to the current comparison task.  Remove the previous
	handling of this case.
	(compare_task::maybe_emit_pretty_error_message_to_output): Define
	new member function to get the output stream associated to the
	current task massage it and stick to result into the pretty output
	string to be emitted to the user in the end, namely the
	compare_task::pretty_output string data member.
	({compares, self_compare}_task::perform): Adjust this to call the
	new maybe_emit_pretty_error_message_to_output in case of error.
	* tests/data/test-diff-pkg/libxfce4ui-devel-4.12.1-8.fc27.ppc64-self-report-0.txt:
	Adjust.
	* tests/data/test-diff-pkg/test-dbus-glib-0.80-3.fc12.x86_64-report-0.txt:
	Likewise.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-fe-iface.cc                           |   6 +-
 ...evel-4.12.1-8.fc27.ppc64-self-report-0.txt |  11 +-
 ...-dbus-glib-0.80-3.fc12.x86_64-report-0.txt |   2 +
 tools/abipkgdiff.cc                           | 288 ++++++++++--------
 4 files changed, 173 insertions(+), 134 deletions(-)
  

Patch

diff --git a/src/abg-fe-iface.cc b/src/abg-fe-iface.cc
index bc272425..6ec50bfd 100644
--- a/src/abg-fe-iface.cc
+++ b/src/abg-fe-iface.cc
@@ -397,13 +397,13 @@  status_to_diagnostic_string(fe_iface::status s)
   std::string str;
 
   if (s & fe_iface::STATUS_DEBUG_INFO_NOT_FOUND)
-    str += "could not find debug info\n";
+    str += "could not find debug info";
 
   if (s & fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND)
-    str += "could not find alternate debug info\n";
+    str += "could not find alternate debug info";
 
   if (s & fe_iface::STATUS_NO_SYMBOLS_FOUND)
-    str += "could not load ELF symbols\n";
+    str += "could not load ELF symbols";
 
   return str;
 }
diff --git a/tests/data/test-diff-pkg/libxfce4ui-devel-4.12.1-8.fc27.ppc64-self-report-0.txt b/tests/data/test-diff-pkg/libxfce4ui-devel-4.12.1-8.fc27.ppc64-self-report-0.txt
index 61169111..0188bc9b 100644
--- a/tests/data/test-diff-pkg/libxfce4ui-devel-4.12.1-8.fc27.ppc64-self-report-0.txt
+++ b/tests/data/test-diff-pkg/libxfce4ui-devel-4.12.1-8.fc27.ppc64-self-report-0.txt
@@ -1,4 +1,7 @@ 
-abipkgdiff: Could not find alternate debug info file: ../../../../.dwz/libxfce4ui-4.12.1-8.fc27.ppc64
-==== Error happened during processing of 'libxfce4uiglade.so' ====
-could not find alternate debug info
-==== End of error for 'libxfce4uiglade.so' ====
+abipkgdiff: ==== Error happened during processing of 'libxfce4uiglade.so' ====
+abipkgdiff: could not find alternate debug info:
+abipkgdiff: While reading elf file 'libxfce4uiglade.so', could not find alternate debug info in provided debug info package(s) '/home/dodji/git/libabigail/fixes/tests/data/test-diff-pkg/libxfce4ui-devel-debuginfo-4.12.1-8.fc27.ppc64.rpm'
+abipkgdiff: The alternate debug info file being looked for is: ../../../../.dwz/libxfce4ui-4.12.1-8.fc27.ppc64
+abipkgdiff: You must provide the additional debug info package that contains that alternate debug info file, using an additional --d1/--d2 switch
+abipkgdiff: ==== End of error for 'libxfce4uiglade.so' ====
+
diff --git a/tests/data/test-diff-pkg/test-dbus-glib-0.80-3.fc12.x86_64-report-0.txt b/tests/data/test-diff-pkg/test-dbus-glib-0.80-3.fc12.x86_64-report-0.txt
index e69de29b..5cb62b12 100644
--- a/tests/data/test-diff-pkg/test-dbus-glib-0.80-3.fc12.x86_64-report-0.txt
+++ b/tests/data/test-diff-pkg/test-dbus-glib-0.80-3.fc12.x86_64-report-0.txt
@@ -0,0 +1,2 @@ 
+No ABI change detected
+No ABI change detected
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index 2765284e..d10706a9 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -281,6 +281,9 @@  get_interesting_files_under_dir(const string	dir,
 				options&	opts,
 				vector<string>& interesting_files);
 
+static string
+get_pretty_printed_list_of_packages(const vector<string>& packages);
+
 /// Abstract ELF files from the packages which ABIs ought to be
 /// compared
 class elf_file
@@ -1302,6 +1305,68 @@  set_generic_options(abigail::elf_based_reader& rdr, const options& opts)
     opts.assume_odr_for_cplusplus;
 }
 
+/// Emit an error message on standard error about alternate debug info
+/// not being found.
+///
+/// @param reader the ELF based reader being used.
+///
+/// @param elf_file the ELF file being looked at.
+///
+/// @param opts the options passed to the tool.
+///
+/// @param is_old_package if this is true, then we are looking at the
+/// first (the old) package of the comparison.  Otherwise, we are
+/// looking at the second (the newest) package of the comparison.
+static void
+emit_alt_debug_info_not_found_error(abigail::elf_based_reader&	reader,
+				    const elf_file&		elf_file,
+				    const options&		opts,
+				    ostream&			out,
+				    bool			is_old_package)
+{
+  ABG_ASSERT(is_old_package
+	     ? !opts.debug_packages1.empty()
+	     : !opts.debug_packages2.empty());
+
+  string filename;
+  tools_utils::base_name(elf_file.path, filename);
+  emit_prefix("abipkgdiff", out)
+    << "While reading elf file '"
+    << filename
+    << "', could not find alternate debug info in provided "
+    "debug info package(s) "
+    << get_pretty_printed_list_of_packages(is_old_package
+					   ? opts.debug_packages1
+					   : opts.debug_packages2)
+    << "\n";
+
+  string alt_di_path;
+#ifdef WITH_CTF
+  if (opts.use_ctf)
+    ;
+  else
+#endif
+#ifdef WITH_BTF
+    if (opts.use_btf)
+      ;
+    else
+#endif
+      reader.refers_to_alt_debug_info(alt_di_path);
+  if (!alt_di_path.empty())
+    {
+      emit_prefix("abipkgdiff", out)
+	<<  "The alternate debug info file being looked for is: "
+	<< alt_di_path << "\n";
+    }
+  else
+    emit_prefix("abipkgdiff", out) << "\n";
+
+  emit_prefix("abipkgdiff", out)
+    << "You must provide the additional "
+    << "debug info package that contains that alternate "
+    << "debug info file, using an additional --d1/--d2 switch\n";
+}
+
 /// Compare the ABI two elf files, using their associated debug info.
 ///
 /// The result of the comparison is emitted to standard output.
@@ -1341,6 +1406,7 @@  compare(const elf_file&		elf1,
 	abigail::ir::environment&	env,
 	corpus_diff_sptr&		diff,
 	diff_context_sptr&		ctxt,
+	ostream&			out,
 	abigail::fe_iface::status*	detailed_error_status = 0)
 {
   char *di_dir1 = (char*) debug_dir1.c_str(),
@@ -1437,6 +1503,15 @@  compare(const elf_file&		elf1,
 	bail_out = true;
       }
 
+    if (c1_status & abigail::fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND)
+      {
+	emit_alt_debug_info_not_found_error(*reader, elf1, opts, out,
+					    /*is_old_package=*/true);
+	if (detailed_error_status)
+	  *detailed_error_status = c1_status;
+	bail_out = true;
+      }
+
     if (opts.fail_if_no_debug_info)
       {
 	bool debug_info_error = false;
@@ -1457,36 +1532,6 @@  compare(const elf_file&		elf1,
 	    debug_info_error = true;
 	  }
 
-	if (c1_status & abigail::fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND)
-	  {
-	    if (opts.verbose)
-	      emit_prefix("abipkgdiff", cerr)
-		<< "while reading file" << elf1.path << "\n";
-
-	    emit_prefix("abipkgdiff", cerr)
-	      << "Could not find alternate debug info file";
-	    string alt_di_path;
-#ifdef WITH_CTF
-            if (opts.use_ctf)
-              ;
-            else
-#endif
-#ifdef WITH_BTF
-	      if (opts.use_btf)
-		;
-	      else
-#endif
-	      reader->refers_to_alt_debug_info(alt_di_path);
-	    if (!alt_di_path.empty())
-	      cerr << ": " << alt_di_path << "\n";
-	    else
-	      cerr << "\n";
-
-	    if (detailed_error_status)
-	      *detailed_error_status = c1_status;
-	    debug_info_error = true;
-	  }
-
 	if (debug_info_error)
 	  bail_out = true;
       }
@@ -1547,6 +1592,15 @@  compare(const elf_file&		elf1,
 	bail_out = true;
       }
 
+    if (c2_status & abigail::fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND)
+      {
+	emit_alt_debug_info_not_found_error(*reader, elf2, opts, out,
+					    /*is_old_package=*/false);
+	if (detailed_error_status)
+	  *detailed_error_status = c2_status;
+	bail_out = true;
+      }
+
     if (opts.fail_if_no_debug_info)
       {
 	bool debug_info_error = false;
@@ -1567,36 +1621,6 @@  compare(const elf_file&		elf1,
 	    debug_info_error = true;
 	  }
 
-	if (c2_status & abigail::fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND)
-	  {
-	    if (opts.verbose)
-	      emit_prefix("abipkgdiff", cerr)
-		<< "while reading file" << elf2.path << "\n";
-
-	    emit_prefix("abipkgdiff", cerr)
-	      << "Could not find alternate debug info file";
-	    string alt_di_path;
-#ifdef WITH_CTF
-            if (opts.use_ctf)
-              ;
-            else
-#endif
-#ifdef WITH_BTF
-            if (opts.use_btf)
-              ;
-            else
-#endif
-	      reader->refers_to_alt_debug_info(alt_di_path);
-	    if (!alt_di_path.empty())
-	      cerr << ": " << alt_di_path << "\n";
-	    else
-	      cerr << "\n";
-
-	    if (detailed_error_status)
-	      *detailed_error_status = c2_status;
-	    debug_info_error = true;
-	  }
-
 	if (debug_info_error)
 	  bail_out = true;
       }
@@ -1661,6 +1685,7 @@  compare_to_self(const elf_file&		elf,
 		abigail::ir::environment&	env,
 		corpus_diff_sptr&		diff,
 		diff_context_sptr&		ctxt,
+		ostream&			out,
 		abigail::fe_iface::status*	detailed_error_status = 0)
 {
   char *di_dir = (char*) debug_dir.c_str();
@@ -1718,15 +1743,10 @@  compare_to_self(const elf_file&		elf,
       }
     else if (c_status & abigail::fe_iface::STATUS_ALT_DEBUG_INFO_NOT_FOUND)
       {
-	if (opts.verbose)
-	  emit_prefix("abipkgdiff", cerr)
-	    << "Could not read find alternate DWARF debug info for '"
-	    << elf.path
-	    << "'.  You might have forgotten to provide a debug info package\n";
-
+	emit_alt_debug_info_not_found_error(*reader, elf, opts, out,
+					    /*is_old_package=*/true);
 	if (detailed_error_status)
 	  *detailed_error_status = c_status;
-
 	return abigail::tools_utils::ABIDIFF_ERROR;
       }
 
@@ -2160,28 +2180,10 @@  public:
       status(abigail::tools_utils::ABIDIFF_OK)
   {}
 
-  /// The job performed by the task.
-  ///
-  /// This compares two ELF files, gets the resulting test report and
-  /// stores it in an output stream.
-  virtual void
-  perform()
+  void
+  maybe_emit_pretty_error_message_to_output(const corpus_diff_sptr& diff,
+					    abigail::fe_iface::status detailed_status)
   {
-    abigail::ir::environment env;
-    diff_context_sptr ctxt;
-    corpus_diff_sptr diff;
-
-    abigail::fe_iface::status detailed_status =
-      abigail::fe_iface::STATUS_UNKNOWN;
-
-    if (args->opts.exported_interfaces_only.has_value())
-      env.analyze_exported_interfaces_only
-	(*args->opts.exported_interfaces_only);
-
-    status |= compare(args->elf1, args->debug_dir1, args->private_types_suppr1,
-		      args->elf2, args->debug_dir2, args->private_types_suppr2,
-		      args->opts, env, diff, ctxt, &detailed_status);
-
     // If there is an ABI change, tell the user about it.
     if ((status & abigail::tools_utils::ABIDIFF_ABI_CHANGE)
 	||( diff && diff->has_net_changes()))
@@ -2198,7 +2200,10 @@  public:
     else
       {
 	if (args->opts.show_identical_binaries)
-	  out << "No ABI change detected\n";
+	  {
+	    out << "No ABI change detected\n";
+	    pretty_output += out.str();
+	  }
       }
 
     // If an error happened while comparing the two binaries, tell the
@@ -2212,13 +2217,47 @@  public:
 	    "Unknown error.  Please run the tool again with --verbose\n";
 
 	string name = args->elf1.name;
-	pretty_output +=
-	  "==== Error happened during processing of '" + name + "' ====\n";
-	pretty_output += diagnostic;
-	pretty_output +=
-	  "==== End of error for '" + name + "' ====\n";
+	std::stringstream o;
+	emit_prefix("abipkgdiff", o)
+	  << "==== Error happened during processing of '"
+	  << name
+	  << "' ====\n";
+	emit_prefix("abipkgdiff", o)
+	  << diagnostic
+	  << ":\n"
+	  << out.str();
+	emit_prefix("abipkgdiff", o)
+	  << "==== End of error for '"
+	  << name
+	  << "' ====\n\n";
+	pretty_output += o.str();
       }
   }
+
+  /// The job performed by the task.
+  ///
+  /// This compares two ELF files, gets the resulting test report and
+  /// stores it in an output stream.
+  virtual void
+  perform()
+  {
+    abigail::ir::environment env;
+    diff_context_sptr ctxt;
+    corpus_diff_sptr diff;
+
+    abigail::fe_iface::status detailed_status =
+      abigail::fe_iface::STATUS_UNKNOWN;
+
+    if (args->opts.exported_interfaces_only.has_value())
+      env.analyze_exported_interfaces_only
+	(*args->opts.exported_interfaces_only);
+
+    status |= compare(args->elf1, args->debug_dir1, args->private_types_suppr1,
+		      args->elf2, args->debug_dir2, args->private_types_suppr2,
+		      args->opts, env, diff, ctxt, out, &detailed_status);
+
+    maybe_emit_pretty_error_message_to_output(diff, detailed_status);
+  }
 }; // end class compare_task
 
 /// Convenience typedef for a shared_ptr of @ref compare_task.
@@ -2252,44 +2291,14 @@  public:
       abigail::fe_iface::STATUS_UNKNOWN;
 
     status |= compare_to_self(args->elf1, args->debug_dir1,
-			      args->opts, env, diff, ctxt,
+			      args->opts, env, diff, ctxt, out,
 			      &detailed_status);
 
     string name = args->elf1.name;
     if (status == abigail::tools_utils::ABIDIFF_OK)
       pretty_output += "==== SELF CHECK SUCCEEDED for '"+ name + "' ====\n";
-    else if ((status & abigail::tools_utils::ABIDIFF_ABI_CHANGE)
-	     ||( diff && diff->has_net_changes()))
-      {
-	// There is an ABI change, tell the user about it.
-	diff->report(out, /*indent=*/"  ");
-
-	pretty_output +=
-	  string("======== comparing'") + name +
-	  "' to itself wrongly yielded result: ===========\n"
-	  + out.str()
-	  + "===SELF CHECK FAILED for '"+ name + "'\n";
-      }
-
-    // If an error happened while comparing the two binaries, tell the
-    // user about it.
-    if (status & abigail::tools_utils::ABIDIFF_ERROR)
-      {
-	string diagnostic =
-	  abigail::status_to_diagnostic_string(detailed_status);
-
-	if (diagnostic.empty())
-	  diagnostic =
-	    "Unknown error.  Please run the tool again with --verbose\n";
-
-	string name = args->elf1.name;
-	pretty_output +=
-	  "==== Error happened during self check of '" + name + "' ====\n";
-	pretty_output += diagnostic;
-	pretty_output +=
-	  "==== SELF CHECK FAILED for '" + name + "' ====\n";
-
-      }
+    else
+      maybe_emit_pretty_error_message_to_output(diff, detailed_status);
   }
 }; // end class self_compare
 
@@ -2396,6 +2405,31 @@  get_interesting_files_under_dir(const string dir,
   return is_ok;
 }
 
+/// Return a string representing a list of packages that can be
+/// printed out to the user.
+///
+/// @param packages a vector of package names
+///
+/// @return a string representing the list of packages @p packages.
+static string
+get_pretty_printed_list_of_packages(const vector<string>& packages)
+{
+  if (packages.empty())
+    return string();
+
+  bool need_comma = false;
+  std::stringstream o;
+  for (auto p : packages)
+    {
+      if (need_comma)
+	o << ", ";
+      else
+	need_comma = true;
+      o << "'" << p << "'";
+    }
+  return o.str();
+}
+
 /// Create maps of the content of a given package.
 ///
 /// The maps contain relevant metadata about the content of the