public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Bug 29144 - abidiff doesn't report base class re-ordering
@ 2022-06-07  9:59 Dodji Seketeli
  0 siblings, 0 replies; only message in thread
From: Dodji Seketeli @ 2022-06-07  9:59 UTC (permalink / raw)
  To: gprocida; +Cc: libabigail

Hello Giuliano,

Libabigail fails to report when a base class gets re-ordered, even
though the comparison engine detects the change.

This patch detects the base class re-ordering in
class_diff::ensure_lookup_tables_populated by interpreting the edit
script resulting of the comparison, populates a new
class_diff::priv::moved_bases_ vector with the bases that "changed
position" and adds a new class_diff::moved_bases() accessor for the "base
classes that changed position".

A new function maybe_report_base_class_reordering is defined to
actually report a base class re-ordering if class_diff::moved_bases()
returns a non-empty vector, meaning that class has some base classes
that got re-ordered.

That new function is thus called in the overload of
{default_reporter, leaf_reporter}::report() for class_diff to emit a
description of the change when necessary.

Would you this patch work for your testing case?

Thanks.

	* include/abg-comparison.h (class_diff::moved_bases): Declare new
	method.
	* src/abg-comparison-priv.h (class_diff::priv::moved_bases_):
	Define new data member.
	* src/abg-reporter-priv.h (maybe_report_base_class_reordering):
	Declare new function.
	* src/abg-reporter-priv.cc (maybe_report_base_class_reordering):
	Define new function.
	* src/abg-comparison.cc
	(class_diff::ensure_lookup_tables_populated):  Detect that a base
	class moved position and record it into
	class_diff::priv::moved_bases_ data member.
	* src/abg-default-reporter.cc (default_reporter::report): Use the
	new maybe_report_base_class_reordering.
	* src/abg-leaf-reporter.cc (leaf_reporter::report): Likewise.
	* tests/data/test-abidiff-exit/test-PR29144-report-2.txt: New
	testing input file.
	* tests/data/test-abidiff-exit/test-PR29144-report.txt: Likewise.
	* tests/data/test-abidiff-exit/test-PR29144-v0.cc: Likewise.
	* tests/data/test-abidiff-exit/test-PR29144-v0.o: Likewise.
	* tests/data/test-abidiff-exit/test-PR29144-v1.cc: Likewise.
	* tests/data/test-abidiff-exit/test-PR29144-v1.o: Likewise.
	* tests/data/Makefile.am: Add the new files to source
	distribution.
	* tests/test-abidiff-exit.cc (in_out_specs): Add the new tests to
	this harness.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 include/abg-comparison.h                      |   3 ++
 src/abg-comparison-priv.h                     |   1 +
 src/abg-comparison.cc                         |  15 ++++++
 src/abg-default-reporter.cc                   |   3 ++
 src/abg-leaf-reporter.cc                      |   2 +
 src/abg-reporter-priv.cc                      |  48 ++++++++++++++++++
 src/abg-reporter-priv.h                       |   4 ++
 tests/data/Makefile.am                        |  16 ++++--
 .../test-PR29144-report-2.txt                 |   8 +++
 .../test-abidiff-exit/test-PR29144-report.txt |  10 ++++
 .../data/test-abidiff-exit/test-PR29144-v0.cc |  17 +++++++
 .../data/test-abidiff-exit/test-PR29144-v0.o  | Bin 0 -> 2560 bytes
 .../data/test-abidiff-exit/test-PR29144-v1.cc |  18 +++++++
 .../data/test-abidiff-exit/test-PR29144-v1.o  | Bin 0 -> 2560 bytes
 tests/test-abidiff-exit.cc                    |  22 ++++++++
 15 files changed, 162 insertions(+), 5 deletions(-)
 create mode 100644 tests/data/test-abidiff-exit/test-PR29144-report-2.txt
 create mode 100644 tests/data/test-abidiff-exit/test-PR29144-report.txt
 create mode 100644 tests/data/test-abidiff-exit/test-PR29144-v0.cc
 create mode 100644 tests/data/test-abidiff-exit/test-PR29144-v0.o
 create mode 100644 tests/data/test-abidiff-exit/test-PR29144-v1.cc
 create mode 100644 tests/data/test-abidiff-exit/test-PR29144-v1.o

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index 78e4102e..f5169aeb 100644
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -1716,6 +1716,9 @@ public:
   const base_diff_sptrs_type&
   changed_bases();
 
