diff mbox series

[applied] Bug 28505 - Fix a case of pointer to void* change filtering

Message ID 87jz8540ip.fsf@redhat.com
State New
Headers
Series [applied] Bug 28505 - Fix a case of pointer to void* change filtering |

Commit Message

Dodji Seketeli March 31, 2025, 4:39 p.m. UTC 
  Hello,

Libabigail has started to categorize "void* to pointer" changes as
harmles since the patch below:

    Bug 23708 - categorize void* to pointer change as harmless

    Changing a void* pointer into another pointer of the same size is a
    change that is harmless in terms of data layout.

    This commit thus categorizes such a change as harmless.

I realized that since libabigail started to support "incompatible
changes" categorization, detecting pointer to void* changes and
classifying them as harmless started to misbehave.  Note that this is
a remnant of an ancient problem that was reported at
https://sourceware.org/bugzilla/show_bug.cgi?id=28505.

The issue with classifying pointer to void* change as harmless can be
summarized in the test below:

	$ diff -u PR28505-test-v0.c PR28505-test-v1.c
	--- PR28505-test-v0.c	2025-03-31 11:59:10.367026287 +0200
	+++ PR28505-test-v1.c	2025-03-31 11:59:17.111050102 +0200
	@@ -1,5 +1,5 @@
	-char*
	-chop(char*)
	+void*
	+chop(void*)
	 {
	   return 0;
	 }
	$

	$ abidiff PR28505-test-v0.o PR28505-test-v1.o
	Functions changes summary: 0 Removed, 1 Changed, 0 Added function
	Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

	1 function with incompatible sub-type changes:
$

This is because when a pointer-to-void* change is detected, the
underlying diff node where the distinct leaf-type-to-void change
occurred usually has a NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY or a
NON_COMPATIBLE_NAME_CHANGE_CATEGORY.  When that category gets propagated to
the pointer-to-void* diff node, it gets transformed into a
VOID_PTR_TO_PTR_CHANGE_CATEGORY.

The problem is that transforming the
NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY or
NON_COMPATIBLE_NAME_CHANGE_CATEGORY into a
VOID_PTR_TO_PTR_CHANGE_CATEGORY only happens in the category of the
pointer diff node, not on its /local/ category, as it should.  Yet
that local category is used to determine the non-compatible changes on
function return types, for instance.  Note that local changes or
pointed-to-types are considered local to pointers (and reference)
types.

This patch fixes the issue by clearing the
NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY or
NON_COMPATIBLE_NAME_CHANGE_CATEGORY category from local categories of
pointer and reference diff nodes when propagating
NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY or
NON_COMPATIBLE_NAME_CHANGE_CATEGORY from their underlying diff nodes.

	* include/abg-comparison.h (print_category): Declare ...
	* src/abg-comparison.cc (print_category): ... new debugging
	function.
	(category_propagation_visitor::visit_end): Clear
	NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY
	NON_COMPATIBLE_NAME_CHANGE_CATEGORY from the local category of
	pointer/reference diff nodes when propagating those categorizing
	bits from the underlying diff nodes of pointer/reference diff
	nodes.
	* tests/data/test-abidiff-exit/PR28505-test-report.txt: New test
	reference output file.
	* tests/data/test-abidiff-exit/PR28505-test-report-2.txt:
	Likewise.
	* tests/data/test-abidiff-exit/PR28505-test-v{0,1}.{c,o}: Source
	code and binary test input files.
	* tests/data/Makefile.am: Add the new test material above to
	source distribution.
	* tests/test-abidiff-exit.cc (in_out_specs): Add the new input
	tests to this test harness.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to master.
---
 include/abg-comparison.h                      |   3 ++
 src/abg-comparison.cc                         |  24 ++++++++++++++
 tests/data/Makefile.am                        |   6 ++++
 .../PR28505-test-report-2.txt                 |  15 +++++++++
 .../test-abidiff-exit/PR28505-test-report.txt |   3 ++
 .../data/test-abidiff-exit/PR28505-test-v0.c  |   5 +++
 .../data/test-abidiff-exit/PR28505-test-v0.o  | Bin 0 -> 2992 bytes
 .../data/test-abidiff-exit/PR28505-test-v1.c  |   5 +++
 .../data/test-abidiff-exit/PR28505-test-v1.o  | Bin 0 -> 2944 bytes
 tests/test-abidiff-exit.cc                    |  30 ++++++++++++++++++
 10 files changed, 91 insertions(+)
 create mode 100644 tests/data/test-abidiff-exit/PR28505-test-report-2.txt
 create mode 100644 tests/data/test-abidiff-exit/PR28505-test-report.txt
 create mode 100644 tests/data/test-abidiff-exit/PR28505-test-v0.c
 create mode 100644 tests/data/test-abidiff-exit/PR28505-test-v0.o
 create mode 100644 tests/data/test-abidiff-exit/PR28505-test-v1.c
 create mode 100644 tests/data/test-abidiff-exit/PR28505-test-v1.o

