public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Make integral types of same base and size compatible
@ 2022-07-22 23:19 Dodji Seketeli
  2022-07-22 23:28 ` [PATCH 1/3] ir: Disambiguate sorting of array element types Dodji Seketeli
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Dodji Seketeli @ 2022-07-22 23:19 UTC (permalink / raw)
  To: libabigail

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

-- 
2.36.1

Cheers,

-- 
		Dodji


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] ir: Disambiguate sorting of array element types
  2022-07-22 23:19 [PATCH 0/3] Make integral types of same base and size compatible Dodji Seketeli
@ 2022-07-22 23:28 ` Dodji Seketeli
  2022-07-22 23:31 ` [PATCH 2/3] dwarf-reader: Remove redundant qualifiers from qualified types Dodji Seketeli
  2022-07-22 23:32 ` [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent Dodji Seketeli
  2 siblings, 0 replies; 14+ messages in thread
From: Dodji Seketeli @ 2022-07-22 23:28 UTC (permalink / raw)
  To: Dodji Seketeli via Libabigail; +Cc: Dodji Seketeli

Hello,

When using non-internal pretty representation of array types (e.g, for
sorting of types in a given scope for the purpose of serialization),
some array element types might have the same name, even though they
don't have the same qualified name.  In those cases, the serialized
abixml output is not stable.

This patches uses qualified names for array element names for type
sorting purposes.

However, this patch uncovers a problem that shows up in the tests
outputs for test-abidiff-exit and test-diff-filter, where emitting
qualified names of qualified types shows that there can be redundant
qualifiers in the serialized output.  This issue will be fixed
separately in a later commit.  For now, the output of these tests is
temporarily updated to have the tests pass.

	* src/abg-ir.cc (get_type_representation): In the overload for
	array_type_def, use qualified names for element types.
	* tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt: Adjust.
	* tests/data/test-annotate/test15-pr18892.so.abi: Adjust.
	* tests/data/test-annotate/test17-pr19027.so.abi: Adjust.
	* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Adjust.
	* tests/data/test-diff-filter/test-PR26739-2-report-0.txt: Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to master.
---
 src/abg-ir.cc                                 |   2 +-
 .../qualifier-typedef-array-report-1.txt      |  20 ++--
 .../data/test-annotate/test15-pr18892.so.abi  |  32 +++---
 .../data/test-annotate/test17-pr19027.so.abi  | 104 +++++++++---------
 ...19-pr19023-libtcmalloc_and_profiler.so.abi |  14 +--
 .../test-PR26739-2-report-0.txt               |   2 +-
 6 files changed, 87 insertions(+), 87 deletions(-)

diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 07736db9..5c1ac35b 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -17066,7 +17066,7 @@ get_type_representation(const array_type_def& a, bool internal)
 	  + a.get_subrange_representation();
       else
 	r = (e_type
-	     ? get_type_name(e_type, /*qualified=*/false, /*internal=*/false)
+	     ? get_type_name(e_type, /*qualified=*/true, /*internal=*/false)
 	     : string("void"))
 	  + a.get_subrange_representation();
     }
diff --git a/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt b/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt
index e9bf3d46..13949efc 100644
--- a/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt
+++ b/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt
@@ -50,36 +50,36 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
             entity changed from 'typedef C' to compatible type 'const volatile void* const[7]'
               array element type 'void* const' changed:
                 'void* const' changed to 'const volatile void* const'
-              type name changed from 'void* const[7]' to 'volatile void* const[7]'
+              type name changed from 'void* const[7]' to 'const volatile void* const[7]'
               type size hasn't changed
           type of 'C r_c' changed:
-            entity changed from 'typedef C' to compatible type 'restrict void* const[7]'
+            entity changed from 'typedef C' to compatible type 'restrict const void* const[7]'
               array element type 'void* const' changed:
                 'void* const' changed to 'restrict const void* const'
-              type name changed from 'void* const[7]' to 'restrict void* const[7]'
+              type name changed from 'void* const[7]' to 'restrict const void* const[7]'
               type size hasn't changed
           type of 'D v_d' changed:
             entity changed from 'typedef D' to compatible type 'const volatile void* const[7]'
               array element type 'void* const' changed:
                 'void* const' changed to 'const volatile void* const'
-              type name changed from 'void* const[7]' to 'volatile void* const[7]'
+              type name changed from 'void* const[7]' to 'const volatile void* const[7]'
               type size hasn't changed
           type of 'D r_d' changed:
-            entity changed from 'typedef D' to compatible type 'restrict void* const[7]'
+            entity changed from 'typedef D' to compatible type 'restrict const void* const[7]'
               array element type 'void* const' changed:
                 'void* const' changed to 'restrict const void* const'
-              type name changed from 'void* const[7]' to 'restrict void* const[7]'
+              type name changed from 'void* const[7]' to 'restrict const void* const[7]'
               type size hasn't changed
           type of 'E r_e' changed:
-            entity changed from 'typedef E' to compatible type 'restrict const volatile void* const[7]'
+            entity changed from 'typedef E' to compatible type 'restrict const volatile volatile void* const[7]'
               array element type 'const volatile void* const' changed:
                 'const volatile void* const' changed to 'restrict const volatile volatile void* const'
-              type name changed from 'volatile void* const[7]' to 'restrict const volatile void* const[7]'
+              type name changed from 'const volatile void* const[7]' to 'restrict const volatile volatile void* const[7]'
               type size hasn't changed
           type of 'F r_f' changed:
-            entity changed from 'typedef F' to compatible type 'restrict const volatile void* const[7]'
+            entity changed from 'typedef F' to compatible type 'restrict const volatile volatile void* const[7]'
               array element type 'const volatile void* const' changed:
                 'const volatile void* const' changed to 'restrict const volatile volatile void* const'
-              type name changed from 'volatile void* const[7]' to 'restrict const volatile void* const[7]'
+              type name changed from 'const volatile void* const[7]' to 'restrict const volatile volatile void* const[7]'
               type size hasn't changed
 
