From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by sourceware.org (Postfix) with ESMTPS id F23D83858C60 for ; Mon, 10 Jan 2022 17:00:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F23D83858C60 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id BB01DFF814; Mon, 10 Jan 2022 17:00:05 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id E00EF581C2E; Mon, 10 Jan 2022 18:00:04 +0100 (CET) From: Dodji Seketeli To: Matthias Maennich Cc: libabigail@sourceware.org, gprocida@google.com, kernel-team@android.com Subject: Re: [PATCH 3/5] XML writer: track emitted types by bare pointer Organization: Me, myself and I References: <20211203114622.2944173-1-maennich@google.com> <20211203114622.2944173-4-maennich@google.com> <87ilvwrawm.fsf@seketeli.org> X-Operating-System: Fedora 36 X-URL: http://www.seketeli.net/~dodji Date: Mon, 10 Jan 2022 18:00:04 +0100 In-Reply-To: (Matthias Maennich's message of "Thu, 16 Dec 2021 16:07:23 +0000") Message-ID: <87wnj7cyqz.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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 List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Jan 2022 17:00:10 -0000 Matthias Maennich a =C3=A9crit: [...] > If the XML writer considers two equivalent declaration-only types to be > different, one question to ask is: what is the real difference, that is, > how will this affect the outcome of abidiff? The problem is not necessarily at the abidiff level per say. The problem would be duplication of decl-only types in the abixml output, I think, and maybe infinite loops in those cases. The infinite loops are easy to debug, though. So I am not concerned about them. > If the types never change > (kind, name or declaration/definition status), nothing should ever be > reported. If a type does change... there are two possibilities: either > the types were really one type and now perhaps abidiff reports diffs for > the same name in two different ways; or the types were really two > different ones and abidiff has a simpler job. In my experience, abidiff > doesn't always report declaration-only/defined transitions. It doesn't > sound like there will be any really bad impact on diffs from having this > kind of duplication. However, if someone can come up with a test case of > the kind you mention, that would give some extra reassurance. The reason why I was pointing to this "general" issue is to make sure you are aware of this. As type duplications in abixml was something you guys were tracking (and rightly so) I thought I'd point out that we still have the risk here. But because the type id map (writer_context::m_type_id_map) is not affected, the duplicated types will correctly be identified as such by the reader; thus I don't think abidiff is going to be affected. >>So maybe it would be better have an equality operator that uses >>is_non_canonicalized_type() to detect those rare cases and use >>structural comparison in those cases? > > That might come at higher cost than it is beneficial. I could not tell, as I don't necessarily have the right binaries at hand. I trust you. > >> >>What do you think? > > For us specifically - building with clang and for our use cases - if we > keep structural equality of any kind then we need a hash function to go > along with this and, as we've sadly found out, this isn't working well > at the moment. We are currently on a bit dated version of libabigail for > our production use, but would like to close that gap again to come > closer to master. > > The risk of infinite loops and the reality of 30x slowdowns for certain > workloads mean we would need to apply these changes to remove structural > equality testing from the XML writer and then maintain an Android > version of libabigail as a more heavily-patched fork, to whatever extent > is feasible. I would rather we find a good solution that works for all > to get again close to upstream and not having to maintain such a fork. > > Yet, as an additional piece of assurance: the testing we have done does > not only include kernels, but of course we heavily examined the > libabigail test suite. Additionally, we maintain a large set of small > test cases specifically created for ABI stability testing and to cover > corner cases of all sorts. We are in the process of publishing those as > well. So far, this has served as great input for this patch series as > well. > > Does this make sense? What do you think? If you don't really care about the potential type duplication in the abixml as stated above, frankly, let's just get this patch in. Are you okay with that? Cheers, --=20 Dodji