[0/3] Make integral types of same base and size compatible

  Hello,

On some platforms, 'long int' and 'long long int' have the same size.
On those platforms, those two types should be considered ABI
compatible.  Today, libabigail always consider these types as
different.  This leads some spurious type changes, especially when,
e.g, a type struct Foo is defined in two different translation unit,
once using a "long long int" through a typedef, and once using a "long
int" through a typedef.  In that case, libabigail (wrongly) considers
the two struct Foo as different.  And that leads to weird and hard to
debug self comparison failures down the road.  For instance, the
following command fails:

    $ time tools/fedabipkgdiff --debug --self-compare -a --from fc36 btrfs-progs

Fixing this issue uncovers two other issues that needed fixing
altogether.

First, in some cases, the sorting of array types can be non-stable in
the abixml output.  This is due to some ambiguities that can happen
with the representation of array element types.  The first patch of
the series address that.

Second, some qualified types can be sometimes represented with
redundant (and thus very confusing) CV-qualifiers.  The second patch
removes those and provides a hopefully less confusing pretty
representation of qualified types.

Below is the summary of the patch series that was applied to master.

Dodji Seketeli (3):
  ir: Disambiguate sorting of array element types
  dwarf-reader: Remove redundant qualifiers from qualified types
  ir: Consider integral types of same kind and size as equivalent

 include/abg-fwd.h                             |     9 +
 include/abg-ir.h                              |    10 +
 src/abg-dwarf-reader.cc                       |     5 +-
 src/abg-ir-priv.h                             |    11 +-
 src/abg-ir.cc                                 |   419 +-
 src/abg-reader.cc                             |     3 +-
 .../qualifier-typedef-array-report-1.txt      |    40 +-
 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  | 12362 +++--
 .../data/test-annotate/test17-pr19027.so.abi  |  2246 +-
 ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 11742 +++--
 ...19-pr19023-libtcmalloc_and_profiler.so.abi | 16188 +++---
 ...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 +-
 49 files changed, 129753 insertions(+), 129553 deletions(-)
  

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.

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

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.

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

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.

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.

> 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
  

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
>
  

Hi Ben.

On Thu, 11 Aug 2022 at 03:22, Ben Woodard <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.

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

Giuliano Procida <gprocida@google.com> writes:

> Hi Dodji.

Hello Giuliano,

[...]

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

Right.  But as usual with these things (API vs ABI conformance), we try
to accommodate people's need as much as possible, with a preference with
ABI conformance when we can't ensure both at the same time.  That's what
we have done historically, but of course, my stance is not cast in
stone.  I am open to discussion and change.

In this particular case of /C type/ program, it seems to be that the
programmers expect the types to be ABI compatible.

C++ of course being strongly type as it is, I understand that we might
want to be stricter.

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

For C++ (or Ada, or in those strongly type languages) I think I
understand why some change reports might be confusing.  In your mind,
can we have the issue with C however? (real question).


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


I am not sure to understand the question.  This kind of adjustment of
what is is read from the binary typically tends to happen at the core
level (either DWARF reader, IR construction/transformation, comparison,
etc) rather at the level of a specific tool.  Am I missing something
from what you have in mind?

>
> We might not end up with stable XML but the finger of blame should be
> pointed at the btrfs-progs in any case.

I understand and I sympathesize with your point of view.  But just to
explain where I sit on the matter, there have unfortunately been plenty
of real life cases where the programs (those written by the app/library
developers or by the compiler/linker developers) are not what libabigail
would expect in a perfect world.  So far we try hard to "accommodate"
when we can, if that can lead to a better experience for libabigail
users.  But I agree that we shouldn't try harder.  I guess knowing the
difference is the crux of this art.  So I am open to discussion to try
to accommodate your point of view too.

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

Would it be useful for your case if this behaviour happens just for C
programs?

> Is there an easy way of putting this under flag control?

I guess so, yes.  But just a flag would not be optimal from a user
standpoint, would it?

Thank you for raising this and sorry for the inconvenience.

I hope we resolve this.

Cheers,
  

