From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by sourceware.org (Postfix) with ESMTPS id CFC7F3858D3C; Sun, 5 Nov 2023 09:43:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CFC7F3858D3C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CFC7F3858D3C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.70.183.193 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699177394; cv=none; b=CXb3nWei+gL68qEak3Dw6FLF9JGgEnVFBoEHS+iC6E/iTS7kTcaf55E/UNA+HH4Y5Wl6NMfDcTYylQpZvu61c7idW7vWsrn/joKoFe+/OvuBD1P8adzOIK3kse7ehvQXbXHyJeGCEKTvaXLTSE287F6os8AGBEFoqruAYjAw5PI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699177394; c=relaxed/simple; bh=6CFgcs1ITy9ttojNsRu4Z0wB285W+Mz8nauJ6DHI8B4=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=M9vn8t/Jq7ce7VJb7cgztbWs42u15845fEnvGXyOz6u/OYc1HE1oK2v3631NJmjmoBU6Ntz+rCxV4uZR4dg5+x277o8zAFiJQ19Nz4VxbSrHVwKF+RoAFAXyV7/OcO/E4x8udBc8kJZfNVbi+LZ/r+Hg6jsjgqt3BMzDvRLH2xg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail.gandi.net (Postfix) with ESMTPSA id 6E611240004; Sun, 5 Nov 2023 09:43:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seketeli.org; s=gm1; t=1699177388; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ncs7ooeHbSkXQmr7RVmVvz4geszbOIEbarBaN7y0ImQ=; b=BNoKN0dgLscCP6VUcz8QzzccYh9UgTSoR5yCbXFay1P1UWsNTpl3KEzyAv4TQ6kq6+krfR FfgmTz1sC87QK+8anABmp5M/dxFEuFcZf6pmgpRjRAZIDPNTZGdpIijPhBsJn5wkIFkreQ MqJcymrFcOgvvvu0YC9BBZqvh/vU222SefpVS7DPbv7Acoq9USXlbswpjLvd4ITpSm/Q5E VtyoBGOucSF2SFwiLWDcabC5Sjbkc2NxUrHgjt3flMoz/zcGSL48dy/xK27l5D4EKQPGGE Lez1sHC0SwPlMepI9FrnRz6wOphfzgq4iJq0SOFUD8KXtxMo7OdC8Au/fOpQ8A== Received: by localhost (Postfix, from userid 1000) id 543945077C76; Sun, 5 Nov 2023 10:43:07 +0100 (CET) From: Dodji Seketeli To: "quic_johmoo at quicinc dot com" Cc: libabigail@sourceware.org Subject: Re: [Bug default/31017] Flex array conversion suppression Organization: Me, myself and I References: X-Operating-System: AlmaLinux 9.2 X-URL: http://www.seketeli.net/~dodji Date: Sun, 05 Nov 2023 10:43:07 +0100 In-Reply-To: (quic johmoo at quicinc dot com's message of "Sat, 04 Nov 2023 00:49:42 +0000") Message-ID: <87bkc8cz84.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-GND-Sasl: dodji@seketeli.org X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,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: "quic_johmoo at quicinc dot com" a =C3=A9crit: > https://sourceware.org/bugzilla/show_bug.cgi?id=3D31017 > > --- Comment #2 from John Moon --- > > Thanks for the implementation! I think this looks great. I tested it and = it > seems to be working properly for our use case in the kernel! > > One question about this block: > > + // Support for the > + // "has_strict_flexible_array_data_member_conversion =3D true" > + // clause. > + if (has_strict_fam_conversion()) > + { > + // Let's detect if the first class of the diff has a fake > + // flexible array data member that got turned into a real > + // flexible array data member. > + if (!( > + (has_fake_flexible_array_data_member(first_class) > + && has_flexible_array_data_member(second_class)) > + // A fake flexible array member has been changed into > + // a real flexible array ... > + && > + ((first_class->get_size_in_bits() > + =3D=3D second_class->get_size_in_bits()) > + || get_has_size_change()) > + // There was no size change or the suppression has a > + // "has_size_change =3D true" clause. > + )) > + return false; > + } > > Is it possible for a structure to meet the first condition (fake flex -> = flex) > *without* a size change? As a general rule, suppression specifications (aka supprspecs) don't apply = to a type which size has changed, unless the user /really/ wants the supprspec to apply. If she really wants it, then she has to explicitly say "has_size_change =3D yes". This is prevents "too eager" supprspecs to be applied without the user noticing the supprspecs is too eager. Basically, if a type's size changed, more often than not, we don't want to suppress its change report, for obvious reasons. That is why, throughout type_suppression::suppresses_diff you see the careful attention to the size change condition, when evaluating supprspecs. In this particular case, I think that we can have "fake flex -> flex" changes without a size change because there can other /additional/ changes that counter the size change we would have expected. That change could have been introduced, on purpose, to keep the ABI stable. For instance: $ diff -u test-PR31017-2-v0.c test-PR31017-2-v1.c=20 --- test-PR31017-2-v0.c 2023-11-05 10:04:36.433000539 +0100 +++ test-PR31017-2-v1.c 2023-11-05 10:07:00.579810832 +0100 @@ -2,7 +2,8 @@ { int x; int y; - int end[1]; + int padding; + int end[]; }; $ /home/dodji/git/libabigail/PR31017/build/tools/abidiff test-PR31017-2= -v0.o test-PR31017-2-v1.o=20 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 fun(foo*)' at test-PR31017-2-v0.c:9:1 has some ind= irect sub-type changes: parameter 1 of type 'foo*' has sub-type changes: in pointed to type 'struct foo' at test-PR31017-2-v1.c:1:1: type size hasn't changed 1 data member insertion: 'int padding', at offset 64 (in bits) at test-PR31017-2-v1.c:= 5:1 1 data member change: type of 'int end[1]' changed: type name changed from 'int[1]' to 'int[]' array type size changed from 32 to 'unknown' array type subrange 1 changed length from 1 to 'unknown' and offset changed from 64 to 96 (in bits) (by +32 bits) $=20 One reason why I think it's important to keep this "rigour" with the type size change thing is that abidiff actually returns a code that is a bit field that tells callers about the categories of the changes it encountered. From https://sourceware.org/libabigail/manual/abidiff.html#return-values, we see that if the ABIDIFF_ABI_CHANGE bit is set, it means there were some ABI changes. But then if the ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit is set, it means abidiff is 100% sure that at least of the changes causes an ABI incompatibility. In a continuous integration context, for instance, if the ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit is set, it means we are sure the change is incompatible, whereas if only the ABIDIFF_ABI_CHANGE bit is set, it means the change might or might not incompatible and thus needs a human review to decide. To wraps this all up, I'd say that only changes that would NOT set the ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit should be able to be suppressed, unless the user really knows what she is doing. Thinking about this, maybe check-uapi.sh could use the return code of abidiff rather than grepping its output. check-uapi.sh would then only reject changes categorically only if the ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit is set. If only ABIDIFF_ABI_CHANGE bit is set, check-uapi.sh would just propose the user to review the changes detected and possibly waive them. One result of the waiving process would thus be a new supprspec written and added to the stock of supprspecs to make sure the same kind of reviews is not requested in the future. With time, if a supprspec is recognized to be needed for this particular project, libabigail can even integrate it and install it by default for that project to use. We do this for various projects and their default supprspecs are included in the default.abignore file that is installed libabigail. You can browse it at https://sourceware.org/git/?p=3Dlibabigail.git;a=3Dblob;f=3Ddefault.abignore and learn about what project requested default supprspecs. > I'd think not, but may be missing something. I hope my explanation above helps shed some light in this apparently weird way of doing things. > Basically, I think you can get rid of the first_class->get_size_in_bits()= =3D=3D > second_class->get_size_in_bits() check. I would rather keep it, at least for the sake of consistency in the behaviour supprspecs evaluation in general, especially with the unwritten rule: "only changes that would NOT set the ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit should be able to be suppressed, unless the user really knows what she is doing." I guess we need (better) documentation about all this :-( > Also, if we know a size change is a tautology, you could move the > get_has_size_change() check to be first and save a few CPU cycles. > > Other than that, LGTM! > > I went ahead and made that change and added (at least a start) on the > documentation/tests. I don't have access to make a branch, so I just sent= a > patch separately. We can continue discussion there as needed. Thanks a lot for moving forward on this! I'll wait for your feedback on these comments and we can proceed with merging your patch accordingly. In the mean time, if I have comments, I'll follow-up on the patch thread, indeed. --=20 Dodji