diff mbox series

[applied] abg-comparison[-priv]: Better detection of incompatible unreachable type changes

Message ID 87cyxur879.fsf@redhat.com
State New
Headers
Series [applied] abg-comparison[-priv]: Better detection of incompatible unreachable type changes |

Commit Message

Dodji Seketeli Oct. 4, 2023, 10:30 a.m. UTC 
  Hello,

Whenever there is a change in unreachable type,
corpus_diff::has_incompatible_changes always reports that the
corresponding diff node carries an incompatible change.  It does this
even if the change is known to be compatible.

To be able to say if a diff node for a unreachable type carries a
compatible (not incompatible) change,
corpus_diff::has_incompatible_changes must look at the change category
of that diff node, instead of saying that any change to an unreachable
type is incompatible.

While looking at this, I noted that
corpus_diff::priv::apply_filters_and_compute_diff_stats doesn't
categorize the diffs in
corpus_diff::priv::changed_unreachable_types_, so it's not possible to
look at the categories of the changes held by that data member to see
if they are incompatible or not.

This patch thus categorizes the diff nodes held by
corpus_diff::priv::changed_unreachable_types_ and makes
corpus_diff::has_incompatible_changes look at those diff nodes to
detect if they are incompatible.

Let's see the result of this patch.

Consider the change in the input test source code from included in
this patch, from test-enumerator-changes1-v0.c to test-enumerator-changes1-v1.c:

    $ diff -u test-enumerator-changes1-v0.c test-enumerator-changes1-v1.c
    --- test-enumerator-changes1-v0.c	2023-10-04 11:25:30.722989530 +0200
    +++ test-enumerator-changes1-v1.c	2023-10-04 11:25:30.722989530 +0200
    @@ -1,13 +1,14 @@
     /*
      *
      * Compile this with:
    - *    gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v0.c
    + *    gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v1.c
      */

     enum foo
       {
	 E1_O,
    -    E1_1
    +    E1_1,
    +    E1_2
       };

     void
    $

The enumerator E1_2 has been added to the 'foo' enum.

Now, let's see what abidiff prior to this patch would say about the
change between the two result binaries test-enumerator-changes1-v0.o
and test-enumerator-changes1-v1.o:

    $ abidiff --non-reachable-types --harmless test-enumerator-changes1-v0.o test-enumerator-changes1-v1.o || echo "return value: $?"
    Functions changes summary: 0 Removed, 0 Changed, 0 Added function
    Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
    Unreachable types summary: 0 removed, 1 changed, 0 added type

    1 changed type unreachable from any public interface:

      [C] 'enum foo' changed:
	type size hasn't changed
	1 enumerator insertion:
	  'foo::E1_2' value '2'

    return value: 12
    $

See the return value of 12, that is actually the bits
abigail::tools_utils::ABIDIFF_ABI_CHANGE (of value 4) and
abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE (of value 8) being set.

Normally, only the bit abigail::tools_utils::ABIDIFF_ABI_CHANGE (of
value 4) should be set.

Now, let's look at what abidiff says with this patch:

    $ abidiff --non-reachable-types --harmless test-enumerator-changes1-v0.o test-enumerator-changes1-v1.o || echo "return value: $?"
    Functions changes summary: 0 Removed, 0 Changed, 0 Added function
    Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
    Unreachable types summary: 0 removed, 1 changed, 0 added type

    1 changed type unreachable from any public interface:

      [C] 'enum foo' changed:
	type size hasn't changed
	1 enumerator insertion:
	  'foo::E1_2' value '2'

    return value: 4
    $

Now the return value is 4, which is the bit
abigail::tools_utils::ABIDIFF_ABI_CHANGE being set, as we would expect
because that change is known to be not incompatible.

