diff mbox series

[applied,to,mainline] abicompat: Fix exit code in weak mode

Message ID 87plvv1dbt.fsf@redhat.com
State New
Headers
Series [applied,to,mainline] abicompat: Fix exit code in weak mode |

Commit Message

Dodji Seketeli March 15, 2024, 7:53 p.m. UTC 
  Hello,

It turns out the tool is almost always returning ABIDIFF_OK in weak
mode.  Oops.  Fixed thus.  Also, update the test-abicompat.cc to test
for expected exit codes to catch this kind of regressions in the
future.

	* tools/abicompat.cc (perform_compat_check_in_weak_mode): Do not
	override the status code when doing the comparison in the reverse
	direction.
	(compare_expected_against_provided_functions)
	(compare_expected_against_provided_variables): Set the status code
	close to the detected diff.  In the future, this might help us
	provide finer grained status.
	* tests/test-abicompat.cc (InOutSpec::status): Add a new data
	member to represent the expected exit code.
	(in_out_specs): Adjust the array of tests.
	(main): If the actual exit code is different from the expected
	one, then the test failed so let's report it.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to mainline.
---
 tests/test-abicompat.cc | 42 ++++++++++++++++++++++++++++++++++++++---
 tools/abicompat.cc      | 40 +++++++++++++++++++--------------------
 2 files changed, 59 insertions(+), 23 deletions(-)
diff mbox series

Patch

diff --git a/tests/test-abicompat.cc b/tests/test-abicompat.cc
index a7fcc520..e9f00050 100644
--- a/tests/test-abicompat.cc
+++ b/tests/test-abicompat.cc
@@ -28,6 +28,7 @@ 
 using std::string;
 using std::cerr;
 using std::cout;
+using abigail::tools_utils::abidiff_status;
 
 struct InOutSpec
 {
@@ -36,6 +37,7 @@  struct InOutSpec
   const char* in_lib2_path;
   const char* suppressions;
   const char* options;
+  abidiff_status status;
   const char* in_report_path;
   const char* out_report_path;
 };
@@ -48,6 +50,7 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest0-fn-changed-libapp-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test0-fn-changed-report-0.txt",
     "output/test-abicompat/test0-fn-changed-report-0.txt",
   },
@@ -57,6 +60,7 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest0-fn-changed-libapp-v1.so",
     "data/test-abicompat/test0-fn-changed-0.suppr",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abicompat/test0-fn-changed-report-1.txt",
     "output/test-abicompat/test0-fn-changed-report-1.txt",
   },
@@ -66,6 +70,7 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest0-fn-changed-libapp-v1.so",
     "",
     "--show-base-names --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test0-fn-changed-report-2.txt",
     "output/test-abicompat/test0-fn-changed-report-2.txt",
   },
@@ -75,6 +80,7 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest0-fn-changed-libapp-v1.so",
     "data/test-abicompat/test0-fn-changed-1.suppr",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abicompat/test0-fn-changed-report-3.txt",
     "output/test-abicompat/test0-fn-changed-report-3.txt",
   },
@@ -84,6 +90,8 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest1-fn-removed-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE
+    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
     "data/test-abicompat/test1-fn-removed-report-0.txt",
     "output/test-abicompat/test1-fn-removed-report-0.txt",
   },
@@ -93,6 +101,8 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest2-var-removed-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE
+    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
     "data/test-abicompat/test2-var-removed-report-0.txt",
     "output/test-abicompat/test2-var-removed-report-0.txt",
   },
@@ -102,6 +112,8 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest3-fn-removed-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE
+    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
     "data/test-abicompat/test3-fn-removed-report-0.txt",
     "output/test-abicompat/test3-fn-removed-report-0.txt",
   },
@@ -111,6 +123,8 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest4-soname-changed-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE
+    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
     "data/test-abicompat/test4-soname-changed-report-0.txt",
     "output/test-abicompat/test4-soname-changed-report-0.txt",
   },