> On Aug 16, 2022, at 9:54 AM, Dodji Seketeli <dodji@seketeli.org> wrote:
> 
> Giuliano Procida <gprocida@google.com <mailto:gprocida@google.com>> writes:
> 
>> Hi Dodji.
> 
> Hello Giuliano,
> 
> [...]
> 
>> 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.
> 
> Right.  But as usual with these things (API vs ABI conformance), we try
> to accommodate people's need as much as possible, with a preference with
> ABI conformance when we can't ensure both at the same time.  That's what
> we have done historically, but of course, my stance is not cast in
> stone.  I am open to discussion and change.
> 
> In this particular case of /C type/ program, it seems to be that the
> programmers expect the types to be ABI compatible.
> 
> C++ of course being strongly type as it is, I understand that we might
> want to be stricter.
> 
>> 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.
> 
> For C++ (or Ada, or in those strongly type languages) I think I
> understand why some change reports might be confusing.  In your mind,
> can we have the issue with C however? (real question).
> 
> 
>>> 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?
> 
> 
> I am not sure to understand the question.  This kind of adjustment of
> what is is read from the binary typically tends to happen at the core
> level (either DWARF reader, IR construction/transformation, comparison,
> etc) rather at the level of a specific tool.  Am I missing something
> from what you have in mind?
> 
>> 
>> We might not end up with stable XML but the finger of blame should be
>> pointed at the btrfs-progs in any case.
> 
> I understand and I sympathesize with your point of view.  But just to
> explain where I sit on the matter, there have unfortunately been plenty
> of real life cases where the programs (those written by the app/library
> developers or by the compiler/linker developers) are not what libabigail
> would expect in a perfect world.  So far we try hard to "accommodate"
> when we can, if that can lead to a better experience for libabigail
> users.  But I agree that we shouldn't try harder.  I guess knowing the
> difference is the crux of this art.  So I am open to discussion to try
> to accommodate your point of view too.
> 
I kind of think that in some of these difficult to handle cases, we should consider submitting patches to the underlying projects. 

Are there any compiler warning options that can point out some of these type mismatches. I know that in this case we are dealing with C but I wonder if some of the stronger C++ tyope logic could be applied and emit a warning. I would think that in some cases this particular bug might end up being an architecture specific problem in cases where the size of types are not the same.


>>> 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.
> 
> Would it be useful for your case if this behaviour happens just for C
> programs?
> 
>> Is there an easy way of putting this under flag control?
> 
> I guess so, yes.  But just a flag would not be optimal from a user
> standpoint, would it?
> 
> Thank you for raising this and sorry for the inconvenience.
> 
> I hope we resolve this.
> 
> Cheers,
> 
> -- 
> 		Dodji
  

Sorry, I have to be brief...

On Tue, 16 Aug 2022 at 17:54, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Giuliano Procida <gprocida@google.com> writes:
>
> > Hi Dodji.
>
> Hello Giuliano,
>
> [...]
>
> > 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.
>
> Right.  But as usual with these things (API vs ABI conformance), we try
> to accommodate people's need as much as possible, with a preference with
> ABI conformance when we can't ensure both at the same time.  That's what
> we have done historically, but of course, my stance is not cast in
> stone.  I am open to discussion and change.
>
> In this particular case of /C type/ program, it seems to be that the
> programmers expect the types to be ABI compatible.

I think with so much multi-architecture code about and the difficulty
of (say) ABI
monitoring less common architectures, detecting API changes that will be ABI
breaks on another architecture seems like a big positive.

> C++ of course being strongly type as it is, I understand that we might
> want to be stricter.
>
> > 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.
>
> For C++ (or Ada, or in those strongly type languages) I think I
> understand why some change reports might be confusing.  In your mind,
> can we have the issue with C however? (real question).
>
>
> >> 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?
>
>
> I am not sure to understand the question.  This kind of adjustment of
> what is is read from the binary typically tends to happen at the core
> level (either DWARF reader, IR construction/transformation, comparison,
> etc) rather at the level of a specific tool.  Am I missing something
> from what you have in mind?

The earlier the re-interpretation is, the less visible it is and the original
information cannot be recovered.

Alternatively, isn't this sort of thing exactly what the suppression logic in
abidiff is supposed to be used for?