Please note that this patch originates from the patch set on the
branch "better-anon-enums" to work on the support for the UAPI checker
tool tracked by https://sourceware.org/PR30931.  It's being applied to
the master branch as it turns out it's quite independent from that
task.


	* Src/abg-comparison-priv.h
	(corpus_diff::priv::changed_unreachable_types): Declare ...
	* src/abg-comparison.cc
	(corpus_diff::priv::changed_unreachable_types): ... new function.
	(corpus_diff::priv::apply_filters_and_compute_diff_stats): Walk
	the nodes returned by corpus_diff:priv::changed_unreachable_types
	and apply the filters (including categorization filters) to them.
	Also make the loop similarly applying filters to the nodes
	returned by corpus_diff::priv::changed_unreachable_types_sorted be
	a ranged-based one, for the sake of consistency.
	(corpus_diff::has_incompatible_changes): Now that diff nodes
	returned by corpus_diff::priv::changed_unreachable_types are
	categorized, look at their change categories to see if they are
	incompatible or not.
	* tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt:
	New test output reference.
	* tests/data/test-abidiff-exit/test-enumerator-changes1-v{0,1}.o:
	New test input binaries.
	* tests/data/test-abidiff-exit/test-enumerator-changes1-v{0,1}.c:
	New source code for the new test input binaries.
	* tests/data/Makefile.am: Add the new test material above to
	source distribution.
	* tests/test-abidiff-exit.cc (in_out_specs): Add the new test
	input binaries to the test harness.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

Applied to master.
---
 src/abg-comparison-priv.h                     |   3 +
 src/abg-comparison.cc                         |  54 ++++++++++++++----
 tests/data/Makefile.am                        |   5 ++
 .../test-enumerator-changes1-report-1.txt     |  11 ++++
 .../test-enumerator-changes1-v0.c             |  16 ++++++
 .../test-enumerator-changes1-v0.o             | Bin 0 -> 3144 bytes
 .../test-enumerator-changes1-v1.c             |  17 ++++++
 .../test-enumerator-changes1-v1.o             | Bin 0 -> 3176 bytes
 tests/test-abidiff-exit.cc                    |  15 +++++
 9 files changed, 110 insertions(+), 11 deletions(-)
 create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt
 create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c
 create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v0.o
 create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v1.c
 create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v1.o

new file mode 100644
index 00000000..abc1d1b7
index e539242a..fb3d6fff 100644
diff mbox series

Patch

diff --git a/src/abg-comparison-priv.h b/src/abg-comparison-priv.h
index 481fc9c6..9802e78f 100644
--- a/src/abg-comparison-priv.h
+++ b/src/abg-comparison-priv.h
@@ -1159,6 +1159,9 @@  struct corpus_diff::priv
 			       size_t &num_filtered_removed,
 			       size_t &num_filtered_changed);
 
+  const string_diff_sptr_map&
+  changed_unreachable_types() const;
+
   const vector<diff_sptr>&
   changed_unreachable_types_sorted() const;
 
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 1cfc0952..1207729c 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -10129,6 +10129,14 @@  corpus_diff::priv::count_unreachable_types(size_t &num_added,
       ++num_filtered_changed;
 }
 
+/// Get the map of diff nodes representing changed unreachable types.
+///
+/// @return the map of diff nodes representing changed unreachable
+/// types.
+const string_diff_sptr_map&
+corpus_diff::priv::changed_unreachable_types() const
+{return changed_unreachable_types_;}
+
 /// Get the sorted vector of diff nodes representing changed
 /// unreachable types.
 ///
@@ -10239,14 +10247,11 @@  corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
 
       // walk the changed unreachable types to apply categorization
       // filters