diff --git a/tests/data/test-annotate/test15-pr18892.so.abi b/tests/data/test-annotate/test15-pr18892.so.abi
index bbabb567..dbcdb390 100644
--- a/tests/data/test-annotate/test15-pr18892.so.abi
+++ b/tests/data/test-annotate/test15-pr18892.so.abi
@@ -4588,7 +4588,7 @@
     </function-type>
   </abi-instr>
   <abi-instr address-size='64' path='../../.././libsanitizer/sanitizer_common/sanitizer_common.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-ImG4Cf/gcc-4.9.2/x86_64-unknown-linux-gnu/libsanitizer/sanitizer_common' language='LANG_C_plus_plus'>
-    <!-- AddressRange[6] -->
+    <!-- __sanitizer::LoadedModule::AddressRange[6] -->
     <array-type-def dimensions='1' type-id='type-id-121' size-in-bits='768' id='type-id-122'>
       <!-- <anonymous range>[6] -->
       <subrange length='6' type-id='type-id-49' id='type-id-123'/>
@@ -10820,17 +10820,17 @@
     </namespace-decl>
   </abi-instr>
   <abi-instr address-size='64' path='../../.././libsanitizer/tsan/tsan_interceptors.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-ImG4Cf/gcc-4.9.2/x86_64-unknown-linux-gnu/libsanitizer/tsan' language='LANG_C_plus_plus'>
-    <!-- LibCodeRange[128] -->
+    <!-- __sanitizer::LibIgnore::LibCodeRange[128] -->
     <array-type-def dimensions='1' type-id='type-id-361' size-in-bits='16384' id='type-id-362'>
       <!-- <anonymous range>[128] -->
       <subrange length='128' type-id='type-id-49' id='type-id-363'/>
     </array-type-def>
-    <!-- Lib[128] -->
+    <!-- __sanitizer::LibIgnore::Lib[128] -->
     <array-type-def dimensions='1' type-id='type-id-364' size-in-bits='32768' id='type-id-365'>
       <!-- <anonymous range>[128] -->
       <subrange length='128' type-id='type-id-49' id='type-id-363'/>
     </array-type-def>
-    <!-- SignalDesc[64] -->
+    <!-- __tsan::SignalDesc[64] -->
     <array-type-def dimensions='1' type-id='type-id-366' size-in-bits='552960' id='type-id-367'>
       <!-- <anonymous range>[64] -->
       <subrange length='64' type-id='type-id-49' id='type-id-368'/>
@@ -10907,7 +10907,7 @@
       <!-- <anonymous range>[4] -->
       <subrange length='4' type-id='type-id-49' id='type-id-388'/>
     </array-type-def>
-    <!-- void*[128] -->
+    <!-- void ()*[128] -->
     <array-type-def dimensions='1' type-id='type-id-153' size-in-bits='8192' id='type-id-389'>
       <!-- <anonymous range>[128] -->
       <subrange length='128' type-id='type-id-49' id='type-id-363'/>
@@ -27650,42 +27650,42 @@
   <abi-instr address-size='64' path='../../.././libsanitizer/tsan/tsan_interface_atomic.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-ImG4Cf/gcc-4.9.2/x86_64-unknown-linux-gnu/libsanitizer/tsan' language='LANG_C_plus_plus'>
     <!-- __int128 -->
     <type-decl name='__int128' size-in-bits='128' id='type-id-1270'/>
-    <!-- SizeClassInfo[53] -->
+    <!-- __sanitizer::SizeClassAllocator32<0ul, 140737488355328ull, 16ul, __sanitizer::SizeClassMap<17ul, 64ul, 14ul>, 24ul, __sanitizer::TwoLevelByteMap<2048ull, 4096ull, __sanitizer::NoOpMapUnmapCallback>, __sanitizer::NoOpMapUnmapCallback>::SizeClassInfo[53] -->
     <array-type-def dimensions='1' type-id='type-id-1271' size-in-bits='27136' id='type-id-1272'>
       <!-- <anonymous range>[53] -->
       <subrange length='53' type-id='type-id-49' id='type-id-1273'/>
     </array-type-def>
-    <!-- PerClass[53] -->
+    <!-- __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator32<0ul, 140737488355328ull, 16ul, __sanitizer::SizeClassMap<17ul, 64ul, 14ul>, 24ul, __sanitizer::TwoLevelByteMap<2048ull, 4096ull, __sanitizer::NoOpMapUnmapCallback>, __sanitizer::NoOpMapUnmapCallback> >::PerClass[53] -->
     <array-type-def dimensions='1' type-id='type-id-185' size-in-bits='440960' id='type-id-189'>
       <!-- <anonymous range>[53] -->
       <subrange length='53' type-id='type-id-49' id='type-id-1273'/>
     </array-type-def>
-    <!-- PerClass[53] -->
+    <!-- __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator64<137438953472000ul, 1099511627776ul, 16ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __tsan::MapUnmapCallback> >::PerClass[53] -->
     <array-type-def dimensions='1' type-id='type-id-1274' size-in-bits='875136' id='type-id-1275'>
       <!-- <anonymous range>[53] -->
       <subrange length='53' type-id='type-id-49' id='type-id-1273'/>
     </array-type-def>
-    <!-- atomic_uint64_t[4] -->
+    <!-- __sanitizer::atomic_uint64_t[4] -->
     <array-type-def dimensions='1' type-id='type-id-1276' size-in-bits='256' id='type-id-1277'>
       <!-- <anonymous range>[4] -->
       <subrange length='4' type-id='type-id-49' id='type-id-388'/>
     </array-type-def>
-    <!-- atomic_uintptr_t[2048] -->
+    <!-- __sanitizer::atomic_uintptr_t[2048] -->
     <array-type-def dimensions='1' type-id='type-id-1232' size-in-bits='131072' id='type-id-1278'>
       <!-- <anonymous range>[2048] -->
       <subrange length='2048' type-id='type-id-49' id='type-id-1279'/>
     </array-type-def>
-    <!-- MD5Hash[2] -->
+    <!-- __tsan::MD5Hash[2] -->
     <array-type-def dimensions='1' type-id='type-id-1280' size-in-bits='256' id='type-id-1281'>
       <!-- <anonymous range>[2] -->
       <subrange length='2' type-id='type-id-49' id='type-id-1282'/>
     </array-type-def>