> >
> > We might not end up with stable XML but the finger of blame should be
> > pointed at the btrfs-progs in any case.
>
> I understand and I sympathesize with your point of view.  But just to
> explain where I sit on the matter, there have unfortunately been plenty
> of real life cases where the programs (those written by the app/library
> developers or by the compiler/linker developers) are not what libabigail
> would expect in a perfect world.  So far we try hard to "accommodate"
> when we can, if that can lead to a better experience for libabigail
> users.  But I agree that we shouldn't try harder.  I guess knowing the
> difference is the crux of this art.  So I am open to discussion to try
> to accommodate your point of view too.
>
> >> 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.
>
> Would it be useful for your case if this behaviour happens just for C
> programs?

We support both ARM32 and ARM64 Android userspace and there are
both C and C++ libraries. So just for ourselves, no.

> > Is there an easy way of putting this under flag control?
>
> I guess so, yes.  But just a flag would not be optimal from a user
> standpoint, would it?

If there is, then it would be easy to disable in a less intrusive way than
carrying a rollback commit in AOSP. We don't actually need the flag
control.

> Thank you for raising this and sorry for the inconvenience.
>
> I hope we resolve this.

Sure. One way or another. :-)

Thanks for the reply.

Giuliano.

> Cheers,
>
> --
>                 Dodji
  

> 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
  

Giuliano Procida <gprocida@google.com> a écrit:

> Sorry, I have to be brief...

No problem.

[...]

>> Right.  But as usual with these things (API vs ABI conformance), we try
>> to accommodate people's need as much as possible, with a preference with
>> ABI conformance when we can't ensure both at the same time.  That's what
>> we have done historically, but of course, my stance is not cast in
>> stone.  I am open to discussion and change.
>>
>> In this particular case of /C type/ program, it seems to be that the
>> programmers expect the types to be ABI compatible.
>
> I think with so much multi-architecture code about and the difficulty
> of (say) ABI
> monitoring less common architectures, detecting API changes that will be ABI
> breaks on another architecture seems like a big positive.

This is interesting.

Historically, I chose to analyse binaries rather than source code
precisely because I wanted to compare the ABIs of the binaries directly,
architecture per architecture, rather than trying to infer what API
change might incur an ABI change.  The main assumption is that we do
*HAVE* access to the actual binaries.  And what I really cared about was
actual ABI changes, not API changes.

Doing the API compatibility verification came afterwards, in a "nice to
have manner", from the request of users over time, like icing on the
cake, so to speak.  The core of what I wanted really was ABI comparison.

It's funny to see how the API side of the requirement got stronger over
time.

Anyway, I think I get your point.

[...]

>> >> 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?
>>
>>
>> I am not sure to understand the question.  This kind of adjustment of
>> what is is read from the binary typically tends to happen at the core
>> level (either DWARF reader, IR construction/transformation, comparison,
>> etc) rather at the level of a specific tool.  Am I missing something
>> from what you have in mind?
>
> The earlier the re-interpretation is, the less visible it is and the original
> information cannot be recovered.

Of course.  But keep some information all the way can be more costly
than trimming them off early, if we know we don't need them.  It's a
tradeoff that seems clear in my mind.

> Alternatively, isn't this sort of thing exactly what the suppression logic in
> abidiff is supposed to be used for?

Self-comparing the IR from a binary against it's abixml counterpart is
supposed to be done without any suppression specification applied.

OK, so here is what I am proposing.

Either I disable this thing altogether (namely, saying that int, short
int, long int and long long int are the same if the types have the same
size; note that other integral types are not touched by this) and give
up on the self-comparison check failure of the btrfs-progs package or I can
use this "abi-only-comparison" only when we do self-comparison check,
i.e, when we do abidw --abidiff and abipkgdiff --self-check.

I have a candidate patch for the later and the former is even easier to
do.

@Ben and @Giuliano, what would you prefer?

Cheers,
  

I do not believe applying the fundamental type folding only to self comparison will meet our needs. The real reason that self comparison is important to us is we need to throw away the binary and then save the abixml. Then later on we can compare the abixml to a new build to find out if something happened that broke ABI. Thus for our needs, we will not be in a self comparison operation at the time when we come across this in a non-testing environment. I believe that the ordering of the TUs that make up the binary could make this problem appear, thus I do not think it will serve our purposes.

