From 85929105f507616c494db2cf499a6604d8a0fee7 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Wed, 3 Jun 2015 10:44:07 +0200 Subject: [PATCH] Fix redundancy marking for change of types used directly If a type T is used directly (i.e, not through a pointer or reference) as a function parameter or as a base class, a change in T should never be marked as redundant in that context. Otherwise, the change in that context might be filtered out, possibly hiding real ABI incompatible changes. This patch implements this policy. Also, it turned out in some circumstances, we where marking the first visited diff node of a given class of equivalence of nodes as being redundant, while we should only mark the *subsequently* visited nodes of that class of equivalence as visited. The patch also fixes that. * include/abg-comparison.h (pointer_map): Make this be a map of {size_t, size_t} pairs, rather than {size_t, bool}, so that each pointer in the map can be associated to another one. (diff_context::diff_has_been_visited): Return the pointer to the first diff node of the equivalence class that has been visited. * src/abg-comparison.cc (is_pointer_diff, is_reference_diff) (is_reference_or_pointer_diff, is_fn_parm_diff, is_base_diff) (is_child_node_of_function_parm_diff, is_child_node_of_base_diff): Define new static functions. (diff_context::diff_has_been_visited): Return the pointer to the first diff node of the equivalence class that has been visited. (diff_context::mark_diff_as_visited): Save the pointer to the first diff node of a given class of equivalence that has been visited. (redundancy_marking_visitor::visit_begin): If a diff node is a child node of a function parameter diff or base diff node and if it's not a pointer or reference diff node, then do not mark it as redundant. Also, make sure to not mark the first diff node of a given class of equivalence that has been visited, as redundant; only the other subsequent nodes should be marked redundant; we were hitting this case because of an optimization that makes equivalent class diff nodes to share their private (pimpl) data. * tests/data/test-diff-filter/test29-finer-redundancy-marking-v{0,1}.o: New test input binaries. * tests/data/test-diff-filter/test29-finer-redundancy-marking-v{0,1}.cc: Source code of the new test input binaries above. * tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt: New test input. * tests/data/Makefile.am: Add the new test material above to the source distribution. * tests/test-diff-filter.cc (in_out_specs): Make this test harness run over the additional test input above. * tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt: Adjust. Signed-off-by: Dodji Seketeli --- include/abg-comparison.h | 9 +- src/abg-comparison.cc | 122 ++++++++++++++++-- tests/data/Makefile.am | 5 + ...st29-finer-redundancy-marking-report-0.txt | 28 ++++ .../test29-finer-redundancy-marking-v0.cc | 24 ++++ .../test29-finer-redundancy-marking-v0.o | Bin 0 -> 3376 bytes .../test29-finer-redundancy-marking-v1.cc | 25 ++++ .../test29-finer-redundancy-marking-v1.o | Bin 0 -> 3440 bytes .../test5-fn-suppr-report-0.txt | 8 +- tests/test-diff-filter.cc | 7 + 10 files changed, 212 insertions(+), 16 deletions(-) create mode 100644 tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt create mode 100644 tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.cc create mode 100644 tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.o create mode 100644 tests/data/test-diff-filter/test29-finer-redundancy-marking-v1.cc create mode 100644 tests/data/test-diff-filter/test29-finer-redundancy-marking-v1.o diff --git a/include/abg-comparison.h b/include/abg-comparison.h index 05e39111..95bc9cf0 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -117,8 +117,9 @@ class class_diff; typedef shared_ptr class_diff_sptr; /// Convenience typedef for a map of pointer values. The Key is a -/// pointer value and the value is a boolean. -typedef unordered_map pointer_map; +/// pointer value and the value is potentially another pointer value +/// associated to the first one. +typedef unordered_map pointer_map; /// Convenience typedef for a map which key is a string and which /// value is a @ref decl_base_sptr. @@ -934,10 +935,10 @@ public: void initialize_canonical_diff(const diff_sptr diff); - bool + diff* diff_has_been_visited(const diff*) const; - bool + diff_sptr diff_has_been_visited(const diff_sptr) const; void diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 2d97ec83..e0b21a29 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -339,6 +339,82 @@ is_function_decl_diff(const diff* diff) return d; } +/// Test if a diff node is about differences between two pointers. +/// +/// @param diff the diff node to consider. +/// +/// @return the @p diff converted into an instance of @ref +/// pointer_diff iff @p diff is about differences between two +/// pointers. +static const pointer_diff* +is_pointer_diff(const diff* diff) +{return dynamic_cast(diff);} + +/// Test if a diff node is about differences between two references. +/// +/// @param diff the diff node to consider. +/// +/// @return the @p diff converted into an instance of @ref +/// reference_diff iff @p diff is about differences between two +/// references. +static const reference_diff* +is_reference_diff(const diff* diff) +{return dynamic_cast(diff);} + +/// Test if a diff node is either a reference diff node or a pointer +/// diff node. +/// +/// @param diff the diff node to test. +/// +/// @return true iff @p diff is either reference diff node or a +/// pointer diff node. +static bool +is_reference_or_pointer_diff(const diff* diff) +{return is_reference_diff(diff) || is_pointer_diff(diff);} + +/// Test if a diff node is about differences between two function +/// parameters. +/// +/// @param diff the diff node to consider. +/// +/// @return the @p diff converted into an instance of @ref +/// reference_diff iff @p diff is about differences between two +/// function parameters. +static const fn_parm_diff* +is_fn_parm_diff(const diff* diff) +{return dynamic_cast(diff);} + +/// Test if a diff node is about differences between two base class +/// specifiers. +/// +/// @param diff the diff node to consider. +/// +/// @return the @p diff converted into an instance of @ref base_diff +/// iff @p diff is about differences between two base class +/// specifiers. +static const base_diff* +is_base_diff(const diff* diff) +{return dynamic_cast(diff);} + +/// Test if a diff node is a child node of a function parameter diff node. +/// +/// @param diff the diff node to test. +/// +/// @return true iff @p diff is a child node of a function parameter +/// diff node. +static bool +is_child_node_of_function_parm_diff(const diff* diff) +{return diff && is_fn_parm_diff(diff->parent_node());} + +/// Test if a diff node is a child node of a base diff node. +/// +/// @param diff the diff node to test. +/// +/// @return true iff @p diff is a child node of a base diff node. +static bool +is_child_node_of_base_diff(const diff* diff) +{return diff && is_base_diff(diff->parent_node());} + /// The default traverse function. /// /// @return true. @@ -3160,39 +3236,53 @@ diff_context::initialize_canonical_diff(const diff_sptr diff) /// Test if a diff node has been traversed. /// /// @param d the diff node to consider. -bool +/// +/// @return the first diff node against which @p d is redundant. +diff* diff_context::diff_has_been_visited(const diff* d) const { const diff* canonical = d->get_canonical_diff(); assert(canonical); size_t ptr_value = reinterpret_cast(canonical); - return (priv_->visited_diff_nodes_.find(ptr_value) - != priv_->visited_diff_nodes_.end()); + pointer_map::iterator it = priv_->visited_diff_nodes_.find(ptr_value); + if (it != priv_->visited_diff_nodes_.end()) + return reinterpret_cast(it->second); + else + return 0; } /// Test if a diff node has been traversed. /// /// @param d the diff node to consider. -bool +/// +/// @return the first diff node against which @p d is redundant. +diff_sptr diff_context::diff_has_been_visited(const diff_sptr d) const -{return diff_has_been_visited(d.get());} +{ + diff_sptr diff(diff_has_been_visited(d.get())); + return diff; +} /// Mark a diff node as traversed by a traversing algorithm. /// /// Actually, it's the @ref CanonicalDiff "canonical diff" of this /// node that is marked as traversed. /// -/// Subsequent invocations of diff_has_been_visited() on the diff -/// node will yield true. +/// Subsequent invocations of diff_has_been_visited() on the diff node +/// will yield true. void diff_context::mark_diff_as_visited(const diff* d) { + if(diff_has_been_visited(d)) + return; + const diff* canonical = d->get_canonical_diff(); assert(canonical); - size_t ptr_value = reinterpret_cast(canonical); - priv_->visited_diff_nodes_[ptr_value] = true; + size_t canonical_ptr_value = reinterpret_cast(canonical); + size_t diff_ptr_value = reinterpret_cast(d);; + priv_->visited_diff_nodes_[canonical_ptr_value] = diff_ptr_value; } /// Unmark all the diff nodes that were marked as being traversed. @@ -13346,7 +13436,19 @@ struct redundancy_marking_visitor : public diff_node_visitor // should never be marked redundant because we want to see // them all. && !is_diff_of_variadic_parameter(d) - && !is_diff_of_variadic_parameter_type(d)) + && !is_diff_of_variadic_parameter_type(d) + // If the *same* diff node (not one that is merely + // equivalent to this one) has already been visited + // the do not mark it as beind redundant. It's only + // the other nodes that are equivalent to this one + // that must be marked redundant. + && d->context()->diff_has_been_visited(d) != d + // If the diff node is a function parameter and is not + // a reference/pointer then do not mark it as + // redundant. + && (is_reference_or_pointer_diff(d) + || (!is_child_node_of_function_parm_diff(d) + && !is_child_node_of_base_diff(d)))) { d->add_to_category(REDUNDANT_CATEGORY); // As we said in preamble, as this node is marked as diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 8379ad96..53b70f11 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -403,6 +403,11 @@ test-diff-filter/test28-redundant-and-filtered-children-nodes-report-0.txt \ test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt \ test-diff-filter/test28-redundant-and-filtered-children-nodes-v0.cc \ test-diff-filter/test28-redundant-and-filtered-children-nodes-v1.cc \ +test-diff-filter/test29-finer-redundancy-marking-report-0.txt \ +test-diff-filter/test29-finer-redundancy-marking-v0.o \ +test-diff-filter/test29-finer-redundancy-marking-v1.cc \ +test-diff-filter/test29-finer-redundancy-marking-v0.cc \ +test-diff-filter/test29-finer-redundancy-marking-v1.o \ \ test-diff-suppr/test0-type-suppr-v0.cc \ test-diff-suppr/test0-type-suppr-v1.cc \ diff --git a/tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt b/tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt new file mode 100644 index 00000000..a6665ce3 --- /dev/null +++ b/tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt @@ -0,0 +1,28 @@ +Functions changes summary: 0 Removed, 3 Changed, 0 Added functions +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +3 functions with some indirect sub-type change: + + [C]'function void bar(base&)' has some indirect sub-type changes: + parameter 1 of type 'base&' has sub-type changes: + in referenced type 'struct base': + type size changed from 32 to 64 bits + 1 data member insertion: + 'char base::m1', at offset 32 (in bits) + + [C]'function void baz(base)' has some indirect sub-type changes: + parameter 1 of type 'struct base' has sub-type changes: + details were reported earlier + + [C]'function void foo(inherited*)' has some indirect sub-type changes: + parameter 1 of type 'inherited*' has sub-type changes: + in pointed to type 'struct inherited': + type size changed from 64 to 96 bits + 1 base class change: + 'struct base' changed: + details were reported earlier + + 1 data member change: + 'int inherited::m0' offset changed from 32 to 64 (in bits) + + diff --git a/tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.cc b/tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.cc new file mode 100644 index 00000000..92fe32a8 --- /dev/null +++ b/tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.cc @@ -0,0 +1,24 @@ +// Compile this with: +// g++ -g -Wall -c test29-finer-redundancy-marking-v0.cc + +struct base +{ + int m0; +}; + +struct inherited : public base +{ + int m0; +}; + +void +bar(base&) +{} + +void +baz(base) +{} + +void +foo(inherited*) +{} diff --git a/tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.o b/tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..3591565d16edf22b517a06aac57caeb249ba0674 GIT binary patch literal 3376 zcmbtW-D@0G6hC)ov%ASAn@ytFkP0jK(NJeLNkdJlsjD$I#VR4tm!KxIyLWdd&CbS| zNl7C`2_odpf>4D%2!ap(1wQ!ZQ(sCSd=g&;9~D80KIl1j?#WJXMl5<@?m6doKJK~a z-nla$tXz3LBLpxBxCniXQGjo_2xSL6KLTYgh{0`y25d4Yry@CJAy;<;5TgV$ z4QWB#`4pu*4VhaVD145>V1Y7V&;f`El#XdoOXAL5l=9@2xwQqkR<8%*iDNeu`ssfo zPZ}uwP~i%Bi81j^X`*PArb=UCJYUAp%!IH?#o~qHi$$8#8Ud?7%H@fLA}--+C>Adk ztpcv@G{jiW5id{1PQFT0J9YcjNea#npiAZyK;c;wa6M&=CShn)3-CM@M5O?xOi;4r z2^cm|$-qlE3h`%Y1V?o+aNh((GK}UH?Uv`uzz$?{&u_YZuP2B?Agk^v*4Vo%+1Ws&n(W) zR?e);W@XilV%&aVx?;C>1GyXY8Zr#gW_P1KUoN#JR^>HNb%RD{X@B9oeSW@Tx1rkU zb!D~LYi@hhwii`*yt-TW+OD_bkQ(moV!AN&dVW}?6^7NO8@Wofn_jDh>)nZD09ZSo zuJ!r48%j`vLJnf!5qooKdHF)+3@&y0U)KrnSlmkx2kRjA%VKIcU-$%f;xyv)5UKO` zN~!#DW_cvHf)0q0jdqYoon*GZ*0;YMn^WV#32$^Gw~jmtV%}jXlKT-j)FE){VF*qj zcU-3_>wE1@&u{h6rewEX59IZS%x>WNZTWD>4xS|r!70FL^txS%@44gmBI#hG>@ad0 z*K{J;*=z-FSL)*bC!4}}x{z{4o|@Lpcf`E6oE8##`B=@UvAm&^EP zI)0x4&3XL{)3-RSe?RN~t!hW=UkM+MPxdvf)Ur3kD0{zI(7n2fbNRbt^Ce)eDrlBU)*40p(qDTo8p{U91?%mz7yR%_t zQqrg>D%OI61wj-a1i=^o0Rpsy(zx# z*{*JQal$)~Emob*PAGT6VM|64+MGeW8_2U=iIw^-9C~4^cXn^}v~zl;>U3f2$|>B{ zOKYQ?{u&$ z{aA*S8IR-UO4ExZsE1ea~&n=5BY(4?07%DeN_yp}cm;?1f&?m4`$6c&s=CrvSG#91J900&XyjrHk!y zqS$L))rn+ps}p(ysf+)gY98aUouICpRIY}`=IfHWCY8s7_@sDC`?K0+U}!V`5wz2O z{nz+9#?t(zY-?iDFd9R3O_()N=pJ5ABt0E4jhOuSGU03SEOKT}q@b_K4``4Q)0b{? zlOy*(HMx|EC20CSi(FRyw zvHvFsh&#CUwc$G*zsHVR^ZJ?o2tii=7IgnMv?KKozl}%wM|@VPt2*fyI-ZqTe^%|} z`#EhE%9}c11~HYy_4B@xTh{zl{r+sCJuT+^Xndsh}N%et$nXPD^$E+Kg z=kF==clq1q(APcvVMlEIpumYxxPvqgW8!%w#M9Z~_nyu%3G>Ma;64qQ|5T6Xv^LN0 zV#=S-Dp78Vr+I(TWBS=g!uh$Sbgz*xe-h&K%p%hO literal 0 HcmV?d00001 diff --git a/tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt b/tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt index 79a7feaf..76ff8710 100644 --- a/tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt +++ b/tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt @@ -1,7 +1,7 @@ -Functions changes summary: 0 Removed, 1 Changed (1 filtered out), 0 Added function +Functions changes summary: 0 Removed, 2 Changed, 0 Added functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable -1 function with some indirect sub-type change: +2 functions with some indirect sub-type change: [C]'function void bar(S*)' has some indirect sub-type changes: parameter 1 of type 'S*' has sub-type changes: @@ -12,4 +12,8 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 1 data member change: 'int S::m1' offset changed from 0 to 32 (in bits) + [C]'function void bar(int, S)' has some indirect sub-type changes: + parameter 2 of type 'struct S' has sub-type changes: + details were reported earlier + diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc index 8f00adcf..1b67bb68 100644 --- a/tests/test-diff-filter.cc +++ b/tests/test-diff-filter.cc @@ -362,6 +362,13 @@ InOutSpec in_out_specs[] = "data/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt", "output/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt", }, + { + "data/test-diff-filter/test29-finer-redundancy-marking-v0.o", + "data/test-diff-filter/test29-finer-redundancy-marking-v1.o", + "--no-linkage-name --no-redundant", + "data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt", + "output/test-diff-filter/test29-finer-redundancy-marking-report-0.txt", + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL} };