-      for (diff_sptrs_type::const_iterator i =
-	     changed_unreachable_types_sorted().begin();
-	   i != changed_unreachable_types_sorted().end();
-	   ++i)
-	{
-	  diff_sptr diff = *i;
-	  ctxt->maybe_apply_filters(diff);
-	}
+      for (auto& diff : changed_unreachable_types_sorted())
+	ctxt->maybe_apply_filters(diff);
+
+      for (auto& entry : changed_unreachable_types())
+	ctxt->maybe_apply_filters(entry.second);
 
       if (get_context()->do_log())
 	{
@@ -11194,7 +11199,8 @@  corpus_diff::has_incompatible_changes() const
   const diff_stats& stats = const_cast<corpus_diff*>(this)->
     apply_filters_and_suppressions_before_reporting();
 
-  return (soname_changed() || architecture_changed()
+  bool has_incompatible_changes  =
+    (soname_changed() || architecture_changed()
 	  || stats.net_num_func_removed() != 0
 	  || (stats.num_func_with_virtual_offset_changes() != 0
 	      // If all reports about functions with sub-type changes
@@ -11205,8 +11211,34 @@  corpus_diff::has_incompatible_changes() const
 	  || stats.net_num_vars_removed() != 0
 	  || stats.net_num_removed_func_syms() != 0
 	  || stats.net_num_removed_var_syms() != 0
-	  || stats.net_num_removed_unreachable_types() != 0
-	  || stats.net_num_changed_unreachable_types() != 0);
+	  || stats.net_num_removed_unreachable_types() != 0);
+
+  // If stats.net_num_changed_unreachable_types() != 0 then walk the
+  // corpus_diff::priv::changed_unreachable_types_, and see if there
+  // is one that is harmful by bitwise and-ing their category with
+  // abigail::comparison::get_default_harmful_categories_bitmap().
+  if (!has_incompatible_changes
+      && stats.net_num_changed_unreachable_types())
+    {
+      // The changed unreachable types can carry harmful changes.
+      // Let's figure if they actually do.
+
+      diff_context_sptr ctxt = context();
+      for (auto &entry : priv_->changed_unreachable_types())
+	{
+	  diff_sptr dif = entry.second;
+
+	  // Let's see if any of the categories of this diff node
+	  // belong to the "harmful" ones.
+	  if (dif->get_category() & get_default_harmful_categories_bitmap())
+	    {
+	      has_incompatible_changes |= true;
+	      break;
+	    }
+	}
+    }
+
+  return has_incompatible_changes;
 }
 
 /// Test if the current instance of @ref corpus_diff carries subtype
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index b3434da5..0569af34 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -351,6 +351,11 @@  test-abidiff-exit/test-PR30034/split/lib64/librte_telemetry.so \
 test-abidiff-exit/test-PR30034/split/lib64/librte_telemetry.so.23 \
 test-abidiff-exit/test-PR30034/split/lib64/librte_telemetry.so.23.2 \
 test-abidiff-exit/test-PR30034/test-PR30034-report-1.txt \
+test-abidiff-exit/test-enumerator-changes1-report-1.txt \
+test-abidiff-exit/test-enumerator-changes1-v0.c \
+test-abidiff-exit/test-enumerator-changes1-v0.o \
+test-abidiff-exit/test-enumerator-changes1-v1.c \
+test-abidiff-exit/test-enumerator-changes1-v1.o \
 \
 test-diff-dwarf/test0-v0.cc		\
 test-diff-dwarf/test0-v0.o			\
diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt b/tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt
new file mode 100644
index 00000000..cf1c3c82
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt
@@ -0,0 +1,11 @@ 
+Functions changes summary: 0 Removed, 0 Changed, 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+Unreachable types summary: 0 removed, 1 changed, 0 added type
+
+1 changed type unreachable from any public interface:
+
+  [C] 'enum foo' changed:
+    type size hasn't changed
+    1 enumerator insertion:
+      'foo::E1_2' value '2'
+
diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c b/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c
new file mode 100644
index 00000000..b02c3a07
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c
@@ -0,0 +1,16 @@ 
+/*
+ *
+ * Compile this with:
+ *    gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v0.c
+ */
+
+enum foo
+  {
+    E1_O,
+    E1_1
+  };
+
+void
+foo()
+{
+}
diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.o b/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..c7cc06e9f50601f5f42e43a63599f8255ec89ab6
GIT binary patch
literal 3144
zcmb_d&2Jl35TEC@<Fwnj`9P48f|rk~X~eGWP(n$S;1ZPtwLn$1H;`G|&-PM#x7uAt
zZaHu(LR@m-$dL<IkoX7q2l`JSaYArI>H%h+=Q++}lOkZGeKYf$nK$#kW<S4v^WB^f
zph>_s9BLW`*ef2&btTr}8CZh5dtd&1@bORg5GsI}B}<Nm7igHKsd%tDQ3S>O`)C)%
z8O4;VN-Q9sSI!0U5~WEsdIs?gh*hvk=WWcjATE@aY^%IdUJ#4L`R4$vys&1sZQFit
zY16)Hzh#Rhfc&dq+uOEP!ZJO0`<yj9woy=h`QbTC{~CU(bWp2M1<|0>P5>76kQdN*
z4Y?5FNokH4Rf``AWmR}ilyhgt)2g_FfKwCIDv2983!Sgm8Z{V(Nze^t+X=!H5cv>x
zKe+4c)ZTPzHSe-lbJnZtuU9v!Z#b*B(QD67+2F2Ua@;{W4CPi=hB6LXsQPiMw>7%5
z;ci@Z_tzb_>$sgTa%Ddl1ficwcNh*6*>>BqIqbUWgZnapR3@n_!{I>2ej3GYtLKMZ
znbh3<s@H-_FB-^7J8FL#RJuW0=?6`}8Fc-iUunuTm9guGQK;gR3Qd(%+J5RQ(M8X8
z(CN5x6r}&-Oea@-vK4lBb~c?=+~iCDV29P|0p9sBiHEmAjONA4nd0mLp7eQy=_67%
z=2~g~QEq2$<{fkx!<i#pl05wWXO<CfMivYf-&50Qp(Tqf%#cME%w)khOZJ_&;M6hx
z_!2Y7f-@N?zEHqtm_Zht%|OY2?r(S*k;SR~nN)o8nNv$qq|f9uYSX)*GZ9X!#fGA2
zkvZY?X?m6gjlanH^UUc4C+{8QHO9$b*Fey<DV*}^e7gqDD%>*g0_z_!H}mQ>QeN##
zuJgzEzs$H^SC4h-l0+K+eyl=`xyB!}ewDddkHa|a&Gi0&mpmAxeiL~bk9ki^sT`%?
zQBMJ{nIz!FvhRDUfi;4#6QPZoS;KEOW4V9C?8mB;9l`oRD0K*?&?3PNrMIHNKw>ZU
z!YGv<_6avh{nkC6MD`k;*dIt<$bV!j7*Bho&q+^LXJK-`Sts9VdI=T6%nqRM&$KVe
zX8c9O=sy12cshS`PJ}?`=VwQdBGZ@d6?Lh)vK|h9aO?Vwp#|gw>9@1Y_!e?=PBQ(-
z_B+fqGkxhjnHc$=X0WOQnVY^h5YMXr7k+Fyp6aKwH0y68X3mKaY%+M0lqoWO8)(bQ
zzswiv6ON_))L+c}_YtExb)6_Gdc=vo&;WCtU-Qop&x)Vp$NlS5;=ej3{t3rdo)Z5x
z#~W_2W?pD^&~<D7?>PQQFG8%!@mI7XbG?5W{uYI-{U7oE>w8N3Pk%WbuW_o!%#9HA
rIDd}|rvC!{J<WJJ)2yXFrlOEu8MDb~uAl14ia+4`f0#s$1vCCHcoqu*

literal 0
HcmV?d00001

diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-v1.c b/tests/data/test-abidiff-exit/test-enumerator-changes1-v1.c
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-enumerator-changes1-v1.c
@@ -0,0 +1,17 @@ 
+/*
+ *
+ * Compile this with:
+ *    gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v1.c
+ */
+
+enum foo
+  {
+    E1_O,
+    E1_1,
+    E1_2
+  };
+
+void
+foo()
+{
+}
diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-v1.o b/tests/data/test-abidiff-exit/test-enumerator-changes1-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..9f7e944f8a7a675b5656503939e3374f600a60e9
GIT binary patch
literal 3176
zcmb_d&2!sS5Z`Cnv5OkVNji{{g2mU6c90yKHiUK(TrwqLT0W*wZj6!bXIo&)L6S3R
zIdFqv=%EJ=95`~}#=w;u{{Tn+fnJ&6%mH?v_qobLNf}^gq}~1PZ{P0w(9@^aZoHNg
z0<;L&h6kQT0S3hfc2k*6I0MUYYwwHu_ul{cV}uGIN)*Y_>j`?LC6y0WZ#;%#{vC{q
z;+#^7stQk=$w5A^QVPTs3$vc|IN}=+Yhabn*;sK=JXc<}t?FuZQ7jcJj{#VDVU@OR
z+kSodvVFyV*%r$H`Deklw{5G8&3f?4S*tXSQBrf|&RJ~Y1$@*5K+Q$9MK4`;2C%V*
zx`5mZsD%)Jl;=rNxA>q@RgLFFHMcMwR^=50oSM1Tg1DZukbJG)s>39VgMKKxP7o%5
z$Xl@c<}GKZ{*qI#dl$XB(^zl3xW2J|$yvLJtUW*BfZI6lxT9ne%B{W(WfXMK^`p*U
z>u__!-MHu;G#t0@xV>=f%3&}HLO+r2B%H*u>vm;((sz^F`!c4Y4M=31xH6oKWaKB~
z$n6aLurK4fdr<c}P#cU#veq4UKMZR9AgK+5w%-o=elV=HWs=Cq^}}(f^5Yr}71z3c
z;w#fdW;f{dTzMEI|Kms}cYJabc6W9zJ8QVo=l{Vn(-xjl`-oe7=O&25ida2eEZxJs
zK8H9xB<jpuEmuCv?aa@;ii9bg`KDu%hu{Cq3gX$wg2CdGY6dNgWQm0pvc!V9EEq@0
zKJ^xyI);zmVuo07Is?UT6!1gL5DOMEP~y-34X+}ylv<xz$0u(%T`EfSo;-qHdKGjf
z!fCd6r6^fqO*lPT9%V!0PqP0wYudrt7mxM@#)+?cAn0ler@Fe{u7R@)Zy5OJ?7wH=
z1@_yl&3^RUsUOWHYULRI(~Rr+4%nyrlSJb`OkJq6*7z^%Utw+b<1kKZmR>jT;@hLd
zZ=+74sUB!6k%tL*bkTs<j$`m5IrKeswY7q<H^vxUb1lE!j^x3Supg=G?FcpuLa9@*
zgf<CoDZeuwjU?XEUN}yqhnI&NCw}K69z+gWy~rO)p2&YxtC&w~r1wb=*JNXM?%5}9
zTAo6OF!N^6=PJ!5+suC+F*>LJHlOz291|hX^(W+{#N^VsqT8#P;wY03Yl#yU5hR}e
zrdF80h1wjGLO*f*UDjHe+_y1cV#KX5xULgfo80S&XZ2s=4b}P7KkcR2e-|-xOoX7x
z;0<z8Vscv;%c{T0^*`WTs!#WhS$`if8dLX)rjol{=&lA>>-t)Mf_PT`3V+<cJtY6j
zWAf+tK-M0T{}tyOG`M6QXeQ{sHUB%#KXMl#w#oUMn#fwOpN79hBWwM?=JnU-l-8gA
zemY;{)Q?#kA=u~o18$i97xY&(^J!1Brur*2h4Rc)&EDqxsjsa3@45dUXPHyO%>N5*
C@d`Eo

literal 0
HcmV?d00001

diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -1017,6 +1017,21 @@  InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/test-PR30034/test-PR30034-report-1.txt",
     "output/test-abidiff-exit/test-PR30034/test-PR30034-report-1.txt"
   },
+  {
+    "data/test-abidiff-exit/test-enumerator-changes1-v0.o",
+    "data/test-abidiff-exit/test-enumerator-changes1-v1.o",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "--no-default-suppression --non-reachable-types --harmless",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    "data/test-abidiff-exit/test-enumerator-changes1-report-1.txt",
+    "output/test-abidiff-exit/test-enumerator-changes1-report-1.txt"
+  },
 #ifdef WITH_BTF
   {
     "data/test-abidiff-exit/btf/test0-v0.o",