Increase stability of child diff order

  Bug 28939 - diff output is sensitive to implementation of std::sort

This change increases the stability of child diff order.

One test case is affected, potentially indicating the presence of a
bug (should the parameter 1 diff not be reported unconditionally?).

	* src/abg-comparison-priv.h
	(diff_less_than_functor::operator()): This now compares the
        second diff subject names if the first diff subject names are
        the same. It also handles all possble null pointer values,
        though in practice they should never occur.
	* src/abg-comparison.cc (diff::append_child_node): Replace
	std::sort with std::stable_sort.
	* tests/data/test-abidiff-exit/test-member-size-report0.txt:
	Refresh test.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-comparison-priv.h                     | 34 +++++++++++++++----
 src/abg-comparison.cc                         |  6 ++--
 .../test-member-size-report0.txt              | 15 ++++----
 3 files changed, 37 insertions(+), 18 deletions(-)
  

Hello Giuliano,

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

[...]

I am putting below a copy of the error reported on the bug, that is
allegedly triggered by a different implementation of std::sort:

    --- tests/data/test-abidiff-exit/test-member-size-report0.txt
    2022-01-30 18:02:19.412941913 -0800
    +++ tests/output/test-abidiff-exit/test-member-size-report0.txt
    2022-01-30 18:02:20.180941537 -0800
    @@ -4,16 +4,15 @@
     2 functions with some indirect sub-type change:

       [C] 'function void reg1(S*, T*, T*)' at test-member-size-v1.cc:26:1
       has some indirect sub-type changes:
       -    parameter 1 of type 'S*' has sub-type changes:
       -      in pointed to type 'struct S' at test-member-size-v1.cc:3:1:
       -        type size changed from 128 to 192 (in bits)
       -        1 data member insertion:
       -          'int y', at offset 128 (in bits) at
       test-member-size-v1.cc:6:1
       -        no data member change (1 filtered);
       -    parameter 2 of type 'T*' has sub-type changes:
       +    parameter 3 of type 'T*' has sub-type changes:
              in pointed to type 'struct T' at test-member-size-v1.cc:14:1:
                       type size changed from 192 to 256 (in bits)
                       -        1 data member changes (1 filtered):
                       +        2 data member changes:
                       +          type of 'S s' changed:
                       +            type size changed from 128 to 192 (in
       bits)
       +            1 data member insertion:
       +              'int y', at offset 128 (in bits) at
       test-member-size-v1.cc:6:1
       +            no data member change (1 filtered);
                  'int a' offset changed from 128 to 192 (in bits) (by +64 bits)

From what I am seeing here, it looks like this is because the order in
which the diff nodes representing the function parameter diffs are
present as children of the diff node for the function type diff.

Said otherwise, the diff node for the function type diff has children
nodes that are the diff nodes representing function parameter diffs.

You can see that in the function
function_type_diff::chain_into_hierarchy in abg-comparison.cc:

  for (vector<fn_parm_diff_sptr>::const_iterator i =
	 priv_->sorted_changed_parms_by_id_.begin();
       i != priv_->sorted_changed_parms_by_id_.end();
       ++i)
    if (diff_sptr d = *i)
      append_child_node(d);

So, the parameters diffs are sorted by parameter id; meaning, the
parameter diff for the first function parameter comes before the
parameter diff for the second function parameter, which comes before the
parameter diff for the third function parameter, etc.

But then in the problem report, it seems that the order of those
parameter diff nodes is changed somehow, because the diff node for the
third parameter comes first.

Let's look at the code of diff::append_child_node that is called in the
code snippet above, in the expression "append_child_node(d)".  It
contains this snippet:

> @@ -1999,9 +1999,9 @@ diff::append_child_node(diff_sptr d)
>    priv_->children_.push_back(d.get());
>  
>    diff_less_than_functor comp;
>    std::sort(priv_->children_.begin(),
>   	       priv_->children_.end(),
>  	       comp);