new file mode 100644
index 00000000..fe9cf958
index 17cf0a5f..ee566d2c 100644
diff mbox series

Patch

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index 49952553..db434ed3 100644
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -2960,6 +2960,9 @@  void
 print_diff_tree(corpus_diff_sptr diff_tree,
 		std::ostream&);
 
+void
+print_category(diff_category c);
+
 void
 categorize_redundancy(diff* diff_tree);
 
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index e5ed1435..63f7d42c 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -12963,6 +12963,21 @@  struct category_propagation_visitor : public diff_node_visitor
 	c &= (~NON_COMPATIBLE_NAME_CHANGE_CATEGORY
 	      & ~NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY);
 	d->set_category(c);
+	if (is_pointer_diff(d) || is_reference_diff(d))
+	  {
+	    // For pointers and references, changes to
+	    // pointed-to-types are considered local.  So, if some
+	    // pointed-to-types have non-compatible name or distinct
+	    // change, then the local category of the
+	    // pointer/reference will reflect that.  Let's thus clear
+	    // those NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY and
+	    // NON_COMPATIBLE_NAME_CHANGE_CATEGORY bits from the local
+	    // category as well.
+	    c = d->get_local_category();
+	    c &= (~NON_COMPATIBLE_NAME_CHANGE_CATEGORY
+		  & ~NON_COMPATIBLE_DISTINCT_CHANGE_CATEGORY);
+	    d->set_local_category(c);
+	  }
       }
 
     if (filtering::has_benign_array_of_unknown_size_change(d))
@@ -13533,6 +13548,15 @@  print_diff_tree(corpus_diff_sptr diff_tree,
 		std::ostream& o)
 {print_diff_tree(diff_tree.get(), o);}
 
+/// Print a given category out to stdout for debuging purposes
+///
+/// @param c the category to print to stdout.
+void
+print_category(diff_category c)
+{
+  std::cout << c << std::endl;
+}
+
 // <redundancy_marking_visitor>
 
 /// A tree visitor to categorize nodes with respect to the
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 7e63399c..4417ce6e 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -491,6 +491,12 @@  test-abidiff-exit/test-changed-var-1-v0.cc \
 test-abidiff-exit/test-changed-var-1-v0.o \
 test-abidiff-exit/test-changed-var-1-v1.cc \
 test-abidiff-exit/test-changed-var-1-v1.o \
+test-abidiff-exit/PR28505-test-report.txt \
+test-abidiff-exit/PR28505-test-report-2.txt \
+test-abidiff-exit/PR28505-test-v0.c \
+test-abidiff-exit/PR28505-test-v0.o \
+test-abidiff-exit/PR28505-test-v1.c \
+test-abidiff-exit/PR28505-test-v1.o \
 \
 test-diff-dwarf/test0-v0.cc		\
 test-diff-dwarf/test0-v0.o			\
