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 460333858C5E for ; Wed, 27 Sep 2023 17:37:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 460333858C5E 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 (m0279872.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 38REWENU026465; Wed, 27 Sep 2023 17:37:03 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=MK+pP35ZG/QLDrEcHtwpBbddLHjWvALB2moAQvvlUkQ=; b=B23eOiVJOY4mMdlHLme/OTrSf50GJb2qQM6PJoXI85WKGeBUvCFCXhlfwBDNXSK0Q9XO cXlDifA6e/Q85P4tbgJS3g5i1qSnHRXafHj1rC9XQ0gi+Wt71VObUIB9KIeK4Pg5JxBS 0OsIE+7DJzT2ZYV+7WC4OE9a+JViEs4HYDlEaUdCbEosEsAJfDCm/RYoGmM1O5jX4jJD GlG99ZHj6k52W3LsMApMlP4zMsPFeUpnbgfiPorSG0c7cDi8rX6blINB2bSCXdivEYKh pfXUAZF10GPTF0OdGjXOSeZjKax8a1o1tvDLgMJSFvg1NS5PxCyyDRoBhgmnK3GTs8yF bg== Received: from nalasppmta05.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3tc179k39c-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 27 Sep 2023 17:37:03 +0000 Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 38RHb2aa005330 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 27 Sep 2023 17:37:02 GMT Received: from [10.110.91.130] (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.1118.30; Wed, 27 Sep 2023 10:37:01 -0700 Message-ID: Date: Wed, 27 Sep 2023 10:37:00 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird 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> <87fs36e8sr.fsf@seketeli.org> <87bkdue89g.fsf@seketeli.org> <44d76150-3e34-4bef-9970-d321f5bc224c@quicinc.com> <871qelgwia.fsf@seketeli.org> Content-Language: en-US From: John Moon In-Reply-To: <871qelgwia.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: nasanex01a.na.qualcomm.com (10.52.223.231) 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: s6ayWkKL3XJygpOkwAW7DjQaEMdcxQpr X-Proofpoint-ORIG-GUID: s6ayWkKL3XJygpOkwAW7DjQaEMdcxQpr X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-09-27_12,2023-09-27_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 impostorscore=0 lowpriorityscore=0 suspectscore=0 mlxlogscore=999 clxscore=1015 spamscore=0 bulkscore=0 priorityscore=1501 adultscore=0 phishscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000 definitions=main-2309270149 X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 List-Id: 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