+  const vector<class_decl::base_spec_sptr>&
+  moved_bases() const;
+
   virtual bool
   has_changes() const;
 
diff --git a/src/abg-comparison-priv.h b/src/abg-comparison-priv.h
index ef271d8b..0953d567 100644
--- a/src/abg-comparison-priv.h
+++ b/src/abg-comparison-priv.h
@@ -613,6 +613,7 @@ struct class_diff::priv
   class_decl::base_specs sorted_inserted_bases_;
   string_base_diff_sptr_map changed_bases_;
   base_diff_sptrs_type sorted_changed_bases_;
+  vector<class_decl::base_spec_sptr> moved_bases_;
 
   class_decl::base_spec_sptr
   base_has_changed(class_decl::base_spec_sptr) const;
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 2ce3f5d4..1313e8fd 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -5217,6 +5217,11 @@ class_diff::ensure_lookup_tables_populated(void) const
 		if (j->second != b)
 		  get_priv()->changed_bases_[name] =
 		    compute_diff(j->second, b, context());
+		else
+		  // The base class changed place.  IOW, the base
+		  // classes got re-arranged.  Let's keep track of the
+		  // base classes that moved.
+		  get_priv()->moved_bases_.push_back(b);
 		get_priv()->deleted_bases_.erase(j);
 	      }
 	    else
@@ -5547,6 +5552,16 @@ const base_diff_sptrs_type&
 class_diff::changed_bases()
 {return get_priv()->sorted_changed_bases_;}
 
+/// Getter for the vector of bases that "moved".
+/// That is, the vector of base types which position changed.  If this
+/// vector is not empty, it means the bases of the underlying class
+/// type got re-ordered.
+///
+/// @return the vector of bases that moved.
+const vector<class_decl::base_spec_sptr>&
+class_diff::moved_bases() const
+{return get_priv()->moved_bases_;}
+
 /// @return the edit script of the bases of the two classes.
 edit_script&
 class_diff::base_changes()
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index a5cebef4..909d1de4 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -1348,6 +1348,9 @@ default_reporter::report(const class_diff& d, ostream& out,
 	      out << "\n";
 	    }
 	}
+
+      // Report base classes re-organisation
+      maybe_report_base_class_reordering(d, out, indent);
     }
 
   d.class_or_union_diff::report(out, indent);
diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
index 07391720..79b0286b 100644
--- a/src/abg-leaf-reporter.cc
+++ b/src/abg-leaf-reporter.cc
@@ -691,6 +691,8 @@ leaf_reporter::report(const class_diff& d,
 
   d.class_or_union_diff::report(out, indent);
 
+  maybe_report_base_class_reordering(d, out, indent);
+
   maybe_report_interfaces_impacted_by_diff(&d, out, indent);
 
   d.reported_once(true);
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 7012f5dc..06145a1b 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -1451,5 +1451,53 @@ maybe_report_data_members_replaced_by_anon_dm(const class_or_union_diff &d,
     }
 }
 
+/// Report about the base classes of a class having been re-ordered.
+///
+/// @param d the class diff to consider.
+///
+/// @param out the output stream to report the change to.
+///
+/// @param indent the indentation string to use.
+void
+maybe_report_base_class_reordering(const class_diff	&d,
+				   ostream		&out,
+				   const string	&indent)
+{
+  if (d.moved_bases().empty())
+    return;
+
+  class_decl_sptr first = d.first_class_decl(),
+    second = d.second_class_decl();
+
+  ABG_ASSERT(!first->get_base_specifiers().empty());
+  ABG_ASSERT(!second->get_base_specifiers().empty());
+
+  out << indent << "base classes of '"
+      << first->get_pretty_representation()
+      << "' are re-ordered from: ";
+
+  vector<class_decl_sptr> classes = {first, second};
+  unsigned nb_classes_seen = 0;
+  for (auto &klass : classes)
+    {
+      if (nb_classes_seen >= 1)
+	out << " to: ";
+      out << "'";
+      bool needs_comma = false;
+      for (auto &b : klass->get_base_specifiers())
+	{
+	  if (needs_comma)
+	    out << ", ";
+	  if (b->get_is_virtual())
+	    out << "virtual ";
+	  out << b->get_base_class()->get_qualified_name();
+	  needs_comma = true;
+	}
+      out << "'";
+      nb_classes_seen++;
+    }
+  if (nb_classes_seen)
+    out << "\n";
+}
 } // Namespace comparison
 } // end namespace abigail