@@ -120,6 +134,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --no-show-locs --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test5-fn-changed-report-0.txt",
     "output/test-abicompat/test5-fn-changed-report-0.txt",
   },
@@ -129,6 +144,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test5-fn-changed-report-1.txt",
     "output/test-abicompat/test5-fn-changed-report-1.txt",
   },
@@ -138,6 +154,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --no-show-locs --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test6-var-changed-report-0.txt",
     "output/test-abicompat/test6-var-changed-report-0.txt",
   },
@@ -147,6 +164,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test6-var-changed-report-1.txt",
     "output/test-abicompat/test6-var-changed-report-1.txt",
   },
@@ -156,6 +174,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test6-var-changed-report-2.txt",
     "output/test-abicompat/test6-var-changed-report-2.txt",
   },
@@ -165,6 +184,7 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest7-fn-changed-libapp-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test7-fn-changed-report-0.txt",
     "output/test-abicompat/test7-fn-changed-report-0.txt",
   },
@@ -175,6 +195,7 @@  InOutSpec in_out_specs[] =
     "data/test-abicompat/libtest7-fn-changed-libapp-btf-v1.so",
     "",
     "--show-base-names --no-show-locs --no-redundant --btf",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test7-fn-changed-report-0.1.txt",
     "output/test-abicompat/test7-fn-changed-report-0.1.txt",
   },
@@ -185,6 +206,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --no-show-locs --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test7-fn-changed-report-1.txt",
     "output/test-abicompat/test7-fn-changed-report-1.txt",
   },
@@ -194,6 +216,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test7-fn-changed-report-2.txt",
     "output/test-abicompat/test7-fn-changed-report-2.txt",
   },
@@ -204,6 +227,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --no-show-locs --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test7-fn-changed-report-2.1.txt",
     "output/test-abicompat/test7-fn-changed-report-2.1.txt",
   },
@@ -214,6 +238,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test8-fn-changed-report-0.txt",
     "output/test-abicompat/test8-fn-changed-report-0.txt",
   },
@@ -223,6 +248,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names --weak-mode",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test9-fn-changed-report-0.txt",
     "output/test-abicompat/test9-fn-changed-report-0.txt",
   },
@@ -232,6 +258,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names",
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abicompat/test10/test10-fn-changed-report-0.txt",
     "output/test-abicompat/test10/test10-fn-changed-report-0.txt",
   },
@@ -241,6 +268,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names",
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abicompat/test10/test10-fn-changed-report-0.txt",
     "output/test-abicompat/test10/test10-fn-changed-report-0.txt",
   },
@@ -250,6 +278,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test10/test10-fn-changed-report-1.txt",
     "output/test-abicompat/test10/test10-fn-changed-report-1.txt",
   },
@@ -259,6 +288,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test10/test10-fn-changed-report-2.txt",
     "output/test-abicompat/test10/test10-fn-changed-report-2.txt",
   },
@@ -268,6 +298,7 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test10/test10-fn-changed-report-3.txt",
     "output/test-abicompat/test10/test10-fn-changed-report-3.txt",
   },
@@ -277,11 +308,12 @@  InOutSpec in_out_specs[] =
     "",
     "",
     "--show-base-names",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
     "data/test-abicompat/test10/test10-fn-changed-report-4.txt",
     "output/test-abicompat/test10/test10-fn-changed-report-4.txt",
   },
   // This entry must be the last one.
-  {0, 0, 0, 0, 0, 0, 0}
+  {0, 0, 0, 0, 0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
 };
 
 int
@@ -334,8 +366,9 @@  main()
       cmd += " > " + out_report_path;
 
       bool abicompat_ok = true;
-      abidiff_status status = static_cast<abidiff_status>(system(cmd.c_str()));
-      if (abigail::tools_utils::abidiff_status_has_error(status))
+      int code = system(cmd.c_str());
+      abidiff_status status = static_cast<abidiff_status>(WEXITSTATUS(code));
+      if (status != s->status)
 	abicompat_ok = false;
 
       if (abicompat_ok)