I think that std::sort should not be there, because the children nodes
are already sorted, using a sorting criterion that is specific for
function parameter diffs.  So that would be the bug.  I think that call
to std::sort should be purely removed.

I am thus proposing the patch below.  Would it address your issue?


------------------------------------>8<--------------------------------------------------------
From 03213b9c97bbfbce7b3c4df9e47a3f353db8403e Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Mon, 7 Mar 2022 17:55:05 +0100
Subject: [PATCH] comparison: Avoid sorting diff nodes with wrong criteria

This should address PR28939, reported at
https://sourceware.org/bugzilla/show_bug.cgi?id=28939

In function_type_diff::chain_into_hierarchy, the diff details
represented by the diff nodes of the function parameters are already
sorted in the vector priv->sorted_changed_parms_by_id_.  Note that the
sorting of function parameter diff nodes was done using a criterion
that takes into account the position of each function parameter.

Members of the vector priv->sorted_changed_parms_by_id_ are then added
to the diff graph using diff::append_child_node.

The problem is that diff::append_child_node sorts the children nodes
/again/, this time using a criterion that is different from the one
used in function_type_diff::chain_into_hierarchy.  This is wrong.

This patch prevents diff::append_child_node from sorting the children
node because they have been sorted already.

	* src/abg-comparison.cc (diff::append_child_node): Do not sort
	children nodes here because they must have been sorted already.
	* tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt: Adjust.
	* tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt: Likewise.
	* tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt: Likewise.
	* tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt: Likewise.
	* tests/data/test-diff-filter/test41-report-0.txt: Likewise.
	* tests/data/test-diff-pkg/libICE-1.0.6-1.el6.x86_64.rpm--libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt:
	Likewise.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-comparison.cc                         |  5 --
 .../test30-pr18904-rvalueref-report0.txt      |  6 +--
 .../test30-pr18904-rvalueref-report1.txt      |  6 +--
 .../test30-pr18904-rvalueref-report2.txt      |  6 +--
 .../test35-pr18754-no-added-syms-report-0.txt |  6 +--
 .../data/test-diff-filter/test41-report-0.txt | 49 ++++++++++---------
 ...libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt | 19 +++----
 7 files changed, 45 insertions(+), 52 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 5e61ba50..193af52f 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -2005,11 +2005,6 @@ diff::append_child_node(diff_sptr d)
   // above.
   priv_->children_.push_back(d.get());
 
-  diff_less_than_functor comp;
-  std::sort(priv_->children_.begin(),
-	    priv_->children_.end(),
-	    comp);
-
   d->priv_->parent_ = this;
 }
 
diff --git a/tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt b/tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt
index 20b3859f..77510262 100644
--- a/tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt
+++ b/tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt
@@ -1270,8 +1270,7 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                 'void* alloc', at offset 320 (in bits)
                 'OffloadDescriptor::OmpAsyncLastEventType omp_last_event_type', at offset 608 (in bits)
               4 data member changes (3 filtered):
-                'CeanReadRanges* read_rng_src' offset changed from 320 to 384 (in bits) (by +64 bits)
-                type of 'CeanReadRanges* read_rng_dst' changed:
+                type of 'CeanReadRanges* read_rng_src' changed:
                   in pointed to type 'struct CeanReadRanges':
                     type size changed from 512 to 576 (in bits)
                     1 data member insertion:
@@ -1284,7 +1283,8 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                       'int last_noncont_ind' offset changed from 256 to 320 (in bits) (by +64 bits)
                       'int64_t init_offset' offset changed from 320 to 384 (in bits) (by +64 bits)
                       'CeanReadDim Dim[1]' offset changed from 384 to 448 (in bits) (by +64 bits)
