From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 2B1B43858D28 for ; Mon, 31 Jan 2022 14:38:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2B1B43858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com IronPort-SDR: 6GD/O4IKDxzA5XpWIVE6huYO5lTnz6Vl8H5/BAXfs97IUeM6bvdZ+aHb9i07CE1wI5TZGfRqyd NJZEp0BiL3LzNrXSIZXmOvPp/0ODSJ7JeD+6aajQbqPDKUyeBncESxruH+vaDrdir+rnpfSG0Q oRbmBE6IiguTR5Jwv/Rbt2WeXS88IRB+IxRZ67xYGWYg8uboGEKX9xod3acYGYQjEsaPhUPfTZ ze/9YpG9Z0seDwP23rfEaxeFbQSC/CiBhPzBY4BAq1NwB9G06hC4a4YzRkF+Fs0NAlyjIrciMc jJKEeazJ5nrjxeFq1Mu+uE2+ X-IronPort-AV: E=Sophos;i="5.88,331,1635235200"; d="scan'208";a="73950840" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 31 Jan 2022 06:38:54 -0800 IronPort-SDR: OaeJJJgSFf0Zvd/RwkHeSvMqpdwxdLpPbU59oAnkuOY2wu+QZNq4zAPFc6WKU1cnueq3MD74bb rRpnPDkM/bxlRAfTOebjfJ5inmTtpd4VEiKzZwoXVGwdKmQUxilo+katLHQHzfvQzISOgtnZlq dH1IsRwJuPRBHRGXEp3e58BufJttV5tVhunGNNtkcIIeZSuz/stUswvEAtG7grg5aBtcUn9Xey M6r75/4H27QEV8v5CQjc8e7Q8L2lgpvMZw01lbxMqayDQEDeJ9I3rEI8yRAhmqdRE3of7i32rH prI= From: Thomas Schwinge To: CC: Dodji Seketeli , Mark Wielaard , Giuliano Procida Subject: Re: 'src/abg-dwarf-reader.cc:compare_dies_string_attribute_value' optimization In-Reply-To: <87wnijv616.fsf@dirichlet.schwinge.homeip.net> References: <87wnijv616.fsf@dirichlet.schwinge.homeip.net> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/27.1 (x86_64-pc-linux-gnu) Date: Mon, 31 Jan 2022 15:38:44 +0100 Message-ID: <87r18oynpn.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-11.mgc.mentorg.com (139.181.222.11) To svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: Mon, 31 Jan 2022 14:39:00 -0000 Hi! On 2022-01-28T23:39:49+0100, I wrote: > It's late on a Friday evening, and very well this may just be me not > understanding libabigail/DWARF ;-) -- in context of > or > , I did a > simple experiment to disable the > 'src/abg-dwarf-reader.cc:compare_dies_string_attribute_value' > optimization: OK, something doesn't seem right here: > --- src/abg-dwarf-reader.cc > +++ src/abg-dwarf-reader.cc > [...] > /// This function is a fast routine (optimization) to compare the > /// values of two string attributes of two DIEs. > /// > /// @param l the first DIE to consider. > /// > /// @param r the second DIE to consider. > /// > /// @param attr_name the name of the attribute to compare, on the tw= o > /// DIEs above. > /// > /// @param result out parameter. This is set to the result of the > /// comparison. If the value of attribute @p attr_name on DIE @p l > /// equals the value of attribute @p attr_name on DIE @p r, then the > /// the argument of this parameter is set to true. Otherwise, it's > /// set to false. Note that the argument of this parameter is set i= ff > /// the function returned true. > /// > /// @return true iff the comparison could be performed. There are > /// cases in which the comparison cannot be performed. For instance= , > /// if one of the DIEs does not have the attribute @p attr_name. In > /// any case, if this function returns true, then the parameter @p > /// result is set to the result of the comparison. > static bool > compare_dies_string_attribute_value(const Dwarf_Die *l, const Dwarf_= Die *r, > unsigned attr_name, > bool &result) > { > Dwarf_Attribute l_attr, r_attr; > if (!dwarf_attr_integrate(const_cast(l), attr_name, &l= _attr) > || !dwarf_attr_integrate(const_cast(r), attr_name,= &r_attr)) > return false; > > ABG_ASSERT(l_attr.form =3D=3D DW_FORM_strp > || l_attr.form =3D=3D DW_FORM_string > || l_attr.form =3D=3D DW_FORM_GNU_strp_alt > || form_is_DW_FORM_strx(l_attr.form) > || form_is_DW_FORM_line_strp(l_attr.form)); > > ABG_ASSERT(r_attr.form =3D=3D DW_FORM_strp > || r_attr.form =3D=3D DW_FORM_string > || r_attr.form =3D=3D DW_FORM_GNU_strp_alt > || form_is_DW_FORM_strx(r_attr.form) > || form_is_DW_FORM_line_strp(r_attr.form)); > > +#if 0 > if ((l_attr.form =3D=3D DW_FORM_strp > && r_attr.form =3D=3D DW_FORM_strp) > || (l_attr.form =3D=3D DW_FORM_GNU_strp_alt > && r_attr.form =3D=3D DW_FORM_GNU_strp_alt) > || (form_is_DW_FORM_strx(l_attr.form) > && form_is_DW_FORM_strx(r_attr.form)) > || (form_is_DW_FORM_line_strp(l_attr.form) > && form_is_DW_FORM_line_strp(r_attr.form))) > { > // So these string attributes are actually pointers into a > // string table. The string table is most likely de-duplicate= d > // so comparing the *values* of the pointers should be enough. > // > // This is the fast path. > if (l_attr.valp =3D=3D r_attr.valp) > result =3D true; > else if (l_attr.valp && r_attr.valp) > result =3D *l_attr.valp =3D=3D *r_attr.valp; > else > result =3D false; > return true; > } > +#endif > > // If we reached this point it means we couldn't use the fast path > // because the string atttributes are strings that are "inline" in > // the debug info section. Let's just compare them the slow and > // obvious way. > string l_str =3D die_string_attribute(l, attr_name), > r_str =3D die_string_attribute(r, attr_name); > result =3D l_str =3D=3D r_str; > > return true; > } I re-enabled the "optimized" code, but added a simple 'assert' to verify that its 'result' does match the "slow" 'result'. (This indeed does cause a number of more testsuite failures than the single one that I'd mentioned before!) Here's one case where the 'result's don't match. First, the "slow" code: (gdb) call die_string_attribute(l, attr_name) $20 =3D "dwarf_getarangeinfo.c" (gdb) call die_string_attribute(r, attr_name) $21 =3D "dwarf_onearange.c" Clearly not identical; thus ought to 'return false;'. However, the "optimized" code: (gdb) ptype l_attr type =3D struct Dwarf_Attribute { unsigned int code; unsigned int form; unsigned char *valp; Dwarf_CU *cu; } (gdb) print l_attr $5 =3D {code =3D 3, form =3D 14, valp =3D 0x7ffff45c97d9 "\351\024", cu= =3D 0x7fffe8614670} (gdb) print r_attr $6 =3D {code =3D 3, form =3D 14, valp =3D 0x7ffff45c970d "\351\r", cu = =3D 0x7fffe8614550} So these are both 'DW_FORM_strp', eligible for "optimized" comparison: (gdb) print l_attr.valp =3D=3D r_attr.valp $7 =3D false OK. Then: (gdb) print l_attr.valp && r_attr.valp $8 =3D true (gdb) print *l_attr.valp =3D=3D *r_attr.valp $9 =3D true This now does 'return true;'! Given 'unsigned char *valp', what this only checks is that one byte at 'l_attr.valp' equals one byte at 'r_attr.valp' -- which evidently isn't sufficient to demonstrate that 'l_attr' and 'r_attr' do match. Therefore it seems that this optimization is not correct? I understand the purpose here is that for the supported and matching 'DW_FORM_*', not to call 'dwarf_formstring' (via the "slow" code's 'die_string_attribute'), because we don't actually need the strings, but just identifiers (like, table indices) to compare. But how to do this properly? Gr=C3=BC=C3=9Fe Thomas > My assumption was that this shouldn't change anything other than possibly > regress performance. However: > > [-PASS:-]{+FAIL:+} runtestreaddwarf > > --- build-libabigail/tests/runtestreaddwarf.log > +++ build-libabigail/tests/runtestreaddwarf.log > [...] > -PASS runtestreaddwarf (exit status: 0) > +--- [...]/source-libabigail/tests/data/test-read-dwarf/PR25007-sdhci= .ko.abi 2022-01-28 22:48:07.049805043 +0100 > ++++ [...]/build-libabigail/tests/output/test-read-dwarf/PR25007-sdhc= i.ko.abi 2022-01-28 23:15:30.261483063 +0100 > +@@ -10107,10 +10107,19 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > + > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -10138,6 +10147,10 @@ > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -10181,6 +10194,16 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10370,6 +10393,13 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10519,6 +10549,31 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10531,6 +10586,17 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10591,6 +10657,10 @@ > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -10631,6 +10701,22 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10746,6 +10832,10 @@ > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -10770,6 +10860,14 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10815,6 +10913,9 @@ > + > + > + > ++ > ++ > ++ > + > + > + > +@@ -10859,6 +10960,15 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10885,11 +10995,44 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10923,6 +11066,18 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10936,6 +11091,14 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -10949,6 +11112,10 @@ > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -11075,12 +11242,26 @@ > + > + > + > ++ > ++ > ++ > ++ > + > + > + > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -11124,6 +11305,15 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > + > +@@ -11192,10 +11382,19 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > + > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -11210,6 +11409,10 @@ > + > + > + > ++ > ++ > ++ > ++ > + > + > + > +@@ -11224,6 +11427,9 @@ > + > + > + > ++ > ++ > ++ > + > + > + > +@@ -11241,223 +11447,6 @@ > + > + > + > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > + > + > + > +@@ -11477,48 +11466,19 @@ > + > + > + > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > ++ > + > + > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > +- > + > + > + > +ABIs differ: > +[...]/source-libabigail/tests/data/test-read-dwarf/PR25007-sdhci.ko.= abi > +and: > +[...]/build-libabigail/tests/output/test-read-dwarf/PR25007-sdhci.ko= .abi > + > +FAIL runtestreaddwarf (exit status: 1) > > Only that one test case. > > The reordered 'function-type's -- but I've not yet verified whether > they're really just reordered -- may hint towards a sorting stability > issue, but there are also the disappearing 'class-decl's. > > Unless somebody points out any misunderstanding on my side, I suppose I > shall try to figure out what's going wrong. > > > Gr=C3=BC=C3=9Fe > Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955