diff --git a/src/abg-reporter-priv.h b/src/abg-reporter-priv.h
index a7c4878c..42425ef2 100644
--- a/src/abg-reporter-priv.h
+++ b/src/abg-reporter-priv.h
@@ -241,6 +241,10 @@ maybe_report_data_members_replaced_by_anon_dm(const class_or_union_diff &d,
 					      const string		indent);
 
 
+void
+maybe_report_base_class_reordering(const class_diff	&d,
+				   ostream		&out,
+				   const string	&indent);
 } // end namespace comparison
 } // end namespace abigail
 
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index ee23aff6..a0c086d9 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -98,12 +98,16 @@ test-abidiff/test-PR27985-v0.o.abi	 \
 test-abidiff/test-PR27985-v1.c		 \
 test-abidiff/test-PR27985-v1.o		 \
 test-abidiff/test-PR27985-v1.o.abi	 \
+test-abidiff/test-PR27616-squished-v0.abi \
+test-abidiff/test-PR27616-squished-v1.abi \
+test-abidiff/test-PR27616-v0.xml \
+test-abidiff/test-PR27616-v1.xml \
+\
 test-abidiff-exit/test-PR28316-v0.cc     \
 test-abidiff-exit/test-PR28316-v1.cc     \
 test-abidiff-exit/test-PR28316-v0.o      \
 test-abidiff-exit/test-PR28316-v1.o      \
 test-abidiff-exit/test-PR28316-report.txt \
-\
 test-abidiff-exit/test1-voffset-change-report0.txt \
 test-abidiff-exit/test1-voffset-change-report1.txt \
 test-abidiff-exit/test1-voffset-change.abignore \
@@ -218,10 +222,12 @@ test-abidiff-exit/test-crc-v1.abi \
 test-abidiff-exit/test-missing-alias-report.txt \
 test-abidiff-exit/test-missing-alias.abi \
 test-abidiff-exit/test-missing-alias.suppr \
-test-abidiff/test-PR27616-squished-v0.abi \
-test-abidiff/test-PR27616-squished-v1.abi \
-test-abidiff/test-PR27616-v0.xml \
-test-abidiff/test-PR27616-v1.xml \
+test-abidiff-exit/test-PR29144-report-2.txt \
+test-abidiff-exit/test-PR29144-report.txt \
+test-abidiff-exit/test-PR29144-v0.cc \
+test-abidiff-exit/test-PR29144-v0.o \
+test-abidiff-exit/test-PR29144-v1.cc \
+test-abidiff-exit/test-PR29144-v1.o \
 \
 test-diff-dwarf/test0-v0.cc		\
 test-diff-dwarf/test0-v0.o			\