I know that C doesn’t have a ODR but I’m fairly certain that code along the lines of what is in btrfs-progs will have portability problems between architectures. Therefore, I think that programs like this should be fixed. I’m start a discussion with the btrfs guys and explain the problem. I can imagine a possible reason for this problem in btrfs-progs though, it could be caused by the need to read a FS created on one arch on a machine of another arch. I ran into that a few times many years ago and worked with upstream to fix them. (Ia64 era IIRC)

However, the fact that libabigail catches this problem is far too late. At the very least I’d say the compiler should warn about issues like this. We should talk to the compiler guys and see if we can have them create a warning about this situation. Dodji could you start a discussion with our compiler team on this and I will weigh in on it.

As for libabigail, this problem is too subtle for a normal user to understand given the current output and so we need to do something. When reading a corpus of a binary when you come across a case where two different typedefs from two different TUs have different types but the ABI of the fundamental type is compatible, you should emit a warning saying something like: “two typedefs types from different TUs have conflicting types. ABI comparisons cannot be reliably done. file1.c:<line no> and file2.c:<line no>“. So basically at the place where you currently fold  the fundamental integral types in this patch if they are ABI compatible, print a warning instead. That will let me know what the problem is when I do a test run and I will report it to the upstream package rather than you. I’ll try to fix the world and it won’t be your problem.

Maybe have a big comment near the logic in the DWARF reader or where this is detected that explains the decision for the next guy. 

So in summary: 
1) Revert or replace this patch and add warning to libabigail. That should satisfy both me and Giuliano. 
2) Start a discussion with the GCC folk about adding a warning. I’ll follow up.
3) I’ll use the GCC warning discussion to bring this up with upstream btrfs people and possibly suggest a patch to fix their stuff.
4) I’ll do the same with any other packages that have a similar problem.

-ben

> On Aug 18, 2022, at 9:29 AM, Dodji Seketeli <dodji@seketeli.org> wrote:
> 
> Giuliano Procida <gprocida@google.com> a écrit:
> 
>> Sorry, I have to be brief...
> 
> No problem.
> 
> [...]
> 
>>> Right.  But as usual with these things (API vs ABI conformance), we try
>>> to accommodate people's need as much as possible, with a preference with
>>> ABI conformance when we can't ensure both at the same time.  That's what
>>> we have done historically, but of course, my stance is not cast in
>>> stone.  I am open to discussion and change.
>>> 
>>> In this particular case of /C type/ program, it seems to be that the
>>> programmers expect the types to be ABI compatible.
>> 
>> I think with so much multi-architecture code about and the difficulty
>> of (say) ABI
>> monitoring less common architectures, detecting API changes that will be ABI
>> breaks on another architecture seems like a big positive.
> 
> This is interesting.
> 
> Historically, I chose to analyse binaries rather than source code
> precisely because I wanted to compare the ABIs of the binaries directly,
> architecture per architecture, rather than trying to infer what API
> change might incur an ABI change.  The main assumption is that we do
> *HAVE* access to the actual binaries.  And what I really cared about was
> actual ABI changes, not API changes.
> 
> Doing the API compatibility verification came afterwards, in a "nice to
> have manner", from the request of users over time, like icing on the
> cake, so to speak.  The core of what I wanted really was ABI comparison.
> 
> It's funny to see how the API side of the requirement got stronger over
> time.
> 
> Anyway, I think I get your point.
> 
> [...]
> 
>>>>> 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?
>>> 
>>> 
>>> I am not sure to understand the question.  This kind of adjustment of
>>> what is is read from the binary typically tends to happen at the core
>>> level (either DWARF reader, IR construction/transformation, comparison,
>>> etc) rather at the level of a specific tool.  Am I missing something
>>> from what you have in mind?
>> 
>> The earlier the re-interpretation is, the less visible it is and the original
>> information cannot be recovered.
> 
> Of course.  But keep some information all the way can be more costly
> than trimming them off early, if we know we don't need them.  It's a
> tradeoff that seems clear in my mind.
> 
>> Alternatively, isn't this sort of thing exactly what the suppression logic in
>> abidiff is supposed to be used for?
> 
> Self-comparing the IR from a binary against it's abixml counterpart is
> supposed to be done without any suppression specification applied.
> 
> OK, so here is what I am proposing.
> 
> Either I disable this thing altogether (namely, saying that int, short
> int, long int and long long int are the same if the types have the same
> size; note that other integral types are not touched by this) and give
> up on the self-comparison check failure of the btrfs-progs package or I can
> use this "abi-only-comparison" only when we do self-comparison check,
> i.e, when we do abidw --abidiff and abipkgdiff --self-check.
> 
> I have a candidate patch for the later and the former is even easier to
> do.
> 
> @Ben and @Giuliano, what would you prefer?
> 
> Cheers,
> 
> -- 
>        Dodji
>
  