-    <!-- Desc[16] -->
+    <!-- __tsan::MutexSet::Desc[16] -->
     <array-type-def dimensions='1' type-id='type-id-1283' size-in-bits='3072' id='type-id-1284'>
       <!-- <anonymous range>[16] -->
       <subrange length='16' type-id='type-id-49' id='type-id-382'/>
     </array-type-def>
-    <!-- Part[1009] -->
+    <!-- __tsan::SyncTab::Part[1009] -->
     <array-type-def dimensions='1' type-id='type-id-1285' size-in-bits='516608' id='type-id-1286'>
       <!-- <anonymous range>[1009] -->
       <subrange length='1009' type-id='type-id-49' id='type-id-1287'/>
@@ -35556,12 +35556,12 @@
     </namespace-decl>
   </abi-instr>
   <abi-instr address-size='64' path='../../.././libsanitizer/tsan/tsan_mman.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-ImG4Cf/gcc-4.9.2/x86_64-unknown-linux-gnu/libsanitizer/tsan' language='LANG_C_plus_plus'>
-    <!-- Header*[262144] -->
+    <!-- __sanitizer::LargeMmapAllocator<__sanitizer::CrashOnMapUnmap>::Header*[262144] -->
     <array-type-def dimensions='1' type-id='type-id-1518' size-in-bits='16777216' id='type-id-1519'>
       <!-- <anonymous range>[262144] -->
       <subrange length='262144' type-id='type-id-49' id='type-id-1520'/>
     </array-type-def>
-    <!-- Header*[262144] -->
+    <!-- __sanitizer::LargeMmapAllocator<__tsan::MapUnmapCallback>::Header*[262144] -->
     <array-type-def dimensions='1' type-id='type-id-1521' size-in-bits='16777216' id='type-id-1522'>
       <!-- <anonymous range>[262144] -->
       <subrange length='262144' type-id='type-id-49' id='type-id-1520'/>
@@ -40807,7 +40807,7 @@
     </namespace-decl>
   </abi-instr>
   <abi-instr address-size='64' path='../../.././libsanitizer/tsan/tsan_rtl_thread.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-ImG4Cf/gcc-4.9.2/x86_64-unknown-linux-gnu/libsanitizer/tsan' language='LANG_C_plus_plus'>
-    <!-- TraceHeader[256] -->
+    <!-- __tsan::TraceHeader[256] -->
     <array-type-def dimensions='1' type-id='type-id-1690' size-in-bits='5062656' id='type-id-1691'>
       <!-- <anonymous range>[256] -->
       <subrange length='256' type-id='type-id-49' id='type-id-202'/>
diff --git a/tests/data/test-annotate/test17-pr19027.so.abi b/tests/data/test-annotate/test17-pr19027.so.abi
index a303267b..a9f7b565 100644
--- a/tests/data/test-annotate/test17-pr19027.so.abi
+++ b/tests/data/test-annotate/test17-pr19027.so.abi
@@ -528,7 +528,7 @@
       <!-- <anonymous range>[40] -->
       <subrange length='40' type-id='type-id-4' id='type-id-5'/>
     </array-type-def>
-    <!-- hb_user_data_item_t[2] -->
+    <!-- hb_user_data_array_t::hb_user_data_item_t[2] -->
     <array-type-def dimensions='1' type-id='type-id-6' size-in-bits='384' id='type-id-7'>
       <!-- <anonymous range>[2] -->
       <subrange length='2' type-id='type-id-4' id='type-id-8'/>
@@ -3931,12 +3931,12 @@
     <typedef-decl name='hb_tag_t' type-id='type-id-100' filepath='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src/hb-common.h' line='91' column='1' id='type-id-185'/>
   </abi-instr>
   <abi-instr address-size='64' path='hb-face.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src' language='LANG_C_plus_plus'>
-    <!-- OffsetTo<OT::OffsetTable, OT::IntType<unsigned int, 4u> >[1] -->
+    <!-- OT::OffsetTo<OT::OffsetTable, OT::IntType<unsigned int, 4u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-186' id='type-id-187'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- TableRecord[1] -->
+    <!-- OT::TableRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-188' id='type-id-189'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
@@ -10736,52 +10736,52 @@
     </function-decl>
   </abi-instr>
   <abi-instr address-size='64' path='hb-ot-font.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src' language='LANG_C_plus_plus'>
-    <!-- BYTE[256] -->
+    <!-- OT::BYTE[256] -->
     <array-type-def dimensions='1' type-id='type-id-670' size-in-bits='2048' id='type-id-671'>
       <!-- <anonymous range>[256] -->
       <subrange length='256' type-id='type-id-4' id='type-id-672'/>
     </array-type-def>
-    <!-- CmapSubtableLongGroup[1] -->
+    <!-- OT::CmapSubtableLongGroup[1] -->
     <array-type-def dimensions='1' type-id='type-id-673' id='type-id-674'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- EncodingRecord[1] -->
+    <!-- OT::EncodingRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-675' id='type-id-676'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- IntType<short unsigned int, 2u>[1] -->
+    <!-- OT::IntType<short unsigned int, 2u>[1] -->
     <array-type-def dimensions='1' type-id='type-id-237' id='type-id-677'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- LongMetric[1] -->
+    <!-- OT::LongMetric[1] -->
     <array-type-def dimensions='1' type-id='type-id-678' id='type-id-679'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- SHORT[1] -->
+    <!-- OT::SHORT[1] -->
     <array-type-def dimensions='1' type-id='type-id-573' id='type-id-680'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- USHORT[1] -->
+    <!-- OT::USHORT[1] -->
     <array-type-def dimensions='1' type-id='type-id-371' id='type-id-681'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- UVSMapping[1] -->
+    <!-- OT::UVSMapping[1] -->
     <array-type-def dimensions='1' type-id='type-id-682' id='type-id-683'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- UnicodeValueRange[1] -->
+    <!-- OT::UnicodeValueRange[1] -->
     <array-type-def dimensions='1' type-id='type-id-684' id='type-id-685'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- VariationSelectorRecord[1] -->
