Re: [PATCH] Increase stability of child diff order

[Giuliano Procida <gprocida@google.com>]

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.