diff --git a/tests/data/test-abidiff-exit/PR28505-test-report-2.txt b/tests/data/test-abidiff-exit/PR28505-test-report-2.txt
new file mode 100644
index 00000000..4e7cb185
--- /dev/null
+++ b/tests/data/test-abidiff-exit/PR28505-test-report-2.txt
@@ -0,0 +1,15 @@ 
+Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+1 function with some indirect sub-type change:
+
+  [C] 'function char* chop(char*)' at PR31642-test-v0.c:2:1 has some indirect sub-type changes:
+    return type changed:
+      in pointed to type 'char':
+        type name changed from 'char' to 'void'
+        type size changed from 8 to 0 (in bits)
+    parameter 1 of type 'char*' changed:
+      in pointed to type 'char':
+        type name changed from 'char' to 'void'
+        type size changed from 8 to 0 (in bits)
+
diff --git a/tests/data/test-abidiff-exit/PR28505-test-report.txt b/tests/data/test-abidiff-exit/PR28505-test-report.txt
new file mode 100644
index 00000000..9666a8fd
--- /dev/null
+++ b/tests/data/test-abidiff-exit/PR28505-test-report.txt
@@ -0,0 +1,3 @@ 
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
diff --git a/tests/data/test-abidiff-exit/PR28505-test-v0.c b/tests/data/test-abidiff-exit/PR28505-test-v0.c
new file mode 100644
index 00000000..2b292622
--- /dev/null
+++ b/tests/data/test-abidiff-exit/PR28505-test-v0.c
@@ -0,0 +1,5 @@ 
+char*
+chop(char*)
+{
+  return 0;
+}
diff --git a/tests/data/test-abidiff-exit/PR28505-test-v0.o b/tests/data/test-abidiff-exit/PR28505-test-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..3987d323e41794dbf7d3b11eee4ef858623441aa
GIT binary patch
literal 2992
zcmbtW&1)N15TCc}jh!f#KNOO<Zt=Dx<b&?6<Ty^^;8ex5vD=aumq2qU&PrNKn^;ni
zHbzM)Ed&aom!|aCV=lcF3Z=(F|A|5`z4Q<0&5%Pfvzn2fpEgjMf!&$;&F8-N=1E_D
z@X3dkQXr(@8Vof-0e(I-l*_4DhGS5GTkB8$S%327{%?eR{znzx4yXyz(ll?8q|=n9
zSR^Y;-PSO%s~-uq>191_z}Un#N{;Hcp=Q+Sd?9Bq%rDHS+41QU=;W*S=Is1R?rQEz
z?#$dqfehu7kLFYX(>SnEuucMuO+hZVfrxVog=;xGk6GWP8u$RDeWwz=q?G}{!pF!R
z^gWMUDfK)*NsK-h9~2hSFSpc!HPug>#yJWO57cAMk;jg@ZCU7l{l+bKt#rvPmHdl-
z(OoPqE)_48uDEBnqMExNCT#F7o^!os@}L#1)}vO`iL0oFooZvXcX`=cUh?)9U9S$+
zM%cku`#x-LT_`OtEqY1RO}xFLUxlF2Zbm_^UAr3x^*9N3<4RbG>tVbbuya7p-JljG
zVJdoPuf;n%Uet?|*Nz|UXcU3#Yild+SzMHJ|6xV&4E9Mq+5**^R`W;4C%(h=e-mMP
zh|+z0KR^AAwKjR^BXsD)nQz*<G5GgImeJ0f3_M+YddwIbEm>q^g)Fi`8#2wohH*_B
ziZ&b`Dw<?NPSb{>4M&HHCfPt!FiQ3-CcuWt5oa7JnAO&2Fgv3=oE}cugCiKHf3;*L
zT>rXRu1GjNLdRJUT>Rc(oc41tbts?L{G|ti0u86SpR&Hf+{{xk{BN)>-$MO3j1ik=
z+063}<I?{Y>vSg(3I2Uwg%WeYpRs<GxtZT(oYp{J2k^TOn@Lzfo^<-WAyN|c67cB`
z0Kd}hg5QaDL%$YP9@Mwv)=nF3sT@`+ooMfXIqZb3dUP;kH*Q4|f+a*Gcstdr?PfD-
zVaZlIiF}-VubYI`dz?6GZ0~f!X2c)znrt59X^mu`Bwd+>!MSCfe06#o6~fHPzNtHD
zvKfB{ZFKJbZ#?b4nUl&%{W>WcnZ9(c==LdVJ|y<|O8S%W8RP`XjNor)N~`!bbCTiL
z)CwST!0L$aCK|Hh#~3V1Aam1~etWa}-{fJFc<P^SO|$<RVrEW+V2MFlN5Y6qU-}o4
zRsU<Qe}{sQs6O2zX8rq!QBLU-#gra%q0a<hF7<_ffp}K@U+f_JAZa8{__wc!f6A9b
zAc4#!PWTTTZ@9tl2QHVPRpJEsiQ`RoiN7q4%w_!qeuhHU`v1-AFXxoje+nOo7o7Sr
wYa;~v3^ur7`W>NPMKhlE)SO*};0`r~Bz5}Q==q_(vf}r-|6e4Kxw(EX0SCO?o&W#<

literal 0
HcmV?d00001

diff --git a/tests/data/test-abidiff-exit/PR28505-test-v1.c b/tests/data/test-abidiff-exit/PR28505-test-v1.c
--- /dev/null
+++ b/tests/data/test-abidiff-exit/PR28505-test-v1.c
@@ -0,0 +1,5 @@ 
+void*
+chop(void*)
+{
+  return 0;
+}
diff --git a/tests/data/test-abidiff-exit/PR28505-test-v1.o b/tests/data/test-abidiff-exit/PR28505-test-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..4a772e105540f7c5aae59caa23e2a03f7766ec36
GIT binary patch
literal 2944
zcmbtV-D@0G6hC(++oauQ^RY!6Q^_buKX7KUF-_Z)=$4>OTd5&Zu%M)~yEnUIv%6(?
z8e&Bh3Wd^_Dn9yV9~55%L41;eFa8<6^sO&_EBK)2%$~D*b9YL`9=LPP`JK=EIbXi}
z{(Fv6Af(`J7-)h5e6?>Nms7C}hoA^o*6u!9yZgn{-w6BUkB{)112slkn&wrKOqwth
zhh*ibs|F@^^&O!ueVnfwFfs;eO1)7i=H0p3xhXY0I{7kM0`<;}TUgGY&7a91ow-mX
zL;3Kn8CAsC99$?mhXF<=AfLxPZ&EHC0BL`zL?0>H2Uxg-Jcqs~kSnGBE{qeSY2iR&
zE}fyH=A4OM+B7bwV1Hjdlsj-cr>;5<`k%ji#ak_%@=B%PWH9e7%r7j?pDLa4j&10g
zw-zOA@J}B1{AO~qrB~{@r8{vI)u>Z#tn8j%@|PC<odwUYL$%Sq0qYxYm6jG4{6u#X
zf2R~wA#AjpI;^#8pT=Q5PQvZD5>?`Q6mN&@9FlW4tVKzbiay$F@z$2FcjM%R<A*yM
zM&SJF>auqX*W>tqSd4U0@vQYo-P!=PJE>+5j*i{O^*@3z4We`(&lM)`IIH9PE}}yZ
z&ivBWjlh$CvW#x#4Zs7%r^k+Q(UL_jR>&e3jLT#=xG-vH1JQ;31I3VBpi@0eE*u;N
z5pkjL3_SY=xG+BCkM|0ujrHkA4sUaMLS+vQV4OK?$xOI8yJ@aSI1QnfSP)$NUSXW}
zvw!PQK4JJv4+Mn<PIWJ{zRcXtBRx~y9QzQPWZCwAopI@NgLQg)hy?$pr$ULj;15_o
z%iPZIF-~(e=L<pivt|-ikSCoUZ-|uW-2?)<@DNnGT?jgQI|^#La<je}x3=17OXa9i
z>FAw3=BN|3>UwX;cHGhuf+a*Gcr(?j?PgQAuw<*9=m4*|-%X<Gbxy1sn_Hczsd*wV
z$YwE~)=2hA(v?~0pHJ4w*QD1_A<VwPADB*tY{wr(8=bHJ8&CUh=cIB{|6@`NvVG}X
z(JNCnd`P^;PtQZfr;rmQcL9HkDXrkw&Pj%!QY*kY=E7{>bu`!*`99`=dFH}wU;4*p
z_3!azlX&W%UPrtC8e(=%gy0B+vW|oi*}j{IWz}Ej`qwB3iR#mPVb{Nb80C~cQB3JN
z7y3{D=2Bny=ZI&;|IQA-J}3U(GvX6|9Kz?sf6MWf8+^a#aT!>pZ$W<Kc-#F422P73
zb6G!uAE1!6{y+2j%Q>a>pTHsUf>S?sZG_-21{>Tk{V(Y6XUEf?+IJTrSfi$pq)ty8
UK0nk~R{Wpb|1T2A++M$b0r0%np#T5?

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
@@ -1548,6 +1548,36 @@  InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/test-changed-var-1-report-1.txt",
     "output/test-abidiff-exit/test-changed-var-1-report-1.txt"
   },
+  {
+    "data/test-abidiff-exit/PR28505-test-v0.o",
+    "data/test-abidiff-exit/PR28505-test-v1.o",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "--no-default-suppression",
+    abigail::tools_utils::ABIDIFF_OK,
+    "data/test-abidiff-exit/PR28505-test-report.txt",
+    "output/test-abidiff-exit/PR28505-test-report.txt"
+  },
+  {
+    "data/test-abidiff-exit/PR28505-test-v0.o",
+    "data/test-abidiff-exit/PR28505-test-v1.o",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "--no-default-suppression --harmless",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    "data/test-abidiff-exit/PR28505-test-report-2.txt",
+    "output/test-abidiff-exit/PR28505-test-report-2.txt"
+  },
 #ifdef WITH_BTF
   {
     "data/test-abidiff-exit/btf/test0-v0.o",