+    <!-- OT::VariationSelectorRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-686' id='type-id-687'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
@@ -12894,162 +12894,162 @@
     </function-decl>
   </abi-instr>
   <abi-instr address-size='64' path='hb-ot-layout.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src' language='LANG_C_plus_plus'>
-    <!-- EntryExitRecord[1] -->
+    <!-- OT::EntryExitRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-851' id='type-id-852'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- Index[1] -->
+    <!-- OT::Index[1] -->
     <array-type-def dimensions='1' type-id='type-id-853' id='type-id-854'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- IntType<unsigned int, 3u>[1] -->
+    <!-- OT::IntType<unsigned int, 3u>[1] -->
     <array-type-def dimensions='1' type-id='type-id-735' id='type-id-855'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- LookupRecord[1] -->
+    <!-- OT::LookupRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-856' id='type-id-857'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- MarkRecord[1] -->
+    <!-- OT::MarkRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-858' id='type-id-859'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- Offset<OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::Offset<OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-860' id='type-id-861'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Anchor, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::Anchor, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-862' id='type-id-863'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::AnchorMatrix, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::AnchorMatrix, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-864' id='type-id-865'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::ArrayOf<OT::IntType<short unsigned int, 2u>, OT::IntType<short unsigned int, 2u> >, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::ArrayOf<OT::IntType<short unsigned int, 2u>, OT::IntType<short unsigned int, 2u> >, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-866' id='type-id-867'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::CaretValue, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::CaretValue, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-868' id='type-id-869'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::ChainRule, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::ChainRule, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-870' id='type-id-871'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::ChainRuleSet, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::ChainRuleSet, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-872' id='type-id-873'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Coverage, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::Coverage, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-874' id='type-id-875'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Coverage, OT::IntType<unsigned int, 4u> >[1] -->
+    <!-- OT::OffsetTo<OT::Coverage, OT::IntType<unsigned int, 4u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-876' id='type-id-877'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::LigGlyph, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::LigGlyph, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-878' id='type-id-879'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Ligature, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::Ligature, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-880' id='type-id-881'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::LigatureSet, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::LigatureSet, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-882' id='type-id-883'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Lookup, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::Lookup, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-884' id='type-id-885'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::MarkGlyphSets, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::MarkGlyphSets, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-886' id='type-id-887'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::PairSet, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::PairSet, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-888' id='type-id-889'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::PosLookup, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::PosLookup, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-890' id='type-id-891'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::PosLookupSubTable, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::PosLookupSubTable, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-892' id='type-id-893'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Rule, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::Rule, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-894' id='type-id-895'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::RuleSet, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::RuleSet, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-896' id='type-id-897'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::Sequence, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::Sequence, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-898' id='type-id-899'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::SubstLookup, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::SubstLookup, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-900' id='type-id-901'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- OffsetTo<OT::SubstLookupSubTable, OT::IntType<short unsigned int, 2u> >[1] -->
+    <!-- OT::OffsetTo<OT::SubstLookupSubTable, OT::IntType<short unsigned int, 2u> >[1] -->
     <array-type-def dimensions='1' type-id='type-id-902' id='type-id-903'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- RangeRecord[1] -->
+    <!-- OT::RangeRecord[1] -->
     <array-type-def dimensions='1' type-id='type-id-904' id='type-id-905'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- Record<OT::Feature>[1] -->
+    <!-- OT::Record<OT::Feature>[1] -->
     <array-type-def dimensions='1' type-id='type-id-906' id='type-id-907'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- Record<OT::LangSys>[1] -->
+    <!-- OT::Record<OT::LangSys>[1] -->
     <array-type-def dimensions='1' type-id='type-id-908' id='type-id-909'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- Record<OT::Script>[1] -->
+    <!-- OT::Record<OT::Script>[1] -->
     <array-type-def dimensions='1' type-id='type-id-910' id='type-id-911'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
     </array-type-def>
-    <!-- Value[1] -->
+    <!-- OT::Value[1] -->
     <array-type-def dimensions='1' type-id='type-id-912' id='type-id-913'>
       <!-- <anonymous range>[1] -->
       <subrange length='1' type-id='type-id-4' id='type-id-179'/>
@@ -13059,17 +13059,17 @@
       <!-- <anonymous range>[2] -->
       <subrange length='2' type-id='type-id-4' id='type-id-8'/>
     </array-type-def>
-    <!-- feature_map_t[8] -->
+    <!-- hb_ot_map_t::feature_map_t[8] -->
     <array-type-def dimensions='1' type-id='type-id-915' size-in-bits='2304' id='type-id-916'>
       <!-- <anonymous range>[8] -->
       <subrange length='8' type-id='type-id-4' id='type-id-63'/>
     </array-type-def>
-    <!-- lookup_map_t[32] -->
+    <!-- hb_ot_map_t::lookup_map_t[32] -->
     <array-type-def dimensions='1' type-id='type-id-917' size-in-bits='2048' id='type-id-918'>
       <!-- <anonymous range>[32] -->
       <subrange length='32' type-id='type-id-4' id='type-id-919'/>
     </array-type-def>
-    <!-- stage_map_t[4] -->
+    <!-- hb_ot_map_t::stage_map_t[4] -->
     <array-type-def dimensions='1' type-id='type-id-920' size-in-bits='512' id='type-id-921'>
       <!-- <anonymous range>[4] -->
       <subrange length='4' type-id='type-id-4' id='type-id-71'/>
@@ -29612,7 +29612,7 @@
       </class-decl>
       <!-- typedef OT::USHORT OT::Value -->
       <typedef-decl name='Value' type-id='type-id-371' filepath='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src/hb-ot-layout-gpos-table.hh' line='45' column='1' id='type-id-912'/>
-      <!-- typedef Value[1] OT::ValueRecord -->
+      <!-- typedef OT::Value[1] OT::ValueRecord -->
       <typedef-decl name='ValueRecord' type-id='type-id-913' filepath='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src/hb-ot-layout-gpos-table.hh' line='47' column='1' id='type-id-1849'/>
       <!-- typedef bool (hb_set_t*, const OT::USHORT&, void*)* OT::intersects_func_t -->
       <typedef-decl name='intersects_func_t' type-id='type-id-1399' filepath='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src/hb-ot-layout-gsubgpos-private.hh' line='626' column='1' id='type-id-1885'/>
