From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by sourceware.org (Postfix) with ESMTPS id 484EE3858D35 for ; Tue, 23 May 2023 19:59:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 484EE3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=quicinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=quicinc.com 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 34NJwhIr004359; Tue, 23 May 2023 19:59:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=qcppdkim1; bh=uzpXNuPoOmvjZJPVg5EUU+wdqkLuXv10EvWzUz4joUY=; b=QFhbhcEAjknP8ypYcLmU11si4rp0+JnooWiBE8KaZQUNNmvzqOOPPSaHFXORWcZrAxMx zU5YqmaVQhh4t3M1a8uYPmTEf76DS4cYx7WOuPDG3C83m2PEmUDgzeyXm8ZRC8jcTBsj eIUpOA9TgGFouqNDPM4/Vb51wSM9Lrz9+NgPDEXEmlN6m0YkFmX2nC8+4qgm7jmOakbK bOwL/dDrvGHzjpRVi8F8R98TVAfgX6dqWX6b4Omo4tHJtSSKBOWpUzQ/721NQi6ndK3K jx9L9atYSlEUD5BrunODLw7CgT1pBgvhs17tzdGToZ/TDSrghhhKuAAC48pIcOC1I90a vg== Received: from nalasppmta02.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3qre8p33h2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 May 2023 19:59:23 +0000 Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 34NJxMZT020602 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 May 2023 19:59:22 GMT Received: from [10.110.7.250] (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; Tue, 23 May 2023 12:59:21 -0700 Message-ID: <153dc338-ff1f-4273-1663-e934124e4bcc@quicinc.com> Date: Tue, 23 May 2023 12:59:00 -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 Subject: Re: abidiff improvements for kernel UAPI checker To: Dodji Seketeli CC: , Trilok Soni , "Satya Durga Srinivasu Prabhala" References: <5363161d-8167-284e-e35d-9a8ef20adea9@quicinc.com> <877cu5t7tw.fsf@seketeli.org> <87354tt32o.fsf@seketeli.org> <340b33bd-2b43-9f99-58e1-f1b77a51b48a@quicinc.com> <87sfc42rnw.fsf@seketeli.org> Content-Language: en-US From: John Moon In-Reply-To: <87sfc42rnw.fsf@seketeli.org> 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-ORIG-GUID: 1SVp0d8BLjNK1Cz2IPd60rzB02qYrRYC X-Proofpoint-GUID: 1SVp0d8BLjNK1Cz2IPd60rzB02qYrRYC X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26 definitions=2023-05-23_12,2023-05-23_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 suspectscore=0 adultscore=0 mlxlogscore=939 mlxscore=0 malwarescore=0 bulkscore=0 lowpriorityscore=0 spamscore=0 impostorscore=0 clxscore=1011 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000 definitions=main-2305230160 X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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: 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 > 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 > 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