[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] Fix the reporting of leaf change statistics.

HI there.

On Tue, 3 Mar 2020 at 16:43, Matthias Maennich <maennich@google.com> wrote:
>
> On Mon, Mar 02, 2020 at 12:58:53PM +0000, Android Kernel Team wrote:
> >Leaf changes (as reported with --leaf-changes-only) to variables were
> >miscounted as changes to functions.
> >
> >       * src/abg-comparison.cc
> >       (apply_filters_and_compute_diff_stats): Increment the correct
> >       counter for leaf variable changes.
> >       * tests/data/Makefile.am: Add test case files.
> >       * tests/data/test-abidiff-exit/test-leaf0-*: New test case.
>
> Please spell out the single files.
>     * tests/data/test-abidiff-exit/test-leaf0-report.txt: New test case.
>     * tests/data/test-abidiff-exit/test-leaf0-v0.cc: Likewise.
>     * tests/data/test-abidiff-exit/test-leaf0-v0.o: Likewise.
>     * tests/data/test-abidiff-exit/test-leaf0-v1.cc: Likewise.
>     * tests/data/test-abidiff-exit/test-leaf0-v1.o: Likewise.

OK.

> >       * tests/test-abidiff-exit.cc: Run new test case.
> >
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-comparison.cc                            |   4 ++--
> > tests/data/Makefile.am                           |   5 +++++
> > .../data/test-abidiff-exit/test-leaf0-report.txt |  13 +++++++++++++
> > tests/data/test-abidiff-exit/test-leaf0-v0.cc    |   5 +++++
> > tests/data/test-abidiff-exit/test-leaf0-v0.o     | Bin 0 -> 2784 bytes
> > tests/data/test-abidiff-exit/test-leaf0-v1.cc    |   5 +++++
> > tests/data/test-abidiff-exit/test-leaf0-v1.o     | Bin 0 -> 2824 bytes
> > tests/test-abidiff-exit.cc                       |   9 +++++++++
> > 8 files changed, 39 insertions(+), 2 deletions(-)
> > create mode 100644 tests/data/test-abidiff-exit/test-leaf0-report.txt
> > create mode 100644 tests/data/test-abidiff-exit/test-leaf0-v0.cc
> > create mode 100644 tests/data/test-abidiff-exit/test-leaf0-v0.o
> > create mode 100644 tests/data/test-abidiff-exit/test-leaf0-v1.cc
> > create mode 100644 tests/data/test-abidiff-exit/test-leaf0-v1.o
> >
> >diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> >index 5371e843..ef753e6d 100644
> >--- a/src/abg-comparison.cc
> >+++ b/src/abg-comparison.cc
> >@@ -9767,8 +9767,8 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
> >             (stat.num_leaf_var_changes_filtered_out() + 1);
> >       }
> >       if ((*i)->has_local_changes())
> >-      stat.num_leaf_func_changes
> >-        (stat.num_leaf_func_changes() + 1);
> >+      stat.num_leaf_var_changes
> >+        (stat.num_leaf_var_changes() + 1);
> >     }
> >
> >   stat.num_func_syms_added(added_unrefed_fn_syms_.size());
> >diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
> >index 5031e6d3..07077608 100644
> >--- a/tests/data/Makefile.am
> >+++ b/tests/data/Makefile.am
> >@@ -110,6 +110,11 @@ test-abidiff-exit/test-loc-without-locs-report.txt \
> > test-abidiff-exit/test-no-stray-comma-report.txt \
> > test-abidiff-exit/test-no-stray-comma-v0.o \
> > test-abidiff-exit/test-no-stray-comma-v1.o \
> >+test-abidiff-exit/test-leaf0-v0.cc \
>
> I do not think we need to 'distribute' the source files here. I mean, it
> will not harm, but they are mostly there for reference and not needed
> during `make distcheck`.

I was following someone else's example. Agreed, they are not needed in
the Makefile.

