Re: [PATCH] Bug 26309 - Wrong leaf reporting of changes to typedef underlying type

[Giuliano Procida <gprocida@google.com>]

Hi there.

On Mon, 14 Sep 2020 at 11:45, Dodji Seketeli <dodji@redhat.com> wrote:

> Hello,
>
> In leaf mode, libabigail fails to report changes to the underlying
> type of a typedef.
>
> At its core, this is due to the fact that changes to the underlying
> type of a typedef are not considered local.  As the leaf reporter only
> reports local changes (as opposed to non-local changes which are
> changes to sub-types) it doesn't detect those non-local typedef
> changes.
>
> To handle this, this patch makes changes to the underlying type of a
> typedef be considered as local changes.  This is like what we already
> do for pointer and qualified types.
>
> Now that we have another set of changes to report in the leaf
> reporter, we need to handle how to propagate the category and
> redundancy status of those changes.  The patch does this too.
>
> Also, just like we do pointer and qualified type changes, the patch
> avoids marking the diff node carrying the typedef change as being a
> leaf change.  That way, only existing leaf changes carrying that
> typedef diff node will be reported.  For instance, a function whose
> parameter has a typedef change will be reported because that change to
> the function is considered a leaf change.  Otherwise, reporting the
> typedef (or the pointer or qualified) type change on its own is not
> useful unless it impacts those leaf changes that we deem useful.
>
> The patch adds the example given in problem report to the testsuite.
>
>         * src/abg-ir.cc (equals): In the overload for typedef_decls,
>         report changes to the underlying type as being local of kind
>         LOCAL_TYPE_CHANGE_KIND.
>         * src/abg-comparison.cc
>         (leaf_diff_node_marker_visitor::visit_begin): Do not mark typedef
>         diff node as leaf node.
>         (suppression_categorization_visitor::visit_end): Propagate the
>         'suppressed' category of the underlying type to the parent typedef
>         unless the later has a local non-type change.
>         (redundancy_marking_visitor::visit_end): Likewise for the
>         'redundant' category.
>         * include/abg-reporter.h (report_non_type_typedef_changes): Rename
> ...
>         * src/abg-default-reporter.cc (report_non_type_typedef_changes):
>         ... report_local_typedef_changes into this.
>         * src/abg-leaf-reporter.cc (leaf_reporter::report): Make the leaf
>         reporter invoke the reporting method of the default reporter for
>         typedefs as all typedef changes are now local.
>         * tests/data/test-diff-filter/test-PR26309-report-0.txt: Add new
>         test reference output.
>         * tests/data/test-diff-filter/test-PR26309-v{0,1}.o: Add new test
>         binary input.
>         * tests/data/test-diff-filter/test-PR26309-v{0,1}.c: Add source
>         code for new test binary input.
>         * tests/data/Makefile.am: Add the new text material above to
>         source distribution.
>         * tests/test-diff-filter.cc (in_out_specs): Add the new test input
>         above to this test harness.
>         *
> tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt:
> Adjust.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
>  include/abg-reporter.h                             |   6 +-
>  src/abg-comparison.cc                              |  67 ++++++++++------
>  src/abg-default-reporter.cc                        |  14 ++--
>  src/abg-ir.cc                                      |   4 +-
>  src/abg-leaf-reporter.cc                           |   4 +-
>  tests/data/Makefile.am                             |   5 ++
>  .../test-diff-filter/test-PR26309-report-0.txt     |  13 ++++
>  tests/data/test-diff-filter/test-PR26309-v0.c      |   5 ++
>  tests/data/test-diff-filter/test-PR26309-v0.o      | Bin 0 -> 2720 bytes
>  tests/data/test-diff-filter/test-PR26309-v1.c      |   5 ++
>  tests/data/test-diff-filter/test-PR26309-v1.o      | Bin 0 -> 2752 bytes
>  ...-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt |  86
> +--------------------
>  tests/test-diff-filter.cc                          |   7 ++
>  13 files changed, 100 insertions(+), 116 deletions(-)
>  create mode 100644 tests/data/test-diff-filter/test-PR26309-report-0.txt
>  create mode 100644 tests/data/test-diff-filter/test-PR26309-v0.c
>  create mode 100644 tests/data/test-diff-filter/test-PR26309-v0.o
>  create mode 100644 tests/data/test-diff-filter/test-PR26309-v1.c
>  create mode 100644 tests/data/test-diff-filter/test-PR26309-v1.o
>
> diff --git a/include/abg-reporter.h b/include/abg-reporter.h
> index bf113f0..6aec6a6 100644
> --- a/include/abg-reporter.h
> +++ b/include/abg-reporter.h
> @@ -175,9 +175,9 @@ public:
>          const std::string& indent = "") const;
>
>    void
> -  report_local_typedef_changes(const typedef_diff &d,
> -                              std::ostream& out,
> -                              const std::string& indent) const;
> +  report_non_type_typedef_changes(const typedef_diff &d,
> +                                 std::ostream& out,
> +                                 const std::string& indent) const;
>
>    virtual void
>    report(const typedef_diff& d, std::ostream& out,
> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> index 7f91753..11f506f 100644
> --- a/src/abg-comparison.cc
> +++ b/src/abg-comparison.cc
> @@ -10901,14 +10901,15 @@ struct leaf_diff_node_marker_visitor : public
> diff_node_visitor
>         // Similarly, a *local* change describing a type that changed
>         // its nature doesn't make sense.
>         && !is_distinct_diff(d)
> -       // Similarly, a pointer (or reference or array) or qualified
> -       // type change in itself doesn't make sense.  It's would
> -       // rather make sense to show that pointer change as part of
> -       // the variable change whose pointer type changed, for
> +       // Similarly, a pointer (or reference or array), a typedef or
> +       // qualified type change in itself doesn't make sense.  It
> +       // would rather make sense to show that pointer change as part
> +       // of the variable change whose pointer type changed, for
>         // instance.
>         && !is_pointer_diff(d)
>         && !is_reference_diff(d)
>         && !is_qualified_type_diff(d)
> +       && !is_typedef_diff(d)
>         && !is_array_diff(d)
>         // Similarly a parameter change in itself doesn't make sense.
>         // It should have already been reported as part of the change
> @@ -11670,16 +11671,16 @@ struct suppression_categorization_visitor :
> public diff_node_visitor
>         //  2/ and has no local change (unless it's a pointer,
>         //  reference or qualified diff node).
>         //
> -       //  Note that qualified type diff nodes are a bit special.
> -       //  The local changes of the underlying type are considered
> -       //  local for the qualified type, just like for
> -       //  pointer/reference types.  But then the qualified type
> -       //  itself can have local changes of its own, and those
> -       //  changes are of the kind LOCAL_NON_TYPE_CHANGE_KIND.  So a
> -       //  qualified type which have local changes that are *NOT* of
> -       //  LOCAL_NON_TYPE_CHANGE_KIND (or that has no local changes
> -       //  at all) and which is in the PRIVATE_TYPE_CATEGORY or
> -       //  SUPPRESSED_CATEGORY can see these categories be
> +       //  Note that qualified type and typedef diff nodes are a bit
> +       //  special.  The local changes of the underlying type are
> +       //  considered local for the qualified/typedef type, just like
> +       //  for pointer/reference types.  But then the qualified or
> +       //  typedef type itself can have local changes of its own, and
> +       //  those changes are of the kind LOCAL_NON_TYPE_CHANGE_KIND.
> +       //  So a qualified type which have local changes that are
> +       //  *NOT* of LOCAL_NON_TYPE_CHANGE_KIND (or that has no local
> +       //  changes at all) and which is in the PRIVATE_TYPE_CATEGORY
> +       //  or SUPPRESSED_CATEGORY can see these categories be
>         //  propagated.
>         //
>         // Note that all pointer/reference diff node changes are
> @@ -11687,7 +11688,7 @@ struct suppression_categorization_visitor : public
> diff_node_visitor
>         // pointed-to-type are considered local to the pointer itself.
>         //
>         // Similarly, changes local to the type of function parameters,
> -       // variables (and data members) and classe (that are not of
> +       // variables (and data members) and classes (that are not of
>         // LOCAL_NON_TYPE_CHANGE_KIND kind) and that have been
>         // suppressed can propagate their SUPPRESSED_CATEGORY-ness to
>         // those kinds of diff node.
> @@ -11697,6 +11698,8 @@ struct suppression_categorization_visitor : public
> diff_node_visitor
>             || is_reference_diff(d)
>             || (is_qualified_type_diff(d)
>                 && (!(d->has_local_changes() &
> LOCAL_NON_TYPE_CHANGE_KIND)))
> +           || (is_typedef_diff(d)
> +               && (!(d->has_local_changes() &
> LOCAL_NON_TYPE_CHANGE_KIND)))
>             || (is_function_decl_diff(d)
>                 && (!(d->has_local_changes() &
> LOCAL_NON_TYPE_CHANGE_KIND)))
>             || (is_fn_parm_diff(d)
> @@ -11758,12 +11761,19 @@ struct suppression_categorization_visitor :
> public diff_node_visitor
>               canonical_diff->add_to_category(SUPPRESSED_CATEGORY);
>           }
>
> -       if (// We don't propagate "private type"-ness to typedefs
> -           // because defining "public" typedefs of private (opaque)
> -           // types is a common idiom.  So the typedef must stay
> -           // public.
> -           !is_typedef_diff(d)
> -           && has_non_empty_child
> +       // Note that the private-ness of a an underlying type won't be
> +       // propagated to its parent typedef, by virtue of the big "if"
> +       // clause at the beginning of this function.  So we don't have
> +       // to handle that case here.  So the idiom of defining
> +       // typedefs of private (opaque) types will be respected;
> +       // meaning that changes to opaque underlying type will be
> +       // flagged as private and the typedef will be flagged private
> +       // as well, unless the typedef itself has local non-type
> +       // changes.  In the later case, changes to the typedef will be
> +       // emitted because the typedef won't inherit the privateness
> +       // of its underlying type.  So in practise, the typedef
> +       // remains public for the purpose of change reporting.
> +       if (has_non_empty_child
>             && has_private_child
>             && !has_non_private_child)
>           {
> @@ -12223,8 +12233,21 @@ struct redundancy_marking_visitor : public
> diff_node_visitor
>                 // in the same sense as other types.  So we always
>                 // propagate redundancy to them, regardless of if they
>                 // have local changes or not.
> +               //
> +               // We also propagate redundancy to typedef types if
> +               // these /only/ carry changes to their underlying
> +               // type.
> +               //
> +               // Note that changes to the underlying type of a
> +               // typedef is considered local of
> +               // LOCAL_TYPE_CHANGE_KIND kind.  The other changes to the
> +               // typedef itself are considered local of
> +               // LOCAL_NON_TYPE_CHANGE_KIND kind.
>                 || is_pointer_diff(d)
> -               || is_qualified_type_diff(d)))
> +               || is_qualified_type_diff(d)
> +               || (is_typedef_diff(d)
> +                   && (!(d->has_local_changes()
> +                         & LOCAL_NON_TYPE_CHANGE_KIND)))))
>           {
>             bool has_non_redundant_child = false;
>             bool has_non_empty_child = false;
> diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
> index f8572f2..0b572db 100644
> --- a/src/abg-default-reporter.cc
> +++ b/src/abg-default-reporter.cc
> @@ -231,7 +231,11 @@ default_reporter::report(const enum_diff& d, ostream&
> out,
>    d.reported_once(true);
>  }
>
> -/// For a @ref typedef_diff node, report the changes that are local.
> +/// For a @ref typedef_diff node, report the local changes to the
> +/// typedef rather the changes to its underlying type.
> +///
> +/// Note that changes to the underlying type are also considered
> +/// local.
>  ///
>  /// @param d the @ref typedef_diff node to consider.
>  ///
> @@ -239,9 +243,9 @@ default_reporter::report(const enum_diff& d, ostream&
> out,
>  ///
>  /// @param indent the white space string to use for indentation.
>  void
> -default_reporter::report_local_typedef_changes(const typedef_diff &d,
> -                                              ostream& out,
> -                                              const string& indent) const
> +default_reporter::report_non_type_typedef_changes(const typedef_diff &d,
> +                                                 ostream& out,
> +                                                 const string& indent)
> const
>  {
>    if (!d.to_be_reported())
>      return;
> @@ -283,7 +287,7 @@ default_reporter::report(const typedef_diff& d,
>
>    typedef_decl_sptr f = d.first_typedef_decl(), s =
> d.second_typedef_decl();
>
> -  report_local_typedef_changes(d, out, indent);
> +  report_non_type_typedef_changes(d, out, indent);
>
>    diff_sptr dif = d.underlying_type_diff();
>    if (dif && dif->has_changes())
> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index 088eebb..dbb4364 100644
> --- a/src/abg-ir.cc
> +++ b/src/abg-ir.cc
> @@ -15778,9 +15778,11 @@ equals(const typedef_decl& l, const typedef_decl&
> r, change_kind* k)
>
>    if (*l.get_underlying_type() != *r.get_underlying_type())
>      {
> +      // Changes to the underlying type of a typedef are considered
> +      // local, a bit like for pointers.
>        result = false;
>        if (k)
> -       *k |= SUBTYPE_CHANGE_KIND;
> +       *k |= LOCAL_TYPE_CHANGE_KIND;
>        else
>         return false;
>      }
> diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
> index 0875c96..c2a766e 100644
> --- a/src/abg-leaf-reporter.cc
> +++ b/src/abg-leaf-reporter.cc
> @@ -202,7 +202,9 @@ leaf_reporter::report(const typedef_diff& d,
>    if (!diff_to_be_reported(&d))
>      return;
>
> -  report_local_typedef_changes(d, out, indent);
> +  // all changes carried by a typedef_diff are considered local, so
> +  // let's just call the default reporter here.
> +  default_reporter::report(d, out, indent);
>
>    maybe_report_interfaces_impacted_by_diff(&d, out, indent);
>  }
> diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
> index 5e2e8b1..7334db2 100644
> --- a/tests/data/Makefile.am
> +++ b/tests/data/Makefile.am
> @@ -867,6 +867,11 @@ test-diff-filter/test-PR25661-7-report-1.txt \
>  test-diff-filter/test-PR25661-7-report-2.txt \
>  test-diff-filter/test-PR25661-7-report-3.txt \
>  test-diff-filter/test-PR25661-7-report-4.txt \
> +test-diff-filter/test-PR26309-v0.c           \
> +test-diff-filter/test-PR26309-v1.c           \
> +test-diff-filter/test-PR26309-v0.o           \
> +test-diff-filter/test-PR26309-v1.o           \
> +test-diff-filter/test-PR26309-report-0.txt   \
>  \
>  test-diff-suppr/test0-type-suppr-v0.cc \
>  test-diff-suppr/test0-type-suppr-v1.cc \
> diff --git a/tests/data/test-diff-filter/test-PR26309-report-0.txt
> b/tests/data/test-diff-filter/test-PR26309-report-0.txt
> new file mode 100644
> index 0000000..dec3758
> --- /dev/null
> +++ b/tests/data/test-diff-filter/test-PR26309-report-0.txt
> @@ -0,0 +1,13 @@
> +Leaf changes summary: 1 artifact changed
> +Changed leaf types summary: 0 leaf type changed
> +Removed/Changed/Added functions summary: 0 Removed, 1 Changed, 0 Added
> function
> +Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added
> variable
> +
> +1 function with some sub-type change:
> +
> +  [C] 'function void changed_fun(changed)' at test-PR26309-v1.c:3:1 has
> some sub-type changes:
> +    parameter 1 of type 'typedef changed' changed:
> +      underlying type 'int' changed:
> +        type name changed from 'int' to 'long int'
> +        type size changed from 32 to 64 (in bits)
> +
> diff --git a/tests/data/test-diff-filter/test-PR26309-v0.c
> b/tests/data/test-diff-filter/test-PR26309-v0.c
> new file mode 100644
> index 0000000..853084b
> --- /dev/null
> +++ b/tests/data/test-diff-filter/test-PR26309-v0.c
> @@ -0,0 +1,5 @@
> +typedef int changed;
> +
> +void changed_fun(changed y) {
> +  (void) y;
> +}
> diff --git a/tests/data/test-diff-filter/test-PR26309-v0.o
> b/tests/data/test-diff-filter/test-PR26309-v0.o
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..0e6fd52dcd00da7ba74fa6c2e88907223f9a6f3c
> GIT binary patch
> literal 2720
> zcmbVN-D?v;5TCtkY&16X!>>}&6BS$VF0r*GD%EKHs3=k^f-j|9l1*}&4@s`2D&mU>
> zz9|SI_~x_tNBB?p=!<`V&k8!Tx077A7X%k>XZE-Ao0;94-F<QG_H|1L;3nWA^hb;W
> zd{6cDqEd@60poCg`Sp+2&wsCdEg?9hCen((!^v(n8Fdy3Aqv>rL#dz@MpR)((*SHN
> zKBk_4uwBHhw+-ZsiIdrhjGdjzj*0Q%(SwK|06Tj=b0Kqc0==DpOy*+7PGd5jxHK8f
> zoSG!#;-U9fCIM0@u+x#fbqM{QCf_P9l46Zhh(Ss%VsF7T_Coy1j$p6057#Rol-<x>
> zS)E;&%b#;M^IlmoV0Yvn2m3ZH3mN6Q->S*VdUd0fz{%CyO_{5-D-VNQEeLaspyZc=
> znjbWB+INlub#oOz^p)yXf@;;R28~d5Ks$#UckVk&&RMVE%{#OCGxPa{**Rx=RaTs3
> zKa6m9?%a&yHp7jUT&&5K>;z>T`kiupajUT4E}V7T8o-UErSr}-x|{j8w`jq*P_2lk
> zt01;U#nj&6^c&ox!-&&IP3`dI?C2|NX=K+`G>G8Dp2}$me*Q_Cv=b{9i$VWalVjsd
> zQnPgdn{zhoG9fj#VR!(ZTst;o2H1}bz_SDJy*qHMeLRi*L)Xh-XGzA3ueY*2$d9j*
> zBM?r$B!1M!A_0uE-F^c<qQ`jPRHMv#rvUYt2v&aL|J{q>6n9OJiw3TTa9t~=e_jja
> z8N0e*bb$T*0Nl|y<sQ!;yzY}`=$BB2ok-SM3*}Y_9vx2bO5HAa9og``iY#r^)`M2H
> zjdM!(OQnw7+&1lZ=wjGTX#_3F6bu1gx!r6^yfeL4JCq)tOSc>P<wx3)tglx)ev^Ng
> z&i{YK6cTA~c|X}tQ7gSV)8l>xM!+!?gqbJqrnWbQ$v=e#R3HD#KZ9J;C&lFWRT{*a
> zX-lsIJ!{37jZ&kx4!&ue>WUz)WqMhtE#hbTB>W3?*zQH5*PFI0IAdaDY-upxV@8T;
> zdkf*D{J-h^nNRuCZD!_QLCo}t5X@@uDh*=Iv|Y!!r1)ulD0!bKKGl*L|1o0Zlk-GX
> zwM`x9GXq+3e3s7;PvRffhnepicEdcDFEyX%S>D}t88EtYuRdtLY5qV1g+5hjyncq?
> zp^~(JPQw1tYr%ZpS6YV|n|!~~;EG---RtxUnSA;u=Gj9CzH9I(4PuS=jUn=z#4qUl
> KKe3^fCjT$}O}BFZ
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/data/test-diff-filter/test-PR26309-v1.c
> b/tests/data/test-diff-filter/test-PR26309-v1.c
> new file mode 100644
> index 0000000..5f453a3
> --- /dev/null
> +++ b/tests/data/test-diff-filter/test-PR26309-v1.c
> @@ -0,0 +1,5 @@
> +typedef long changed;
> +
> +void changed_fun(changed y) {
> +  (void) y;
> +}
> diff --git a/tests/data/test-diff-filter/test-PR26309-v1.o
> b/tests/data/test-diff-filter/test-PR26309-v1.o
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..4b9339a5972c97a595ff6555ddaf82efea0f975e
> GIT binary patch
> literal 2752
> zcmbtW-D?v;5TDJqHX0jJ>sP7Z35qRvmwr=~)@W_5A4sVRf=In2o8&aPNOIMxh%ZI(
> zO+iq>H~#^j{0ID71RwQH@Ihzpc9QG%M9_iTnfcAv?q+s3&layNrGx+`0rSw+7)8FL
> zD;Gks0E4gxZY;m}vi#!dkK3P%i1vh*beLdUmR+GS8dAiBn8i`1dr1jHg(Q)PoyGg!
> zM<Aq)Q0i3;V+O>r>|jP_hqD7>Phai;;`>2nPi0PLE)HV0qmaqWqa8Ck;>=KFIX*<r
> zg@bR-4FUA_V$H}ubrAEOpxpQYq{ER1o&xL43n-*u1V<r$X8Umr_Yc=AAW-eVURj-(
> znH)Q5KNxpPQP9pPKPr1SQz?un)m*=-%4?N%AL>oNYI%MD`C7A~^5tgvu9vTRLB8%4
> z-J(}@y?S1!%v1h$zU&5WDB5MOQn4#uJy5NL+PVr{xO&4{u%?_@XWE(=8=oGVnV7Uj
> zR#n+rc7q7FCr^%Ab|YB#)m&Bis^yhXbz7y{+{WyTJu_w5Re%c%3#Y6ROgH-XY|+a1
> zMhwK`RS+9FF}%Ak{Svq85aRR^g&Tb~n|q#G=-;u34iTI<QdjlB_g_gyncTS`_0Ma@
> zNVFu8)O~E)Wc(1sj1u}{T33{?v#Z1;37MGI6(#g{m6#*}yE%a*?B0gr5x`T~J$s!d
> z-rkw<9_*}){9{MGQ-N?=jgKGEK*Yk|j_~VR-)G>g<GBloX4W|jD33;Pm?!?bJ29N{
> zSG0cGz<CD>2Ch|D(t=r+*GTIM?;q`#`-A`VA8<?Kw5IsFz-d2f1a1*|(297Cr9f>2
> z;Lsrkr`T?T(^7TUDXZdob<OiDO|*rwTP(KJgDqyaMfb+mkh<q94#5)Ol$wo(!u!PW
> zn}KrhwA$^!E#1?JRBf%&avMrd{6E<+#?#*Nesa2k7CQZ>^=@ZIz!7AGnJ4j*_BWXs
> ze;ggCZ~irY9AnL#RFms--w{S+`qJY;PgeoIIHjYf5WZ=U`ida#ae8=&=I}Ff68@0}
> zq-zoDjHd4jT1<?br3S}3k&$5fUPd@+{y+5mIX;Jebjz9ZFC%8=L<lA|c%GCPnZ9di
> zOR7Jj4<+vt)u(&FtbZRd%E|LYF{FpO&?g2o=laZ_Af6O&>%+|V5U1fd=9@a6?aXg&
> zxeOTHzE|&byy<>N1N09mPP~4`UZaq-e}#noqgRCEd0%NAW^Ky-R)Z^gp>*HV>tx2$
> gZ(^Q3gy5?N50erj-ZzFQZ&LhCJ^v5vsJR*c8;XItO#lD@
>
> literal 0
> HcmV?d00001
>
> diff --git
> a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt
> b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt
> index aaf28bf..3e50dc7 100644
> ---
> a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt
> +++
> b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt
> @@ -1,6 +1,6 @@
>  ================ changes of 'libspice-server.so.1.8.0'===============
> -Leaf changes summary: 11 artifacts changed (11 filtered out)
> -  Changed leaf types summary: 2 (11 filtered out) leaf types changed
> +Leaf changes summary: 10 artifacts changed (10 filtered out)
> +  Changed leaf types summary: 1 (10 filtered out) leaf types changed
>    Removed/Changed/Added functions summary: 1 Removed, 0 Changed, 8 Added
> functions
>    Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added
> variable
>
> @@ -120,87 +120,5 @@ Leaf changes summary: 11 artifacts changed (11
> filtered out)
>        function int spice_server_set_zlib_glz_compression(SpiceServer*,
> spice_wan_compression_t)
>        function void spice_server_vm_start(SpiceServer*)
>        function void spice_server_vm_stop(SpiceServer*)
> -  'typedef spice_image_compression_t at spice.h:479:1' changed:
> -    typedef name changed from spice_image_compression_t to
> SpiceImageCompression at enums.h:197:1
> -    79 impacted interfaces:
> -      function void spice_qxl_add_memslot(QXLInstance*, QXLDevMemSlot*)
> -      function void spice_qxl_add_memslot_async(QXLInstance*,
> QXLDevMemSlot*, uint64_t)
> -      function void spice_qxl_create_primary_surface(QXLInstance*,
> uint32_t, QXLDevSurfaceCreate*)
> -      function void spice_qxl_create_primary_surface_async(QXLInstance*,
> uint32_t, QXLDevSurfaceCreate*, uint64_t)
> -      function void spice_qxl_del_memslot(QXLInstance*, uint32_t,
> uint32_t)
> -      function void spice_qxl_destroy_primary_surface(QXLInstance*,
> uint32_t)
> -      function void spice_qxl_destroy_primary_surface_async(QXLInstance*,
> uint32_t, uint64_t)
> -      function void spice_qxl_destroy_surface_async(QXLInstance*,
> uint32_t, uint64_t)
> -      function void spice_qxl_destroy_surface_wait(QXLInstance*, uint32_t)
> -      function void spice_qxl_destroy_surfaces(QXLInstance*)
> -      function void spice_qxl_destroy_surfaces_async(QXLInstance*,
> uint64_t)
> -      function void spice_qxl_driver_unload(QXLInstance*)
> -      function void spice_qxl_flush_surfaces_async(QXLInstance*, uint64_t)
> -      function void spice_qxl_loadvm_commands(QXLInstance*,
> QXLCommandExt*, uint32_t)
> -      function void spice_qxl_monitors_config_async(QXLInstance*,
> QXLPHYSICAL, int, uint64_t)
> -      function void spice_qxl_oom(QXLInstance*)
> -      function void spice_qxl_reset_cursor(QXLInstance*)
> -      function void spice_qxl_reset_image_cache(QXLInstance*)
> -      function void spice_qxl_reset_memslots(QXLInstance*)
> -      function void spice_qxl_set_max_monitors(QXLInstance*, unsigned int)
> -      function void spice_qxl_start(QXLInstance*)
> -      function void spice_qxl_stop(QXLInstance*)
> -      function void spice_qxl_update_area(QXLInstance*, uint32_t,
> QXLRect*, QXLRect*, uint32_t, uint32_t)
> -      function void spice_qxl_update_area_async(QXLInstance*, uint32_t,
> QXLRect*, uint32_t, uint64_t)
> -      function void spice_qxl_wakeup(QXLInstance*)
> -      function int spice_server_add_client(SpiceServer*, int, int)
> -      function int spice_server_add_interface(SpiceServer*,
> SpiceBaseInstance*)
> -      function int spice_server_add_renderer(SpiceServer*, const char*)
> -      function int spice_server_add_ssl_client(SpiceServer*, int, int)
> -      function void
> spice_server_char_device_wakeup(SpiceCharDeviceInstance*)
> -      function void spice_server_destroy(SpiceServer*)
> -      function spice_image_compression_t
> spice_server_get_image_compression(SpiceServer*)
> -      function int spice_server_get_num_clients(SpiceServer*)
> -      function int spice_server_get_peer_info(SpiceServer*, sockaddr*,
> socklen_t*)
> -      function int spice_server_get_sock_info(SpiceServer*, sockaddr*,
> socklen_t*)
> -      function int spice_server_init(SpiceServer*, SpiceCoreInterface*)
> -      function int spice_server_is_server_mouse(SpiceServer*)
> -      function int spice_server_migrate_connect(SpiceServer*, const
> char*, int, int, const char*)
> -      function int spice_server_migrate_end(SpiceServer*, int)
> -      function int spice_server_migrate_info(SpiceServer*, const char*,
> int, int, const char*)
> -      function int spice_server_migrate_start(SpiceServer*)
> -      function int spice_server_migrate_switch(SpiceServer*)
> -      function SpiceServer* spice_server_new()
> -      function void
> spice_server_playback_get_buffer(SpicePlaybackInstance*, uint32_t**,
> uint32_t*)
> -      function void
> spice_server_playback_put_samples(SpicePlaybackInstance*, uint32_t*)
> -      function void
> spice_server_playback_set_mute(SpicePlaybackInstance*, uint8_t)
> -      function void
> spice_server_playback_set_volume(SpicePlaybackInstance*, uint8_t, uint16_t*)
> -      function void spice_server_playback_start(SpicePlaybackInstance*)
> -      function void spice_server_playback_stop(SpicePlaybackInstance*)
> -      function void spice_server_port_event(SpiceCharDeviceInstance*,
> uint8_t)
> -      function uint32_t
> spice_server_record_get_samples(SpiceRecordInstance*, uint32_t*, uint32_t)
> -      function void spice_server_record_set_mute(SpiceRecordInstance*,
> uint8_t)
> -      function void spice_server_record_set_volume(SpiceRecordInstance*,
> uint8_t, uint16_t*)
> -      function void spice_server_record_start(SpiceRecordInstance*)
> -      function void spice_server_record_stop(SpiceRecordInstance*)
> -      function void spice_server_set_addr(SpiceServer*, const char*, int)
> -      function int spice_server_set_agent_copypaste(SpiceServer*, int)
> -      function int spice_server_set_agent_file_xfer(SpiceServer*, int)
> -      function int spice_server_set_agent_mouse(SpiceServer*, int)
> -      function int spice_server_set_channel_security(SpiceServer*, const
> char*, int)
> -      function int spice_server_set_compat_version(SpiceServer*,
> spice_compat_version_t)
> -      function int spice_server_set_exit_on_disconnect(SpiceServer*, int)
> -      function int spice_server_set_image_compression(SpiceServer*,
> spice_image_compression_t)
> -      function int spice_server_set_jpeg_compression(SpiceServer*,
> spice_wan_compression_t)
> -      function int spice_server_set_listen_socket_fd(SpiceServer*, int)
> -      function void spice_server_set_name(SpiceServer*, const char*)
> -      function int spice_server_set_noauth(SpiceServer*)
> -      function int spice_server_set_playback_compression(SpiceServer*,
> int)
> -      function int spice_server_set_port(SpiceServer*, int)
> -      function int spice_server_set_sasl(SpiceServer*, int)
> -      function int spice_server_set_sasl_appname(SpiceServer*, const
> char*)
> -      function void spice_server_set_seamless_migration(SpiceServer*, int)
> -      function int spice_server_set_streaming_video(SpiceServer*, int)
> -      function int spice_server_set_ticket(SpiceServer*, const char*,
> int, int, int)
> -      function int spice_server_set_tls(SpiceServer*, int, const char*,
> const char*, const char*, const char*, const char*, const char*)
> -      function void spice_server_set_uuid(SpiceServer*, const uint8_t*)
> -      function int spice_server_set_zlib_glz_compression(SpiceServer*,
> spice_wan_compression_t)
> -      function void spice_server_vm_start(SpiceServer*)
> -      function void spice_server_vm_stop(SpiceServer*)
>  ================ end of changes of
> 'libspice-server.so.1.8.0'===============
>
> diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
> index bdf7d41..b3b1e91 100644
> --- a/tests/test-diff-filter.cc
> +++ b/tests/test-diff-filter.cc
> @@ -766,6 +766,13 @@ InOutSpec in_out_specs[] =
>     "data/test-diff-filter/test-PR25661-7-report-4.txt",
>     "output/test-diff-filter/test-PR25661-7-report-4.txt",
>    },
> +  {
> +   "data/test-diff-filter/test-PR26309-v0.o",
> +   "data/test-diff-filter/test-PR26309-v1.o",
> +   "--no-default-suppression --leaf-changes-only",
> +   "data/test-diff-filter/test-PR26309-report-0.txt",
> +   "output/test-diff-filter/test-PR26309-report-0.txt",
> +  },
>    // This should be the last entry
>    {NULL, NULL, NULL, NULL, NULL}
>  };
> --
> 1.8.3.1
>
>
Looks good to me.

Reviewed-by: Giuliano Procida <gprocida@google.com>


>
> --
>                 Dodji
>
>