-                and offset changed from 384 to 448 (in bits) (by +64 bits)
+                and offset changed from 320 to 384 (in bits) (by +64 bits)
+                'CeanReadRanges* read_rng_dst' offset changed from 384 to 448 (in bits) (by +64 bits)
                 'int64_t ptr_arr_offset' offset changed from 448 to 512 (in bits) (by +64 bits)
                 'bool is_arr_ptr_el' offset changed from 512 to 576 (in bits) (by +64 bits)
           'OffloadHostTimerData* m_timer_data' offset changed from 1984 to 2048 (in bits) (by +64 bits)
diff --git a/tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt b/tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt
index 65fa3f1a..5cf624c1 100644
--- a/tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt
+++ b/tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt
@@ -1270,8 +1270,7 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                 'void* alloc', at offset 320 (in bits) at offload_host.h:222:1
                 'OffloadDescriptor::OmpAsyncLastEventType omp_last_event_type', at offset 608 (in bits) at offload_host.h:227:1
               4 data member changes (3 filtered):
-                'CeanReadRanges* read_rng_src' offset changed from 320 to 384 (in bits) (by +64 bits)
-                type of 'CeanReadRanges* read_rng_dst' changed:
+                type of 'CeanReadRanges* read_rng_src' changed:
                   in pointed to type 'struct CeanReadRanges' at cean_util.h:58:1:
                     type size changed from 512 to 576 (in bits)
                     1 data member insertion:
@@ -1284,7 +1283,8 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                       'int last_noncont_ind' offset changed from 256 to 320 (in bits) (by +64 bits)
                       'int64_t init_offset' offset changed from 320 to 384 (in bits) (by +64 bits)
                       'CeanReadDim Dim[1]' offset changed from 384 to 448 (in bits) (by +64 bits)
-                and offset changed from 384 to 448 (in bits) (by +64 bits)
+                and offset changed from 320 to 384 (in bits) (by +64 bits)
+                'CeanReadRanges* read_rng_dst' offset changed from 384 to 448 (in bits) (by +64 bits)
                 'int64_t ptr_arr_offset' offset changed from 448 to 512 (in bits) (by +64 bits)
                 'bool is_arr_ptr_el' offset changed from 512 to 576 (in bits) (by +64 bits)
           'OffloadHostTimerData* m_timer_data' offset changed from 1984 to 2048 (in bits) (by +64 bits)
diff --git a/tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt b/tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt
index 0248394f..68014539 100644
--- a/tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt
+++ b/tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt
@@ -1270,8 +1270,7 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                 'void* alloc', at offset 0x28 (in bytes) at offload_host.h:222:1
                 'OffloadDescriptor::OmpAsyncLastEventType omp_last_event_type', at offset 0x4c (in bytes) at offload_host.h:227:1
               4 data member changes (3 filtered):
-                'CeanReadRanges* read_rng_src' offset changed from 0x28 to 0x30 (in bytes) (by +0x8 bytes)
-                type of 'CeanReadRanges* read_rng_dst' changed:
+                type of 'CeanReadRanges* read_rng_src' changed:
                   in pointed to type 'struct CeanReadRanges' at cean_util.h:58:1:
                     type size changed from 0x40 to 0x48 (in bytes)
                     1 data member insertion:
@@ -1284,7 +1283,8 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                       'int last_noncont_ind' offset changed from 0x20 to 0x28 (in bytes) (by +0x8 bytes)
                       'int64_t init_offset' offset changed from 0x28 to 0x30 (in bytes) (by +0x8 bytes)
                       'CeanReadDim Dim[1]' offset changed from 0x30 to 0x38 (in bytes) (by +0x8 bytes)
-                and offset changed from 0x30 to 0x38 (in bytes) (by +0x8 bytes)
+                and offset changed from 0x28 to 0x30 (in bytes) (by +0x8 bytes)
+                'CeanReadRanges* read_rng_dst' offset changed from 0x30 to 0x38 (in bytes) (by +0x8 bytes)
                 'int64_t ptr_arr_offset' offset changed from 0x38 to 0x40 (in bytes) (by +0x8 bytes)
                 'bool is_arr_ptr_el' offset changed from 0x40 to 0x48 (in bytes) (by +0x8 bytes)
           'OffloadHostTimerData* m_timer_data' offset changed from 0xf8 to 0x100 (in bytes) (by +0x8 bytes)