> Other than these little nits.
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
>
> Cheers,
> Matthias
>
> >+test-abidiff-exit/test-leaf0-v0.o \
> >+test-abidiff-exit/test-leaf0-v1.cc \
> >+test-abidiff-exit/test-leaf0-v1.o \
> >+test-abidiff-exit/test-leaf0-report.txt \
> > \
> > test-diff-dwarf/test0-v0.cc           \
> > test-diff-dwarf/test0-v0.o                    \
> >diff --git a/tests/data/test-abidiff-exit/test-leaf0-report.txt b/tests/data/test-abidiff-exit/test-leaf0-report.txt
> >new file mode 100644
> >index 00000000..46f39976
> >--- /dev/null
> >+++ b/tests/data/test-abidiff-exit/test-leaf0-report.txt
> >@@ -0,0 +1,13 @@
> >+Leaf changes summary: 2 artifacts 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, 1 Changed, 0 Added variable
> >+
> >+1 function with some sub-type change:
> >+
> >+  [C]'function int changed_fun()' has some sub-type changes:
> >+    return type changed:
> >+      type name changed from 'int' to 'long int'
> >+      type size changed from 32 to 64 (in bits)
> >+
> >+
> >diff --git a/tests/data/test-abidiff-exit/test-leaf0-v0.cc b/tests/data/test-abidiff-exit/test-leaf0-v0.cc
> >new file mode 100644
> >index 00000000..27ba39c9
> >--- /dev/null
> >+++ b/tests/data/test-abidiff-exit/test-leaf0-v0.cc
> >@@ -0,0 +1,5 @@
> >+int changed_var = 0;
> >+
> >+int changed_fun() {
> >+  return 0;
> >+}
> >diff --git a/tests/data/test-abidiff-exit/test-leaf0-v0.o b/tests/data/test-abidiff-exit/test-leaf0-v0.o
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..a79511cc3c6db039aaa1f5528db19b55f75c7a2d
> >GIT binary patch
> >literal 2784
> >zcmbtVOKTHR6h4#N*w)yPHa=P^GJ;QZ#zabO)oL_a>jM$Ng@VLMa?=ExiA*Ln>Ouq&
> >z+__LhaOa=UwSUBoKftvMJ!j^g<o0HY3s1<s=X~dR&wb3xtG8BDmIX`}T!5j(D1g0V
> >z$d@9v1bLW&duwlhekJ+gcWB`>7U?FU5HTMC@sQE@6!DgoF5@&QSRuyJEkBH-SRzd@
> >z88C4G>oZu>*5PbEBeL_^X=`RYw+|q-03v%fb1t)-xt_l`mzo6-hai)=kP#DDnF|-^
> >zSnp@A=gF-!`|<J|Mn8%Fs3Jg1jX0qOMeHh$0*>NnS--PWI7U6hWm^!+Zs=@CuTgL|
> >z3vR6zIi&M287Dw-cY5c|v~@p~Lcdzm^Vem)(&+iX{H7O-M!9zTo?Sk1qPSo$yQkfv
> >zeY#LwF5u60+F{R^E9=siL92$M7u1?7{iQ``alv-hZKvUNpZT?B(D6IHuG90Mw*0yi
> >zdesftg;Qbs2~-{wi<}$l*aWy%E}ylJU6Iw6=g0Y+rQ=58XkELb5tse^E?E7XHNR_o
> >z;vKeOKjQQ!ikf&Sn|qTgPwiMmgBVU6>EMjPuRlpjk$5s`BL<z_;wDg&L_(cXQzt+>
> >z<eE~zxF!u*0h2>P<3zk-4T)I+QzIAw0<zoSyGHOCM5eWM@f|yW_>9({!NDCljl9ux
> >z$VNAwJFpYuF;7KsC4|$ej}wSA9Z$eG-noeO82C#i(`6%~eb#r4<53*_e7Izc#PPX~
> >zb1@vn(Ytz6;cTx&xNG3AmHa@>W<9)iiW5CU$}YE!{o*#bt#DeazFu&<&)RqgaSnrc
> >zZZa3jeh6;e3q5eF-7dI+-0<AGtoGI`Ex*x0UBr9UY9KeasJ(z*ldUcrEnl(=rU19r
> >zX}2Z5vaa6=rHk*1(+$1aV--j?D~-TwOI7%PPV?wbz2$y#xRPQ9=TymLtJ6Ug2s3Xo
> >z@3U@8yy;I*G@U2Y{}ve|@%@RVQMrf;GbY)xf14fEY}%egoN|<uo~N8j9DgYR=v)!R
> >zXP@uU3SwqVit&vGq}%Z{7){$ds4+1z9#>#t;2ATfE%iC6{#kV?*q`dBSJJG%j+hw}
> >zAvmMJ8zg9C+E!4Plz&a-=RQ$>dY8=nPY|P+Tqm*-y;6xjFra46&+`k!ll=Fp%Y5V?
> >z{;!q4VFu^7JO)E8=Vj!R@;A*(3Rq%8HS_uzeveF2{~jgukG?|e&wZtO%-j^aufV1%
> >mn7#w_4x9e8C+6Kl2!1H=6bTxc>!-L${(V*d7tNPg)Bi7zRJKt7
> >
> >literal 0
> >HcmV?d00001
> >
> >diff --git a/tests/data/test-abidiff-exit/test-leaf0-v1.cc b/tests/data/test-abidiff-exit/test-leaf0-v1.cc
> >new file mode 100644
> >index 00000000..020cb761
> >--- /dev/null
> >+++ b/tests/data/test-abidiff-exit/test-leaf0-v1.cc
> >@@ -0,0 +1,5 @@
> >+long changed_var = 0;
> >+
> >+long changed_fun() {
> >+  return 0;
> >+}
> >diff --git a/tests/data/test-abidiff-exit/test-leaf0-v1.o b/tests/data/test-abidiff-exit/test-leaf0-v1.o
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..4a433b9572abb46799391619f21538b54fb3204c
> >GIT binary patch
> >literal 2824
> >zcmbtVOOF#r5UzH!FU(r|0KsYl8k84^%&<nQ)h??AgOh~~5kg2Df`qKG-F_(ccr_kx
> >zmVlH?K;(eL0SO@?gp@y#3nzX{?#Vd^Qq|LCx7))hWx1>B>w0zdV}AYahuxeIz$D-b
> >zj5J08ZcdNnk`_y_3`=nD=I?*~N&15i8g1ivL`$<eOe$+29rBi(BUy;)GO{_z6LNNn
> >zMC~c0&PxVN96-DQu_(SQEazomwXi6bW)@BY<kmqJF6X!NTlu}^t1G!v0P+jS=dYj@
> >z#th)<3eQJ{y=8JMoqBbB1*3n1e_avGaxTp|;TVX@F8(EajiV6%7Upr(eZ*x8NK`-Z
> >zJ1T6f`3J?IR@1&y$6+o_fa>w7qqnBSkGUNB)mmZHRP{<@5CO{vVGI|O?ml#SQPXQj
> >z3Ea4S&nsWJP+a%6f=fZsyR=r^TEm~`cauS+cA6?ual3|U7}r`mhnpMz#=7S>J--q5
> >zA4auS+>3gHzCVb5Zbx-L39B8|hsyn8k#l1HgN)K|l*^aBbKk0JJB-rY{^oh72)#gB
> >zmJ!4+cR?I3h}B~=voEkEpCL|vBHhqyg@vcN^8EBJ8l-UMNQdYM{PQtuQf8i1+LU3J
> >z$J`{&WRWx{)}51J9eQh0!i=SjL<w^v#gb&YVhc${3G)*e0TS@2W{`wq6X+5mi`Kfh
> >zmuC=PvidVT#1kizH=hpN#9<hA;3&qkyTO$ZPOJV3|3r%^1IC{l{e*+#z0^b(kBIi!
> >z-nH`@1s#XiL{MPil$Yz+HaNwxyGoaoi0e6>s&LD2#*GSh9sIG;?;Gyc$vvSs`q?se
> >z3!kvx_yq15oO)pQ0fPR+E?!mSNu2T)ONly6AgG5)2tl>qhags+FsQ5Qpjl~0jULWv
> >zIjmM=bucy^#`NNhyL8%-Vi!ySL9N&ADtvf?sF$b!pC7-UgtebcAl0fg;;^es;s2AZ
> >zqCfSP`^n)-Mi`!7qm!*oXHg-{z2Usiwk^r7KRx4g&RqXHWQ=5=!7ROqpc|9y*uP7P
> >zMXoJ<eCUf;vTR87%m>sn4s@;v;(GWV?I7mHL<ru~fOI=$!_l>+I$ey6^9EcSdZvPF
> >zOMT9&zho{2`&0e&db;)35p!cA1ltDuo)n8*+X~KQ<=;2?xlfdz-VHbZK4KJ;>qJr0
> >z6O-sI1BP>c=8q81^8egiW_}+z4Er(v&G_>?^S!ahfYHfby*B=?`5gmnj$F0G>u2~S
> >z3R(SomeD`@I<Y_ZmDb_rrr6I6*fI;H?*qNhu0QRGd-o86zYTbi6pOfT3{l)H|97VT
> >KKiJT4*Z)8NIl2)5
> >
> >literal 0
> >HcmV?d00001
> >
> >diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
> >index aea57c32..5372b3fe 100644
> >--- a/tests/test-abidiff-exit.cc
> >+++ b/tests/test-abidiff-exit.cc
> >@@ -120,6 +120,15 @@ InOutSpec in_out_specs[] =
> >     "data/test-abidiff-exit/test-no-stray-comma-report.txt",
> >     "output/test-abidiff-exit/test-no-stray-comma-report.txt"
> >   },
> >+  {
> >+    "data/test-abidiff-exit/test-leaf0-v0.o",
> >+    "data/test-abidiff-exit/test-leaf0-v1.o",
> >+    "",
> >+    "--no-show-locs --leaf-changes-only",
> >+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
> >+    "data/test-abidiff-exit/test-leaf0-report.txt",
> >+    "output/test-abidiff-exit/test-leaf0-report.txt"
> >+  },
> >   {0, 0, 0 ,0,  abigail::tools_utils::ABIDIFF_OK, 0, 0}
> > };
> >
> >--
> >2.25.0.265.gbab2e86ba0-goog
> >
> >