Message ID | 20200505180612.232158-1-gprocida@google.com |
---|---|
Headers |
From: gprocida@google.com (Giuliano Procida) Date: Tue, 5 May 2020 19:06:08 +0100 Subject: [PATCH 0/4] Fix incomplete function type bug in abg-reader Message-ID: <20200505180612.232158-1-gprocida@google.com> |
Series |
Fix incomplete function type bug in abg-reader
|
|
Message
Giuliano Procida
May 5, 2020, 6:06 p.m. UTC
This series is to be applied in sequence. The test can be moved after the other changes and folded into one commit, if you prefer. This seemed liked the simplest way to fix the issue, side-stepping some uses of type names completely. Regards, Giuliano. Giuliano Procida (4): Add XML reader test for incomplete function types. abg-reader.cc: Late canonicalise all types. abg-reader.cc: Strip out WIP type tracking. Pass bind_function_type_life_time a complete type. src/abg-reader.cc | 173 ++++-------------- tests/data/Makefile.am | 7 + .../test-fun-param-report.txt | 15 ++ .../test-abidiff-exit/test-fun-param-v0.abi | 44 +++++ .../test-abidiff-exit/test-fun-param-v0.c | 7 + .../test-abidiff-exit/test-fun-param-v0.o | Bin 0 -> 2992 bytes .../test-abidiff-exit/test-fun-param-v1.abi | 46 +++++ .../test-abidiff-exit/test-fun-param-v1.c | 7 + .../test-abidiff-exit/test-fun-param-v1.o | Bin 0 -> 3000 bytes tests/test-abidiff-exit.cc | 9 + 10 files changed, 168 insertions(+), 140 deletions(-) create mode 100644 tests/data/test-abidiff-exit/test-fun-param-report.txt create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.abi create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.c create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.o create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.abi create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.c create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.o
Comments
Giuliano Procida <gprocida@google.com> a écrit: > This series is to be applied in sequence. The test can be moved after > the other changes and folded into one commit, if you prefer. > > This seemed liked the simplest way to fix the issue, side-stepping > some uses of type names completely. I don't think the early canonicalization is the problem. I think the problem is that I wrongly introduced a change that does type name caching too early. That is, it caches type names on non-canonicalized types. The commit that introduced that change should never have went inn to begin with. My bad. Below is what I think is the proper fix. Also, I filed this bug to track this issue https://sourceware.org/bugzilla/show_bug.cgi?id=25986, so that I can refer to it in the commit. Here is the patch that should fix your issue. Please tell me if it fixes your issue. If it does, then I'd prefer to apply this one. From 15f19a0622daa51d2233e21405b1478b9437d6c3 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli <dodji@redhat.com> Date: Wed, 13 May 2020 16:22:01 +0200 Subject: [PATCH] Bug 25986 - Wrong name of function type used in change report We have introduced type name caching long ago to speed up operations involving type names. Then last year, I was looking at some speed optimization in the xml-writer and I started playing with the caching to quantify the speed impact that early caching could have on emitting writting out the abixml, independantly from the potential accuracy issues that could have. And somehow I accidentally committed one of those experimental patches: https://sourceware.org/git/?p=libabigail.git;a=commit;h=7699dfc921d248fa6d5cbdbeab9d501b17ef3f52. This commit reverts that bogus patch. There should be no speed impact because the writter now de-duplicates types at writting time by "simply" writting out the canonical types, as opposed to the naive approach we were using then. This fixes the bug reported at https://sourceware.org/bugzilla/show_bug.cgi?id=25986 which shows the impact of caching the wrong name of the type, which happens when caching occurs before the type is canonicalized. * src/abg-ir.cc (function_type::get_cached_name): Don't cache names for non-canonicalized types. * tests/data/test-abidiff-exit/test-fun-param-report.txt: Add reference output for new test. * tests/data/test-abidiff-exit/test-fun-param-v{0,1}.abi: Add new test input files. * tests/data/test-abidiff-exit/test-fun-param-v{0,1}.c: Add source files for the above. * tests/data/test-abidiff-exit/test-fun-param-v{0,1}.o: Add binaries for the above. Signed-off-by: Dodji Seketeli <dodji@redhat.com> --- src/abg-ir.cc | 4 +- tests/data/Makefile.am | 7 ++++ .../test-abidiff-exit/test-fun-param-report.txt | 15 +++++++ tests/data/test-abidiff-exit/test-fun-param-v0.abi | 44 ++++++++++++++++++++ tests/data/test-abidiff-exit/test-fun-param-v0.c | 7 ++++ tests/data/test-abidiff-exit/test-fun-param-v0.o | Bin 0 -> 2992 bytes tests/data/test-abidiff-exit/test-fun-param-v1.abi | 46 +++++++++++++++++++++ tests/data/test-abidiff-exit/test-fun-param-v1.c | 7 ++++ tests/data/test-abidiff-exit/test-fun-param-v1.o | Bin 0 -> 3000 bytes tests/test-abidiff-exit.cc | 9 ++++ 10 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 tests/data/test-abidiff-exit/test-fun-param-report.txt create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.abi create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.c create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.o create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.abi create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.c create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.o diff --git a/src/abg-ir.cc b/src/abg-ir.cc index bfcaf5d..7b1f460 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -16411,13 +16411,13 @@ function_type::get_cached_name(bool internal) const { if (internal) { - if (priv_->internal_cached_name_.empty()) + if (!get_naked_canonical_type() || priv_->internal_cached_name_.empty()) priv_->internal_cached_name_ = get_function_type_name(this, internal); return priv_->internal_cached_name_; } - if (priv_->cached_name_.empty()) + if (!get_naked_canonical_type() || priv_->cached_name_.empty()) priv_->cached_name_ = get_function_type_name(this, internal); return priv_->cached_name_; diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index a1b9bf6..7e91f52 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -151,6 +151,13 @@ test-abidiff-exit/test-decl-struct-v0.o \ test-abidiff-exit/test-decl-struct-v1.c \ test-abidiff-exit/test-decl-struct-v1.o \ test-abidiff-exit/test-decl-struct-report.txt \ +test-abidiff-exit/test-fun-param-report.txt \ +test-abidiff-exit/test-fun-param-v0.abi \ +test-abidiff-exit/test-fun-param-v0.c \ +test-abidiff-exit/test-fun-param-v0.o \ +test-abidiff-exit/test-fun-param-v1.abi \ +test-abidiff-exit/test-fun-param-v1.c \ +test-abidiff-exit/test-fun-param-v1.o \ \ test-diff-dwarf/test0-v0.cc \ test-diff-dwarf/test0-v0.o \ diff --git a/tests/data/test-abidiff-exit/test-fun-param-report.txt b/tests/data/test-abidiff-exit/test-fun-param-report.txt new file mode 100644 index 0000000..295cce8 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-fun-param-report.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 void reg(ops*)' at test-fun-param-v1.c:7:1 has some indirect sub-type changes: + parameter 1 of type 'ops*' has sub-type changes: + in pointed to type 'struct ops' at test-fun-param-v1.c:1:1: + type size hasn't changed + 1 data member change: + type of 'void (void*, unsigned int, unsigned long int)* ops::bind_class' changed: + in pointed to type 'function type void (void*, unsigned int, unsigned long int)': + parameter 4 of type 'void*' was added + parameter 5 of type 'unsigned long int' was added + diff --git a/tests/data/test-abidiff-exit/test-fun-param-v0.abi b/tests/data/test-abidiff-exit/test-fun-param-v0.abi new file mode 100644 index 0000000..816f74c --- /dev/null +++ b/tests/data/test-abidiff-exit/test-fun-param-v0.abi @@ -0,0 +1,44 @@ +<abi-corpus path='test-fun-param-v0.o' architecture='elf-amd-x86_64'> + <elf-function-symbols> + <elf-symbol name='reg' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/> + </elf-function-symbols> + <abi-instr version='1.0' address-size='64' path='test-fun-param-v0.c' comp-dir-path='/usr/local/google/home/gprocida/dev/libabigail' language='LANG_C99'> + <type-decl name='int' size-in-bits='32' id='type-id-1'/> + <type-decl name='unsigned int' size-in-bits='32' id='type-id-2'/> + <type-decl name='unsigned long int' size-in-bits='64' id='type-id-3'/> + <type-decl name='void' id='type-id-4'/> + <class-decl name='ops' size-in-bits='192' is-struct='yes' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' line='1' column='1' id='type-id-5'> + <data-member access='public' layout-offset-in-bits='0'> + <var-decl name='foo' type-id='type-id-6' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' line='2' column='1'/> + </data-member> + <data-member access='public' layout-offset-in-bits='64'> + <var-decl name='bind_class' type-id='type-id-7' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' line='3' column='1'/> + </data-member> + <data-member access='public' layout-offset-in-bits='128'> + <var-decl name='bar' type-id='type-id-8' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' line='4' column='1'/> + </data-member> + </class-decl> + <pointer-type-def type-id='type-id-9' size-in-bits='64' id='type-id-8'/> + <pointer-type-def type-id='type-id-5' size-in-bits='64' id='type-id-10'/> + <pointer-type-def type-id='type-id-11' size-in-bits='64' id='type-id-6'/> + <pointer-type-def type-id='type-id-12' size-in-bits='64' id='type-id-7'/> + <pointer-type-def type-id='type-id-4' size-in-bits='64' id='type-id-13'/> + <function-decl name='reg' mangled-name='reg' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' line='7' column='1' visibility='default' binding='global' size-in-bits='64' elf-symbol-id='reg'> + <parameter type-id='type-id-10' name='o' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' line='7' column='1'/> + <return type-id='type-id-4'/> + </function-decl> + <function-type size-in-bits='64' id='type-id-9'> + <parameter type-id='type-id-1'/> + <return type-id='type-id-1'/> + </function-type> + <function-type size-in-bits='64' id='type-id-11'> + <return type-id='type-id-4'/> + </function-type> + <function-type size-in-bits='64' id='type-id-12'> + <parameter type-id='type-id-13'/> + <parameter type-id='type-id-2'/> + <parameter type-id='type-id-3'/> + <return type-id='type-id-4'/> + </function-type> + </abi-instr> +</abi-corpus> diff --git a/tests/data/test-abidiff-exit/test-fun-param-v0.c b/tests/data/test-abidiff-exit/test-fun-param-v0.c new file mode 100644 index 0000000..aa1198c --- /dev/null +++ b/tests/data/test-abidiff-exit/test-fun-param-v0.c @@ -0,0 +1,7 @@ +struct ops { + void(*foo)(void); + void(*bind_class)(void *, unsigned int, unsigned long); + int(*bar)(int); +}; + +void reg(struct ops* o) { (void)o; } diff --git a/tests/data/test-abidiff-exit/test-fun-param-v0.o b/tests/data/test-abidiff-exit/test-fun-param-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..64218730b3fa188f61752ebbc917f49a75a92785 GIT binary patch literal 2992 zcmbtW&2QsW5T6(4!>OBW+E7R+5=LTG7GWnXXlWO92^6|5yINI9dqkC;IF3bPSGH5S zAU*^LSPp2BkPs3l_J+ir0~f?U!2yW_LPDID+e#c@#(ppByt)?{HSf**X6DVu`*7cV z`Py!QF%ZPyGTfI03UIf4pYJ7X51OzHH}-D*zIW^Gd$(Wv=~sk{iAqgYSWNiLpk??% za1=p{G1W#;83qBcLbXo@$r4PdeuliN*j5OvsD6t=14`w@yz&mIlHon#KE?s6>O!$X z%z~OsWz|DLyMqJ$R2pEhSn~WB%IX&=)I!GvR+$wh(qmu$g4#M}RC$oN!sjFOFpvYb z#Lm~7no?h_FR|rH;~~^GnWAdiCG9zFOJhwerUr_diu=t?T(pQlJqwgpV66>MJ_nk% zj~<m3w$>(BwFD$rNr8ntj?-lfhGme&ev@n#IjUj-CVRduVy(3)qh9OF7n7yxrA!uu z7UupGl|n@Tx@Jg}oq-Z|7e@u2#F4ST>opvcV}e`DAa<hI7*2iT$O^5AaojbNt%Fh< zs%Z>#Yn7JXFDU3Y^&`*qoq<lh9|yjx&mo;@6n4fz-x_z^AaKV{XB14Fj(ZdaeQ#iO z2F`J3?Aey>xt2GEE7x!6JL}KtTjqwju5WZVy7>3BZcO5-?`*q{?}T0-MJw!&wof)M z8W*3@4OcgYR&>+vkHWwYrjarA-|+l_5nJ}yiNHqwus^n<D8IBTJ3E*3Cth-F&+^m2 z#>VCaDTSnK4<sY@=*?Gwoiy0$Vnw};J$nRk`Vl1^f1%#^sIXHj@1j8p=Z<ufityJz zc}nbU9*ny-+k-j1f|@*1__dXF3e4`toJxV@hRtClC{R6xrFi}sQ=oQ+Js$ZSQsC5+ z0xM{@oaw_XQr;1z3(rYZwt%wWNhKl>PSso|5NRo$K;YkTnc@=({Lxedyx$4Y4rO=a z0>{ZdyYG7$oZ^1N<qssBi}0z0i%x#eaXGH2k>ZxbA~-kBu-`fZ*Evo#W$OYnx;eqy zgmWCG^GI;9a}t9&uwn~L8?TrdI%CTmIQG;%^!#Cfx`elEJ9Lic)cCykE?&^A%h>ZB z;esK+><5#HgHMa;2eD(~`(Q+|)qkBkIitfNK2i=({C`fX=uf>B{S?dfI5RtcT$XR> zV~7(b-^8ojUY=zCH8h}e^I!ef(N|uRVv6{`k&q$TmcAnNh3;i+h`pD7`Qa|d>0A*c z&NzL6h_>-3uSvMc!5>J-kZgN^IvFGLKRMXV_!2AIQk{AE|G_VX@TdIgm6P)y<gAGh zJj^e{D<ot{j(>={y!b7CphTZ2KD`5S{3FC@O_3+E2@QDs9|gc?5ns%2A)e=do*yVW zUs)1<V*U`7g3sDrlMf_U%C=u||GBpVu><bEnOzilQNMucAGf^zb#wYhUlHLi`bzo8 vv1#p3c>DvNFunKmrpo@bCwZm*Oi3Y$cR!U*_lnld^WWq7-w}>{mizZFd^q39 literal 0 HcmV?d00001 diff --git a/tests/data/test-abidiff-exit/test-fun-param-v1.abi b/tests/data/test-abidiff-exit/test-fun-param-v1.abi new file mode 100644 index 0000000..dcc91d1 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-fun-param-v1.abi @@ -0,0 +1,46 @@ +<abi-corpus path='test-fun-param-v1.o' architecture='elf-amd-x86_64'> + <elf-function-symbols> + <elf-symbol name='reg' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/> + </elf-function-symbols> + <abi-instr version='1.0' address-size='64' path='test-fun-param-v1.c' comp-dir-path='/usr/local/google/home/gprocida/dev/libabigail' language='LANG_C99'> + <type-decl name='int' size-in-bits='32' id='type-id-1'/> + <type-decl name='unsigned int' size-in-bits='32' id='type-id-2'/> + <type-decl name='unsigned long int' size-in-bits='64' id='type-id-3'/> + <type-decl name='void' id='type-id-4'/> + <class-decl name='ops' size-in-bits='192' is-struct='yes' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' line='1' column='1' id='type-id-5'> + <data-member access='public' layout-offset-in-bits='0'> + <var-decl name='foo' type-id='type-id-6' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' line='2' column='1'/> + </data-member> + <data-member access='public' layout-offset-in-bits='64'> + <var-decl name='bind_class' type-id='type-id-7' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' line='3' column='1'/> + </data-member> + <data-member access='public' layout-offset-in-bits='128'> + <var-decl name='bar' type-id='type-id-8' visibility='default' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' line='4' column='1'/> + </data-member> + </class-decl> + <pointer-type-def type-id='type-id-9' size-in-bits='64' id='type-id-8'/> + <pointer-type-def type-id='type-id-5' size-in-bits='64' id='type-id-10'/> + <pointer-type-def type-id='type-id-11' size-in-bits='64' id='type-id-6'/> + <pointer-type-def type-id='type-id-12' size-in-bits='64' id='type-id-7'/> + <pointer-type-def type-id='type-id-4' size-in-bits='64' id='type-id-13'/> + <function-decl name='reg' mangled-name='reg' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' line='7' column='1' visibility='default' binding='global' size-in-bits='64' elf-symbol-id='reg'> + <parameter type-id='type-id-10' name='o' filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' line='7' column='1'/> + <return type-id='type-id-4'/> + </function-decl> + <function-type size-in-bits='64' id='type-id-9'> + <parameter type-id='type-id-1'/> + <return type-id='type-id-1'/> + </function-type> + <function-type size-in-bits='64' id='type-id-11'> + <return type-id='type-id-4'/> + </function-type> + <function-type size-in-bits='64' id='type-id-12'> + <parameter type-id='type-id-13'/> + <parameter type-id='type-id-2'/> + <parameter type-id='type-id-3'/> + <parameter type-id='type-id-13'/> + <parameter type-id='type-id-3'/> + <return type-id='type-id-4'/> + </function-type> + </abi-instr> +</abi-corpus> diff --git a/tests/data/test-abidiff-exit/test-fun-param-v1.c b/tests/data/test-abidiff-exit/test-fun-param-v1.c new file mode 100644 index 0000000..e47f49c --- /dev/null +++ b/tests/data/test-abidiff-exit/test-fun-param-v1.c @@ -0,0 +1,7 @@ +struct ops { + void(*foo)(void); + void(*bind_class)(void *, unsigned int, unsigned long, void *, unsigned long); + int(*bar)(int); +}; + +void reg(struct ops* o) { (void) o; } diff --git a/tests/data/test-abidiff-exit/test-fun-param-v1.o b/tests/data/test-abidiff-exit/test-fun-param-v1.o new file mode 100644 index 0000000000000000000000000000000000000000..c84e4f0fe209d872c2309acefceba2cce51a998f GIT binary patch literal 3000 zcmbtV-D@LN6hAkUk4dMAX=Amef+MWd6=%{`S<{uS(Q3EVWhugnFH&Zb$z(8@DKnGW zRYVXGR}i`&s31P*vu{57qW&YkxZs1Zf)5KmxaUmn&EDKjU-XdNd(Q8ibI-^9m=9mQ zwp(Bf1TnY*4<vyC+$%reI~m)7CM?2@y<5NU-TL6(>D#~lfp9TX>DdbN87~cn#22EY z1eO`oT?Dm$6agz#yJV0p!L;gE$m^Qx#K4OBk0>;tRL;yRpOPvYJ|ph-R3r|n`dqO> z%$%N0Wj#Q__yGs{>omZ8u@r<Ul=W{>sKuTStTHW8B+9<IgW5XgR(XWD!tDWiSjYie zU{BVYhE`v#FR;Z*<5AQ$nWh`YW#a{7%V13`s|K1r5%-&IT(pQlT?R@^u(kqFeh3U> zA3Z8dY;A>H^%9U=B?T680jDb%49g&k{U+JWb5zFyZ1&=ch_$vt8FiMwd^uaHUXrpX zEMxA^P$^UdpsPfpYzfNPT^u!d8b`+dsn>AK4h(KGgVak>t3M8{BPVu7*71g&Z6cIb zpgM_xZp)>Gj|v+4jl(4HL$7C2?}t(7n=?pjoW!kR)OCg}KZ^XJ*BV44ujL=bQ8(y0 zt)6$>8V0W82EG#v;p+7p=FZ0R=9ax_Z<w3wo9p;{*0e_HIP|uCFZAM|i=q>E2iqs@ zOV*|5Ov^W|zLVSxyMs6iqj6%5!*_$QXQhri^b&B9KkN>jB*`!B>dww(^Ql)nH*ms9 zU~98|QAq(yei;8m?7cUDoiy0$d__OSjy;AL{fV-Ezf^C0QrM}LchO)1=Z<uvityLJ zc}nbT9*lc7-GLdshMGLm`1MtF8cc7-j7o#-e$8McXiz<aP4N5^ra|o-dpz(tq`{e| z1(wimQR>4qQr-cj>&{73Hixp{NhKl>PSso|5NTmDfxxrJ1Tl(FB=E;4BH-=Lh;~Tc zjf)&7dwJjYB%I=Y$K}rzoQv><f{RZ6!f`dOsFC89#3DF1&avM*2RAuRHOX~>o!lJZ zUBWqyC-XpXsdtis-E&e0Y!@$>9eYE^?s@LmKMcZtgu0A(TsQWPXVmz(gg#zR*<~1n zo^ZhsV0WX@$is)l4x`kw@pZ70)aky%oxH(eAD<|XC;mUDRrIIcihhdaI-HrFKQ5~` z^a;cXQ*Ywy++Lkj|1~tAbMs&QH_%sIlVXbaf07`PYD?b``cii!n~dd$>MqCWT#=nP z<MjO@+Qz@SCgE)k{zQUAs_g;lRE*63=HR;IORQ>3b>`)Nm*+41DSvw9)ckunYa#>} z_+@yF1c}u6hp5YoZ}I~r`b6>R9Z=&RAx3M8Jdw>P<nezN0G~yCF~5&^o_~uUC^}y< z2|qD^j7q`Fc5kW&k}Fl)uetxs+kx0y+`la^ioB>_!1V6r^{<`NKl+LYf6-UUM~zKu spYr$zJYjn8=}lGrX;1P>{f&}B67T**I@>E+H_!io=l{KM<g?np`#%ohBme*a literal 0 HcmV?d00001 diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc index 5afc15b..4d9c194 100644 --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -203,6 +203,15 @@ InOutSpec in_out_specs[] = "data/test-abidiff-exit/test-decl-struct-report.txt", "output/test-abidiff-exit/test-decl-struct-report.txt" }, + { + "data/test-abidiff-exit/test-fun-param-v0.abi", + "data/test-abidiff-exit/test-fun-param-v1.abi", + "", + "", + abigail::tools_utils::ABIDIFF_ABI_CHANGE, + "data/test-abidiff-exit/test-fun-param-report.txt", + "output/test-abidiff-exit/test-fun-param-report.txt" + }, {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} };
Hi. On Wed, 13 May 2020 at 15:38, Dodji Seketeli <dodji@seketeli.org> wrote: > Giuliano Procida <gprocida@google.com> a écrit: > > > This series is to be applied in sequence. The test can be moved after > > the other changes and folded into one commit, if you prefer. > > > > This seemed liked the simplest way to fix the issue, side-stepping > > some uses of type names completely. > > I don't think the early canonicalization is the problem. I think the > problem is that I wrongly introduced a change that does type name > caching too early. That is, it caches type names on non-canonicalized > types. The commit that introduced that change should never have went > inn to begin with. My bad. > The change to eliminate early canonicalisation has some residual value in terms of code simplification. Let me know if you'd like me to adjust it and repost it with that as the aim. Otherwise, I'll leave it be. > Below is what I think is the proper fix. > > Also, I filed this bug to track this issue > https://sourceware.org/bugzilla/show_bug.cgi?id=25986, so that I can > refer to it in the commit. > > Here is the patch that should fix your issue. Please tell me if it > fixes your issue. If it does, then I'd prefer to apply this one. > It clearly fixes the minimal test case. I've also run the code on some much larger XML-XML comparisons both with and without --leaf-changes-only and it appears to fix the issue and not do anything else. So, yes, it does. Regards, Giuliano. > From 15f19a0622daa51d2233e21405b1478b9437d6c3 Mon Sep 17 00:00:00 2001 > From: Dodji Seketeli <dodji@redhat.com> > Date: Wed, 13 May 2020 16:22:01 +0200 > Subject: [PATCH] Bug 25986 - Wrong name of function type used in change > report > > We have introduced type name caching long ago to speed up operations > involving type names. > > Then last year, I was looking at some speed optimization in the > xml-writer and I started playing with the caching to quantify the > speed impact that early caching could have on emitting writting out > the abixml, independantly from the potential accuracy issues that > could have. > > And somehow I accidentally committed one of those experimental > patches: > > https://sourceware.org/git/?p=libabigail.git;a=commit;h=7699dfc921d248fa6d5cbdbeab9d501b17ef3f52 > . > > This commit reverts that bogus patch. There should be no speed impact > because the writter now de-duplicates types at writting time by > "simply" writting out the canonical types, as opposed to the naive > approach we were using then. > > This fixes the bug reported at > https://sourceware.org/bugzilla/show_bug.cgi?id=25986 which shows the > impact of caching the wrong name of the type, which happens when > caching occurs before the type is canonicalized. > > * src/abg-ir.cc (function_type::get_cached_name): Don't cache > names for non-canonicalized types. > * tests/data/test-abidiff-exit/test-fun-param-report.txt: Add > reference output for new test. > * tests/data/test-abidiff-exit/test-fun-param-v{0,1}.abi: Add new > test > input files. > * tests/data/test-abidiff-exit/test-fun-param-v{0,1}.c: Add source > files for the above. > * tests/data/test-abidiff-exit/test-fun-param-v{0,1}.o: Add > binaries for the above. > > Signed-off-by: Dodji Seketeli <dodji@redhat.com> > --- > src/abg-ir.cc | 4 +- > tests/data/Makefile.am | 7 ++++ > .../test-abidiff-exit/test-fun-param-report.txt | 15 +++++++ > tests/data/test-abidiff-exit/test-fun-param-v0.abi | 44 > ++++++++++++++++++++ > tests/data/test-abidiff-exit/test-fun-param-v0.c | 7 ++++ > tests/data/test-abidiff-exit/test-fun-param-v0.o | Bin 0 -> 2992 bytes > tests/data/test-abidiff-exit/test-fun-param-v1.abi | 46 > +++++++++++++++++++++ > tests/data/test-abidiff-exit/test-fun-param-v1.c | 7 ++++ > tests/data/test-abidiff-exit/test-fun-param-v1.o | Bin 0 -> 3000 bytes > tests/test-abidiff-exit.cc | 9 ++++ > 10 files changed, 137 insertions(+), 2 deletions(-) > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-report.txt > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.abi > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.c > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v0.o > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.abi > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.c > create mode 100644 tests/data/test-abidiff-exit/test-fun-param-v1.o > > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > index bfcaf5d..7b1f460 100644 > --- a/src/abg-ir.cc > +++ b/src/abg-ir.cc > @@ -16411,13 +16411,13 @@ function_type::get_cached_name(bool internal) > const > { > if (internal) > { > - if (priv_->internal_cached_name_.empty()) > + if (!get_naked_canonical_type() || > priv_->internal_cached_name_.empty()) > priv_->internal_cached_name_ = get_function_type_name(this, > internal); > > return priv_->internal_cached_name_; > } > > - if (priv_->cached_name_.empty()) > + if (!get_naked_canonical_type() || priv_->cached_name_.empty()) > priv_->cached_name_ = get_function_type_name(this, internal); > > return priv_->cached_name_; > diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am > index a1b9bf6..7e91f52 100644 > --- a/tests/data/Makefile.am > +++ b/tests/data/Makefile.am > @@ -151,6 +151,13 @@ test-abidiff-exit/test-decl-struct-v0.o \ > test-abidiff-exit/test-decl-struct-v1.c \ > test-abidiff-exit/test-decl-struct-v1.o \ > test-abidiff-exit/test-decl-struct-report.txt \ > +test-abidiff-exit/test-fun-param-report.txt \ > +test-abidiff-exit/test-fun-param-v0.abi \ > +test-abidiff-exit/test-fun-param-v0.c \ > +test-abidiff-exit/test-fun-param-v0.o \ > +test-abidiff-exit/test-fun-param-v1.abi \ > +test-abidiff-exit/test-fun-param-v1.c \ > +test-abidiff-exit/test-fun-param-v1.o \ > \ > test-diff-dwarf/test0-v0.cc \ > test-diff-dwarf/test0-v0.o \ > diff --git a/tests/data/test-abidiff-exit/test-fun-param-report.txt > b/tests/data/test-abidiff-exit/test-fun-param-report.txt > new file mode 100644 > index 0000000..295cce8 > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-fun-param-report.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 void reg(ops*)' at test-fun-param-v1.c:7:1 has some > indirect sub-type changes: > + parameter 1 of type 'ops*' has sub-type changes: > + in pointed to type 'struct ops' at test-fun-param-v1.c:1:1: > + type size hasn't changed > + 1 data member change: > + type of 'void (void*, unsigned int, unsigned long int)* > ops::bind_class' changed: > + in pointed to type 'function type void (void*, unsigned int, > unsigned long int)': > + parameter 4 of type 'void*' was added > + parameter 5 of type 'unsigned long int' was added > + > diff --git a/tests/data/test-abidiff-exit/test-fun-param-v0.abi > b/tests/data/test-abidiff-exit/test-fun-param-v0.abi > new file mode 100644 > index 0000000..816f74c > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-fun-param-v0.abi > @@ -0,0 +1,44 @@ > +<abi-corpus path='test-fun-param-v0.o' architecture='elf-amd-x86_64'> > + <elf-function-symbols> > + <elf-symbol name='reg' type='func-type' binding='global-binding' > visibility='default-visibility' is-defined='yes'/> > + </elf-function-symbols> > + <abi-instr version='1.0' address-size='64' path='test-fun-param-v0.c' > comp-dir-path='/usr/local/google/home/gprocida/dev/libabigail' > language='LANG_C99'> > + <type-decl name='int' size-in-bits='32' id='type-id-1'/> > + <type-decl name='unsigned int' size-in-bits='32' id='type-id-2'/> > + <type-decl name='unsigned long int' size-in-bits='64' id='type-id-3'/> > + <type-decl name='void' id='type-id-4'/> > + <class-decl name='ops' size-in-bits='192' is-struct='yes' > visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' > line='1' column='1' id='type-id-5'> > + <data-member access='public' layout-offset-in-bits='0'> > + <var-decl name='foo' type-id='type-id-6' visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' > line='2' column='1'/> > + </data-member> > + <data-member access='public' layout-offset-in-bits='64'> > + <var-decl name='bind_class' type-id='type-id-7' > visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' > line='3' column='1'/> > + </data-member> > + <data-member access='public' layout-offset-in-bits='128'> > + <var-decl name='bar' type-id='type-id-8' visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' > line='4' column='1'/> > + </data-member> > + </class-decl> > + <pointer-type-def type-id='type-id-9' size-in-bits='64' > id='type-id-8'/> > + <pointer-type-def type-id='type-id-5' size-in-bits='64' > id='type-id-10'/> > + <pointer-type-def type-id='type-id-11' size-in-bits='64' > id='type-id-6'/> > + <pointer-type-def type-id='type-id-12' size-in-bits='64' > id='type-id-7'/> > + <pointer-type-def type-id='type-id-4' size-in-bits='64' > id='type-id-13'/> > + <function-decl name='reg' mangled-name='reg' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' > line='7' column='1' visibility='default' binding='global' size-in-bits='64' > elf-symbol-id='reg'> > + <parameter type-id='type-id-10' name='o' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v0.c' > line='7' column='1'/> > + <return type-id='type-id-4'/> > + </function-decl> > + <function-type size-in-bits='64' id='type-id-9'> > + <parameter type-id='type-id-1'/> > + <return type-id='type-id-1'/> > + </function-type> > + <function-type size-in-bits='64' id='type-id-11'> > + <return type-id='type-id-4'/> > + </function-type> > + <function-type size-in-bits='64' id='type-id-12'> > + <parameter type-id='type-id-13'/> > + <parameter type-id='type-id-2'/> > + <parameter type-id='type-id-3'/> > + <return type-id='type-id-4'/> > + </function-type> > + </abi-instr> > +</abi-corpus> > diff --git a/tests/data/test-abidiff-exit/test-fun-param-v0.c > b/tests/data/test-abidiff-exit/test-fun-param-v0.c > new file mode 100644 > index 0000000..aa1198c > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-fun-param-v0.c > @@ -0,0 +1,7 @@ > +struct ops { > + void(*foo)(void); > + void(*bind_class)(void *, unsigned int, unsigned long); > + int(*bar)(int); > +}; > + > +void reg(struct ops* o) { (void)o; } > diff --git a/tests/data/test-abidiff-exit/test-fun-param-v0.o > b/tests/data/test-abidiff-exit/test-fun-param-v0.o > new file mode 100644 > index > 0000000000000000000000000000000000000000..64218730b3fa188f61752ebbc917f49a75a92785 > GIT binary patch > literal 2992 > zcmbtW&2QsW5T6(4!>OBW+E7R+5=LTG7GWnXXlWO92^6|5yINI9dqkC;IF3bPSGH5S > zAU*^LSPp2BkPs3l_J+ir0~f?U!2yW_LPDID+e#c@#(ppByt)?{HSf**X6DVu`*7cV > z`Py!QF%ZPyGTfI03UIf4pYJ7X51OzHH}-D*zIW^Gd$(Wv=~sk{iAqgYSWNiLpk??% > za1=p{G1W#;83qBcLbXo@$r4PdeuliN*j5OvsD6t=14`w@yz&mIlHon#KE?s6>O!$X > z%z~OsWz|DLyMqJ$R2pEhSn~WB%IX&=)I!GvR+$wh(qmu$g4#M}RC$oN!sjFOFpvYb > z#Lm~7no?h_FR|rH;~~^GnWAdiCG9zFOJhwerUr_diu=t?T(pQlJqwgpV66>MJ_nk% > zj~<m3w$>(BwFD$rNr8ntj?-lfhGme&ev@n#IjUj-CVRduVy(3)qh9OF7n7yxrA!uu > z7UupGl|n@Tx@Jg}oq-Z|7e@u2#F4ST>opvcV}e`DAa<hI7*2iT$O^5AaojbNt%Fh< > zs%Z>#Yn7JXFDU3Y^&`*qoq<lh9|yjx&mo;@6n4fz-x_z^AaKV{XB14Fj(ZdaeQ#iO > z2F`J3?Aey>xt2GEE7x!6JL}KtTjqwju5WZVy7>3BZcO5-?`*q{?}T0-MJw!&wof)M > z8W*3@4OcgYR&>+vkHWwYrjarA-|+l_5nJ}yiNHqwus^n<D8IBTJ3E*3Cth-F&+^m2 > z#>VCaDTSnK4<sY@=*?Gwoiy0$Vnw};J$nRk`Vl1^f1%#^sIXHj@1j8p=Z<ufityJz > zc}nbU9*ny-+k-j1f|@*1__dXF3e4`toJxV@hRtClC{R6xrFi}sQ=oQ+Js$ZSQsC5+ > z0xM{@oaw_XQr;1z3(rYZwt%wWNhKl>PSso|5NRo$K;YkTnc@=({Lxedyx$4Y4rO=a > z0>{ZdyYG7$oZ^1N<qssBi}0z0i%x#eaXGH2k>ZxbA~-kBu-`fZ*Evo#W$OYnx;eqy > zgmWCG^GI;9a}t9&uwn~L8?TrdI%CTmIQG;%^!#Cfx`elEJ9Lic)cCykE?&^A%h>ZB > z;esK+><5#HgHMa;2eD(~`(Q+|)qkBkIitfNK2i=({C`fX=uf>B{S?dfI5RtcT$XR> > zV~7(b-^8ojUY=zCH8h}e^I!ef(N|uRVv6{`k&q$TmcAnNh3;i+h`pD7`Qa|d>0A*c > z&NzL6h_>-3uSvMc!5>J-kZgN^IvFGLKRMXV_!2AIQk{AE|G_VX@TdIgm6P)y<gAGh > zJj^e{D<ot{j(>={y!b7CphTZ2KD`5S{3FC@O_3+E2@QDs9|gc?5ns%2A)e=do*yVW > zUs)1<V*U`7g3sDrlMf_U%C=u||GBpVu><bEnOzilQNMucAGf^zb#wYhUlHLi`bzo8 > vv1#p3c>DvNFunKmrpo@bCwZm*Oi3Y$cR!U*_lnld^WWq7-w}>{mizZFd^q39 > > literal 0 > HcmV?d00001 > > diff --git a/tests/data/test-abidiff-exit/test-fun-param-v1.abi > b/tests/data/test-abidiff-exit/test-fun-param-v1.abi > new file mode 100644 > index 0000000..dcc91d1 > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-fun-param-v1.abi > @@ -0,0 +1,46 @@ > +<abi-corpus path='test-fun-param-v1.o' architecture='elf-amd-x86_64'> > + <elf-function-symbols> > + <elf-symbol name='reg' type='func-type' binding='global-binding' > visibility='default-visibility' is-defined='yes'/> > + </elf-function-symbols> > + <abi-instr version='1.0' address-size='64' path='test-fun-param-v1.c' > comp-dir-path='/usr/local/google/home/gprocida/dev/libabigail' > language='LANG_C99'> > + <type-decl name='int' size-in-bits='32' id='type-id-1'/> > + <type-decl name='unsigned int' size-in-bits='32' id='type-id-2'/> > + <type-decl name='unsigned long int' size-in-bits='64' id='type-id-3'/> > + <type-decl name='void' id='type-id-4'/> > + <class-decl name='ops' size-in-bits='192' is-struct='yes' > visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' > line='1' column='1' id='type-id-5'> > + <data-member access='public' layout-offset-in-bits='0'> > + <var-decl name='foo' type-id='type-id-6' visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' > line='2' column='1'/> > + </data-member> > + <data-member access='public' layout-offset-in-bits='64'> > + <var-decl name='bind_class' type-id='type-id-7' > visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' > line='3' column='1'/> > + </data-member> > + <data-member access='public' layout-offset-in-bits='128'> > + <var-decl name='bar' type-id='type-id-8' visibility='default' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' > line='4' column='1'/> > + </data-member> > + </class-decl> > + <pointer-type-def type-id='type-id-9' size-in-bits='64' > id='type-id-8'/> > + <pointer-type-def type-id='type-id-5' size-in-bits='64' > id='type-id-10'/> > + <pointer-type-def type-id='type-id-11' size-in-bits='64' > id='type-id-6'/> > + <pointer-type-def type-id='type-id-12' size-in-bits='64' > id='type-id-7'/> > + <pointer-type-def type-id='type-id-4' size-in-bits='64' > id='type-id-13'/> > + <function-decl name='reg' mangled-name='reg' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' > line='7' column='1' visibility='default' binding='global' size-in-bits='64' > elf-symbol-id='reg'> > + <parameter type-id='type-id-10' name='o' > filepath='/usr/local/google/home/gprocida/dev/libabigail/test-fun-param-v1.c' > line='7' column='1'/> > + <return type-id='type-id-4'/> > + </function-decl> > + <function-type size-in-bits='64' id='type-id-9'> > + <parameter type-id='type-id-1'/> > + <return type-id='type-id-1'/> > + </function-type> > + <function-type size-in-bits='64' id='type-id-11'> > + <return type-id='type-id-4'/> > + </function-type> > + <function-type size-in-bits='64' id='type-id-12'> > + <parameter type-id='type-id-13'/> > + <parameter type-id='type-id-2'/> > + <parameter type-id='type-id-3'/> > + <parameter type-id='type-id-13'/> > + <parameter type-id='type-id-3'/> > + <return type-id='type-id-4'/> > + </function-type> > + </abi-instr> > +</abi-corpus> > diff --git a/tests/data/test-abidiff-exit/test-fun-param-v1.c > b/tests/data/test-abidiff-exit/test-fun-param-v1.c > new file mode 100644 > index 0000000..e47f49c > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-fun-param-v1.c > @@ -0,0 +1,7 @@ > +struct ops { > + void(*foo)(void); > + void(*bind_class)(void *, unsigned int, unsigned long, void *, unsigned > long); > + int(*bar)(int); > +}; > + > +void reg(struct ops* o) { (void) o; } > diff --git a/tests/data/test-abidiff-exit/test-fun-param-v1.o > b/tests/data/test-abidiff-exit/test-fun-param-v1.o > new file mode 100644 > index > 0000000000000000000000000000000000000000..c84e4f0fe209d872c2309acefceba2cce51a998f > GIT binary patch > literal 3000 > zcmbtV-D@LN6hAkUk4dMAX=Amef+MWd6=%{`S<{uS(Q3EVWhugnFH&Zb$z(8@DKnGW > zRYVXGR}i`&s31P*vu{57qW&YkxZs1Zf)5KmxaUmn&EDKjU-XdNd(Q8ibI-^9m=9mQ > zwp(Bf1TnY*4<vyC+$%reI~m)7CM?2@y<5NU-TL6(>D#~lfp9TX>DdbN87~cn#22EY > z1eO`oT?Dm$6agz#yJV0p!L;gE$m^Qx#K4OBk0>;tRL;yRpOPvYJ|ph-R3r|n`dqO> > z%$%N0Wj#Q__yGs{>omZ8u@r<Ul=W{>sKuTStTHW8B+9<IgW5XgR(XWD!tDWiSjYie > zU{BVYhE`v#FR;Z*<5AQ$nWh`YW#a{7%V13`s|K1r5%-&IT(pQlT?R@^u(kqFeh3U> > zA3Z8dY;A>H^%9U=B?T680jDb%49g&k{U+JWb5zFyZ1&=ch_$vt8FiMwd^uaHUXrpX > zEMxA^P$^UdpsPfpYzfNPT^u!d8b`+dsn>AK4h(KGgVak>t3M8{BPVu7*71g&Z6cIb > zpgM_xZp)>Gj|v+4jl(4HL$7C2?}t(7n=?pjoW!kR)OCg}KZ^XJ*BV44ujL=bQ8(y0 > zt)6$>8V0W82EG#v;p+7p=FZ0R=9ax_Z<w3wo9p;{*0e_HIP|uCFZAM|i=q>E2iqs@ > zOV*|5Ov^W|zLVSxyMs6iqj6%5!*_$QXQhri^b&B9KkN>jB*`!B>dww(^Ql)nH*ms9 > zU~98|QAq(yei;8m?7cUDoiy0$d__OSjy;AL{fV-Ezf^C0QrM}LchO)1=Z<uvityLJ > zc}nbT9*lc7-GLdshMGLm`1MtF8cc7-j7o#-e$8McXiz<aP4N5^ra|o-dpz(tq`{e| > z1(wimQR>4qQr-cj>&{73Hixp{NhKl>PSso|5NTmDfxxrJ1Tl(FB=E;4BH-=Lh;~Tc > zjf)&7dwJjYB%I=Y$K}rzoQv><f{RZ6!f`dOsFC89#3DF1&avM*2RAuRHOX~>o!lJZ > zUBWqyC-XpXsdtis-E&e0Y!@$>9eYE^?s@LmKMcZtgu0A(TsQWPXVmz(gg#zR*<~1n > zo^ZhsV0WX@$is)l4x`kw@pZ70)aky%oxH(eAD<|XC;mUDRrIIcihhdaI-HrFKQ5~` > z^a;cXQ*Ywy++Lkj|1~tAbMs&QH_%sIlVXbaf07`PYD?b``cii!n~dd$>MqCWT#=nP > z<MjO@+Qz@SCgE)k{zQUAs_g;lRE*63=HR;IORQ>3b>`)Nm*+41DSvw9)ckunYa#>} > z_+@yF1c}u6hp5YoZ}I~r`b6>R9Z=&RAx3M8Jdw>P<nezN0G~yCF~5&^o_~uUC^}y< > z2|qD^j7q`Fc5kW&k}Fl)uetxs+kx0y+`la^ioB>_!1V6r^{<`NKl+LYf6-UUM~zKu > spYr$zJYjn8=}lGrX;1P>{f&}B67T**I@>E+H_!io=l{KM<g?np`#%ohBme*a > > literal 0 > HcmV?d00001 > > diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc > index 5afc15b..4d9c194 100644 > --- a/tests/test-abidiff-exit.cc > +++ b/tests/test-abidiff-exit.cc > @@ -203,6 +203,15 @@ InOutSpec in_out_specs[] = > "data/test-abidiff-exit/test-decl-struct-report.txt", > "output/test-abidiff-exit/test-decl-struct-report.txt" > }, > + { > + "data/test-abidiff-exit/test-fun-param-v0.abi", > + "data/test-abidiff-exit/test-fun-param-v1.abi", > + "", > + "", > + abigail::tools_utils::ABIDIFF_ABI_CHANGE, > + "data/test-abidiff-exit/test-fun-param-report.txt", > + "output/test-abidiff-exit/test-fun-param-report.txt" > + }, > {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} > }; > > -- > 1.8.3.1 > > -- > Dodji >
Hello, Giuliano Procida <gprocida@google.com> a écrit: > The change to eliminate early canonicalisation has some residual value in > terms of code simplification. Let me know if you'd like me to adjust it and > repost it with that as the aim. Otherwise, I'll leave it be. Early canonicalization was introduced because it does save time during canonicalization of big binaries. Presumably, one reason why you don't see its advantage when reading abixml files in practice at the moment is because abixml files are now (since quite recently) de-duplicated; meaning they contain each type just once. This de-duplication happens thanks to the type canonicalization that is done when reading binaries and the de-duplication work done at abixml writting time. But then, we still want to be able to read non-de-duplicated abixml files, canonicalize them efficiently and write them back in a de-duplicated form. So I prefer not applying that early canonicalization elimitation patch. It does introduce complexity, but that complexity is there for a reason. >> Below is what I think is the proper fix. >> >> Also, I filed this bug to track this issue >> https://sourceware.org/bugzilla/show_bug.cgi?id=25986, so that I can >> refer to it in the commit. >> >> Here is the patch that should fix your issue. Please tell me if it >> fixes your issue. If it does, then I'd prefer to apply this one. >> > > It clearly fixes the minimal test case. I've also run the code on some much > larger XML-XML comparisons both with and without --leaf-changes-only and it > appears to fix the issue and not do anything else. So, yes, it does. Thank you for having tested it. I've applied it and closed the associated bug report. [...] Cheers,