diff --git a/tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt b/tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt
index 75b7116d..e14819a4 100644
--- a/tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt
+++ b/tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt
@@ -186,8 +186,7 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                 'void* alloc', at offset 320 (in bits)
                 'OffloadDescriptor::OmpAsyncLastEventType omp_last_event_type', at offset 608 (in bits)
               4 data member changes (3 filtered):
-                'CeanReadRanges* read_rng_src' offset changed from 320 to 384 (in bits) (by +64 bits)
-                type of 'CeanReadRanges* read_rng_dst' changed:
+                type of 'CeanReadRanges* read_rng_src' changed:
                   in pointed to type 'struct CeanReadRanges':
                     type size changed from 512 to 576 (in bits)
                     1 data member insertion:
@@ -200,7 +199,8 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
                       'int last_noncont_ind' offset changed from 256 to 320 (in bits) (by +64 bits)
                       'int64_t init_offset' offset changed from 320 to 384 (in bits) (by +64 bits)
                       'CeanReadDim Dim[1]' offset changed from 384 to 448 (in bits) (by +64 bits)
-                and offset changed from 384 to 448 (in bits) (by +64 bits)
+                and offset changed from 320 to 384 (in bits) (by +64 bits)
+                'CeanReadRanges* read_rng_dst' offset changed from 384 to 448 (in bits) (by +64 bits)
                 'int64_t ptr_arr_offset' offset changed from 448 to 512 (in bits) (by +64 bits)
                 'bool is_arr_ptr_el' offset changed from 512 to 576 (in bits) (by +64 bits)
           'OffloadHostTimerData* m_timer_data' offset changed from 1984 to 2048 (in bits) (by +64 bits)
diff --git a/tests/data/test-diff-filter/test41-report-0.txt b/tests/data/test-diff-filter/test41-report-0.txt
index e6caf368..2da31ef6 100644
--- a/tests/data/test-diff-filter/test41-report-0.txt
+++ b/tests/data/test-diff-filter/test41-report-0.txt
@@ -42,7 +42,7 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
         in unqualified underlying type 'class abigail::xml_writer::write_context' at abg-writer.cc:155:1:
           type size hasn't changed
           4 data member changes (3 filtered):
-            type of 'abigail::xml_writer::type_ptr_map m_emitted_decl_only_map' changed:
+            type of 'abigail::xml_writer::type_ptr_map m_type_id_map' changed:
               underlying type 'class std::tr1::unordered_map<abigail::ir::type_base*, abigail::interned_string, abigail::xml_writer::type_hasher, abigail::diff_utils::deep_ptr_eq_functor, std::allocator<std::pair<abigail::ir::type_base* const, abigail::interned_string> > >' at unordered_map.h:180:1 changed:
                 type name changed from 'std::tr1::unordered_map<abigail::ir::type_base*, abigail::interned_string, abigail::xml_writer::type_hasher, abigail::diff_utils::deep_ptr_eq_functor, std::allocator<std::pair<abigail::ir::type_base* const, abigail::interned_string> > >' to 'std::tr1::unordered_map<abigail::ir::type_base *, abigail::interned_string, abigail::xml_writer::type_hasher, abigail::diff_utils::deep_ptr_eq_functor, std::allocator<std::pair<abigail::ir::type_base *const, abigail::interned_string> > >'
                 type size hasn't changed
@@ -113,23 +113,25 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
             1 base class insertion:
               class std::allocator<std::__cxx11::basic_string<char> > at allocator.h:108:1
             3 data member changes (1 filtered):