Hello,

Ben Woodard <woodard@redhat.com> a écrit:

> I do not believe applying the fundamental type folding only to self
> comparison will meet our needs. The real reason that self comparison
> is important to us is we need to throw away the binary and then save
> the abixml. Then later on we can compare the abixml to a new build to
> find out if something happened that broke ABI. Thus for our needs, we
> will not be in a self comparison operation at the time when we come
> across this in a non-testing environment. I believe that the ordering
> of the TUs that make up the binary could make this problem appear,
> thus I do not think it will serve our purposes.

OK.

> I know that C doesn’t have a ODR but I’m fairly certain that code
> along the lines of what is in btrfs-progs will have portability
> problems between architectures. Therefore, I think that programs like
> this should be fixed. I’m start a discussion with the btrfs guys and
> explain the problem. I can imagine a possible reason for this problem
> in btrfs-progs though, it could be caused by the need to read a FS
> created on one arch on a machine of another arch. I ran into that a
> few times many years ago and worked with upstream to fix them. (Ia64
> era IIRC)
>
> However, the fact that libabigail catches this problem is far too
> late. At the very least I’d say the compiler should warn about issues
> like this. We should talk to the compiler guys and see if we can have
> them create a warning about this situation.

This is a well known problem in the compiler space.  G++ now has the
-Wno-odr warning to detect ODR violation and emit a warning.  But that's
for C++.

For C however, I am not sure.


> Dodji could you start a discussion with our compiler team on this and
> I will weigh in on it.

Yeah, I'll see what I can do.

> As for libabigail, this problem is too subtle for a normal user to
> understand given the current output and so we need to do
> something. When reading a corpus of a binary when you come across a
> case where two different typedefs from two different TUs have
> different types but the ABI of the fundamental type is compatible, you
> should emit a warning saying something like: “two typedefs types from
> different TUs have conflicting types. ABI comparisons cannot be
> reliably done. file1.c:<line no> and file2.c:<line no>“. So basically
> at the place where you currently fold the fundamental integral types
> in this patch if they are ABI compatible, print a warning
> instead. That will let me know what the problem is when I do a test
> run and I will report it to the upstream package rather than you. I’ll
> try to fix the world and it won’t be your problem.

Emitting a comment that makes sense would be nice, indeed.  But I'll
need to tackle this separately because it's not exactly a one
liner. There can be a lot of types being in that case, just because they
happen to use a type that exhibits the problem.  We need to emit the
comment just for the type that is at the root cause of the problem.  So
it takes a little bit of thinking, I think.

Also, I might have a way to handle this in a way that enable us to
represent those ODR-violating types regardless.  I don't like how
libabigail is fragile here.  But I need to play with the idea a little.
And I might be wrong.  I think this topic needs some more hashing out.

> Maybe have a big comment near the logic in the DWARF reader or where this is detected that explains the decision for the next guy. 
>
> So in summary: 
> 1) Revert or replace this patch and add warning to libabigail. That should satisfy both me and Giuliano. 


OK, I have just done this.  The patch is https://sourceware.org/pipermail/libabigail/2022q3/004666.html.

> 2) Start a discussion with the GCC folk about adding a warning. I’ll follow up.
> 3) I’ll use the GCC warning discussion to bring this up with upstream btrfs people and possibly suggest a patch to fix their stuff.

As I said above, I need to think about this a little bit more.

> 4) I’ll do the same with any other packages that have a similar problem.

Yeah.  The problem though is that it needs very careful debugging to
understand the problem.  That is where your point about libabigail
emitting a warning makes sense.

Thank you for your handling this.

[...]

Cheers,