Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent

[Ben Woodard <woodard@redhat.com>]

> On Aug 12, 2022, at 8:26 AM, Giuliano Procida <gprocida@google.com> wrote:
> 
> Hi Ben.
> 
> On Thu, 11 Aug 2022 at 03:22, Ben Woodard <woodard@redhat.com <mailto:woodard@redhat.com>> wrote:
>> 
>> 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.
>> 
> 
> 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.

I like this formulation of the process. This is markedly different than the current codebase though and moving from what we have to what you propose would be a long process. Even at my most ambition, I’ve been tinkering around with the API to make it more generally easy to apply to different projects. However, what you are suggesting has some real appeal it essentially turns it into a kind of compiler of sorts.

Front ends:
ELF + DWARF
ELF + CTF
ABIXML

All of which generate a IR

Then very much a compiler you have a set of passes that transform the IR. You listed many of these above.
Then you have a pass manager that assembles and orders the passes which are applied for the desired results. Some of these are required and some of these would be specified by the source language of the TU. 
Then as you suggest some of these could be controlled in groups based on your desired outcome.

It sort of seems like the whole compiler analogy breaks down when we get to the output. There is nothing like codegen in the comparison and output side of the program. 

What the team that I work with would like are:
1) find one critical problem and terminate mode.  Once you find one problem, you don’t need to continue with the comparison.
2) A machine readable output format in addition to the human readable text mode.
3) They would like the comparison of the IR to be written in some logic programming language like ASP. This of course would need a method within the rulesets to define rulesets which are full breaks vs. ones which can be overlooked because they are arguably compatible.
4) a python interface to the IR (this is relatively simple — the challenge really falls back into redesigning the libabigail API for general too use.)

> 
> 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-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