-              name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Deque_impl::_M_map' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Deque_impl::_M_map' at stl_deque.h:550:1
-              name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Deque_impl::_M_start' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Deque_impl::_M_start' at stl_deque.h:552:1
-              type of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::iterator _M_finish' changed:
+              type of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Map_pointer _M_map' changed:
+                typedef name changed from std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Map_pointer to std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Map_pointer at stl_deque.h:542:1
+                underlying type 'typedef std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::_Map_pointer' at stl_deque.h:123:1 changed:
+                  typedef name changed from std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::_Map_pointer to std::_Deque_iterator<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> &, std::__cxx11::basic_string<char> *>::_Map_pointer at stl_deque.h:112:1
+                  underlying type 'typedef std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::__ptr_to' at stl_deque.h:116:1 changed:
+                    entity changed from 'typedef std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::__ptr_to' to compatible type 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >**'
+                      in pointed to type 'class std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >':
+                        entity changed from 'class std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >' to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*'
+                        type size changed from 256 to 64 (in bits)
+              and name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Deque_impl::_M_map' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Deque_impl::_M_map' at stl_deque.h:550:1
+              type of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::iterator _M_start' changed:
                 typedef name changed from std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::iterator to std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::iterator at stl_deque.h:485:1
                 underlying type 'struct std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>' at stl_deque.h:106:1 changed:
                   type name changed from 'std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>' to 'std::_Deque_iterator<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> &, std::__cxx11::basic_string<char> *>'
                   type size hasn't changed
                   1 data member changes (3 filtered):
-                    type of 'std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::_Map_pointer _M_node' changed:
-                      typedef name changed from std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::_Map_pointer to std::_Deque_iterator<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> &, std::__cxx11::basic_string<char> *>::_Map_pointer at stl_deque.h:112:1
-                      underlying type 'typedef std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::__ptr_to' at stl_deque.h:116:1 changed:
-                        entity changed from 'typedef std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::__ptr_to' to compatible type 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >**'
-                          in pointed to type 'class std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >':
-                            entity changed from 'class std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >' to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*'
-                            type size changed from 256 to 64 (in bits)
-                    and name of 'std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::_M_node' changed to 'std::_Deque_iterator<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> &, std::__cxx11::basic_string<char> *>::_M_node' at stl_deque.h:140:1
-              and name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Deque_impl::_M_finish' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Deque_impl::_M_finish' at stl_deque.h:553:1
+                    name of 'std::_Deque_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*>::_M_node' changed to 'std::_Deque_iterator<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> &, std::__cxx11::basic_string<char> *>::_M_node' at stl_deque.h:140:1
+              and name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Deque_impl::_M_start' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Deque_impl::_M_start' at stl_deque.h:552:1
+              name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_Deque_impl::_M_finish' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_Deque_impl::_M_finish' at stl_deque.h:553:1
           and name of 'std::_Deque_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_M_impl' changed to 'std::_Deque_base<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >::_M_impl' at stl_deque.h:631:1
 
   [C] 'method void std::_Deque_base<unsigned int, std::allocator<unsigned int> >::_M_initialize_map(std::size_t)' at stl_deque.h:625:1 has some indirect sub-type changes:
@@ -139,20 +141,21 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
         1 data member change:
           type of 'std::_Deque_base<unsigned int, std::allocator<unsigned int> >::_Deque_impl _M_impl' changed:
             type size hasn't changed