@@ -356,6 +389,9 @@  main()
                << DEFAULT_TERMINAL_COLOR
                << cmd
                << std::endl;
+	  if (status != s->status)
+	    cout << BRIGHT_RED_COLOR
+		 << "expected abicompat exit code: " << s->status << ", got: " << status << std::endl;
 	  cnt_failed++;
 	}
       cnt_total++;
diff --git a/tools/abicompat.cc b/tools/abicompat.cc
index 9c4771c9..2860b24a 100644
--- a/tools/abicompat.cc
+++ b/tools/abicompat.cc
@@ -460,6 +460,7 @@  compare_expected_against_provided_functions(diff_context_sptr&		ctxt,
 					    vector<fn_change>&		fn_changes,
 					    bool			reverse_direction)
 {
+  abidiff_status status = abigail::tools_utils::ABIDIFF_OK;
   for (auto expected_fn :
 	 reverse_direction
 	 ? lib_corpus->get_sorted_undefined_functions()
@@ -483,19 +484,18 @@  compare_expected_against_provided_functions(diff_context_sptr&		ctxt,
 			     exported_fn->get_type(),
 			     ctxt);
 	      if (fn_type_diff && fn_type_diff->to_be_reported())
-		// So there is a type change between the function
-		// expected by the application and the function
-		// exported by the library.  Let's record that
-		// change so that we can report about it later.
-		fn_changes.push_back(fn_change(expected_fn, fn_type_diff, reverse_direction));
+		{
+		  // So there is a type change between the function
+		  // expected by the application and the function
+		  // exported by the library.  Let's record that
+		  // change so that we can report about it later.
+		  fn_changes.push_back(fn_change(expected_fn, fn_type_diff, reverse_direction));
+		  status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
+		}
 	    }
 	}
     }
 
-  abidiff_status status = abigail::tools_utils::ABIDIFF_OK;
-  if (!fn_changes.empty())
-    status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
-
   return status;
 }
 
@@ -534,6 +534,7 @@  compare_expected_against_provided_variables(diff_context_sptr&		ctxt,
 					    vector<var_change>&	var_changes,
 					    bool			reverse_direction)
 {
+  abidiff_status status = abigail::tools_utils::ABIDIFF_OK;
   for (auto expected_var :
 	 reverse_direction
 	 ? lib_corpus->get_sorted_undefined_variables()
@@ -555,18 +556,17 @@  compare_expected_against_provided_variables(diff_context_sptr&		ctxt,
 			 exported_var->get_type(),
 			 ctxt);
 	  if (type_diff && type_diff->to_be_reported())
-	    // So there is a type change between the variable
-	    // expected by the application and the variable
-	    // exported by the library.  Let's record that
-	    // change so that we can report about it later.
-	    var_changes.push_back(var_change(expected_var, type_diff, reverse_direction));
+	    {
+	      // So there is a type change between the variable
+	      // expected by the application and the variable
+	      // exported by the library.  Let's record that
+	      // change so that we can report about it later.
+	      var_changes.push_back(var_change(expected_var, type_diff, reverse_direction));
+	      status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
+	    }
 	}
     }
 
-  abidiff_status status = abigail::tools_utils::ABIDIFF_OK;
-  if (!var_changes.empty())
-    status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
-
   return status;
 }
 
@@ -807,8 +807,8 @@  perform_compat_check_in_weak_mode(options& opts,
 
   {
     vector<fn_change> fn_changes;
-    status = compare_expected_against_provided_functions(ctxt, app_corpus, lib_corpus,
-							 fn_changes, /*reverse_direction=*/true);
+    status |= compare_expected_against_provided_functions(ctxt, app_corpus, lib_corpus,
+							  fn_changes, /*reverse_direction=*/true);
     report_function_changes(opts, fn_changes);
   }