@@ -30055,12 +30055,12 @@
     </function-type>
   </abi-instr>
   <abi-instr address-size='64' path='hb-ot-map.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src' language='LANG_C_plus_plus'>
-    <!-- feature_info_t[32] -->
+    <!-- hb_ot_map_builder_t::feature_info_t[32] -->
     <array-type-def dimensions='1' type-id='type-id-1898' size-in-bits='7168' id='type-id-1899'>
       <!-- <anonymous range>[32] -->
       <subrange length='32' type-id='type-id-4' id='type-id-919'/>
     </array-type-def>
-    <!-- stage_info_t[8] -->
+    <!-- hb_ot_map_builder_t::stage_info_t[8] -->
     <array-type-def dimensions='1' type-id='type-id-1900' size-in-bits='1024' id='type-id-1901'>
       <!-- <anonymous range>[8] -->
       <subrange length='8' type-id='type-id-4' id='type-id-63'/>
@@ -31395,7 +31395,7 @@
     </function-decl>
   </abi-instr>
   <abi-instr address-size='64' path='hb-set.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-04g73E/harfbuzz-0.9.37/src' language='LANG_C_plus_plus'>
-    <!-- elt_t[2048] -->
+    <!-- hb_set_t::elt_t[2048] -->
     <array-type-def dimensions='1' type-id='type-id-2025' size-in-bits='65536' id='type-id-2026'>
       <!-- <anonymous range>[2048] -->
       <subrange length='2048' type-id='type-id-4' id='type-id-2027'/>
diff --git a/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi b/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi
index 44e1db28..8cb8aeba 100644
--- a/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi
+++ b/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi
@@ -2841,7 +2841,7 @@
     </function-decl>
   </abi-instr>
   <abi-instr address-size='64' path='src/base/low_level_alloc.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-kFgaKP/gperftools-2.4' language='LANG_C_plus_plus'>
-    <!-- AllocList*[30] -->
+    <!-- low_level_alloc_internal::AllocList*[30] -->
     <array-type-def dimensions='1' type-id='type-id-92' size-in-bits='1920' id='type-id-93'>
       <!-- <anonymous range>[30] -->
       <subrange length='30' type-id='type-id-23' id='type-id-94'/>
@@ -22310,12 +22310,12 @@
   <abi-instr address-size='64' path='src/profiledata.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-kFgaKP/gperftools-2.4' language='LANG_C_plus_plus'>
   </abi-instr>
   <abi-instr address-size='64' path='src/profiler.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-kFgaKP/gperftools-2.4' language='LANG_C_plus_plus'>
-    <!-- Slot[64] -->
+    <!-- ProfileData::Entry::Slot[64] -->
     <array-type-def dimensions='1' type-id='type-id-1229' size-in-bits='4096' id='type-id-1230'>
       <!-- <anonymous range>[64] -->
       <subrange length='64' type-id='type-id-23' id='type-id-1231'/>
     </array-type-def>
-    <!-- Entry[4] -->
+    <!-- ProfileData::Entry[4] -->
     <array-type-def dimensions='1' type-id='type-id-1232' size-in-bits='16896' id='type-id-1233'>
       <!-- <anonymous range>[4] -->
       <subrange length='4' type-id='type-id-23' id='type-id-188'/>
@@ -25418,22 +25418,22 @@
       <!-- <anonymous range>[88] -->
       <subrange length='88' type-id='type-id-23' id='type-id-1359'/>
     </array-type-def>
-    <!-- TCEntry[64] -->
+    <!-- tcmalloc::CentralFreeList::TCEntry[64] -->
     <array-type-def dimensions='1' type-id='type-id-1361' size-in-bits='8192' id='type-id-1362'>
       <!-- <anonymous range>[64] -->
       <subrange length='64' type-id='type-id-23' id='type-id-1231'/>
     </array-type-def>
-    <!-- CentralFreeListPadded[88] -->
+    <!-- tcmalloc::CentralFreeListPadded[88] -->
     <array-type-def dimensions='1' type-id='type-id-1363' id='type-id-1364'>
       <!-- <anonymous range>[88] -->
       <subrange length='88' type-id='type-id-23' id='type-id-1359'/>
     </array-type-def>
-    <!-- SpanList[128] -->
+    <!-- tcmalloc::PageHeap::SpanList[128] -->
     <array-type-def dimensions='1' type-id='type-id-1365' size-in-bits='98304' id='type-id-1366'>
       <!-- <anonymous range>[128] -->
       <subrange length='128' type-id='type-id-23' id='type-id-1357'/>
     </array-type-def>
-    <!-- FreeList[88] -->
+    <!-- tcmalloc::ThreadCache::FreeList[88] -->
     <array-type-def dimensions='1' type-id='type-id-1367' size-in-bits='16896' id='type-id-1368'>
       <!-- <anonymous range>[88] -->
       <subrange length='88' type-id='type-id-23' id='type-id-1359'/>
diff --git a/tests/data/test-diff-filter/test-PR26739-2-report-0.txt b/tests/data/test-diff-filter/test-PR26739-2-report-0.txt
index a98473b4..999d9614 100644
--- a/tests/data/test-diff-filter/test-PR26739-2-report-0.txt
+++ b/tests/data/test-diff-filter/test-PR26739-2-report-0.txt
@@ -12,6 +12,6 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
             entity changed from 'const volatile const int[5]' to compatible type 'typedef array_type1' at test-PR26739-2-v1.c:3:1
               array element type 'const volatile const int' changed:
                 'const volatile const int' changed to 'const int'
-              type name changed from 'volatile const int[5]' to 'const int[5]'
+              type name changed from 'const volatile const int[5]' to 'const int[5]'
               type size hasn't changed
 
-- 
2.36.1