diff --git a/tests/data/test-abidiff-exit/test-PR29144-report-2.txt b/tests/data/test-abidiff-exit/test-PR29144-report-2.txt
new file mode 100644
index 00000000..3aff12d4
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-PR29144-report-2.txt
@@ -0,0 +1,8 @@
+Leaf changes summary: 1 artifact changed
+Changed leaf types summary: 1 leaf type changed
+Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
+Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
+
+'struct D at 1.cc:13:1' changed:
+  type size hasn't changed
+  base classes of 'struct D' are re-ordered from: 'A, B, C' to: 'B, A, C'
diff --git a/tests/data/test-abidiff-exit/test-PR29144-report.txt b/tests/data/test-abidiff-exit/test-PR29144-report.txt
new file mode 100644
index 00000000..88c78fdf
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-PR29144-report.txt
@@ -0,0 +1,10 @@
+Functions changes summary: 0 Removed, 0 Changed, 0 Added function
+Variables changes summary: 0 Removed, 1 Changed, 0 Added variable
+
+1 Changed variable:
+
+  [C] 'D order' was changed at 2.cc:17:1:
+    type of variable changed:
+      type size hasn't changed
+      base classes of 'struct D' are re-ordered from: 'A, B, C' to: 'B, A, C'
+
diff --git a/tests/data/test-abidiff-exit/test-PR29144-v0.cc b/tests/data/test-abidiff-exit/test-PR29144-v0.cc
new file mode 100644
index 00000000..8805cfd9
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-PR29144-v0.cc
@@ -0,0 +1,17 @@
+struct A {
+  int x;
+};
+
+struct B {
+  int y;
+};
+
+struct C {
+  int z;
+};
+
+struct D: A, B, C {
+  int d;
+};
+
+D order;
diff --git a/tests/data/test-abidiff-exit/test-PR29144-v0.o b/tests/data/test-abidiff-exit/test-PR29144-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..2964a6802a82dbcbcaad22cb5a6be6de85804576
GIT binary patch
literal 2560
zcmbVNOK;Oa5T3P@l42T~M&i|qxClgf#A%|?mZFwGfPz4XhY%;IoMf9C^1^XZ3kNE}
z0SR$HLL4}A;K+?Dzk?Hhfink~S?>mSYXo8>@63EV-|Toin>@RC?UKP5C}FS&zdeNl
zRFV-}m=(yv7`#I}2`oiWf|f~Il(2J<U`6zdbudU{a3bk-0$}GMk-SPf8!!~^uqq^m
z?-TR{c1q|1jE+1<$UKM(m~*%!OroWX2Baxy7YyMFEMpRN*d#{?%Q7=%S@V<ZjCFc4
z#j==q1k98Kc3jEMS(tJHLL-r3N77l#%#G(p_hOlc!Lk-D6JKno3}$-X!YtO6?AiUs
zK1@G>k0Fi`Z!WCpQbH#QDL9UrG4>-pLW&xT4;neqV}s?4-C-I*?)nYx!_xITcJ;)G
z;;daPIYp;vm!?al=~L6Q_S7xjurIrT+{hP?+4**`*5M0H-r>GiN7wc1s|)LMGx?da
zoo_;6wcF-}Mz`_6D>S{J(DG_-&1<?|tFYp&^M0Y{2fQEj3pZ|+P8Z8%C_44}f3gG>
zma5fxdkSCT*gw=SX2!;sr}07#HkEx5b0iu<?FxO-FNwt|+E2jupK*I;IGNkF%y4vK
z@ZoJ30Vd$U#E6MQs0$5c1{q35l_8oGerST!`0PaccQn9=j3=CWBZeIHSOh05ye9yA
zG+erHAc9Bx`XYiyt~R`pIQ4Ufv`MM+sh-Hh!XX($IK|9J|D=XDrLXE$@hypi)8A+Z
zZVh$dhk8|60bdWmX}Ez4POaYu$LB5AY4F-wbJ^>xba7AEZms6?$6K~t-|aN{R!Yn3
za7Do*lu7Wi@ax@no8xbk)9D7>!Kuvm1GoNA`pwRo)APGM?gtyH`2W}ANTpt@{;EtB
zWoTNz2a~u@Saf!XQlTYAbpBC{p&9%We=xT)m+sT|)N@j3g%*_ep&G1uEX@hcWn~Z%
z8k@49j}oVIDNfxN`Vl5sz(>zX_*Ys`?uBZw>ajO)N5?35R)RJ;5z%AmzbLN$DFRVc
zom4;FC%yg#VtP)5;I0JKeIkrRk6lJA?)-{8|FU2K(E0SMrk~$KjB-xn6P4HUpxaRp
zZFRoVPbH@FzsZ0{f8oFS1OKKG5(|Ifzm<F~3^pZS$5q`5|9}R47d>3@_4iW!Q+&#&
rb7}9B1iz4jlEEkL`_nhk?;geXB&h$h==D?HIQ~od{XbK(i0J%ZNgcXU

literal 0
HcmV?d00001

diff --git a/tests/data/test-abidiff-exit/test-PR29144-v1.cc b/tests/data/test-abidiff-exit/test-PR29144-v1.cc
new file mode 100644
index 00000000..eb476828
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-PR29144-v1.cc
@@ -0,0 +1,18 @@
+struct A {
+  int x;
+};
+
+struct B {
+  int y;
+};
+
+struct C {
+  int z;
+};
+
+struct D: B, A, C {
+  int d;
+};
+
+D order;
+
diff --git a/tests/data/test-abidiff-exit/test-PR29144-v1.o b/tests/data/test-abidiff-exit/test-PR29144-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..03eb7a91c2c5768af8fab0e3eca907ddd481d8f2
GIT binary patch
literal 2560
zcmbVNOK;Oa5T3P@l42T~M&i|qxClgf#A%|?mZFwGfPz4XhY%;IoMf9C^1^XZ3kNE}
z0SR$HLL4}A;K+?Dzk?Hhfink~S?>mSYXo8>@63EV-|Toin>@RC?UKP5C}FS&zdeNl
zRFV-}m=(yv7`#I}2`oiWf|f~Il(2J<U`6zdbudU{a3bk-0$}GMk-SPf8!!~^uqq^m
z?-TR{c1q|1jE+1<$ehCk%!9ZjOroWX2Baxy7YyMFEMpRN*d#{?%Q7=%S@V<ZjCFc4
z#j==q1k98Kc3jEMS(tJHLL-r3N77l#%#G(p_hOlc!Lk-D6JKno3}$-X!YtO6?AiUs
zK1@G>k0Fi`Z!WCpQbH#QDL9UrG4>-pLW&xT4;neqV}s?4-C-I*?)nYx!_xITcJ;)G
z;;daPIYp;vm!?al=~L6Q_S7xjurIrT+{hP?+4**`*5M0H-r>GiN7wc1s|)LMGx?da
zoo_;6wcF-}Mz`_6D>S{J(DG_-&1<?|tFYp&^M0Y{2fQEj3pZ|+P8Z8%C^_}|f3gG>
zma5fxdkSCT*gw=SX2!;sr}07#HkEx5b0iu<?FxO-FNwt|+E2jupK*I;IGNkF%y4vK
z@ZoJ30Vd$U#E6MQs0$5c1{q35l_8oGerST!`0PaccQn9=j3=CWBZeIHSOh05ye9yA
zG+erHAc9Bx`XYiyt~R`pIQ4Ufv`MM+sh-Hh!XX($IK|9J|D=XDrLXE$@hypi)8A+Z
zZVh$dhk8|60bdWmX}Ez4POaYu$LB5AY4F-wbJ^>xba7AEZms6?$6K~t-|aN{R!Yn3
za7Do*lu7Wi@ax@no8xbk)9D7>!Kuvm1GoNA`pwRo)APGM?gtyH`2W}ANTpt@{;EtB
zWoTNz2a~u@Saf!XQlTYAbpBC{p&9%We=xT)m+sT|)N@j3g%*_ep&G1uEX@hcWn~Z%
z8k@49j}oVIDNfxN`Vl5sz(>zX_*Ys`?uBZw>ajO)N5?35R)RJ;5z%AmzbLN$DFRVc
zom4;FC%yg#VtP)5;I0JKeIkrRk6lJA?)-{8|FU2K(E0SMrk~$KjB-xn6P4HUpxaRp
zZFRoVPbH@FzsZ0{f8oFS1OKKG5(|Ifzm<F~3^pZS$5q`5|9}R47d>3@_4iW!Q+&#&
rb7}9B1iz4jlEEkL`_nhk?;geXB&h$h==D?HIQ~od{XbK(i0J%ZXQaAT

literal 0
HcmV?d00001

diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 15acfed9..4932d949 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -427,6 +427,28 @@ InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/test-PR28316-report.txt",
     "output/test-abidiff-exit/test-PR28316-report.txt"
   },
+  {
+    "data/test-abidiff-exit/test-PR29144-v0.o",
+    "data/test-abidiff-exit/test-PR29144-v1.o",
+    "",
+    "",
+    "",
+    "--no-default-suppression --harmless",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    "data/test-abidiff-exit/test-PR29144-report.txt",
+    "output/test-abidiff-exit/test-PR29144-report.txt"
+  },
+  {
+    "data/test-abidiff-exit/test-PR29144-v0.o",
+    "data/test-abidiff-exit/test-PR29144-v1.o",
+    "",
+    "",
+    "",
+    "--leaf-changes-only --no-default-suppression --harmless",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    "data/test-abidiff-exit/test-PR29144-report-2.txt",
+    "output/test-abidiff-exit/test-PR29144-report-2.txt"
+  },
   {0, 0, 0 ,0, 0, 0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
 };
 
-- 
2.36.1


-- 
		Dodji


^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2022-06-07  9:59 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-07  9:59 [PATCH] Bug 29144 - abidiff doesn't report base class re-ordering Dodji Seketeli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).