From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 254903858C52 for ; Thu, 22 Sep 2022 00:20:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 254903858C52 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663806043; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type; bh=sJD8AsAh1+r5op+Z6xAIgc181WvHK34vtFhZBO2aCLE=; b=IH2cDr+7N7wCckq0FXMZClOIZGWe79yBz9kKmT2w/b5l4F6VzJjVqdu/QE/3bytJqdlHh+ +KTvTKEsVrGqROsZ0xe7+0HfuKCIeu4GIfY8rP/h7I1df+OmNpWkAv0CUxwjqQSjN3Ll2s IH8KIG33ndN0mKFZGauO/1Im0q5N4LM= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-381-VSyIetrkOwWETMMLfkmYIQ-1; Wed, 21 Sep 2022 20:20:42 -0400 X-MC-Unique: VSyIetrkOwWETMMLfkmYIQ-1 Received: by mail-qv1-f69.google.com with SMTP id oj15-20020a056214440f00b004ac6db57cacso5392728qvb.17 for ; Wed, 21 Sep 2022 17:20:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:user-agent:message-id:date:organization:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date; bh=sJD8AsAh1+r5op+Z6xAIgc181WvHK34vtFhZBO2aCLE=; b=zo3mkNVSzhqFW18uSPSH5W0w/7hkoIGZNIwTKDj/099E0fLoan1Q6hfFj5ACUlPydN 4X8LDc5P84tcM+IBbYfzHHeZTNu6Co5G45O6WY6Y4piyQzpt0HNhS1XldRl/rPIlJuLo ZComCDjSXHVSjjxj3GuafCI/afp2GAL9r53hgCik7xIZrdv5xpVR3hRnvFL4H0j5cbCW JqMtrFJYjKbA2A3DRvObPieU4haX17wkH/Ddbajm6JfnQ2STLuXpGHxYcR9GA5Vj35Um MXYpPII9jwpUFy+ObBzdcLlTijKUtNVC1OHkuRqT6HBUBGxoZTYcMDHAUADZwMEWr/oN yaVw== X-Gm-Message-State: ACrzQf20HYksIOVQajTM+L/fHnyAQNgOS6wEjc/HkVUH4vR7Yxbonl+j x23gbIIq6ojsCanoHxhstFtrAWyQL0Y9j+YqIt0VxhYaFB2+nVM4cDbWKeUfKcpliXFZn1XH+vc Z7waWT5hy1pD1EK9sjzd6 X-Received: by 2002:a0c:802c:0:b0:478:de2c:9b6f with SMTP id 41-20020a0c802c000000b00478de2c9b6fmr680337qva.89.1663806041681; Wed, 21 Sep 2022 17:20:41 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7ppsSENG5wFpt3MjRK8yIKoSRtHE/QgEmaL8rZhfjAJxh5mW1rqcIOnfBQ6Ffuxpj8+9iMxg== X-Received: by 2002:a0c:802c:0:b0:478:de2c:9b6f with SMTP id 41-20020a0c802c000000b00478de2c9b6fmr680323qva.89.1663806041367; Wed, 21 Sep 2022 17:20:41 -0700 (PDT) Received: from localhost ([88.120.130.27]) by smtp.gmail.com with ESMTPSA id cf14-20020a05622a400e00b0031f41ea94easm2497683qtb.28.2022.09.21.17.20.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Sep 2022 17:20:40 -0700 (PDT) Received: by localhost (Postfix, from userid 1000) id 5FC4C581C2F; Thu, 22 Sep 2022 02:20:38 +0200 (CEST) From: Dodji Seketeli To: libabigail@sourceware.org Cc: arsen@aarsen.me Subject: [PATCH, applied] comparison: Ensure that fn parms with basic types can't be redundant Organization: Red Hat / France X-Operating-System: Fedora 38 X-URL: http://www.redhat.com Date: Thu, 22 Sep 2022 02:20:38 +0200 Message-ID: <874jx05kfd.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hello, When comparing the two libc binaries from "https://the.owo.foundation/Ac1Ksw8.tgz", abidiff crashes. It appears to be due to an assert that is hit because the overload of default_reporter::report for fn_parm_diff momentarily removes the redundant-ness from the categorization of the diff node of the type of a function parameter. The problem is that the sole child diff node of that type diff node is itself redundant. So the function parameter type diff node should really be redundant too. Oops, there is a logic violation there, hence the assert violation. I commented out the line that removes the redundant-ness. After all, if function parameter types shouldn't be redundant, that should have been taken care of by the redundancy_marking_visitor code in abg-comparison.cc as well as its associated category propagation code. But then consider what happens with a reproducer of the libc binaries above: $ cat test-PR29387-v0.c typedef int Integer; void f0(Integer i, char c) { i + c; } void f1(Integer i, unsigned char c) { i + c; } $ $ cat test-PR29387-v1.c typedef long int Integer; void f0(Integer i, char c) { i + c; } void f1(Integer i, unsigned char c) { i + c; } $ gcc -g test-PR29387-v0.c $ gcc -g test-PR29387-v1.c $ $ abidiff test-PR29387-v0.o test-PR29387-v1.o Functions changes summary: 0 Removed, 2 Changed, 0 Added functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 2 functions with some indirect sub-type change: [C] 'function void f0(Integer, char)' at PR29387-test-v1.c:4:1 has some indirect sub-type changes: parameter 1 of type 'typedef Integer' changed: underlying type 'int' changed: type name changed from 'int' to 'long int' type size changed from 32 to 64 (in bits) [C] 'function void f1(Integer, unsigned char)' at PR29387-test-v1.c:10:1 has some indirect sub-type changes: $ $ So, the problem is this peace of report: [C] 'function void f1(Integer, unsigned char)' at PR29387-test-v1.c:10:1 has some indirect sub-type changes: You see that the report is empty; the reporter could not say what changed. What changed is the typedef "Integer" that changed from "int" to "long int". The redundancy_marking_visitor pass marked the change of the underlying type of the Integer typedef as being redundant. This is because that typedef change has already been reported on the f0 function interface. The problem is that by design, the 'int' to 'long int' change should not have been marked as being redundant by the redundancy_marking_visitor pass. This is because, we want to see all the "basic type changes" on function parameters types. They are deemed "local changes" of the function types, and we want to see all local changes to functions because it is almost 100% sure these are non-compatible changes. The root cause of the problem is that the function has_basic_type_change_only in abg-comparison.cc fails to detect that the parameter change carries a typedef-to-basic-type change, so the function parameter is wrongly marked as being redundant even though it ultimately carries a basic type change. This patch thus teaches has_basic_type_change_only to look through parameter changes to better detect basic type changes. * include/abg-comparison.h (peel_fn_parm_diff) (peel_typedef_qualified_type_or_parameter_diff): Declare ... * src/abg-comparison.cc (peel_fn_parm_diff) (peel_typedef_qualified_type_or_parameter_diff): ... new functions. (has_basic_type_change_only): Look through function parameters, typedefs and qualified types. * src/abg-default-reporter.cc (default_reporter::report): Remove the temporary removal of the redundant categorisation. Redundant-ness should have been handled by the redundancy_marking_visitor pass. * tests/data/test-diff-filter/test-PR29387-report.txt: Reference test output. * tests/data/test-diff-filter/test-PR29387-v{0,1}.c: Source of the input tests. * tests/data/test-diff-filter/test-PR29387-v{0,1}.o: Input test binaries. * tests/data/Makefile.am: Add the new test material above to the source distribution. * tests/test-diff-filter.cc (in_out_specs): Add the test binaries above to the test harness. Signed-off-by: Dodji Seketeli Applied to master. --- include/abg-comparison.h | 6 +++ src/abg-comparison.cc | 45 ++++++++++++++++++ src/abg-default-reporter.cc | 5 +- tests/data/Makefile.am | 5 ++ .../test-diff-filter/test-PR29387-report.txt | 17 +++++++ tests/data/test-diff-filter/test-PR29387-v0.c | 13 +++++ tests/data/test-diff-filter/test-PR29387-v0.o | Bin 0 -> 3216 bytes tests/data/test-diff-filter/test-PR29387-v1.c | 13 +++++ tests/data/test-diff-filter/test-PR29387-v1.o | Bin 0 -> 3240 bytes tests/test-diff-filter.cc | 7 +++ 10 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 tests/data/test-diff-filter/test-PR29387-report.txt create mode 100644 tests/data/test-diff-filter/test-PR29387-v0.c create mode 100644 tests/data/test-diff-filter/test-PR29387-v0.o create mode 100644 tests/data/test-diff-filter/test-PR29387-v1.c create mode 100644 tests/data/test-diff-filter/test-PR29387-v1.o diff --git a/include/abg-comparison.h b/include/abg-comparison.h index e51a5021..a3c197b9 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -2860,11 +2860,17 @@ peel_reference_diff(const diff* dif); const diff* peel_qualified_diff(const diff* dif); +const diff* +peel_fn_parm_diff(const diff* dif); + const diff* peel_pointer_or_qualified_type_diff(const diff* dif); const diff* peel_typedef_or_qualified_type_diff(const diff* dif); + +const diff* +peel_typedef_qualified_type_or_parameter_diff(const diff *dif); }// end namespace comparison }// end namespace abigail diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 27982112..c536f66f 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -12424,6 +12424,21 @@ peel_qualified_diff(const diff* dif) return dif; } +/// If a diff node is about changes between two function parameters +/// get the diff node about changes between the types of the parameters. +/// +/// @param dif the dif node to consider. +/// +/// @return the diff of the types of the parameters. +const diff* +peel_fn_parm_diff(const diff* dif) +{ + const fn_parm_diff *d = 0; + while ((d = is_fn_parm_diff(dif))) + dif = d->type_diff().get(); + return dif; +} + /// If a diff node is about changes between two pointer, reference or /// qualified types, get the diff node about changes between the /// underlying types. @@ -12480,6 +12495,34 @@ peel_typedef_or_qualified_type_diff(const diff *dif) return dif; } +/// If a diff node is about changes between two typedefs or qualified +/// types, get the diff node about changes between the underlying +/// types. +/// +/// Note that this function walks the tree of underlying diff nodes +/// returns the first diff node about types that are neither typedef, +/// qualified type nor parameters. +/// +/// @param dif the dif node to consider. +/// +/// @return the diff node about changes between the underlying types. +const diff* +peel_typedef_qualified_type_or_parameter_diff(const diff *dif) +{ + while (true) + { + if (const typedef_diff *d = is_typedef_diff(dif)) + dif = peel_typedef_diff(d); + else if (const qualified_type_diff *d = is_qualified_type_diff(dif)) + dif = peel_qualified_diff(d); + else if (const fn_parm_diff *d = is_fn_parm_diff(dif)) + dif = peel_fn_parm_diff(d); + else + break; + } + return dif; +} + /// Test if a diff node represents a diff between two class or union /// types. /// @@ -12522,6 +12565,8 @@ has_local_type_change_only(const diff *d) bool has_basic_type_change_only(const diff *d) { + d = peel_typedef_qualified_type_or_parameter_diff(d); + if (is_diff_of_basic_type(d, true) && d->has_changes()) return true; else if (const var_diff * v = dynamic_cast(d)) diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc index 36af26bf..8c705060 100644 --- a/src/abg-default-reporter.cc +++ b/src/abg-default-reporter.cc @@ -518,9 +518,7 @@ default_reporter::report(const fn_parm_diff& d, ostream& out, { diff_sptr type_diff = d.type_diff(); ABG_ASSERT(type_diff->has_changes()); - diff_category saved_category = type_diff->get_category(); - // Parameter type changes are never redundants. - type_diff->set_category(saved_category & ~REDUNDANT_CATEGORY); + out << indent; if (f->get_is_artificial()) out << "implicit "; @@ -535,7 +533,6 @@ default_reporter::report(const fn_parm_diff& d, ostream& out, out << "' changed:\n"; type_diff->report(out, indent + " "); - type_diff->set_category(saved_category); } } diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 401d4c8c..7bddd646 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -1099,6 +1099,11 @@ test-diff-filter/test-PR27995.abi \ test-diff-filter/test-PR28013-fn-variadic.c.report.txt \ test-diff-filter/test-PR28013-fn-variadic.c.0.abi \ test-diff-filter/test-PR28013-fn-variadic.c.1.abi \ +test-diff-filter/test-PR29387-v1.c \ +test-diff-filter/test-PR29387-v0.c \ +test-diff-filter/test-PR29387-v1.o \ +test-diff-filter/test-PR29387-v0.o \ +test-diff-filter/test-PR29387-report.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-PR29387-report.txt b/tests/data/test-diff-filter/test-PR29387-report.txt new file mode 100644 index 00000000..d6541d4d --- /dev/null +++ b/tests/data/test-diff-filter/test-PR29387-report.txt @@ -0,0 +1,17 @@ +Functions changes summary: 0 Removed, 2 Changed, 0 Added functions +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +2 functions with some indirect sub-type change: + + [C] 'function void f0(Integer, char)' at PR29387-test-v1.c:4:1 has some indirect sub-type changes: + parameter 1 of type 'typedef Integer' changed: + underlying type 'int' changed: + type name changed from 'int' to 'long int' + type size changed from 32 to 64 (in bits) + + [C] 'function void f1(Integer, unsigned char)' at PR29387-test-v1.c:10:1 has some indirect sub-type changes: + parameter 1 of type 'typedef Integer' 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-PR29387-v0.c b/tests/data/test-diff-filter/test-PR29387-v0.c new file mode 100644 index 00000000..cf0ee4c9 --- /dev/null +++ b/tests/data/test-diff-filter/test-PR29387-v0.c @@ -0,0 +1,13 @@ +typedef int Integer; + +void +f0(Integer i, char c) +{ + i + c; +} + +void +f1(Integer i, unsigned char c) +{ + i + c; +} diff --git a/tests/data/test-diff-filter/test-PR29387-v0.o b/tests/data/test-diff-filter/test-PR29387-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..061eaf5636f08d7385c297c4035b58c05c7d4a14 GIT binary patch literal 3216 zcmb_d&2Jl35TCcUlQd4!d=VN#iGzTq2(Le=+B7A_$ZDHHg`!B6!U4kCUfT=yI$Cc^ zT2#TQYQ&|LIB?*=fh*$UP%b%CLPGo_+{=j~kYMKRjJ`dB~UYmC>T@<$5>rb=hA1>muIYF=(N?V^MWm@6Z2DQdbo7##=M#V zNIVDWC3QZXn7~C&spltV(uu`|MGBdG64yTmiHX&8G#D-nqth5nlY9w3i!y?qveu}J zZ%PkDWgWj1#=eMBDfRorI0-r~{7_lcCugZe>u5AAdXS;uNY9)*C>>Y{48HW{)l9i? zCQ~Td1-p*m0+TP0P?CDdP zYy)=vuGjF9R&6=~=pl${AppB{X|2Z&&}1Ar21;m&)bU%rb7m$$zi|xQTRoGqjobAq$U9-Pbo7 zZEA{beFA9DY83Gso1pe4)xy~D$Q^V(jW~VO^sRhtV)B+%9v^xg0}vzME9AsB$RBwQ z_mnpYzx;XFgBNsQMH3!hL7$m`s1Eu_q$S{p328e4>Brz>hj6SHn2yayuMx31#>Eyn z;6h+w{+Ol2vkBj_BJpprEo&q^A;BmoSvSXdmT`)w^_JPD zCyAQiVPy99N$^{YpJQo`pJANV+IxIZFB!P(1K8c`?a-;941-8EMGM`%5bT;0I$&42 zU9bbU<=8d1vfJ49{CWp{T6Zdyz`gc}+X)=M;XX=fdA=(YOd%@4+uE*n+HDtqKDOTp zT^p-C+YO!S`)oJ-U3(|!?6^UAopZU(?Rwy}U0&ON5erDBy_EBmu}Z9%zXpnpwZC^3!W>=HEe##*}%Ys@rdzs4f6Y$uIIl#0T-W`4RZ+Z}^`) zhW{(`<$aZ5B(KO%nJ<2kFPIma9m12iFHxa$;dBDSGV{%Sk^Pgv15{{ClfT9MEeT|4 z^1nxX(D`lggCP0o{3h`ezTE#oxhd5*oc{qC*yuk@T$4|CV$fE7L6gEZABkRX?%x!8 Q2Jvt6{D15*BgN$Z1qPh{;s5{u literal 0 HcmV?d00001 diff --git a/tests/data/test-diff-filter/test-PR29387-v1.c b/tests/data/test-diff-filter/test-PR29387-v1.c new file mode 100644 index 00000000..295e6a4a --- /dev/null +++ b/tests/data/test-diff-filter/test-PR29387-v1.c @@ -0,0 +1,13 @@ +typedef long int Integer; + +void +f0(Integer i, char c) +{ + i + c; +} + +void +f1(Integer i, unsigned char c) +{ + i + c; +} diff --git a/tests/data/test-diff-filter/test-PR29387-v1.o b/tests/data/test-diff-filter/test-PR29387-v1.o new file mode 100644 index 0000000000000000000000000000000000000000..cdc25a89e2c6a3d9f72e6733c727d3ac065859d0 GIT binary patch literal 3240 zcmbtV&2Jl35TECbn>0>>^Fc@mRW=Be6ybFosy0nYF|yjGYK5vumCB(?*7n-o60f86 zx=M=>;!+WDX(bLEIBf7?3zgEg|Tq|Rl|W-d>s<}msOWHRS7)&zj%sQ2b2yrh=r zC)Lz&aqjNCn#Ng9sy8R5GuGn5BEi{LZ~Vw@Ch6Mkqc>K#j|#nel2x0;?-aJ=izJ}y2^S(tq3gX?ywaKYHzs_2Xd!Q+PP-b3B1j^7kHsxLDvl{jm?Ag)49{9>|7l>LEEnf zNUJp55cXO@-S&eBE(eiU_d?L!qfRh<&{ih|LIocN^3}RxOMTkKD)LYlzcFqL1YJ6WM#I()h6}n1C3?-lZVQpm-eV zZ>jDOJo~dhg2#2>jV3%fgC5g@_zv`t*s|cb32EPg%nR_bJ{&IyOeMyn=ZM%WrTp)TPlCv4-IN{0JOeOHdlcwuai1G=SyjKZCq4n)vCba1)l2E>Wr?dx< zXDxNh>~9+QhwNWuE&a>94~+aP?0;tDvkTuF`M21Yxzl(oNk%Poq^^WsXM^%-9ZKxe zlSLwU6uTfBA^1JU&#^Z9w;89kOs;3qz-4^kwC^+{w~RUpW8Dxd@(v`k?HmH7<57aJmsXo2(X8nD{s81Ounp&Q5p_TxwrM~Eo5g)|geJ-kVEzqC zN{Gq-5o3e)Z=d&1>eK#Z@e{tB|3S5>+(WMalpGZLKNHvF)0r5wRNv5`Q08ORZO!$Y Q#K=HCkN?*$Gd4{AUqpKV=Kufz literal 0 HcmV?d00001 diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc index 861fc48d..7f9355bd 100644 --- a/tests/test-diff-filter.cc +++ b/tests/test-diff-filter.cc @@ -815,6 +815,13 @@ InOutSpec in_out_specs[] = "data/test-diff-filter/test-PR28013-fn-variadic.c.report.txt", "output/test-diff-filter/test-PR28013-fn-variadic.c.report.txt", }, + { + "data/test-diff-filter/test-PR29387-v0.o", + "data/test-diff-filter/test-PR29387-v1.o", + "--no-default-suppression", + "data/test-diff-filter/test-PR29387-report.txt", + "output/test-diff-filter/test-PR29387-report.txt", + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL} }; -- 2.37.2 -- Dodji