Message ID | 5363161d-8167-284e-e35d-9a8ef20adea9@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <libabigail-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E58B03858C36 for <patchwork@sourceware.org>; Tue, 11 Apr 2023 00:45:58 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E58B03858C36 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1681173958; bh=lBXbxqNr9AZREH2dbFOTAW6/G3mZ1ptAdaB2PzEsYYY=; h=Date:To:CC:Subject:List-Id:List-Unsubscribe:List-Archive: List-Help:List-Subscribe:From:Reply-To:From; b=fE0629sX0Ck+DejmSheaIymUH423prN5tKfuwWAVK5oeG8QrPFlHfC+Ce2nRNKv29 BOq5pwSweyT153/Fa/4aR9fVT0A0Eqs7+r6umpgULNKY6WqYg3NE6QehF2kLK+SY6X WpC/qMZWDsVfp1NCB3ZCpD5qwWjyU40E6e0qA2jw= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by sourceware.org (Postfix) with ESMTPS id EFDD33858D38 for <libabigail@sourceware.org>; Tue, 11 Apr 2023 00:45:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EFDD33858D38 Received: from pps.filterd (m0279868.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 33B0frO5027095; Tue, 11 Apr 2023 00:45:50 GMT Received: from nalasppmta01.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3pvm97h1qj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 11 Apr 2023 00:45:50 +0000 Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 33B0jnoZ015289 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 11 Apr 2023 00:45:49 GMT Received: from [10.110.49.239] (10.80.80.8) by nalasex01c.na.qualcomm.com (10.47.97.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.42; Mon, 10 Apr 2023 17:45:48 -0700 Message-ID: <5363161d-8167-284e-e35d-9a8ef20adea9@quicinc.com> Date: Mon, 10 Apr 2023 17:45:48 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Content-Language: en-US To: <libabigail@sourceware.org>, <dodji@seketeli.org> CC: Trilok Soni <quic_tsoni@quicinc.com>, Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com> Subject: abidiff improvements for kernel UAPI checker Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01c.na.qualcomm.com (10.47.97.35) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: 8SQcrk5UMt_NDgXBdNjwbcEKnN3aDMoI X-Proofpoint-ORIG-GUID: 8SQcrk5UMt_NDgXBdNjwbcEKnN3aDMoI X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-04-10_18,2023-04-06_03,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 malwarescore=0 adultscore=0 mlxlogscore=999 lowpriorityscore=0 mlxscore=0 clxscore=1011 priorityscore=1501 impostorscore=0 suspectscore=0 spamscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2304110004 X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, 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 X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project <libabigail.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libabigail>, <mailto:libabigail-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libabigail/> List-Help: <mailto:libabigail-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libabigail>, <mailto:libabigail-request@sourceware.org?subject=subscribe> From: John Moon via Libabigail <libabigail@sourceware.org> Reply-To: John Moon <quic_johmoo@quicinc.com> Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" <libabigail-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
abidiff improvements for kernel UAPI checker
|
|
Commit Message
John Moon
April 11, 2023, 12:45 a.m. UTC
Hi all, As you may be aware from the Linux kernel mailing list threads I've been CC'ing you on, we're in the process of upstreaming a shell script that uses abidiff to compare Linux kernel userspace APIs between revisions. Linux UAPIs are supposed to be stable forever, so we're trying to detect when a patch breaks a UAPI. If you haven't been following, the latest thread is here: https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/ At a high level, we're taking the header file before/after the patch, building it in a dummy C file, then taking advantage of abidiff's "--non-reachable-types" argument to compare all the symbols. This method works great! However, there are a couple of classes of ABI differences that are detected but don't qualify as UAPI-breaking. By far the most common class of differences occur with enum expansions. For example, this change triggers finding: https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a Specifically this change to include/uapi/linux/dbcnl.h: abidiff correctly reports there's an ABI difference: [C] 'enum ieee_attrs' changed: type size hasn't changed 1 enumerator insertion: 'ieee_attrs::DCB_ATTR_DCB_REWR_TABLE' value '12' 1 enumerator change: 'ieee_attrs::__DCB_ATTR_IEEE_MAX' from value '12' to '13' at dcbnl.h:416:1 It's quite common in the kernel to have the last variant in your enum be *_MAX, then define the max value of the enum based on that variant. However, just adding a variant to this enum doesn't break userspace (unless you have a program that does something silly like this): if (DCB_ATTR_IEEE_MAX > 12) { // crash the program } Assuming that programs will use the MAX value in a reasonable way, we can consider this change safe for userspace backwards-compatibility if it meets the following criteria: 1. No variants are removed/renamed 2. No variants have a changed value (unless it's the last one) Technically, we could parse the stdout of abidiff to see if these conditions are met, but that'd be quite complicated and fragile. It seems a better choice to detect them when the data structures are available. Do you have any ideas on how these condition could be detected from abidiff? We could spend some time to implement the logic, but I'm not sure if this kind of specific use case is within the scope of your project. If so, could something be added like "--ignore-enum-expansion" (or some other flag)? There are other similar "false positives" discussed in the LKML thread, but this first one is the largest and probably the easiest to detect algorithmically, so I figure it's a good starting point. Thank you in advance for any help! - John Moon
Comments
On 4/10/2023 5:45 PM, John Moon wrote: > Hi all, > > As you may be aware from the Linux kernel mailing list threads I've been > CC'ing you on, we're in the process of upstreaming a shell script that > uses abidiff to compare Linux kernel userspace APIs between revisions. > Linux UAPIs are supposed to be stable forever, so we're trying to detect > when a patch breaks a UAPI. > > If you haven't been following, the latest thread is here: > https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/ > > At a high level, we're taking the header file before/after the patch, > building it in a dummy C file, then taking advantage of abidiff's > "--non-reachable-types" argument to compare all the symbols. > > This method works great! However, there are a couple of classes of ABI > differences that are detected but don't qualify as UAPI-breaking. > > By far the most common class of differences occur with enum expansions. > > For example, this change triggers finding: > https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a Any inputs here from the libabigail mailing list here? ---Trilok Soni
Hello, John & Satya, John Moon <quic_johmoo@quicinc.com> a écrit: [...] > As you may be aware from the Linux kernel mailing list threads I've > been CC'ing you on, we're in the process of upstreaming a shell script > that uses abidiff to compare Linux kernel userspace APIs between > revisions. Linux UAPIs are supposed to be stable forever, so we're > trying to detect when a patch breaks a UAPI. > > If you haven't been following, the latest thread is here: > https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/ > > At a high level, we're taking the header file before/after the patch, > building it in a dummy C file, then taking advantage of abidiff's > "--non-reachable-types" argument to compare all the symbols. > > This method works great! However, there are a couple of classes of ABI > differences that are detected but don't qualify as UAPI-breaking. > > By far the most common class of differences occur with enum expansions. > > For example, this change triggers finding: > https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a > > > Specifically this change to include/uapi/linux/dbcnl.h: > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h > index 99047223ab26..7e15214aa5dd 100644 > --- a/include/uapi/linux/dcbnl.h > +++ b/include/uapi/linux/dcbnl.h > @@ -411,6 +411,7 @@ enum dcbnl_attrs { > * @DCB_ATTR_IEEE_PEER_PFC: peer PFC configuration - get only > * @DCB_ATTR_IEEE_PEER_APP: peer APP tlv - get only > * @DCB_ATTR_DCB_APP_TRUST_TABLE: selector trust table > + * @DCB_ATTR_DCB_REWR_TABLE: rewrite configuration > */ > enum ieee_attrs { > DCB_ATTR_IEEE_UNSPEC, > @@ -425,6 +426,7 @@ enum ieee_attrs { > DCB_ATTR_IEEE_QCN_STATS, > DCB_ATTR_DCB_BUFFER, > DCB_ATTR_DCB_APP_TRUST_TABLE, > + DCB_ATTR_DCB_REWR_TABLE, > __DCB_ATTR_IEEE_MAX > }; > > > abidiff correctly reports there's an ABI difference: > > [C] 'enum ieee_attrs' changed: > type size hasn't changed > 1 enumerator insertion: > 'ieee_attrs::DCB_ATTR_DCB_REWR_TABLE' value '12' > 1 enumerator change: > 'ieee_attrs::__DCB_ATTR_IEEE_MAX' from value '12' to '13' at > dcbnl.h:416:1 > > It's quite common in the kernel to have the last variant in your enum > be *_MAX, then define the max value of the enum based on that variant. > > However, just adding a variant to this enum doesn't break userspace > (unless you have a program that does something silly like this): > > if (DCB_ATTR_IEEE_MAX > 12) { > // crash the program > } > > Assuming that programs will use the MAX value in a reasonable way, we > can consider this change safe for userspace backwards-compatibility if > it meets the following criteria: > > 1. No variants are removed/renamed > 2. No variants have a changed value (unless it's the last one) I think feature this would not be difficult to add and would indeed be a useful feature for others, I believe. Would it be okay if you'd have to carry a suppression specification file that would have: $ cat filter-enums.suppr [suppress_type] type_kind = enum has_last_enumerator_change_only = true Then you'd invoke the tool by doing: $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2 To get an idea of what suppression specifications do, you can read more at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications. Please note that the "has_last_enumerator_change_only" property is not yet implemented. I'd like to know first if this "user interface" would fit your use case. If it does then we can go ahead and add it. [...] > Technically, we could parse the stdout of abidiff to see if these > conditions are met, but that'd be quite complicated and fragile. It > seems a better choice to detect them when the data structures are > available. Indeed, that is why I am proposing to extend the suppression specifications we already have to handle this kind of cases. > Do you have any ideas on how these condition could be detected from > abidiff? We could spend some time to implement the logic, but I'm not > sure if this kind of specific use case is within the scope of your project. Suppression specifications are precisely designed to handle this kind of needs, and yes, that is in the scope of the project. They are added on a case by case basis as projects like yours raise the need for it. > If so, could something be added like "--ignore-enum-expansion" (or > some other flag)? I think the suppression specification route would be better as the combination of cases to "filter out" depending on each use case is probably too big to handle via command line switches. > There are other similar "false positives" discussed in the LKML > thread, but this first one is the largest and probably the easiest to > detect algorithmically, so I figure it's a good starting point. Right, I'd be glad to be made aware about those and help tackle them. > Thank you in advance for any help! Thank you for considering abidiff and I hope we'll be useful. Cheers,
Hello, Dodji Seketeli <dodji@seketeli.org> a écrit: > > I think feature this would not be difficult to add and would indeed be a > useful feature for others, I believe. > > Would it be okay if you'd have to carry a suppression specification > file that would have: > > $ cat filter-enums.suppr > > [suppress_type] > type_kind = enum > has_last_enumerator_change_only = true > > > Then you'd invoke the tool by doing: > > $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2 > > To get an idea of what suppression specifications do, you can read more > at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications. > > Please note that the "has_last_enumerator_change_only" property is not > yet implemented. I'd like to know first if this "user interface" would > fit your use case. If it does then we can go ahead and add it. Actually, I went ahead and played a little with this idea, in case you'd be interested, so I have a patched libabigail tree with the above implemented. The actual property to set in the suppression specification file is "allow_last_enumerator_change_only". Here is an example of what it does: $ cat -n test0-v0.c 1 enum enum_type 2 { 3 E0, 4 E1, 5 E_LAST 6 }; 7 8 struct some_type 9 { 10 enum enum_type m; 11 }; 12 13 void 14 foo(struct some_type* s __attribute__((unused))) 15 { 16 } $ cat -n test0-v1.c 1 enum enum_type 2 { 3 E0, 4 E1, 5 E2, 6 E_LAST 7 }; 8 9 struct some_type 10 { 11 enum enum_type m; 12 }; 13 14 void 15 foo(struct some_type* s __attribute__((unused))) 16 { 17 } $ $ ./build/tools/abidiff test0-v0.o test0-v1.o || echo "exit code: $?" Functions changes summary: 0 Removed, 1 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 1 function with some indirect sub-type change: [C] 'function void foo(some_type*)' at test0-v0.c:14:1 has some indirect sub-type changes: parameter 1 of type 'some_type*' has sub-type changes: in pointed to type 'struct some_type' at test0-v1.c:9:1: type size hasn't changed 1 data member change: type of 'enum_type m' changed: type size hasn't changed 1 enumerator insertion: 'enum_type::E2' value '2' 1 enumerator change: 'enum_type::E_LAST' from value '2' to '3' at test0-v1.c:1:1 exit code: 4 $ cat last-enum-change-only.suppr [suppress_type] type_kind = enum allow_last_enumerator_change_only = yes $ $ /home/dodji/git/libabigail/filter-last-enumerator-change/build/tools/abidiff --suppr ./last-enum-change-only.suppr test0-v0.o test0-v1.o && echo "exit code: $?" Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable exit code: 0 $ The git tree that you can check you, compile and test at your convenience is https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change. The patch in question is this one: https://sourceware.org/git/?p=libabigail.git;a=commit;h=239ecaa7dc5de9e9a37650248aa00b5cfc2e578e The git command to use would be: $ git clone -b 'users/dodji/filter-last-enumerator-change' git://sourceware.org/git/libabigail.git Once you've got the necessary dependencies for your system, I guess compiling the tree would take: $ autoreconf $ configure --prefix=</your/prefix> $ make -j<amount-of-cpu-cores> $ make install Then play with /your/prefix/usr/bin/abidiff. [...] I hope this helps to at least start a productive discussion. Cheers,
On 4/21/2023 1:03 PM, Dodji Seketeli wrote: > Hello, > > Dodji Seketeli <dodji@seketeli.org> a écrit: > >> >> I think feature this would not be difficult to add and would indeed be a >> useful feature for others, I believe. >> >> Would it be okay if you'd have to carry a suppression specification >> file that would have: >> >> $ cat filter-enums.suppr >> >> [suppress_type] >> type_kind = enum >> has_last_enumerator_change_only = true >> >> >> Then you'd invoke the tool by doing: >> >> $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2 >> >> To get an idea of what suppression specifications do, you can read more >> at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications. >> >> Please note that the "has_last_enumerator_change_only" property is not >> yet implemented. I'd like to know first if this "user interface" would >> fit your use case. If it does then we can go ahead and add it. > > Actually, I went ahead and played a little with this idea, in case you'd > be interested, so I have a patched libabigail tree with the above > implemented. The actual property to set in the suppression > specification file is "allow_last_enumerator_change_only". > > Here is an example of what it does: > > $ cat -n test0-v0.c > 1 enum enum_type > 2 { > 3 E0, > 4 E1, > 5 E_LAST > 6 }; > 7 > 8 struct some_type > 9 { > 10 enum enum_type m; > 11 }; > 12 > 13 void > 14 foo(struct some_type* s __attribute__((unused))) > 15 { > 16 } > $ cat -n test0-v1.c > 1 enum enum_type > 2 { > 3 E0, > 4 E1, > 5 E2, > 6 E_LAST > 7 }; > 8 > 9 struct some_type > 10 { > 11 enum enum_type m; > 12 }; > 13 > 14 void > 15 foo(struct some_type* s __attribute__((unused))) > 16 { > 17 } > $ > > $ ./build/tools/abidiff test0-v0.o test0-v1.o || echo "exit code: $?" > Functions changes summary: 0 Removed, 1 Changed, 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > 1 function with some indirect sub-type change: > > [C] 'function void foo(some_type*)' at test0-v0.c:14:1 has some indirect sub-type changes: > parameter 1 of type 'some_type*' has sub-type changes: > in pointed to type 'struct some_type' at test0-v1.c:9:1: > type size hasn't changed > 1 data member change: > type of 'enum_type m' changed: > type size hasn't changed > 1 enumerator insertion: > 'enum_type::E2' value '2' > 1 enumerator change: > 'enum_type::E_LAST' from value '2' to '3' at test0-v1.c:1:1 > > exit code: 4 > $ cat last-enum-change-only.suppr > [suppress_type] > type_kind = enum > allow_last_enumerator_change_only = yes > $ > $ /home/dodji/git/libabigail/filter-last-enumerator-change/build/tools/abidiff --suppr ./last-enum-change-only.suppr test0-v0.o test0-v1.o && echo "exit code: $?" > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > exit code: 0 > $ > > > The git tree that you can check you, compile and test at your > convenience is https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change. > > The patch in question is this one: https://sourceware.org/git/?p=libabigail.git;a=commit;h=239ecaa7dc5de9e9a37650248aa00b5cfc2e578e > > The git command to use would be: > > $ git clone -b 'users/dodji/filter-last-enumerator-change' git://sourceware.org/git/libabigail.git > > Once you've got the necessary dependencies for your system, I guess > compiling the tree would take: > > $ autoreconf > $ configure --prefix=</your/prefix> > $ make -j<amount-of-cpu-cores> > $ make install > > Then play with /your/prefix/usr/bin/abidiff. > > [...] > > I hope this helps to at least start a productive discussion. > > Cheers, > Hi Dodji, Thanks so much for getting the ball rolling on this! I agree that suppressions seem to be the correct mechanism here and I don't foresee any issue using them from the check script we're working on. Before seeing this, I had gotten started on implementing some (hacky) filters based on the output of abidiff. While testing on the Linux codebase, I found some interesting corner cases. I'll go into the details here so hopefully we can have more discussion and design the right solution. There are basically three classes of "false positives" that we'd like to eventually be able to filter: 1) Enum expansion 2) Padding field expansion 3) Flex array expansion === Enum expansion === Here are some places in the kernel where the script reported expansion as a breaking change: One "MAX" member: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789 Two "MAX" members: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398 Three "MAX" members: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311 Having two is pretty common. I can only find the one occurrence of having three of these variants... maybe the suppression could take an "n_allowed" parameter of some sort? Also, would it make sense to limit the check to enumerator variants that have somewhat common names for that case? It could evolve with time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being fairly common. Maybe the suppression could also take "allowed_strings"? Something like this? [suppress_type] type_kind = enum allow_n_last_enumerator_changes = 3 allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", ".*NUM.*", ".*NBITS.*"] This is the algorithm I was working on to detect these changes: - The type size did not change - The change is to an enum - The number of changed variants is < 4 - The changed variants contain at least one of the above strings - The changed variants' values are all >= the last newly-inserted value === Padding field expansion === For example, you'll see lots of padding fields like this: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908 These don't necessarily have to occur at the end of a struct. You could expand into a padding field in the middle somewhere. This is the algorithm I was working on to detect these changes: - The type size did not change - There is only one data member being changed - The changed member name contains at least one of: "pad", "reserved", "end", or "last" - The first new member being inserted has an offset equal to the changed member's previous offset === Flex array expansion === For example: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149. Leaving a flex array at the end essentially gives the struct an "infinite" padding field. The algorithm to detect looks almost identical to the ordinary padding expansion except this class can only occur with the flex array at the end of the struct. === Anonymous Enums === Another issue that comes up when comparing ABI across wide swaths of kernel commit history are changes to anonymous enums. From what I can tell, there's not a great way to handle this. If someone adds a new anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) can change, so abidiff reports a new anonymous enum with all the same fields even though it was basically just a "rename". For reference, this file is full of anonymous enums: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. Basically any change in there is triggering a failure from the script. I'm not sure how to handle these changes... there are possible ABI breakages when changing anonymous enums, but it's unclear to me how to detect them. Again, thanks so much for taking the initiative here. Let me know what you think about the above topics and what next steps we could take! - John
Hello John, [...] > Thanks so much for getting the ball rolling on this! You are welcome! > I agree that suppressions seem to be the correct mechanism here and > I don't foresee any issue using them from the check script we're > working on. Nice to hear :-) > Before seeing this, I had gotten started on implementing some (hacky) > filters based on the output of abidiff. While testing on the Linux > codebase, I found some interesting corner cases. I'll go into the > details here so hopefully we can have more discussion and design the > right solution. Thank you. > There are basically three classes of "false positives" that we'd like > to eventually be able to filter: > > 1) Enum expansion > 2) Padding field expansion > 3) Flex array expansion ACK. > === Enum expansion === > Here are some places in the kernel where the script reported expansion > as a breaking change: > > One "MAX" member: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789 > > Two "MAX" members: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398 > > Three "MAX" members: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311 > > Having two is pretty common. I can only find the one occurrence of > having three of these variants... maybe the suppression could take an > "n_allowed" parameter of some sort? I guess this can certainly be done, yes. And it does make sense to me. > Also, would it make sense to limit the check to enumerator variants > that have somewhat common names for that case? It could evolve with > time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being > fairly common. Maybe the suppression could also take > "allowed_strings"? Something like this? Just so you know, there is already the "changed_enumerators" property that is available[1]. It takes the list of enumerators that are allowed to change. It doesn't support regular expressions, yet. Maybe we can either make it take enumerators or add a new one named "changed_enumerators_regexp" which takes a list of regular expressions describing the allowed changed enumerators. In any case, that property will have to be made to work in tandem with the allow_n_last_enumarator_changes property. [1]: https://sourceware.org/libabigail/manual/libabigail-concepts. [Look for the "changed_enumerator" word in there] > [suppress_type] > type_kind = enum > allow_n_last_enumerator_changes = 3 > allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", ".*NUM.*", ".*NBITS.*"] Right. Or, rather, the syntax would be: [suppress_type] type_kind = enum allow_n_last_enumerator_changes = 3 changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.* (no need for the string delimiters as everything is considered a string by default) Would that be OK? > This is the algorithm I was working on to detect these changes: > - The type size did not change > - The change is to an enum > - The number of changed variants is < 4 > - The changed variants contain at least one of the above strings > - The changed variants' values are all >= the last newly-inserted value ACK. > === Padding field expansion === > For example, you'll see lots of padding fields like this: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908 > > These don't necessarily have to occur at the end of a struct. You > could expand into a padding field in the middle somewhere. > > This is the algorithm I was working on to detect these changes: > - The type size did not change > - There is only one data member being changed > - The changed member name contains at least one of: "pad", > "reserved", "end", or "last" > - The first new member being inserted has an offset equal to the > changed member's previous offset Just so you know, there are a number of properties of the [suppress_type] section that are designed to address this kind of suppressions. I think some can be extended to address this use case. E.g, there is the has_data_member_inserted_at property which I believe can help with this use case. Here is a quick screen shot to show you what it does so far, in the latest 2.3 libabigail version that got recently released: $ cat -n test-v0.cc 1 #include <cstdint> 2 3 struct S 4 { 5 int a; 6 char padding[64]; 7 int b; 8 }; 9 10 void 11 foo(S&) 12 {} $ cat -n test-v1.cc 1 #include <cstdint> 2 3 struct S 4 { 5 int a; 6 uint32_t added; 7 char padding[60]; 8 int b; 9 }; 10 11 void 12 foo(S&) 13 {} $ diff -u test-v0.cc test-v1.cc --- test-v0.cc 2023-05-10 13:51:19.798072787 +0200 +++ test-v1.cc 2023-05-10 13:54:53.879082831 +0200 @@ -3,7 +3,8 @@ struct S { int a; - char padding[64]; + uint32_t added; + char padding[60]; int b; }; $ gcc -g -c test-v{0,1}.cc $ abidiff test-v0.o test-v1.o Functions changes summary: 0 Removed, 1 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 1 function with some indirect sub-type change: [C] 'function void foo(S&)' at test-v0.cc:11:1 has some indirect sub-type changes: parameter 1 of type 'S&' has sub-type changes: in referenced type 'struct S' at test-v1.cc:3:1: type size hasn't changed 1 data member insertion: 'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1 1 data member change: type of 'char padding[64]' changed: type name changed from 'char[64]' to 'char[60]' array type size changed from 512 to 480 array type subrange 1 changed length from 64 to 60 and offset changed from 32 to 64 (in bits) (by +32 bits) $ cat padding.suppr [suppress_type] type_kind = struct has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding) $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr padding.suppr test-v0.o test-v1.o Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable $ > > > === Flex array expansion === > For example: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149. > > Leaving a flex array at the end essentially gives the struct an > "infinite" padding field. The algorithm to detect looks almost > identical to the ordinary padding expansion except this class can only > occur with the flex array at the end of the struct. I am not sure to understand this case in light of what we have been talking about earlier. If a data member is inserted anywhere around the flex array field, I would have thought that it should be flagged as being a meaningful change to report. What am I missing? > === Anonymous Enums === > Another issue that comes up when comparing ABI across wide swaths of > kernel commit history are changes to anonymous enums. From what I can > tell, there's not a great way to handle this. If someone adds a new > anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) > can change, so abidiff reports a new anonymous enum with all the same > fields even though it was basically just a "rename". > > For reference, this file is full of anonymous enums: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. > Basically any change in there is triggering a failure from the > script. By 'any change', do you mean any addition of enumerator? I think we can do something similar to what we do for anonymous structs/unions, which it to consider content of the enums, rather than just their "made-up" names. I guess we need to have well defined examples of changes we want to handle and I think we can come to a solution. > I'm not sure how to handle these changes... there are possible ABI > breakages when changing anonymous enums, but it's unclear to me how to > detect them. Let's come up with some small examples first, I'd say. > Again, thanks so much for taking the initiative here. Let me know what > you think about the above topics and what next steps we could take! No problem, my pleasure. Cheers,
On 5/10/2023 7:21 AM, Dodji Seketeli wrote: >> === Enum expansion === >> Here are some places in the kernel where the script reported expansion >> as a breaking change: >> >> One "MAX" member: >> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789 >> >> Two "MAX" members: >> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398 >> >> Three "MAX" members: >> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311 >> >> Having two is pretty common. I can only find the one occurrence of >> having three of these variants... maybe the suppression could take an >> "n_allowed" parameter of some sort? > > I guess this can certainly be done, yes. And it does make sense to > me. > >> Also, would it make sense to limit the check to enumerator variants >> that have somewhat common names for that case? It could evolve with >> time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being >> fairly common. Maybe the suppression could also take >> "allowed_strings"? Something like this? > > Just so you know, there is already the "changed_enumerators" property > that is available[1]. It takes the list of enumerators that are allowed > to change. It doesn't support regular expressions, yet. Maybe we can > either make it take enumerators or add a new one named > "changed_enumerators_regexp" which takes a list of regular expressions > describing the allowed changed enumerators. In any case, that > property will have to be made to work in tandem with the > allow_n_last_enumarator_changes property. > > [1]: https://sourceware.org/libabigail/manual/libabigail-concepts. > [Look for the "changed_enumerator" word in there] > > >> [suppress_type] >> type_kind = enum >> allow_n_last_enumerator_changes = 3 >> allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", ".*NUM.*", ".*NBITS.*"] > > Right. Or, rather, the syntax would be: > > [suppress_type] > type_kind = enum > allow_n_last_enumerator_changes = 3 > changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.* > > (no need for the string delimiters as everything is considered a > string by default) > > Would that be OK? Yes, this looks perfect! >> === Padding field expansion === >> For example, you'll see lots of padding fields like this: >> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908 >> >> These don't necessarily have to occur at the end of a struct. You >> could expand into a padding field in the middle somewhere. >> >> This is the algorithm I was working on to detect these changes: >> - The type size did not change >> - There is only one data member being changed >> - The changed member name contains at least one of: "pad", >> "reserved", "end", or "last" >> - The first new member being inserted has an offset equal to the >> changed member's previous offset > > Just so you know, there are a number of properties of the > [suppress_type] section that are designed to address this kind of > suppressions. I think some can be extended to address this use case. > > E.g, there is the has_data_member_inserted_at property which I believe > can help with this use case. Here is a quick screen shot to show you > what it does so far, in the latest 2.3 libabigail version that got > recently released: > > $ cat -n test-v0.cc > 1 #include <cstdint> > 2 > 3 struct S > 4 { > 5 int a; > 6 char padding[64]; > 7 int b; > 8 }; > 9 > 10 void > 11 foo(S&) > 12 {} > > $ cat -n test-v1.cc > 1 #include <cstdint> > 2 > 3 struct S > 4 { > 5 int a; > 6 uint32_t added; > 7 char padding[60]; > 8 int b; > 9 }; > 10 > 11 void > 12 foo(S&) > 13 {} > > $ diff -u test-v0.cc test-v1.cc > --- test-v0.cc 2023-05-10 13:51:19.798072787 +0200 > +++ test-v1.cc 2023-05-10 13:54:53.879082831 +0200 > @@ -3,7 +3,8 @@ > struct S > { > int a; > - char padding[64]; > + uint32_t added; > + char padding[60]; > int b; > }; > > $ gcc -g -c test-v{0,1}.cc > $ abidiff test-v0.o test-v1.o > Functions changes summary: 0 Removed, 1 Changed, 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > 1 function with some indirect sub-type change: > > [C] 'function void foo(S&)' at test-v0.cc:11:1 has some indirect sub-type changes: > parameter 1 of type 'S&' has sub-type changes: > in referenced type 'struct S' at test-v1.cc:3:1: > type size hasn't changed > 1 data member insertion: > 'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1 > 1 data member change: > type of 'char padding[64]' changed: > type name changed from 'char[64]' to 'char[60]' > array type size changed from 512 to 480 > array type subrange 1 changed length from 64 to 60 > and offset changed from 32 to 64 (in bits) (by +32 bits) > > $ cat padding.suppr > [suppress_type] > type_kind = struct > has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding) > > $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr padding.suppr test-v0.o test-v1.o > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > $ > This looks very close! Could we just repeat the suppression for each of the member names we're interested in ignoring or would we need to add support for the "offset_of_first_data_member_regexp()" function to take in a list of regexp? >> >> >> === Flex array expansion === >> For example: >> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149. >> >> Leaving a flex array at the end essentially gives the struct an >> "infinite" padding field. The algorithm to detect looks almost >> identical to the ordinary padding expansion except this class can only >> occur with the flex array at the end of the struct. > > I am not sure to understand this case in light of what we have been > talking about earlier. If a data member is inserted anywhere around > the flex array field, I would have thought that it should be flagged > as being a meaningful change to report. What am I missing? Flex arrays can only be at the end of a struct, so I guess the rule should be "you can add members between existing members and the ending flex array". E.g. this should be fine: struct foo { __u32 a; __u32 b; + __u32 c; char pad[]; }; But not this: struct foo { __u32 a; + __u32 c; __u32 b; char pad[]; }; >> === Anonymous Enums === >> Another issue that comes up when comparing ABI across wide swaths of >> kernel commit history are changes to anonymous enums. From what I can >> tell, there's not a great way to handle this. If someone adds a new >> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) >> can change, so abidiff reports a new anonymous enum with all the same >> fields even though it was basically just a "rename". >> >> For reference, this file is full of anonymous enums: >> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. >> Basically any change in there is triggering a failure from the >> script. > > By 'any change', do you mean any addition of enumerator? > > I think we can do something similar to what we do for anonymous > structs/unions, which it to consider content of the enums, rather than > just their "made-up" names. > > I guess we need to have well defined examples of changes we want to > handle and I think we can come to a solution. > >> I'm not sure how to handle these changes... there are possible ABI >> breakages when changing anonymous enums, but it's unclear to me how to >> detect them. > > Let's come up with some small examples first, I'd say. Here's one example of the issue: https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d (specifically the change to include/uapi/rdma/hns-abi.h). Here, a variant was added to two anonymous enums: enum { HNS_ROCE_EXSGE_FLAGS = 1 << 0, HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1, + HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2, }; enum { HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0, HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1, + HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2, }; abidiff outputs: [C] 'enum __anonymous_enum__1' changed: type size hasn't changed 1 enumerator deletion: '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1' 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 It seems like it mixed them up while processing? I'm not exactly sure what's going on internally. Another example: https://github.com/w1ldptr/linux/commit/d219df60a70ed0739aa5dd34b477763311fc5a7b (specifically the change to include/uapi/linux/bpf.h). Here, just one enum was modified: enum { BPF_F_ADJ_ROOM_FIXED_GSO = (1ULL << 0), BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 = (1ULL << 1), BPF_F_ADJ_ROOM_ENCAP_L3_IPV6 = (1ULL << 2), BPF_F_ADJ_ROOM_ENCAP_L4_GRE = (1ULL << 3), BPF_F_ADJ_ROOM_ENCAP_L4_UDP = (1ULL << 4), BPF_F_ADJ_ROOM_NO_CSUM_RESET = (1ULL << 5), BPF_F_ADJ_ROOM_ENCAP_L2_ETH = (1ULL << 6), + BPF_F_ADJ_ROOM_DECAP_L3_IPV4 = (1ULL << 7), + BPF_F_ADJ_ROOM_DECAP_L3_IPV6 = (1ULL << 8), }; But abidiff outputs: [C] 'enum __anonymous_enum__9' changed: type size hasn't changed 3 enumerator deletions: '__anonymous_enum__9::BPF_F_ZERO_CSUM_TX' value '2' '__anonymous_enum__9::BPF_F_DONT_FRAGMENT' value '4' '__anonymous_enum__9::BPF_F_SEQ_NUMBER' value '8' 9 enumerator insertions: '__anonymous_enum__14::BPF_F_ADJ_ROOM_FIXED_GSO' value '1' '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV4' value '2' '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV6' value '4' '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_GRE' value '8' '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_UDP' value '16' '__anonymous_enum__14::BPF_F_ADJ_ROOM_NO_CSUM_RESET' value '32' '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L2_ETH' value '64' '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV4' value '128' '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV6' value '256' 3 added types unreachable from any public interface: [A] 'enum __anonymous_enum__9' at bpf.h:5781:1 [A] 'struct bpf_rb_node' at bpf.h:6931:1 [A] 'struct bpf_rb_root' at bpf.h:6926:1 Any ideas on how to handle these issues? Thank you! - John
On 5/23/2023 12:59 PM, John Moon wrote: > On 5/10/2023 7:21 AM, Dodji Seketeli wrote: >>> === Enum expansion === >>> Here are some places in the kernel where the script reported expansion >>> as a breaking change: >>> >>> One "MAX" member: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789 >>> >>> Two "MAX" members: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398 >>> >>> Three "MAX" members: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311 >>> >>> Having two is pretty common. I can only find the one occurrence of >>> having three of these variants... maybe the suppression could take an >>> "n_allowed" parameter of some sort? >> >> I guess this can certainly be done, yes. And it does make sense to >> me. >> >>> Also, would it make sense to limit the check to enumerator variants >>> that have somewhat common names for that case? It could evolve with >>> time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being >>> fairly common. Maybe the suppression could also take >>> "allowed_strings"? Something like this? >> >> Just so you know, there is already the "changed_enumerators" property >> that is available[1]. It takes the list of enumerators that are allowed >> to change. It doesn't support regular expressions, yet. Maybe we can >> either make it take enumerators or add a new one named >> "changed_enumerators_regexp" which takes a list of regular expressions >> describing the allowed changed enumerators. In any case, that >> property will have to be made to work in tandem with the >> allow_n_last_enumarator_changes property. >> >> [1]: https://sourceware.org/libabigail/manual/libabigail-concepts. >> [Look for the "changed_enumerator" word in there] >> >> >>> [suppress_type] >>> type_kind = enum >>> allow_n_last_enumerator_changes = 3 >>> allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", >>> ".*NUM.*", ".*NBITS.*"] >> >> Right. Or, rather, the syntax would be: >> >> [suppress_type] >> type_kind = enum >> allow_n_last_enumerator_changes = 3 >> changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.* >> >> (no need for the string delimiters as everything is considered a >> string by default) >> >> Would that be OK? > > Yes, this looks perfect! > >>> === Padding field expansion === >>> For example, you'll see lots of padding fields like this: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908 >>> >>> These don't necessarily have to occur at the end of a struct. You >>> could expand into a padding field in the middle somewhere. >>> >>> This is the algorithm I was working on to detect these changes: >>> - The type size did not change >>> - There is only one data member being changed >>> - The changed member name contains at least one of: "pad", >>> "reserved", "end", or "last" >>> - The first new member being inserted has an offset equal to the >>> changed member's previous offset >> >> Just so you know, there are a number of properties of the >> [suppress_type] section that are designed to address this kind of >> suppressions. I think some can be extended to address this use case. >> >> E.g, there is the has_data_member_inserted_at property which I believe >> can help with this use case. Here is a quick screen shot to show you >> what it does so far, in the latest 2.3 libabigail version that got >> recently released: >> >> $ cat -n test-v0.cc >> 1 #include <cstdint> >> 2 >> 3 struct S >> 4 { >> 5 int a; >> 6 char padding[64]; >> 7 int b; >> 8 }; >> 9 >> 10 void >> 11 foo(S&) >> 12 {} >> >> $ cat -n test-v1.cc >> 1 #include <cstdint> >> 2 >> 3 struct S >> 4 { >> 5 int a; >> 6 uint32_t added; >> 7 char padding[60]; >> 8 int b; >> 9 }; >> 10 >> 11 void >> 12 foo(S&) >> 13 {} >> >> $ diff -u test-v0.cc test-v1.cc >> --- test-v0.cc 2023-05-10 13:51:19.798072787 +0200 >> +++ test-v1.cc 2023-05-10 13:54:53.879082831 +0200 >> @@ -3,7 +3,8 @@ >> struct S >> { >> int a; >> - char padding[64]; >> + uint32_t added; >> + char padding[60]; >> int b; >> }; >> >> $ gcc -g -c test-v{0,1}.cc >> $ abidiff test-v0.o test-v1.o >> Functions changes summary: 0 Removed, 1 Changed, 0 Added function >> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable >> >> 1 function with some indirect sub-type change: >> >> [C] 'function void foo(S&)' at test-v0.cc:11:1 has some >> indirect sub-type changes: >> parameter 1 of type 'S&' has sub-type changes: >> in referenced type 'struct S' at test-v1.cc:3:1: >> type size hasn't changed >> 1 data member insertion: >> 'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1 >> 1 data member change: >> type of 'char padding[64]' changed: >> type name changed from 'char[64]' to 'char[60]' >> array type size changed from 512 to 480 >> array type subrange 1 changed length from 64 to 60 >> and offset changed from 32 to 64 (in bits) (by +32 bits) >> >> $ cat padding.suppr >> [suppress_type] >> type_kind = struct >> has_data_member_inserted_at = >> offset_of_first_data_member_regexp(.*padding) >> >> $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr >> padding.suppr test-v0.o test-v1.o >> Functions changes summary: 0 Removed, 0 Changed (1 filtered out), >> 0 Added function >> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable >> >> $ >> > > This looks very close! Could we just repeat the suppression for each of > the member names we're interested in ignoring or would we need to add > support for the "offset_of_first_data_member_regexp()" function to take > in a list of regexp? > >>> >>> >>> === Flex array expansion === >>> For example: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149. >>> >>> Leaving a flex array at the end essentially gives the struct an >>> "infinite" padding field. The algorithm to detect looks almost >>> identical to the ordinary padding expansion except this class can only >>> occur with the flex array at the end of the struct. >> >> I am not sure to understand this case in light of what we have been >> talking about earlier. If a data member is inserted anywhere around >> the flex array field, I would have thought that it should be flagged >> as being a meaningful change to report. What am I missing? > > Flex arrays can only be at the end of a struct, so I guess the rule > should be "you can add members between existing members and the ending > flex array". > > E.g. this should be fine: > > struct foo { > __u32 a; > __u32 b; > + __u32 c; > char pad[]; > }; > > But not this: > > struct foo { > __u32 a; > + __u32 c; > __u32 b; > char pad[]; > }; > > >>> === Anonymous Enums === >>> Another issue that comes up when comparing ABI across wide swaths of >>> kernel commit history are changes to anonymous enums. From what I can >>> tell, there's not a great way to handle this. If someone adds a new >>> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) >>> can change, so abidiff reports a new anonymous enum with all the same >>> fields even though it was basically just a "rename". >>> >>> For reference, this file is full of anonymous enums: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. >>> Basically any change in there is triggering a failure from the >>> script. >> >> By 'any change', do you mean any addition of enumerator? >> >> I think we can do something similar to what we do for anonymous >> structs/unions, which it to consider content of the enums, rather than >> just their "made-up" names. >> >> I guess we need to have well defined examples of changes we want to >> handle and I think we can come to a solution. >> >>> I'm not sure how to handle these changes... there are possible ABI >>> breakages when changing anonymous enums, but it's unclear to me how to >>> detect them. >> >> Let's come up with some small examples first, I'd say. > > Here's one example of the issue: > https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d (specifically the change to include/uapi/rdma/hns-abi.h). > > Here, a variant was added to two anonymous enums: > > enum { > HNS_ROCE_EXSGE_FLAGS = 1 << 0, > HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1, > + HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2, > }; > > enum { > HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0, > HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1, > + HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2, > }; > > abidiff outputs: > > [C] 'enum __anonymous_enum__1' changed: > type size hasn't changed > 1 enumerator deletion: > '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1' > 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 > > It seems like it mixed them up while processing? I'm not exactly sure > what's going on internally. > > Another example: > https://github.com/w1ldptr/linux/commit/d219df60a70ed0739aa5dd34b477763311fc5a7b (specifically the change to include/uapi/linux/bpf.h). > > Here, just one enum was modified: > > enum { > BPF_F_ADJ_ROOM_FIXED_GSO = (1ULL << 0), > BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 = (1ULL << 1), > BPF_F_ADJ_ROOM_ENCAP_L3_IPV6 = (1ULL << 2), > BPF_F_ADJ_ROOM_ENCAP_L4_GRE = (1ULL << 3), > BPF_F_ADJ_ROOM_ENCAP_L4_UDP = (1ULL << 4), > BPF_F_ADJ_ROOM_NO_CSUM_RESET = (1ULL << 5), > BPF_F_ADJ_ROOM_ENCAP_L2_ETH = (1ULL << 6), > + BPF_F_ADJ_ROOM_DECAP_L3_IPV4 = (1ULL << 7), > + BPF_F_ADJ_ROOM_DECAP_L3_IPV6 = (1ULL << 8), > }; > > > But abidiff outputs: > > [C] 'enum __anonymous_enum__9' changed: > type size hasn't changed > 3 enumerator deletions: > '__anonymous_enum__9::BPF_F_ZERO_CSUM_TX' value '2' > '__anonymous_enum__9::BPF_F_DONT_FRAGMENT' value '4' > '__anonymous_enum__9::BPF_F_SEQ_NUMBER' value '8' > 9 enumerator insertions: > '__anonymous_enum__14::BPF_F_ADJ_ROOM_FIXED_GSO' value '1' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV4' value '2' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV6' value '4' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_GRE' value '8' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_UDP' value '16' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_NO_CSUM_RESET' value '32' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L2_ETH' value '64' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV4' value '128' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV6' value '256' > 3 added types unreachable from any public interface: > [A] 'enum __anonymous_enum__9' at bpf.h:5781:1 > [A] 'struct bpf_rb_node' at bpf.h:6931:1 > [A] 'struct bpf_rb_root' at bpf.h:6926:1 > > Any ideas on how to handle these issues? > > Thank you! > > - John > Hi Dodji, I wanted to follow up with you and see if/how we can assist with these features. I'm happy to jump in and try implementation if we agree on the feature requirements, though I'm not very familiar with the codebase. To summarize, here are the four usecases we have so far and plans to address them: 1. Enum expansion - Add support for this kind of suppression: [suppress_type] type_kind = enum allow_n_last_enumerator_changes = 3 changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.* 2. Padding field expansion - Current feature set is close, but how can we handle multiple names? - Do we want support for this kind suppression (multiple regex's)? [suppress_type] type_kind = struct has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding, .*reserved, .*unused) 3. Flex array expansion - Not sure how to detect/suppress this condition - Flex arrays can only be at the ends of structs, so we just need to make sure new struct members come "second to last" 4. Anonymous enums - Not sure how to associate these enums when diffing since their "made up" names don't always match before/after a patch. I can try tackling items 1 and 2 if you'd like - just let me know! For items 3 and 4, can we work up a path forward? Thanks so much! - John
Hello John, Sorry for the delayed reply. [...] > On 5/10/2023 7:21 AM, Dodji Seketeli wrote: [...] >>> === Enum expansion === [...] >> the syntax would be: >> [suppress_type] >> type_kind = enum >> allow_n_last_enumerator_changes = 3 >> changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.* >> (no need for the string delimiters as everything is considered a >> string by default) >> Would that be OK? John Moon wrote: > > Yes, this looks perfect! > OK, deal. >>> === Padding field expansion === [...] >> $ cat -n test-v0.cc >> 1 #include <cstdint> >> 2 >> 3 struct S >> 4 { >> 5 int a; >> 6 char padding[64]; >> 7 int b; >> 8 }; >> 9 >> 10 void >> 11 foo(S&) >> 12 {} >> $ cat -n test-v1.cc >> 1 #include <cstdint> >> 2 >> 3 struct S >> 4 { >> 5 int a; >> 6 uint32_t added; >> 7 char padding[60]; >> 8 int b; >> 9 }; >> 10 >> 11 void >> 12 foo(S&) >> 13 {} >> $ diff -u test-v0.cc test-v1.cc >> --- test-v0.cc 2023-05-10 13:51:19.798072787 +0200 >> +++ test-v1.cc 2023-05-10 13:54:53.879082831 +0200 >> @@ -3,7 +3,8 @@ >> struct S >> { >> int a; >> - char padding[64]; >> + uint32_t added; >> + char padding[60]; >> int b; >> }; >> $ gcc -g -c test-v{0,1}.cc >> $ abidiff test-v0.o test-v1.o >> Functions changes summary: 0 Removed, 1 Changed, 0 Added function >> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable >> 1 function with some indirect sub-type change: >> [C] 'function void foo(S&)' at test-v0.cc:11:1 has some >> indirect sub-type changes: >> parameter 1 of type 'S&' has sub-type changes: >> in referenced type 'struct S' at test-v1.cc:3:1: >> type size hasn't changed >> 1 data member insertion: >> 'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1 >> 1 data member change: >> type of 'char padding[64]' changed: >> type name changed from 'char[64]' to 'char[60]' >> array type size changed from 512 to 480 >> array type subrange 1 changed length from 64 to 60 >> and offset changed from 32 to 64 (in bits) (by +32 bits) >> $ cat padding.suppr >> [suppress_type] >> type_kind = struct >> has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding) >> $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr >> padding.suppr test-v0.o test-v1.o >> Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function >> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable >> $ >> John Moon wrote: > > This looks very close! Could we just repeat the suppression for each > of the member names we're interested in ignoring or would we need to > add support for the "offset_of_first_data_member_regexp()" function to > take in a list of regexp? This has been designed so that (I haven't tested it right now) having one [suppress_type] directive per data member insertion we want should work, E.g: [suppress_type] type_kind = struct # Suppress a data member insertion after a data member # ending with the suffix 'padding'. has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding) [suppress_type] type_kind = struct # Suppress a data member insertion after a data member ending # with the suffix 'buffer' has_data_member_inserted_at = offset_of_first_data_member_regexp(.*buffer) John Moon wrote: >>> === Flex array expansion === >>> For example: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149. >>> >>> Leaving a flex array at the end essentially gives the struct an >>> "infinite" padding field. The algorithm to detect looks almost >>> identical to the ordinary padding expansion except this class can only >>> occur with the flex array at the end of the struct. Dodji Seketeli wrote: >> I am not sure to understand this case in light of what we have been >> talking about earlier. If a data member is inserted anywhere around >> the flex array field, I would have thought that it should be flagged >> as being a meaningful change to report. What am I missing? John Moon wrote: > > Flex arrays can only be at the end of a struct, so I guess the rule > should be "you can add members between existing members and the ending > flex array". > > E.g. this should be fine: > > struct foo { > __u32 a; > __u32 b; > + __u32 c; > char pad[]; > }; But in this case, the size of "struct foo" changed and the offset of the data member 'pad' changed too. So I'd flag this as a meaningful ABI change to report in all cases. For instance, if a struct embeds this type, then the size of that struct changes etc. What am I missing? > > But not this: > > struct foo { > __u32 a; > + __u32 c; > __u32 b; > char pad[]; > }; > Yes, this change is meaningful to report in all cases too. >>> === Anonymous Enums === >>> Another issue that comes up when comparing ABI across wide swaths of >>> kernel commit history are changes to anonymous enums. From what I can >>> tell, there's not a great way to handle this. If someone adds a new >>> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) >>> can change, so abidiff reports a new anonymous enum with all the same >>> fields even though it was basically just a "rename". >>> >>> For reference, this file is full of anonymous enums: >>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. >>> Basically any change in there is triggering a failure from the >>> script. >> By 'any change', do you mean any addition of enumerator? >> I think we can do something similar to what we do for anonymous >> structs/unions, which it to consider content of the enums, rather than >> just their "made-up" names. >> I guess we need to have well defined examples of changes we want to >> handle and I think we can come to a solution. >> >>> I'm not sure how to handle these changes... there are possible ABI >>> breakages when changing anonymous enums, but it's unclear to me how to >>> detect them. >> Let's come up with some small examples first, I'd say. > > Here's one example of the issue: > https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d > (specifically the change to include/uapi/rdma/hns-abi.h). > > Here, a variant was added to two anonymous enums: > > enum { > HNS_ROCE_EXSGE_FLAGS = 1 << 0, > HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1, > + HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2, > }; > > enum { > HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0, > HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1, > + HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2, > }; > > abidiff outputs: > > [C] 'enum __anonymous_enum__1' changed: > type size hasn't changed > 1 enumerator deletion: > '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1' > 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 > > It seems like it mixed them up while processing? I'm not exactly sure > what's going on internally. Yes, this is a bug. I need to look into it. > > Another example: > https://github.com/w1ldptr/linux/commit/d219df60a70ed0739aa5dd34b477763311fc5a7b > (specifically the change to include/uapi/linux/bpf.h). > > Here, just one enum was modified: > > enum { > BPF_F_ADJ_ROOM_FIXED_GSO = (1ULL << 0), > BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 = (1ULL << 1), > BPF_F_ADJ_ROOM_ENCAP_L3_IPV6 = (1ULL << 2), > BPF_F_ADJ_ROOM_ENCAP_L4_GRE = (1ULL << 3), > BPF_F_ADJ_ROOM_ENCAP_L4_UDP = (1ULL << 4), > BPF_F_ADJ_ROOM_NO_CSUM_RESET = (1ULL << 5), > BPF_F_ADJ_ROOM_ENCAP_L2_ETH = (1ULL << 6), > + BPF_F_ADJ_ROOM_DECAP_L3_IPV4 = (1ULL << 7), > + BPF_F_ADJ_ROOM_DECAP_L3_IPV6 = (1ULL << 8), > }; > > > But abidiff outputs: > > [C] 'enum __anonymous_enum__9' changed: > type size hasn't changed > 3 enumerator deletions: > '__anonymous_enum__9::BPF_F_ZERO_CSUM_TX' value '2' > '__anonymous_enum__9::BPF_F_DONT_FRAGMENT' value '4' > '__anonymous_enum__9::BPF_F_SEQ_NUMBER' value '8' > 9 enumerator insertions: > '__anonymous_enum__14::BPF_F_ADJ_ROOM_FIXED_GSO' value '1' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV4' value '2' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV6' value '4' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_GRE' value '8' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_UDP' value '16' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_NO_CSUM_RESET' value '32' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L2_ETH' value '64' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV4' value '128' > '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV6' value '256' > 3 added types unreachable from any public interface: > [A] 'enum __anonymous_enum__9' at bpf.h:5781:1 > [A] 'struct bpf_rb_node' at bpf.h:6931:1 > [A] 'struct bpf_rb_root' at bpf.h:6926:1 > > Any ideas on how to handle these issues? These are bugs that ought to be fixed, I'll certainly look into them. > > Thank you! Thank you for this excellent write up. [...] John Moon wrote: > Hi Dodji, > > I wanted to follow up with you and see if/how we can assist with these > features. I'm happy to jump in and try implementation if we agree on > the feature requirements, though I'm not very familiar with the > codebase. > > To summarize, here are the four usecases we have so far and plans to > address them: > > 1. Enum expansion > - Add support for this kind of suppression: > [suppress_type] > type_kind = enum > allow_n_last_enumerator_changes = 3 > changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.* Right. > > 2. Padding field expansion > - Current feature set is close, but how can we handle multiple names? > - Do we want support for this kind suppression (multiple regex's)? > [suppress_type] > type_kind = struct > has_data_member_inserted_at = > offset_of_first_data_member_regexp(.*padding, .*reserved, > .*unused) I think this one can be handled already by writing one type suppression specification per pattern of data member after which insertion is allowed. Would this work for you at least for a start? > 3. Flex array expansion > - Not sure how to detect/suppress this condition > - Flex arrays can only be at the ends of structs, so we just need > to make sure new struct members come "second to last" I am still not sure why we think these can be suppressed; I am pretty sure I am missing something but I am not sure what yet. > 4. Anonymous enums > - Not sure how to associate these enums when diffing since their > "made up" names don't always match before/after a patch. > This is a plain bug that ought to be fixed, I think. > I can try tackling items 1 and 2 if you'd like - just let me know! For > items 3 and 4, can we work up a path forward? First thank you for proposing your help, that would be greatly appreciated :-) So, I have created a branch "kuapi-verify" at https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/kuapi-verify. Its purpose is to contain the code tu support this kernel uapi verification effort, on the libabigail side. I've folded the content of the branch "last-enumerator-change" into it. That branch is at https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change. So the idea is that we can fold the necessary features into "kuapi-verify" as we progress on them, if you like. If you fancy, you can start looking into how to extend the content of the branch https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change to make it support the feature 1. In the mean time, I'll look into fixing 4. Until then, I'll be more clear on 3, I guess. I am available on IRC at irc.oftc.net#libabigail during western European working hours if you need higher bandwidth exchanges. I'd be glad to chat with you there! Thank you for following up! Cheers,
Hello John, John Moon <quic_johmoo@quicinc.com> a écrit: [...] > === Anonymous Enums === > Another issue that comes up when comparing ABI across wide swaths of > kernel commit history are changes to anonymous enums. From what I can > tell, there's not a great way to handle this. If someone adds a new > anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) > can change, so abidiff reports a new anonymous enum with all the same > fields even though it was basically just a "rename". > > For reference, this file is full of anonymous enums: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. > Basically any change in there is triggering a failure from the script. [...] Dodji Seketeli <dodji@seketeli.org> a écrit: [...] > I think we can do something similar to what we do for anonymous > structs/unions, which it to consider content of the enums, rather than > just their "made-up" names. So, I have an initial implementation for handling the comparison of anonymous enums and it's in the 'users/dodji/better-anon-enums' branch. Maybe you can try this branch on your test case to see if you still have issues regarding anonymous enums? If you still do, I'd be very much interested in reproducing the issue locally on my system and extract a smaller reproducer that could end up in the regression test suite of libabigail. I hope this can be useful somehow. [...] Cheers and many thanks!
Hey again, [...] Dodji Seketeli <dodji@seketeli.org> a écrit: > So, I have an initial implementation for handling the comparison of > anonymous enums and it's in the 'users/dodji/better-anon-enums' branch. I forgot to add a link to the branch. It's at https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums. So cloning it is via: $ git clone git://sourceware.org/git/libabigail.git -b users/dodji/better-anon-enums [...] Cheers,
On 9/22/2023 4:51 AM, Dodji Seketeli wrote: > Hey again, > > [...] > > Dodji Seketeli <dodji@seketeli.org> a écrit: > >> So, I have an initial implementation for handling the comparison of >> anonymous enums and it's in the 'users/dodji/better-anon-enums' branch. > > I forgot to add a link to the branch. It's at > https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums. > > So cloning it is via: > > $ git clone git://sourceware.org/git/libabigail.git -b users/dodji/better-anon-enums > > [...] > > Cheers, > Hi Dodji, 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 producing the desired behavior. Let's zoom in on this example: https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d (specifically, the change to include/uapi/rdma/hns-abi.h: enum { HNS_ROCE_EXSGE_FLAGS = 1 << 0, HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1, + HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2, }; enum { HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0, HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1, + HNS_ROCE_RSP_CQE_INLINE_FLAGS = 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=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1 [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:93:1 2 added types unreachable from any public interface: [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=4, HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1 [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=4, HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:94:1 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 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. 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? If not, I think this output is at least easier to post-process! 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 me know what you think! Thanks, John
On 9/22/2023 11:28 AM, John Moon via Libabigail wrote: > If not, I think this output is at least easier to post-process! 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". > Proof of concept to filter out anon enum additions from input like this: 2 removed types unreachable from any public interface: [D] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1 [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:93:1 2 added types unreachable from any public interface: [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=4, HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1 [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=4, HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:94:1 --- #!/bin/bash set -o errexit # Take a line and form a normalized string representation get_repr() { local -r line="$1" echo "$line" | grep -o '{.*}' \ | tr -d '{} ' \ | sed -e 's/,$//g' -e 's/,/\n/g' \ | sort -t= -k2 -n \ | xargs } input="${1:-/dev/stdin}" deleted_anon_enum_reprs=() added_anon_enum_reprs=() while read -r line; do if echo "$line" | grep -q "\[D\] 'enum {"; then deleted_anon_enum_reprs+=("$(get_repr "$line")") fi if echo "$line" | grep -q "\[A\] 'enum {"; then added_anon_enum_reprs+=("$(get_repr "$line")") fi done < "$input" # If a deleted enum's representation appears in an added enum's # representation, assume they're the same enum (just being added # on to). legit_breaks=${#deleted_anon_enum_reprs[@]} for del_repr in "${deleted_anon_enum_reprs[@]}"; do for add_repr in "${added_anon_enum_reprs[@]}"; do if echo "$add_repr" | grep -q "^${del_repr}"; then legit_breaks=$((legit_breaks - 1)) break fi done done echo "$legit_breaks breaks" --- There may be a better way to do this sort of thing in a suppression, but I'm not sure how given they operate on diffs (which wouldn't be created in this case). It feels hacky as the abidiff stdout isn't a stable API itself, but if it sounds reasonable to you, I can bake the above filtering logic into the next rev submitted upstream. Just let me know! Thanks, John
Hello John! John Moon <quic_johmoo@quicinc.com> a écrit: [...] > 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 > 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/1d91855304c2046115ee10be2c93161d93d5d40d > (specifically, the change to include/uapi/rdma/hns-abi.h: > > enum { > HNS_ROCE_EXSGE_FLAGS = 1 << 0, > HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1, > + HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2, > }; > > enum { > HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0, > HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1, > + HNS_ROCE_RSP_CQE_INLINE_FLAGS = 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=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' > at hns-abi.h:88:1 > [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, > HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:93:1 > 2 added types unreachable from any public interface: > [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=4, HNS_ROCE_EXSGE_FLAGS=1, > HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1 > [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=4, > HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at > 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 > 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-change-v1.c --- 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-enums-change-v0.c +/* + * Compile this with: + * gcc -c -g -fno-eliminate-unused-debug-types test-anonymous-enums-change-v1.c */ 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-reachable-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=0, E4_1=1, E4_2=2, E4_LAST_ELEMENT=3, }' 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=0, E5_1=1, }' 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-reachable-types --harmless test-anonymous-enums-change-v0.o test-anonymous-enums-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=0, E4_1=1, E4_2=2, E4_LAST_ELEMENT=3, }' changed: type size hasn't changed 2 enumerator deletions: 'E4_2' value '2' 'E4_LAST_ELEMENT' value '3' [C] 'enum {E3_0=0, E3_1=1, }' 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=0, E5_1=1, }' 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,
On 9/26/2023 1:38 AM, Dodji Seketeli wrote: >> 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 >> 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. > Ahh, understood. >> 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. > Excellent! I'm glad this could be handled in the comparison engine. It seems like it's working perfectly! With the latest abidiff from your branch, my script reports 1d91855304c2 in the kernel as compatible! If I add in the --harmless option just to test, it does report: [C] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' changed: type size hasn't changed 1 enumerator insertion: 'HNS_ROCE_RSP_CQE_INLINE_FLAGS' value '4' [C] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' changed: type size hasn't changed 1 enumerator insertion: 'HNS_ROCE_CQE_INLINE_FLAGS' value '4' This is the exact behavior we needed. For some perspective, from kernel version v6.1 -> v6.2, there were 19 such changes to anon enums. Most of them also include a _MAX variant at the end, but at least once I implement the suppression, it should work on these too: [C] 'enum {MDBE_ATTR_SOURCE=1, MDBE_ATTR_UNSPEC=0, __MDBE_ATTR_MAX=2, }' changed: type size hasn't changed 3 enumerator insertions: 'MDBE_ATTR_SRC_LIST' value '2' 'MDBE_ATTR_GROUP_MODE' value '3' 'MDBE_ATTR_RTPROT' value '4' 1 enumerator change: '__MDBE_ATTR_MAX' from value '2' to '5' at if_bridge.h:723:1 > > >> 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 ;-) > Agreed, thanks for steering us back on course! - John
Hello John, > On 9/26/2023 1:38 AM, Dodji Seketeli wrote: [...] >> 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. >> John Moon <quic_johmoo@quicinc.com> a écrit: > Excellent! I'm glad this could be handled in the comparison engine. It > seems like it's working perfectly! With the latest abidiff from your > branch, my script reports 1d91855304c2 in the kernel as compatible! Whoah! > > If I add in the --harmless option just to test, it does report: > > [C] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, > HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' changed: > type size hasn't changed > 1 enumerator insertion: > 'HNS_ROCE_RSP_CQE_INLINE_FLAGS' value '4' > [C] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' > changed: > type size hasn't changed > 1 enumerator insertion: > 'HNS_ROCE_CQE_INLINE_FLAGS' value '4' > > This is the exact behavior we needed. Good. I have however, noticed a little (subtle) issue with the returned value of abidiff when used with the --harmless option. It only sets the ABIDIFF_ABI_CHANGE bit instead of /also/ setting the ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit, as it does when you don't provide the --harmless. I have thus added this patch to the set in the 'better-anon-enums' branch: https://sourceware.org/git/?p=libabigail.git;a=commit;h=58e93b384aa1fe9a3508d108e7b1af7c7489e680. I think (but I can be wrong) that this should not radically change the result of check-uapi.sh. > For some perspective, from kernel version v6.1 -> v6.2, there were 19 > such changes to anon enums. Most of them also include a _MAX variant > at the end, but at least once I implement the suppression, it should > work on these too: > > [C] 'enum {MDBE_ATTR_SOURCE=1, MDBE_ATTR_UNSPEC=0, > __MDBE_ATTR_MAX=2, }' changed: > type size hasn't changed > 3 enumerator insertions: > 'MDBE_ATTR_SRC_LIST' value '2' > 'MDBE_ATTR_GROUP_MODE' value '3' > 'MDBE_ATTR_RTPROT' value '4' > 1 enumerator change: > '__MDBE_ATTR_MAX' from value '2' to '5' at if_bridge.h:723:1 > Thanks for this perspective. To collect all the patches that we are slowly accumulating to support check-uapi.sh, I have created the branch check-uapi-support at https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/check-uapi-support. It doesn't yet have any patches other than what we have in master. But if you find that a patch is "useful" to achieve the goal of supporting check-uapi.sh, we can merge it in that new branch if you agree. Once we are happy with the status of that branch, we can all review it and consider merging it into the mainline. What do you think about that proposal? [...] > thanks for steering us back on course! My pleasure. But really, thank *you* for the quick feedback. [...] Cheers,
Hello, John Moon <quic_johmoo@quicinc.com> a écrit: [...] > To summarize, here are the four usecases we have so far and plans to > address them: [...] > 3. Flex array expansion > - Not sure how to detect/suppress this condition > - Flex arrays can only be at the ends of structs, so we just need > to make sure new struct members come "second to last" So, I have a patch to support this case. It's in the branch https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/fam-suppr. The commit itself is https://sourceware.org/git/?p=libabigail.git;a=commit;h=e609f46734b07b551c1b3dbc1eeb2cea61c4ba34 Here is what the commit log says: Consider this code example: $ cat test-v0.c 1 struct foo 2 { 3 int member0; 4 char pad[]; /* <-- flexible array member. */ 5 }; 6 7 void 8 foo(struct foo * p __attribute__((unused))) 9 { 10 } $ Consider this new version of the code where a data member has been added right before the flexible array member: $ cat -n test-v1.c 1 struct foo 2 { 3 int member0; 4 char member1; /*<-- added member. */ 5 char pad[]; /* <-- flexible array member. */ 6 }; 7 8 void 9 foo(struct foo * p __attribute__((unused))) 10 { 11 } $ Here is what abidiff reports about the change: $ abidiff test-v0.o test-v1.o || echo "returned value: $?" Functions changes summary: 0 Removed, 1 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 1 function with some indirect sub-type change: [C] 'function void foo(foo*)' at test-v0.c:8:1 has some indirect sub-type changes: parameter 1 of type 'foo*' has sub-type changes: in pointed to type 'struct foo' at test-v1.c:1:1: type size changed from 32 to 64 (in bits) 1 data member insertion: 'char member1', at offset 32 (in bits) at test-v1.c:4:1 1 data member change: 'char pad[]' offset changed from 32 to 40 (in bits) (by +8 bits) returned value: 4 $ This patch allows users to suppress this change report using a new property value to the "has_data_member_inserted_at" property of the [suppress_type] directive. The resulting suppression specification reads: $ cat -n foo.suppr 1 [suppress_type] 2 type_kind = struct 3 name = foo 4 has_data_member_inserted_at = offset_of_flexible_array_data_member 5 has_size_change = yes $ With this suppression specification the previous command now gives: $ abidiff --suppr foo.suppr test-v0.o test-v1.o && echo "returned value: $?" Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable returned value: 0 $ I hope this is close enough to what we expect. If after testing and reviewing this patch it's good enough for you, please let me know so that I can fold it into the WIP branch of the "UAPI checker support" at https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/check-uapi-support. Please note that I have filed a bug to track the progress of this overall "check-uapi-support" effort at https://sourceware.org/bugzilla/show_bug.cgi?id=30931. Feel free to follow that bug there. You can reply to the bugzilla's emails using your mail user agent. > 4. Anonymous enums > - Not sure how to associate these enums when diffing since their > "made up" names don't always match before/after a patch. As I said in an other email, this task led to the patch set https://inbox.sourceware.org/libabigail/87r0mapspt.fsf@redhat.com/ which is in the branch https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums. I am waiting for your feedback to fold that patch set into the WIP branch "check-uapi-support". > I can try tackling items 1 and 2 if you'd like - just let me know! For > items 3 and 4, can we work up a path forward? [...] Hopefully things are moving somewhat now :-) Cheers,
diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h index 99047223ab26..7e15214aa5dd 100644 --- a/include/uapi/linux/dcbnl.h +++ b/include/uapi/linux/dcbnl.h @@ -411,6 +411,7 @@ enum dcbnl_attrs { * @DCB_ATTR_IEEE_PEER_PFC: peer PFC configuration - get only * @DCB_ATTR_IEEE_PEER_APP: peer APP tlv - get only * @DCB_ATTR_DCB_APP_TRUST_TABLE: selector trust table + * @DCB_ATTR_DCB_REWR_TABLE: rewrite configuration */ enum ieee_attrs { DCB_ATTR_IEEE_UNSPEC, @@ -425,6 +426,7 @@ enum ieee_attrs { DCB_ATTR_IEEE_QCN_STATS, DCB_ATTR_DCB_BUFFER, DCB_ATTR_DCB_APP_TRUST_TABLE, + DCB_ATTR_DCB_REWR_TABLE, __DCB_ATTR_IEEE_MAX };