From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x142.google.com (mail-il1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) by sourceware.org (Postfix) with ESMTPS id 39E263851C06 for ; Wed, 13 May 2020 18:30:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 39E263851C06 Received: by mail-il1-x142.google.com with SMTP id w6so713399ilg.1 for ; Wed, 13 May 2020 11:30:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0SxW0n/dxNCbClL9liz5m+VV7Ugf34fZmvYxnj2CMxc=; b=Ei3srkqapSuwXPczEkN2vCVMm0zyquaGSCe+WPBHFXV3dyuUO0aZRU6dcuS12rNjEX Qj63WDT9KUJjHldT8koXZhgjlkxcyLoW1usltaz6qu52dFSpqH4SYkeKK3UJnpVeDaEM UJq89R9yBBKraG3vyyKmqUjtIfUP4h+XV+jYAxE0V0gb4MJ6SePdx1LOK+95vKbCjPBc q4OGaUjQEDcguWvMnT/T3w+RkFbZghclDuhuUlPvze2+4hENQl71qX6gYq0Q3EfWFOcq AtPDuTLc7ZCA0mksYJPHJKbL5xYGOvl+L3/kAWX8O59IKOHtgm685SxUjRnvX+1+Q0H/ tYvw== X-Gm-Message-State: AOAM5303rA3Qx+n8AuuRGjtvpLrzVx8HEGdZSVxLw7KY643aQb4fmdf7 RgidvRfierFf0z+7ehZ8rAWLessKoAngmAowqf19OlFz X-Google-Smtp-Source: ABdhPJwBHfRlFi7qmHgk+xHI92GTsTMX5PYdRfe/zBhvv15xygAa3Du/xMLUfRbGY4xjgS9CXhtb/M9RUxt37jkCnww= X-Received: by 2002:a92:8144:: with SMTP id e65mr464867ild.242.1589394631354; Wed, 13 May 2020 11:30:31 -0700 (PDT) MIME-Version: 1.0 References: <20200505180612.232158-1-gprocida@google.com> <865zczdhn5.fsf@seketeli.org> In-Reply-To: <865zczdhn5.fsf@seketeli.org> From: Giuliano Procida Date: Wed, 13 May 2020 19:30:10 +0100 Message-ID: Subject: Re: [PATCH 0/4] Fix incomplete function type bug in abg-reader To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com, =?UTF-8?Q?Matthias_M=C3=A4nnich?= X-Spam-Status: No, score=-30.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, HTML_MESSAGE, KAM_LOTSOFHASH, LIKELY_SPAM_BODY, RCVD_IN_DNSWL_NONE, SCC_5_SHORT_WORD_LINES, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 May 2020 18:30:36 -0000 Hi. On Wed, 13 May 2020 at 15:38, Dodji Seketeli wrote: > Giuliano Procida a =C3=A9crit: > > > 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=3D25986, 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 > 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=3Dlibabigail.git;a=3Dcommit;h=3D7699dfc921d= 248fa6d5cbdbeab9d501b17ef3f52 > . > > 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=3D25986 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 sourc= e > 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 > --- > 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.tx= t > 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_ =3D 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_ =3D 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 @@ > + > + > + visibility=3D'default-visibility' is-defined=3D'yes'/> > + > + comp-dir-path=3D'/usr/local/google/home/gprocida/dev/libabigail' > language=3D'LANG_C99'> > + > + > + > + > + visibility=3D'default' > filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v0.c' > line=3D'1' column=3D'1' id=3D'type-id-5'> > + > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v0.c' > line=3D'2' column=3D'1'/> > + > + > + visibility=3D'default' > filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v0.c' > line=3D'3' column=3D'1'/> > + > + > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v0.c' > line=3D'4' column=3D'1'/> > + > + > + id=3D'type-id-8'/> > + id=3D'type-id-10'/> > + id=3D'type-id-6'/> > + id=3D'type-id-7'/> > + id=3D'type-id-13'/> > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v0.c' > line=3D'7' column=3D'1' visibility=3D'default' binding=3D'global' size-in= -bits=3D'64' > elf-symbol-id=3D'reg'> > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v0.c' > line=3D'7' column=3D'1'/> > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > 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..64218730b3fa188f61752ebbc917f49= a75a92785 > GIT binary patch > literal 2992 > zcmbtW&2QsW5T6(4!>OBW+E7R+5=3DLTG7GWnXXlWO92^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=3D~sk{iAqgYSWNiLpk??% > za1=3Dp{G1W#;83qBcLbXo@$r4PdeuliN*j5OvsD6t=3D14`w@yz&mIlHon#KE?s6>O!$X > z%z~OsWz|DLyMqJ$R2pEhSn~WB%IX&=3D)I!GvR+$wh(qmu$g4#M}RC$oN!sjFOFpvYb > z#Lm~7no?h_FR|rH;~~^GnWAdiCG9zFOJhwerUr_diu=3Dt?T(pQlJqwgpV66>MJ_nk% > zj~(BwFD$rNr8ntj?-lfhGme&ev@n#IjUj-CVRduVy(3)qh9OF7n7yxrA!uu > z7UupGl|n@Tx@Jg}oq-Z|7e@u2#F4ST>opvcV}e`DAa zs%Z>#Yn7JXFDU3Y^&`*qoq z2F`J3?Aey>xt2GEE7x!6JL}KtTjqwju5WZVy7>3BZcO5-?`*q{?}T0-MJw!&wof)M > z8W*3@4OcgYR&>+vkHWwYrjarA-|+l_5nJ}yiNHqwus^n z#>VCaDTSnK4 zc}nbU9*ny-+k-j1f|@*1__dXF3e4`toJxV@hRtClC{R6xrFi}sQ=3DoQ+Js$ZSQsC5+ > z0xM{@oaw_XQr;1z3(rYZwt%wWNhKl>PSso|5NRo$K;YkTnc@=3D({Lxedyx$4Y4rO=3Da > z0>{ZdyYG7$oZ^1NZxbA~-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=3D({C`fX=3Duf>B{S?dfI5RtcT$XR> > zV~7(b-^8ojUY=3DzCH8h}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 zJj^e{D=3D{y!b7CphTZ2KD`5S{3FC@O_3+E2@QDs9|gc?5ns%2A)e=3Ddo*yVW > zUs)1 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 @@ > + > + > + visibility=3D'default-visibility' is-defined=3D'yes'/> > + > + comp-dir-path=3D'/usr/local/google/home/gprocida/dev/libabigail' > language=3D'LANG_C99'> > + > + > + > + > + visibility=3D'default' > filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v1.c' > line=3D'1' column=3D'1' id=3D'type-id-5'> > + > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v1.c' > line=3D'2' column=3D'1'/> > + > + > + visibility=3D'default' > filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v1.c' > line=3D'3' column=3D'1'/> > + > + > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v1.c' > line=3D'4' column=3D'1'/> > + > + > + id=3D'type-id-8'/> > + id=3D'type-id-10'/> > + id=3D'type-id-6'/> > + id=3D'type-id-7'/> > + id=3D'type-id-13'/> > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v1.c' > line=3D'7' column=3D'1' visibility=3D'default' binding=3D'global' size-in= -bits=3D'64' > elf-symbol-id=3D'reg'> > + filepath=3D'/usr/local/google/home/gprocida/dev/libabigail/test-fun-param= -v1.c' > line=3D'7' column=3D'1'/> > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > 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 *, unsigne= d > 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..c84e4f0fe209d872c2309acefceba2c= ce51a998f > GIT binary patch > literal 3000 > zcmbtV-D@LN6hAkUk4dMAX=3DAmef+MWd6=3D%{`S<{uS(Q3EVWhugnFH&Zb$z(8@DKnGW > zRYVXGR}i`&s31P*vu{57qW&YkxZs1Zf)5KmxaUmn&EDKjU-XdNd(Q8ibI-^9m=3D9mQ > zwp(Bf1TnY*4D#~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@rsKuTStTHW8B+9 zU{BVYhE`v#FR;Z*<5AQ$nWh`YW#a{7%V13`s|K1r5%-&IT(pQlT?R@^u(kqFeh3U> > zA3Z8dY;A>H^%9U=3DB?T680jDb%49g&k{U+JWb5zFyZ1&=3Dch_$vt8FiMwd^uaHUXrpX > zEMxA^P$^UdpsPfpYzfNPT^u!d8b`+dsn>AK4h(KGgVak>t3M8{BPVu7*71g&Z6cIb > zpgM_xZp)>Gj|v+4jl(4HL$7C2?}t(7n=3D?pjoW!kR)OCg}KZ^XJ*BV44ujL=3DbQ8(y0 > zt)6$>8V0W82EG#v;p+7p=3DFZ0R=3D9ax_ZE2iqs@ > 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=3DZ zc}nbT9*lc7-GLdshMGLm`1MtF8cc7-j7o#-e$8McXiz z1(wimQR>4qQr-cj>&{73Hixp{NhKl>PSso|5NTmDfxxrJ1Tl(FB=3DE;4BH-=3DLh;~Tc > zjf)&7dwJjYB%I=3DY$K}rzoQv>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#=3DnP > z3b>`)Nm*+41DSvw9)ckunYa#>} > z_+@yF1c}u6hp5YoZ}I~r`b6>R9Z=3D&RAx3M8Jdw>P z2|qD^j7q`Fc5kW&k}Fl)uetxs+kx0y+`la^ioB>_!1V6r^{<`NKl+LYf6-UUM~zKu > spYr$zJYjn8=3D}lGrX;1P>{f&}B67T**I@>E+H_!io=3Dl{KM > 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[] =3D > "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 >