-            1 data member changes (2 filtered):
-              type of 'std::_Deque_base<unsigned int, std::allocator<unsigned int> >::iterator _M_finish' changed:
+            2 data member changes (1 filtered):
+              type of 'std::_Deque_base<unsigned int, std::allocator<unsigned int> >::_Map_pointer _M_map' changed:
+                underlying type 'typedef std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::_Map_pointer' at stl_deque.h:123:1 changed:
+                  typedef name changed from std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::_Map_pointer to std::_Deque_iterator<unsigned int, unsigned int &, unsigned int *>::_Map_pointer at stl_deque.h:112:1
+                  underlying type 'typedef std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::__ptr_to' at stl_deque.h:116:1 changed:
+                    entity changed from 'typedef std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::__ptr_to' to compatible type 'unsigned int**'
+                      in pointed to type 'unsigned int':
+                        entity changed from 'unsigned int' to 'unsigned int*'
+                        type size changed from 32 to 64 (in bits)
+              type of 'std::_Deque_base<unsigned int, std::allocator<unsigned int> >::iterator _M_start' changed:
                 underlying type 'struct std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>' at stl_deque.h:106:1 changed:
                   type name changed from 'std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>' to 'std::_Deque_iterator<unsigned int, unsigned int &, unsigned int *>'
                   type size hasn't changed
                   1 data member changes (3 filtered):
-                    type of 'std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::_Map_pointer _M_node' changed:
-                      typedef name changed from std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::_Map_pointer to std::_Deque_iterator<unsigned int, unsigned int &, unsigned int *>::_Map_pointer at stl_deque.h:112:1
-                      underlying type 'typedef std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::__ptr_to' at stl_deque.h:116:1 changed:
-                        entity changed from 'typedef std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::__ptr_to' to compatible type 'unsigned int**'
-                          in pointed to type 'unsigned int':
-                            entity changed from 'unsigned int' to 'unsigned int*'
-                            type size changed from 32 to 64 (in bits)
-                    and name of 'std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::_M_node' changed to 'std::_Deque_iterator<unsigned int, unsigned int &, unsigned int *>::_M_node' at stl_deque.h:140:1
+                    name of 'std::_Deque_iterator<unsigned int, unsigned int&, unsigned int*>::_M_node' changed to 'std::_Deque_iterator<unsigned int, unsigned int &, unsigned int *>::_M_node' at stl_deque.h:140:1
 
 1 Removed function symbol not referenced by debug info:
 
diff --git a/tests/data/test-diff-pkg/libICE-1.0.6-1.el6.x86_64.rpm--libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt b/tests/data/test-diff-pkg/libICE-1.0.6-1.el6.x86_64.rpm--libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt
index 3c05d925..a837ba48 100644
--- a/tests/data/test-diff-pkg/libICE-1.0.6-1.el6.x86_64.rpm--libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt
+++ b/tests/data/test-diff-pkg/libICE-1.0.6-1.el6.x86_64.rpm--libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt
@@ -16,18 +16,13 @@
           in pointed to type 'struct _IceConn' at ICEconn.h:131:1:
             type size hasn't changed
             2 data member changes (3 filtered):
-              type of 'IceListenObj listen_obj' changed:
-                underlying type '_IceListenObj*' changed:
-                  in pointed to type 'struct _IceListenObj' at ICElibint.h:120:1:
-                    type size hasn't changed
-                    1 data member change:
-                      type of '_XtransConnInfo* trans_conn' changed:
-                        in pointed to type 'struct _XtransConnInfo' at Xtransint.h:136:1:
-                          type size changed from 640 to 768 (in bits)
-                          2 data member insertions:
-                            '_XtransConnFd* recv_fds', at offset 640 (in bits) at Xtransint.h:148:1
-                            '_XtransConnFd* send_fds', at offset 704 (in bits) at Xtransint.h:149:1
-                          no data member change (1 filtered);
+              type of '_XtransConnInfo* trans_conn' changed:
+                in pointed to type 'struct _XtransConnInfo' at Xtransint.h:136:1:
+                  type size changed from 640 to 768 (in bits)
+                  2 data member insertions:
+                    '_XtransConnFd* recv_fds', at offset 640 (in bits) at Xtransint.h:148:1
+                    '_XtransConnFd* send_fds', at offset 704 (in bits) at Xtransint.h:149:1
+                  no data member change (1 filtered);
               type of '_IcePingWait* ping_waits' changed:
                 in pointed to type 'struct _IcePingWait' at ICEconn.h:48:1:
                   entity changed from 'struct _IcePingWait' to compatible type 'typedef _IcePingWait' at ICEconn.h:48:1
  