-- 
		Dodji

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3] dwarf-reader: Remove redundant qualifiers from qualified types
  2022-07-22 23:19 [PATCH 0/3] Make integral types of same base and size compatible Dodji Seketeli
  2022-07-22 23:28 ` [PATCH 1/3] ir: Disambiguate sorting of array element types Dodji Seketeli
@ 2022-07-22 23:31 ` Dodji Seketeli
  2022-07-22 23:32 ` [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent Dodji Seketeli
  2 siblings, 0 replies; 14+ messages in thread
From: Dodji Seketeli @ 2022-07-22 23:31 UTC (permalink / raw)
  To: Dodji Seketeli via Libabigail; +Cc: Dodji Seketeli

Hello,

While looking at something else, I noticed that there are some
qualified types built from the DWARF debug info that read:

        const volatile const int m[5]

That's a tree of (chained) qualified types that end up having some
redundant qualifiers.  That IR tree might look like:

        [C] --> [C|V] --> [int]

We want to edit that IR tree to make it look like:

        [C] --> [V]  --> [int]

And that would serialize as

        const volatile int m[5]

This patch introduces the editing of the qualified type IR tree to
remove the redundant qualifiers, right after the IR is built from
DWARF.

	* include/abg-fwd.h (strip_redundant_quals_from_underyling_types):
	Declare new function.
	* include/abg-ir.h (operator&=): Declare new binary operator for
	qualified_type_def::CV and ...
	* src/abg-ir.cc (+operator&=): ... define it here.
	(strip_redundant_quals_from_underyling_types): Define new function
	and ...
	* src/abg-dwarf-reader.cc (maybe_strip_qualification): ... Use it
	here.
	* tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt:
	Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to master.
---
 include/abg-fwd.h                             |   3 +
 include/abg-ir.h                              |   3 +
 src/abg-dwarf-reader.cc                       |   5 +-
 src/abg-ir.cc                                 | 115 ++++++++++++++++++
 .../qualifier-typedef-array-report-1.txt      |  40 +++---
 5 files changed, 145 insertions(+), 21 deletions(-)

diff --git a/include/abg-fwd.h b/include/abg-fwd.h
index d32e226a..e84e8a85 100644
--- a/include/abg-fwd.h
+++ b/include/abg-fwd.h
@@ -827,6 +827,9 @@ strip_typedef(const type_base_sptr);
 decl_base_sptr
 strip_useless_const_qualification(const qualified_type_def_sptr t);
 
+void
+strip_redundant_quals_from_underyling_types(const qualified_type_def_sptr&);
+
 type_base_sptr
 peel_typedef_type(const type_base_sptr&);
 
diff --git a/include/abg-ir.h b/include/abg-ir.h
index 86a6ce27..2d4b1006 100644
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -2243,6 +2243,9 @@ operator|(qualified_type_def::CV, qualified_type_def::CV);
 qualified_type_def::CV&
 operator|=(qualified_type_def::CV&, qualified_type_def::CV);
 
+qualified_type_def::CV&
+operator&=(qualified_type_def::CV&, qualified_type_def::CV);
+
 qualified_type_def::CV
 operator&(qualified_type_def::CV, qualified_type_def::CV);
 
diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 0c34f617..e5159c89 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -13544,6 +13544,7 @@ maybe_strip_qualification(const qualified_type_def_sptr t,
   decl_base_sptr result = t;
   type_base_sptr u = t->get_underlying_type();
 
+  strip_redundant_quals_from_underyling_types(t);
   result = strip_useless_const_qualification(t);
   if (result.get() != t.get())
     return result;
@@ -13590,6 +13591,7 @@ maybe_strip_qualification(const qualified_type_def_sptr t,
 	  qualified_type_def::CV quals = qualified->get_cv_quals();
 	  quals |= t->get_cv_quals();
 	  qualified->set_cv_quals(quals);
+	  strip_redundant_quals_from_underyling_types(qualified);
 	  result = is_decl(u);
 	}
       else
@@ -13598,6 +13600,7 @@ maybe_strip_qualification(const qualified_type_def_sptr t,
 	    (new qualified_type_def(element_type,
 				    t->get_cv_quals(),
 				    t->get_location()));
+	  strip_redundant_quals_from_underyling_types(qual_type);
 	  add_decl_to_scope(qual_type, is_decl(element_type)->get_scope());
 	  array->set_element_type(qual_type);
 	  ctxt.schedule_type_for_late_canonicalization(is_type(qual_type));
@@ -15584,7 +15587,7 @@ build_ir_node_from_die(read_context&	ctxt,
 	    ctxt.associate_die_to_type(die, ty, where_offset);
 	    result =
 	      add_decl_to_scope(d, ctxt.cur_transl_unit()->get_global_scope());
-	    maybe_canonicalize_type(die, ctxt);
+	    maybe_canonicalize_type(is_type(result), ctxt);
 	  }
       }
       break;
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 5c1ac35b..988b8005 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -6715,6 +6715,113 @@ strip_useless_const_qualification(const qualified_type_def_sptr t)
   return result;
 }
 
+/// Merge redundant qualifiers from a tree of qualified types.
+///
+/// Suppose a tree of qualified types leads to:
+///
+///     const virtual const restrict const int;
+///
+/// Suppose the IR tree of qualified types ressembles (with C meaning
+/// const, V meaning virtual and R meaning restrict):
+///
+///     [C|V]-->[C|R] -->[C] --> [int].
+///
+/// This function walks the IR and remove the redundant CV qualifiers
+/// so the IR becomes:
+///
+///     [C|V] --> [R] --> []  -->[int].
+///
+/// Note that the empty qualified type (noted []) represents a
+/// qualified type with no qualifier.  It's rare, but it can exist.
+/// I've put it here just for the sake of example.
+///
+/// The resulting IR thus represents the (merged) type:
+///
+///    const virtual restrict int.
+///
+/// This function is a sub-routine of the overload @ref
+/// strip_useless_const_qualification which doesn't return any value.
+///
+/// @param t the qualified type to consider.
+///
+/// @param redundant_quals the (redundant) qualifiers to be removed
+/// from the qualifiers of the underlying types of @p t.
+///
+/// @return the underlying type of @p t which might have had its
+/// redundant qualifiers removed.
+static qualified_type_def_sptr
+strip_redundant_quals_from_underyling_types(const qualified_type_def_sptr& t,
+					    qualified_type_def::CV redundant_quals)
+{
+  if (!t)
+    return t;
+
+  // We must NOT edit canonicalized types.
+  ABG_ASSERT(!t->get_canonical_type());
+
+  qualified_type_def_sptr underlying_qualified_type =
+    is_qualified_type(t->get_underlying_type());
+
+  // Let's build 'currated qualifiers' that are the qualifiers of the
+  // current type from which redundant qualifiers are removed.
+  qualified_type_def::CV currated_quals = t->get_cv_quals();
+
+  // Remove the redundant qualifiers from these currated qualifiers
+  currated_quals &= ~redundant_quals;
+  t->set_cv_quals(currated_quals);
+
+  // The redundant qualifiers, moving forward, is now the union of the
+  // previous set of redundant qualifiers and the currated qualifiers.
+  redundant_quals |= currated_quals;
+
+  qualified_type_def_sptr result = t;
+  if (underlying_qualified_type)
+    // Now remove the redundant qualifiers from the qualified types
+    // potentially carried by the underlying type.
+    result =
+      strip_redundant_quals_from_underyling_types(underlying_qualified_type,
+						  redundant_quals);
+
+  return result;
+}
+
+/// Merge redundant qualifiers from a tree of qualified types.
+///
+/// Suppose a tree of qualified types leads to:
+///
+///     const virtual const restrict const int;
+///
+/// Suppose the IR tree of qualified types ressembles (with C meaning
+/// const, V meaning virtual and R meaning restrict):
+///
+///     [C|V]-->[C|R] -->[C] --> [int].
+///
+/// This function walks the IR and remove the redundant CV qualifiers
+/// so the IR becomes:
+///
+///     [C|V] --> [R] --> []  -->[int].
+///
+/// Note that the empty qualified type (noted []) represents a
+/// qualified type with no qualifier.  It's rare, but it can exist.
+/// I've put it here just for the sake of example.
+///
+/// The resulting IR thus represents the (merged) type:
+///
+///    const virtual restrict int.
+///
+/// @param t the qualified type to consider.  The IR below the
+/// argument to this parameter will be edited to remove redundant
+/// qualifiers where applicable.
+void
+strip_redundant_quals_from_underyling_types(const qualified_type_def_sptr& t)
+{
+  if (!t)
+    return;
+
+  qualified_type_def::CV redundant_quals = qualified_type_def::CV_NONE;
+  strip_redundant_quals_from_underyling_types(t, redundant_quals);
+}
+
 /// Return the leaf underlying type node of a @ref typedef_decl node.
 ///
 /// If the underlying type of a @ref typedef_decl node is itself a
@@ -15653,6 +15760,14 @@ operator|=(qualified_type_def::CV& l, qualified_type_def::CV r)
   return l;
 }
 
+/// Overloaded bitwise &= operator for cv qualifiers.
+qualified_type_def::CV&
+operator&=(qualified_type_def::CV& l, qualified_type_def::CV r)
+{
+  l = l & r;
+  return l;
+}
+
 /// Overloaded bitwise AND operator for CV qualifiers.
 qualified_type_def::CV
 operator&(qualified_type_def::CV lhs, qualified_type_def::CV rhs)
diff --git a/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt b/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt
index 13949efc..d45c9c6c 100644
--- a/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt
+++ b/tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt
@@ -47,39 +47,39 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
             underlying type 'typedef A' at qualifier-typedef-array-v0.c:1:1 changed:
               entity changed from 'typedef A' to compatible type 'void*[7]'
           type of 'C v_c' changed:
-            entity changed from 'typedef C' to compatible type 'const volatile void* const[7]'
+            entity changed from 'typedef C' to compatible type 'const volatile void*[7]'
               array element type 'void* const' changed:
-                'void* const' changed to 'const volatile void* const'
-              type name changed from 'void* const[7]' to 'const volatile void* const[7]'
+                'void* const' changed to 'const volatile void*'
+              type name changed from 'void* const[7]' to 'const volatile void*[7]'
               type size hasn't changed
           type of 'C r_c' changed:
-            entity changed from 'typedef C' to compatible type 'restrict const void* const[7]'
+            entity changed from 'typedef C' to compatible type 'restrict const void*[7]'
               array element type 'void* const' changed:
-                'void* const' changed to 'restrict const void* const'
-              type name changed from 'void* const[7]' to 'restrict const void* const[7]'
+                'void* const' changed to 'restrict const void*'
+              type name changed from 'void* const[7]' to 'restrict const void*[7]'
               type size hasn't changed
           type of 'D v_d' changed:
-            entity changed from 'typedef D' to compatible type 'const volatile void* const[7]'
+            entity changed from 'typedef D' to compatible type 'const volatile void*[7]'
               array element type 'void* const' changed:
-                'void* const' changed to 'const volatile void* const'
-              type name changed from 'void* const[7]' to 'const volatile void* const[7]'
+                'void* const' changed to 'const volatile void*'
+              type name changed from 'void* const[7]' to 'const volatile void*[7]'
               type size hasn't changed
           type of 'D r_d' changed:
-            entity changed from 'typedef D' to compatible type 'restrict const void* const[7]'
+            entity changed from 'typedef D' to compatible type 'restrict const void*[7]'
               array element type 'void* const' changed:
-                'void* const' changed to 'restrict const void* const'
-              type name changed from 'void* const[7]' to 'restrict const void* const[7]'
+                'void* const' changed to 'restrict const void*'
+              type name changed from 'void* const[7]' to 'restrict const void*[7]'
               type size hasn't changed
           type of 'E r_e' changed:
-            entity changed from 'typedef E' to compatible type 'restrict const volatile volatile void* const[7]'
-              array element type 'const volatile void* const' changed:
-                'const volatile void* const' changed to 'restrict const volatile volatile void* const'
-              type name changed from 'const volatile void* const[7]' to 'restrict const volatile volatile void* const[7]'
+            entity changed from 'typedef E' to compatible type 'restrict const volatile void*[7]'
+              array element type 'const volatile void*' changed:
+                'const volatile void*' changed to 'restrict const volatile void*'
+              type name changed from 'const volatile void*[7]' to 'restrict const volatile void*[7]'
               type size hasn't changed
           type of 'F r_f' changed:
-            entity changed from 'typedef F' to compatible type 'restrict const volatile volatile void* const[7]'
-              array element type 'const volatile void* const' changed:
-                'const volatile void* const' changed to 'restrict const volatile volatile void* const'
-              type name changed from 'const volatile void* const[7]' to 'restrict const volatile volatile void* const[7]'
+            entity changed from 'typedef F' to compatible type 'restrict const volatile void*[7]'
+              array element type 'const volatile void*' changed:
+                'const volatile void*' changed to 'restrict const volatile void*'
+              type name changed from 'const volatile void*[7]' to 'restrict const volatile void*[7]'
               type size hasn't changed
 
-- 
2.36.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-07-22 23:19 [PATCH 0/3] Make integral types of same base and size compatible Dodji Seketeli
  2022-07-22 23:28 ` [PATCH 1/3] ir: Disambiguate sorting of array element types Dodji Seketeli
  2022-07-22 23:31 ` [PATCH 2/3] dwarf-reader: Remove redundant qualifiers from qualified types Dodji Seketeli
@ 2022-07-22 23:32 ` Dodji Seketeli
  2022-08-10 15:23   ` Giuliano Procida
  2 siblings, 1 reply; 14+ messages in thread
From: Dodji Seketeli @ 2022-07-22 23:32 UTC (permalink / raw)
  To: Dodji Seketeli via Libabigail; +Cc: Dodji Seketeli

[-- Attachment #1: Type: text/plain, Size: 8621 bytes --]

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,


[-- Attachment #2: 0003-ir-Consider-integral-types-of-same-kind-and-size-as-.patch.gz --]
[-- Type: application/gzip, Size: 2204114 bytes --]

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]



-- 
		Dodji

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-07-22 23:32 ` [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent Dodji Seketeli
@ 2022-08-10 15:23   ` Giuliano Procida
  2022-08-11  2:22     ` Ben Woodard
  2022-08-16 16:54     ` Dodji Seketeli
  0 siblings, 2 replies; 14+ messages in thread
From: Giuliano Procida @ 2022-08-10 15:23 UTC (permalink / raw)
  To: Dodji Seketeli
  Cc: Dodji Seketeli via Libabigail, Dodji Seketeli, Matthias Männich

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-10 15:23   ` Giuliano Procida
@ 2022-08-11  2:22     ` Ben Woodard
  2022-08-12 15:26       ` Giuliano Procida
  2022-08-16 16:54     ` Dodji Seketeli
  1 sibling, 1 reply; 14+ messages in thread
From: Ben Woodard @ 2022-08-11  2:22 UTC (permalink / raw)
  To: Giuliano Procida
  Cc: Dodji Seketeli, Dodji Seketeli, Dodji Seketeli via Libabigail,
	Matthias Männich

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
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-11  2:22     ` Ben Woodard
@ 2022-08-12 15:26       ` Giuliano Procida
  2022-08-16 19:56         ` Ben Woodard
  0 siblings, 1 reply; 14+ messages in thread
From: Giuliano Procida @ 2022-08-12 15:26 UTC (permalink / raw)
  To: Ben Woodard
  Cc: Dodji Seketeli, Dodji Seketeli, Dodji Seketeli via Libabigail,
	Matthias Männich

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-10 15:23   ` Giuliano Procida
  2022-08-11  2:22     ` Ben Woodard
@ 2022-08-16 16:54     ` Dodji Seketeli
  2022-08-16 17:06       ` Ben Woodard
  2022-08-16 18:10       ` Giuliano Procida
  1 sibling, 2 replies; 14+ messages in thread
From: Dodji Seketeli @ 2022-08-16 16:54 UTC (permalink / raw)
  To: Giuliano Procida
  Cc: Dodji Seketeli, Dodji Seketeli via Libabigail, Matthias Männich

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,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-16 16:54     ` Dodji Seketeli
@ 2022-08-16 17:06       ` Ben Woodard
  2022-08-16 18:10       ` Giuliano Procida
  1 sibling, 0 replies; 14+ messages in thread
From: Ben Woodard @ 2022-08-16 17:06 UTC (permalink / raw)
  To: Dodji Seketeli
  Cc: Giuliano Procida, Matthias Männich, Dodji Seketeli via Libabigail



> 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-16 16:54     ` Dodji Seketeli
  2022-08-16 17:06       ` Ben Woodard
@ 2022-08-16 18:10       ` Giuliano Procida
  2022-08-18 16:29         ` Dodji Seketeli
  1 sibling, 1 reply; 14+ messages in thread
From: Giuliano Procida @ 2022-08-16 18:10 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Dodji Seketeli via Libabigail, Matthias Männich

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-12 15:26       ` Giuliano Procida
@ 2022-08-16 19:56         ` Ben Woodard
  0 siblings, 0 replies; 14+ messages in thread
From: Ben Woodard @ 2022-08-16 19:56 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: Dodji Seketeli via Libabigail, Matthias Männich



> 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-16 18:10       ` Giuliano Procida
@ 2022-08-18 16:29         ` Dodji Seketeli
  2022-08-18 17:52           ` Ben Woodard
  0 siblings, 1 reply; 14+ messages in thread
From: Dodji Seketeli @ 2022-08-18 16:29 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: woodard, libabigail, Matthias Männich

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-18 16:29         ` Dodji Seketeli
@ 2022-08-18 17:52           ` Ben Woodard
  2022-08-19 15:30             ` Dodji Seketeli
  0 siblings, 1 reply; 14+ messages in thread
From: Ben Woodard @ 2022-08-18 17:52 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Giuliano Procida, libabigail, Matthias Männich

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
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent
  2022-08-18 17:52           ` Ben Woodard
@ 2022-08-19 15:30             ` Dodji Seketeli
  0 siblings, 0 replies; 14+ messages in thread
From: Dodji Seketeli @ 2022-08-19 15:30 UTC (permalink / raw)
  To: Ben Woodard; +Cc: Giuliano Procida, libabigail, Matthias Männich

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,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-08-19 15:31 UTC | newest]

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).