From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd31.google.com (mail-io1-xd31.google.com [IPv6:2607:f8b0:4864:20::d31]) by sourceware.org (Postfix) with ESMTPS id AAB4A3858D28 for ; Fri, 12 Aug 2022 15:26:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AAB4A3858D28 Received: by mail-io1-xd31.google.com with SMTP id d187so1065143iof.4 for ; Fri, 12 Aug 2022 08:26:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc; bh=ueBPHmtP2k7eifxcroIzjoYKWdnWDl4koYxyc8hjRSY=; b=AHRfBLDO4Ir7qbStqHVLfvSl6KC5WyLunVw/jslzb5zrl6A7L0ik6/7thCdc4OSKUa rn1RS7VfW8zT3rKv+d1V6rFmtpV2Ozn6HI40I3ZntPpql4pPYO7ynMYMhSd+4juinHqX cpb+8v/2eAeF19/bpz1JQKQ7+eaahgA7ATRoSNIduwuFkoGyFFVtSODdU8rHsG+vh+Rz q3utD2K0cPAFf14ulhKeUhDYhGRGhHb/9w5MqzRBkXTi3KsCllcWN567nBvf3Kd08snI S+vxRvhU7lDe3yr/95JguizvzSfDE+Y067zsNNxkzDQ2t/mt/Uh5RXzEbzNh5JiBPR15 ECxw== X-Gm-Message-State: ACgBeo1WvTH0PoYPc6CCZmKcXjRr7ukHJqyRigPq/P7tHRFBzu4q50EA y7SuOijBQzaRF0V2UErSuV0MeWjFK7xaNliSI36VRg== X-Google-Smtp-Source: AA6agR6Cb9d6Nu5yKbLX7d6BNhh70/R1QwNei9Z7OdtyP0xDxOEOA0O+tS5fOAE0zRqY4OfG5U9/fTPS7sI28TNk4ZQ= X-Received: by 2002:a05:6602:2f03:b0:678:9c7c:97a5 with SMTP id q3-20020a0566022f0300b006789c7c97a5mr1852301iow.32.1660318014747; Fri, 12 Aug 2022 08:26:54 -0700 (PDT) MIME-Version: 1.0 References: <87sfms91n0.fsf@redhat.com> <87czdw910h.fsf@seketeli.org> In-Reply-To: From: Giuliano Procida Date: Fri, 12 Aug 2022 16:26:18 +0100 Message-ID: Subject: Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent To: Ben Woodard Cc: Dodji Seketeli , Dodji Seketeli , Dodji Seketeli via Libabigail , =?UTF-8?Q?Matthias_M=C3=A4nnich?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-21.5 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Fri, 12 Aug 2022 15:26:58 -0000 Hi Ben. On Thu, 11 Aug 2022 at 03:22, Ben Woodard wrote: > > Dodji is on vacation. Thank you for double checking this. > > > On Aug 10, 2022, at 8:23 AM, Giuliano Procida via Libabigail wrote: > > > > Hi Dodji. > > > > On Sat, 23 Jul 2022 at 00:33, Dodji Seketeli wrote= : > >> > >> Hello, > >> > >> On some platforms, "long int" and "long long int" can have the same > >> size. In that case, we want those two types to be equivalent from ABI > >> standpoint. Otherwise, through the use of typedefs and pointers, two > >> structs "C" defined in different translation units where one uses > >> "long int" in a translation unit and "long long int" in another should > >> be considered ABI compatible if long int and long long int have the > >> same size on that platform. > > > > While such types may be ABI compatible, they are not API compatible as = they > > impact (at least) C++ overload resolution. > > hmm maybe this kind of resolution should only apply to C linkage symbols = and not C++ where they are in fact different. > You of course correct about the difference between ABI and API in this ca= se. > It does bring up the interesting question is libabigail just an ABI chang= e detection tool or is it also a API change detection tool. With name mangl= ing, I think that the dynamic linker will continue to wire all up the corre= ct function call. I can=E2=80=99t think of a case where this may not be tru= e but if you can, please speak up. > > > > > All of char, unsigned char, signed char, int, unsigned, long, etc. are > > distinct types. > > Conflating some subsets of these will result in confusing ABI > > difference reports. > > Interestingly, I have been collaborating with people writing another ABI = tool that would also overlook this kind of difference as well. I wonder how= confusing the error reports get. > As time has passed I've come to the opinion that it's best to be as literal as possible... the ABI extraction and comparison code should be as close to "just building and comparing graphs" as is practically possible. This means all the interpretive logic has to live somewhere else and there is no confusion as to what "equivalent" means at any particular stage. So instead of: ABI extraction in: binary object out: ABI representation ABI comparison in: ABI representation out: difference report We also have: ABI transformations (optional): in/out: ABI representation - restrict the ABI surface (exposed symbols) - normalise integral types (like this change) - eliminate typedefs - normalise qualifiers (pushing them through array types if needed) - remove top-level qualifiers on function parameter types - assume ODR so we can resolve incomplete types to full definitions / detect and report ODR violations - standalone graph deduplication - standalone pruning of unreachable parts of the graph ABI comparison: in: ABI representation out: ABI difference representation ABI difference transformations (optional): in/out: ABI difference representation - diff suppression - prune parts of the difference graph - rewrite removal-addition pairs as renamings, probably detected using heuristics ABI reporting: in: ABI difference representation out: various reporting styles, statistics mode etc. The representations don't necessarily have to correspond to file formats. This is an ideal. I'm not sure if it's actually worth the trouble of implementing a difference representation that will allow the things mentioned, as opposed to doing them during ABI comparison. Giuliano. > > > >> Otherwise, that causes spurious type changes that lead to self > >> comparison change down the road. For instance, the following command > >> fails: > >> > >> $ tools/fedabipkgdiff --debug --self-compare -a --from fc36 btrfs-p= rogs > > > > Shouldn't any tweaking of behaviour happen with abidiff rather than abi= dw? > > > > We might not end up with stable XML but the finger of blame should be > > pointed at the btrfs-progs in any case. > > > >> This patch thus changes the comparison engine of the IR so that the > >> "short, long and long long" modifiers don't change the result of > >> comparing integral types that share the same base type when they have > >> the same size. > > > > We don't want this behaviour and can carry a revert patch in AOSP or > > work a way to disable it that is less likely to cause merge conflicts > > in the future. > > > > Is there an easy way of putting this under flag control? > > > > There's also a secondary issue where base types like "int" and "long > > int" now want to have the same hash-based type id and we end up with > > linear probing and the XML instability that accompanies this. I expect > > this was an unintended side-effect, but haven't yet looked into how it > > might be resolved. > > > > Regards, > > Giuliano. > > > >> * include/abg-fwd.h (is_integral_type): Declare new function. > >> * include/abg-ir.h (type_decl::get_qualified_name): Add a > >> declaration of an implementation of the virtual interface > >> get_qualified_name. > >> * src/abg-ir-priv.h (integral_type::set_modifiers): Define a ne= w > >> setter. > >> (integral_type::to_string): Add an "internal" flag. > >> * src/abg-ir.cc (operator~, operator&=3D): Declare > >> new operators. > >> (get_internal_integral_type_name): Define new static function. > >> (decl_base::priv::{temporary_internal_qualified_name_, > >> internal_qualified_name_}): Define two new data members. > >> (get_type_name): For internal name of integral types, use the n= ew > >> get_internal_integral_type_name function. > >> (is_integral_type): Define new function. > >> (integral_type::set_modifiers): Define new member function. > >> (operator|, operator&): Fix some indentation. > >> (operator~, operator&=3D): Define new operators. > >> (parse_integral_type): Fix the logic of this function. Namely,= it > >> wasn't handling parsing "long long" correctly. > >> (integral_type::to_string): Add an "internal" flag. > >> (equals): In the overload for type_decl, do not take the short, > >> long and long long into account when comparing integral types o= f > >> the same size. > >> (type_decl::get_qualified_name): Define new method. > >> (type_decl::get_pretty_representation): For internal name of > >> integral types, use the new get_internal_integral_type_name > >> function. > >> ({decl,type}_topo_comp::operator()): Use the non-internal prett= y > >> representation of decls/types for sorting purpose. > >> * src/abg-reader.cc (build_type_decl): We don't expect the > >> integral type name from abixml to the same as the name of the > >> parsed integral type, as the abixml file can be old and have an > >> old format. > >> * tests/data/test-annotate/libtest23.so.abi: Adjust. > >> * tests/data/test-annotate/libtest24-drop-fns-2.so.abi: Adjust. > >> * tests/data/test-annotate/libtest24-drop-fns.so.abi: Adjust. > >> * tests/data/test-annotate/test0.abi: Adjust. > >> * tests/data/test-annotate/test15-pr18892.so.abi: Adjust. > >> * tests/data/test-annotate/test17-pr19027.so.abi: Adjust. > >> * tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.= 1.so.abi: > >> Adjust. > >> * tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profi= ler.so.abi: > >> Adjust. > >> * tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.= 1.so.abi: > >> Adjust. > >> * tests/data/test-annotate/test21-pr19092.so.abi: Adjust. > >> * tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt: > >> Adjust. > >> * tests/data/test-diff-filter/test41-report-0.txt: Adjust. > >> * tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-= 4.3-3.20141204.fc23.x86_64-report-0.txt: > >> Adjust. > >> * tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-= 4.3-3.20141204.fc23.x86_64-report-1.txt: > >> Adjust. > >> * tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt: > >> Adjust. > >> * tests/data/test-read-dwarf/PR22015-libboost_iostreams.so.abi: > >> Adjust. > >> * tests/data/test-read-dwarf/PR22122-libftdc.so.abi: Adjust. > >> * tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Adjust. > >> * tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.= 0.0.abi: > >> Adjust. > >> * tests/data/test-read-dwarf/libtest23.so.abi: Adjust. > >> * tests/data/test-read-dwarf/libtest24-drop-fns-2.so.abi: Adjus= t. > >> * tests/data/test-read-dwarf/libtest24-drop-fns.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test-PR26568-1.o.abi: Adjust. > >> * tests/data/test-read-dwarf/test-PR26568-2.o.abi: Adjust. > >> * tests/data/test-read-dwarf/test-libaaudio.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test-libandroid.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test0.abi: Adjust. > >> * tests/data/test-read-dwarf/test0.hash.abi: Adjust. > >> * tests/data/test-read-dwarf/test1.hash.abi: Adjust. > >> * tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test11-pr18828.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test12-pr18844.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test15-pr18892.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test16-pr18904.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test17-pr19027.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-= 6.1.so.abi: > >> Adjust. > >> * tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_pro= filer.so.abi: > >> Adjust. > >> * tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-= 6.1.so.abi: > >> Adjust. > >> * tests/data/test-read-dwarf/test21-pr19092.so.abi: Adjust. > >> * tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17= .so.abi: > >> Adjust. > >> * tests/data/test-read-dwarf/test9-pr18818-clang.so.abi: Adjust= . > >> * tests/data/test-read-write/test22.xml: Adjust. > >> * tests/data/test-read-write/test23.xml: Adjust. > >> * tests/data/test-read-write/test28-without-std-fns-ref.xml: Ad= just. > >> * tests/data/test-read-write/test28-without-std-vars-ref.xml: A= djust. > >> > >> Signed-off-by: Dodji Seketeli > >> Applied to master. > >> --- > >> include/abg-fwd.h | 6 + > >> include/abg-ir.h | 7 + > >> src/abg-ir-priv.h | 11 +- > >> src/abg-ir.cc | 302 +- > >> src/abg-reader.cc | 3 +- > >> tests/data/test-annotate/libtest23.so.abi | 748 +- > >> .../test-annotate/libtest24-drop-fns-2.so.abi | 794 +- > >> .../test-annotate/libtest24-drop-fns.so.abi | 794 +- > >> tests/data/test-annotate/test0.abi | 48 +- > >> .../data/test-annotate/test14-pr18893.so.abi | 2472 +- > >> .../data/test-annotate/test15-pr18892.so.abi | 12330 +++-- > >> .../data/test-annotate/test17-pr19027.so.abi | 2142 +- > >> ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 11742 +++-- > >> ...19-pr19023-libtcmalloc_and_profiler.so.abi | 16174 +++--- > >> ...st20-pr19025-libvtkParallelCore-6.1.so.abi | 16864 +++--- > >> .../data/test-annotate/test21-pr19092.so.abi | 680 +- > >> .../PR25058-liblttng-ctl-report-1.txt | 4 +- > >> .../test-PR26739-2-report-0.txt | 10 +- > >> .../PR22015-libboost_iostreams.so.abi | 3520 +- > >> .../test-read-dwarf/PR22122-libftdc.so.abi | 3929 +- > >> .../data/test-read-dwarf/PR25007-sdhci.ko.abi | 9147 ++-- > >> .../PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi | 169 +- > >> tests/data/test-read-dwarf/libtest23.so.abi | 708 +- > >> .../libtest24-drop-fns-2.so.abi | 760 +- > >> .../test-read-dwarf/libtest24-drop-fns.so.abi | 760 +- > >> .../test-read-dwarf/test-libaaudio.so.abi | 348 +- > >> .../test-read-dwarf/test-libandroid.so.abi | 1296 +- > >> tests/data/test-read-dwarf/test0.abi | 47 +- > >> tests/data/test-read-dwarf/test0.hash.abi | 13 +- > >> tests/data/test-read-dwarf/test1.hash.abi | 4 +- > >> .../test-read-dwarf/test10-pr18818-gcc.so.abi | 7328 ++- > >> .../test-read-dwarf/test11-pr18828.so.abi | 14955 +++--- > >> .../test-read-dwarf/test12-pr18844.so.abi | 25236 +++++---- > >> .../test-read-dwarf/test14-pr18893.so.abi | 1580 +- > >> .../test-read-dwarf/test15-pr18892.so.abi | 11647 +++-- > >> .../test-read-dwarf/test16-pr18904.so.abi | 16732 +++--- > >> .../test-read-dwarf/test17-pr19027.so.abi | 2056 +- > >> ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 11520 +++-- > >> ...19-pr19023-libtcmalloc_and_profiler.so.abi | 15834 +++--- > >> ...st20-pr19025-libvtkParallelCore-6.1.so.abi | 16406 +++--- > >> .../test-read-dwarf/test21-pr19092.so.abi | 656 +- > >> .../test22-pr19097-libstdc++.so.6.0.17.so.abi | 42542 ++++++++-------- > >> .../test9-pr18818-clang.so.abi | 5412 +- > >> tests/data/test-read-write/test22.xml | 7 +- > >> tests/data/test-read-write/test23.xml | 7 +- > >> .../test28-without-std-fns-ref.xml | 648 +- > >> .../test28-without-std-vars-ref.xml | 590 +- > >> 47 files changed, 129532 insertions(+), 129456 deletions(-) > >> > >> The patch is too big for the list so I am attaching it gzipped. > >> > >> Cheers, > >> > >> > >> > >> -- > >> Dodji > > >