From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by sourceware.org (Postfix) with ESMTPS id CF7D23858C2A for ; Tue, 26 Sep 2023 08:38:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CF7D23858C2A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: by mail.gandi.net (Postfix) with ESMTPSA id 726162000C; Tue, 26 Sep 2023 08:38:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seketeli.org; s=gm1; t=1695717486; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FFVtd/07PSE84v/wMIYrx0s7rWTzMs4d3WanRpcuSmk=; b=gqVfAYyPljYcIFpEaD4L/xrcfYJ5Hhbd/T2Pw+3oq5qB9b9WFdLpQkiFBnbVNm2CrfBNCW kpdecNrWyfjBtkj64QCPUP4d0iM7O8m2ZGcVN/0yxQhCnlhxPD6UMMHvKekeYQdPzPqYSa ae8ZLoIMOoC58nwVrsSzj/UlpGPm/2gCs4XRuh5jm7sXOO/sey2XwljKm6IT2ADuVX+ggj Jvatkuli5Hz2oK7LOZAlXXl9cIDqMSNljx9QtdV9/9q2OHR06iRBhXJlx0DT8mLtIknhhy lxprJNWRhFwPi2tCnmQRpdwAd1O1rFZXn5Y9YaDF4rh1qElbR7ah4KUc9VYP3A== Received: by localhost (Postfix, from userid 1000) id 1DE2EB73C5; Tue, 26 Sep 2023 10:38:05 +0200 (CEST) From: Dodji Seketeli To: John Moon Cc: , Trilok Soni , "Satya Durga Srinivasu Prabhala" Subject: Re: abidiff improvements for kernel UAPI checker Organization: Me, myself and I References: <5363161d-8167-284e-e35d-9a8ef20adea9@quicinc.com> <877cu5t7tw.fsf@seketeli.org> <87354tt32o.fsf@seketeli.org> <340b33bd-2b43-9f99-58e1-f1b77a51b48a@quicinc.com> <87fs36e8sr.fsf@seketeli.org> <87bkdue89g.fsf@seketeli.org> <44d76150-3e34-4bef-9970-d321f5bc224c@quicinc.com> X-Operating-System: CentOS Stream release 9 X-URL: http://www.seketeli.net/~dodji Date: Tue, 26 Sep 2023 10:38:05 +0200 In-Reply-To: <44d76150-3e34-4bef-9970-d321f5bc224c@quicinc.com> (John Moon's message of "Fri, 22 Sep 2023 11:28:10 -0700") Message-ID: <871qelgwia.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-GND-Sasl: dodji@seketeli.org X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,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 John! John Moon a =C3=A9crit: [...] > Thanks for the quick turnaround! I gave this a try on my end and it > looks like the patch is doing what it's supposed to, but it's not=20 > producing the desired behavior. Thanks John for quickly testing this and providing feedback! It's really appreciated. > Let's zoom in on this example: > https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d= 93d5d40d=20 > (specifically, the change to include/uapi/rdma/hns-abi.h: > > enum { > HNS_ROCE_EXSGE_FLAGS =3D 1 << 0, > HNS_ROCE_RQ_INLINE_FLAGS =3D 1 << 1, > + HNS_ROCE_CQE_INLINE_FLAGS =3D 1 << 2, > }; > > enum { > HNS_ROCE_RSP_EXSGE_FLAGS =3D 1 << 0, > HNS_ROCE_RSP_RQ_INLINE_FLAGS =3D 1 << 1, > + HNS_ROCE_RSP_CQE_INLINE_FLAGS =3D 1 << 2, > }; > > > Before your patch, abidiff reports: > > [C] 'enum __anonymous_enum__1' changed: > type size hasn't changed > 2 enumerator deletions: > '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1' > '__anonymous_enum__1::HNS_ROCE_RSP_RQ_INLINE_FLAGS' value '2' > 3 enumerator insertions: > '__anonymous_enum__::HNS_ROCE_EXSGE_FLAGS' value '1' > '__anonymous_enum__::HNS_ROCE_RQ_INLINE_FLAGS' value '2' > '__anonymous_enum__::HNS_ROCE_CQE_INLINE_FLAGS' value '4' > 1 added type unreachable from any public interface: > [A] 'enum __anonymous_enum__1' at hns-abi.h:94:1 > > After your patch: > > 2 removed types unreachable from any public interface: > [D] 'enum {HNS_ROCE_EXSGE_FLAGS=3D1, HNS_ROCE_RQ_INLINE_FLAGS=3D2, }' > at hns-abi.h:88:1 > [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=3D1, > HNS_ROCE_RSP_RQ_INLINE_FLAGS=3D2, }' at hns-abi.h:93:1 > 2 added types unreachable from any public interface: > [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=3D4, HNS_ROCE_EXSGE_FLAGS=3D1, > HNS_ROCE_RQ_INLINE_FLAGS=3D2, }' at hns-abi.h:88:1 > [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=3D4, > HNS_ROCE_RSP_EXSGE_FLAGS=3D1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=3D2, }' at= =20 > hns-abi.h:94:1 Right. > > I probably should have thought of this before, but simply changing the > anon enum's "name" to be a concatenation of members wouldn't work to=20 > associate them across a diff. When you add a new enum variant, the > "name" of the enum changes too, so abidiff thinks they're different > enums. Actually, no, it's me who hasn't been clear. This patch was "just" the beginning. The goal is to avoid the "renumbering" that happens on the internal anonymous names (e.g, __anonymous_enum__1 turning into __anonymous_enum__2) whenever a new enum is added in the middle of a translation unit. > I've given it a bit of thought and I'm not sure how the two enums > could be associated when diffing... I'm not sure if the diff corpus > has any context-awareness that could help here. Maybe you can think of > something clever? Now that the renumbering issue is solved by using the flat representation to designate the anonymous enum, we need to teach the comparison engine that an enum that got removed and then added again is actually an enum that 'changed'. We do similar things already for other kinds of types in the comparison engine. Basically, if a removed anonymous enum and an added anonymous enum have enumerators in common, we assume that we are looking at a changed enumerator. So, I have updated the branch with some more patches and I think we should now be close to what we want, regarding this issue. If you update that branch and look at the patch that is at its tip, you can see that in the test suite, in tests/data/test-abidiff-exit/, I've added two files test-anonymous-enums-change-v0.c and test-anonymous-enums-change-v1.c that are compiled and compared. Let's look at the diff of these two files: $ diff -u test-anonymous-enums-change-v0.c test-anonymous-enums-cha= nge-v1.c=20 --- test-anonymous-enums-change-v0.c 2023-09-26 10:13:54.880093568 = +0200 +++ test-anonymous-enums-change-v1.c 2023-09-26 10:14:46.073213038 = +0200 @@ -1,6 +1,6 @@ -/* - * Compile this with: - * gcc -c -g -fno-eliminate-unused-debug-types test-anonymous-e= nums-change-v0.c=20 +/*=20 + * Compile this with: + * gcc -c -g -fno-eliminate-unused-debug-types test-anonymous-e= nums-change-v1.c=20 */ enum @@ -13,14 +13,19 @@ { E3_0, E3_1, + E3_2 }; enum { E4_0, E4_1, - E4_2, - E4_LAST_ELEMENT + }; + +enum + { + E5_0, + E5_1, }; enum $ So, an anonymous enum type has one enumerator added, another one has enumerators removed, and there is an addition of an anonymous enum type to the translation unit. Now let's look at what abidiff reports: $ ~/git/libabigail/better-anon-enums/build/tools/abidiff --non-reac= hable-types test-anonymous-enums-change-v0.o test-anonymous-enums-change-v1= .o || echo "$?" Functions changes summary: 0 Removed, 0 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable Unreachable types summary: 0 removed, 1 changed (1 filtered out), 1= added types 1 changed type unreachable from any public interface: [C] 'enum {E4_0=3D0, E4_1=3D1, E4_2=3D2, E4_LAST_ELEMENT=3D3, }' = changed: type size hasn't changed 2 enumerator deletions: 'E4_2' value '2' 'E4_LAST_ELEMENT' value '3' 1 added type unreachable from any public interface: [A] 'enum {E5_0=3D0, E5_1=3D1, }' at test-anonymous-enums-change-= v1.c:26:1 12 $ Hopefully, that is closer to what we'd expect. We see in the summary of the change report that one change was filtered out. Let's use the '--harmless' option to see what the filtered out change was: $ ~/git/libabigail/better-anon-enums/build/tools/abidiff --non-reac= hable-types --harmless test-anonymous-enums-change-v0.o test-anonymous-enum= s-change-v1.o Functions changes summary: 0 Removed, 0 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable Unreachable types summary: 0 removed, 2 changed, 1 added types 2 changed types unreachable from any public interface: [C] 'enum {E4_0=3D0, E4_1=3D1, E4_2=3D2, E4_LAST_ELEMENT=3D3, }' = changed: type size hasn't changed 2 enumerator deletions: 'E4_2' value '2' 'E4_LAST_ELEMENT' value '3' [C] 'enum {E3_0=3D0, E3_1=3D1, }' changed: type size hasn't changed 1 enumerator insertion: 'E3_2' value '2' 1 added type unreachable from any public interface: [A] 'enum {E5_0=3D0, E5_1=3D1, }' at test-anonymous-enums-change-= v1.c:26:1 So, it's the change involving an enumerator insertion that was initially filtered out as a harmless change. [...] > If not, I think this output is at least easier to post-process! In general, I am not for post-processing the output of abidiff. I think it's much more maintainable to make it emit what we want directly. >For example, I could add logic to the script to do something like "if >there's a deleted enum whose contents are present in an added enum, >assume it's an addition to an anonymous enum and ignore". Let's try harder to get things right, first ;-) Many thanks. Cheers, --=20 Dodji