Hi.

On Tue, 8 Mar 2022 at 17:42, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> [...]
>
> I am putting below a copy of the error reported on the bug, that is
> allegedly triggered by a different implementation of std::sort:
>
>     --- tests/data/test-abidiff-exit/test-member-size-report0.txt
>     2022-01-30 18:02:19.412941913 -0800
>     +++ tests/output/test-abidiff-exit/test-member-size-report0.txt
>     2022-01-30 18:02:20.180941537 -0800
>     @@ -4,16 +4,15 @@
>      2 functions with some indirect sub-type change:
>
>        [C] 'function void reg1(S*, T*, T*)' at test-member-size-v1.cc:26:1
>        has some indirect sub-type changes:
>        -    parameter 1 of type 'S*' has sub-type changes:
>        -      in pointed to type 'struct S' at test-member-size-v1.cc:3:1:
>        -        type size changed from 128 to 192 (in bits)
>        -        1 data member insertion:
>        -          'int y', at offset 128 (in bits) at
>        test-member-size-v1.cc:6:1
>        -        no data member change (1 filtered);
>        -    parameter 2 of type 'T*' has sub-type changes:
>        +    parameter 3 of type 'T*' has sub-type changes:
>               in pointed to type 'struct T' at test-member-size-v1.cc:14:1:
>                        type size changed from 192 to 256 (in bits)
>                        -        1 data member changes (1 filtered):
>                        +        2 data member changes:
>                        +          type of 'S s' changed:
>                        +            type size changed from 128 to 192 (in
>        bits)
>        +            1 data member insertion:
>        +              'int y', at offset 128 (in bits) at
>        test-member-size-v1.cc:6:1
>        +            no data member change (1 filtered);
>                   'int a' offset changed from 128 to 192 (in bits) (by +64 bits)
>
> From what I am seeing here, it looks like this is because the order in
> which the diff nodes representing the function parameter diffs are
> present as children of the diff node for the function type diff.
>
> Said otherwise, the diff node for the function type diff has children
> nodes that are the diff nodes representing function parameter diffs.
>
> You can see that in the function
> function_type_diff::chain_into_hierarchy in abg-comparison.cc:
>
>   for (vector<fn_parm_diff_sptr>::const_iterator i =
>          priv_->sorted_changed_parms_by_id_.begin();
>        i != priv_->sorted_changed_parms_by_id_.end();
>        ++i)
>     if (diff_sptr d = *i)
>       append_child_node(d);
>
> So, the parameters diffs are sorted by parameter id; meaning, the
> parameter diff for the first function parameter comes before the
> parameter diff for the second function parameter, which comes before the
> parameter diff for the third function parameter, etc.
>
> But then in the problem report, it seems that the order of those
> parameter diff nodes is changed somehow, because the diff node for the
> third parameter comes first.
>
> Let's look at the code of diff::append_child_node that is called in the
> code snippet above, in the expression "append_child_node(d)".  It
> contains this snippet:
>
> > @@ -1999,9 +1999,9 @@ diff::append_child_node(diff_sptr d)
> >    priv_->children_.push_back(d.get());
> >
> >    diff_less_than_functor comp;
> >    std::sort(priv_->children_.begin(),
> >              priv_->children_.end(),
> >              comp);
>
> I think that std::sort should not be there, because the children nodes
> are already sorted, using a sorting criterion that is specific for
> function parameter diffs.  So that would be the bug.  I think that call
> to std::sort should be purely removed.
>
> I am thus proposing the patch below.  Would it address your issue?
>

[snip]

Absolutely! There is no chance of sort causing stability issues if
it's not used at all. More importantly, it's a real bug fix.

Thank you,
Giuliano.