public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
From: Ben Woodard <woodard@redhat.com>
To: Giuliano Procida <gprocida@google.com>
Cc: "Dodji Seketeli" <dodji@seketeli.org>,
	"Dodji Seketeli" <dodji@redhat.com>,
	"Dodji Seketeli via Libabigail" <libabigail@sourceware.org>,
	"Matthias Männich" <maennich@google.com>
Subject: Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
Date: Wed, 10 Aug 2022 19:22:44 -0700	[thread overview]
Message-ID: <B65A845A-E56F-4709-8FEB-ACDB3A770748@redhat.com> (raw)
In-Reply-To: <CAGvU0Hk6MM6b0EE+Y+XfQSXipCRpfEYSFoQ_NNw+p0TxnpapGw@mail.gmail.com>

Dodji is on vacation. Thank you for double checking this.

> On Aug 10, 2022, at 8:23 AM, Giuliano Procida via Libabigail <libabigail@sourceware.org> wrote:
> 
> Hi Dodji.
> 
> On Sat, 23 Jul 2022 at 00:33, Dodji Seketeli <dodji@seketeli.org> 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 case.
It does bring up the interesting question is libabigail just an ABI change detection tool or is it also a API change detection tool. With name mangling, I think that the dynamic linker will continue to wire all up the correct function call. I can’t think of a case where this may not be true 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. 

> 
>> 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-progs
> 
> Shouldn't any tweaking of behaviour happen with abidiff rather than abidw?
> 
> 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 new
>>        setter.
>>        (integral_type::to_string): Add an "internal" flag.
>>        * src/abg-ir.cc (operator~, operator&=): 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 new
>>        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&=): 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 of
>>        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 pretty
>>        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_profiler.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: Adjust.
>>        * 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_profiler.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: Adjust.
>>        * tests/data/test-read-write/test28-without-std-vars-ref.xml: Adjust.
>> 
>> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>> 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
> 


  reply	other threads:[~2022-08-11  2:22 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-22 23:19 [PATCH 0/3] Make integral types of same base and size compatible Dodji Seketeli
2022-07-22 23:28 ` [PATCH 1/3] ir: Disambiguate sorting of array element types Dodji Seketeli
2022-07-22 23:31 ` [PATCH 2/3] dwarf-reader: Remove redundant qualifiers from qualified types Dodji Seketeli
2022-07-22 23:32 ` [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent Dodji Seketeli
2022-08-10 15:23   ` Giuliano Procida
2022-08-11  2:22     ` Ben Woodard [this message]
2022-08-12 15:26       ` Giuliano Procida
2022-08-16 19:56         ` Ben Woodard
2022-08-16 16:54     ` Dodji Seketeli
2022-08-16 17:06       ` Ben Woodard
2022-08-16 18:10       ` Giuliano Procida
2022-08-18 16:29         ` Dodji Seketeli
2022-08-18 17:52           ` Ben Woodard
2022-08-19 15:30             ` Dodji Seketeli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B65A845A-E56F-4709-8FEB-ACDB3A770748@redhat.com \
    --to=woodard@redhat.com \
    --cc=dodji@redhat.com \
    --cc=dodji@seketeli.org \
    --cc=gprocida@google.com \
    --cc=libabigail@sourceware.org \
    --cc=maennich@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).