From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x142.google.com (mail-il1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) by sourceware.org (Postfix) with ESMTPS id 140003857C6A for ; Mon, 27 Jul 2020 10:55:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 140003857C6A Received: by mail-il1-x142.google.com with SMTP id c16so859065ils.8 for ; Mon, 27 Jul 2020 03:55:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=2s7khk/NZGmPmhe8QeRS2vjPTw5tkOWtkZbz7mVH+Ro=; b=mIZfSwn5A9ipdaCm1maLXAffrqSCZtsRlE4R6++bAa8HWq3+EZ7MDuEVU0CPN3sBJv qjHmnU3Ze2PvcITG8dZ9F6THgouqk/zcq7EYBhNN7RUb4jyfBQj+RrvYg4+VDAfZveGB /hpqXEV3/aJ+Kahia5sBeMZAXZdKBGf5wqRJPg7Kyz8z5SAbiaWw6Wu9nLJjWKlzv0YG LKHF3nH2GE0mDNppU6A1Fp7dnKALYtvBwzSJNrVsnkgFdAk/hK7znvsPlVCUYFhjAMJh gWHyZX8+XTW16sgaHLLal83Wf2KpcDf4e213wV4GK/iDbVE7y0qcEnLSfiT3yLzWi/uD JkNA== X-Gm-Message-State: AOAM5304kROQ/gZELKxRN4hujzVcy2T/P4kNjfRXBxmv5rRmTh+BRiJc bgtSVaD5WeiYJ8o0JywzUuC3TX7vUf9JHWaZA9iwQel/y3M= X-Google-Smtp-Source: ABdhPJxM0O1h9I6KxU9JC3YzYLQ5cI2/2xfCpWkTP3V1vdiN9F/jTTiojbCHAROfRSonyif7xbOB3PQJr9PVkYgwvZs= X-Received: by 2002:a92:dd0b:: with SMTP id n11mr23647326ilm.241.1595847350358; Mon, 27 Jul 2020 03:55:50 -0700 (PDT) MIME-Version: 1.0 References: <20200708095315.948634-1-gprocida@google.com> <878sf5jtuc.fsf@seketeli.org> In-Reply-To: <878sf5jtuc.fsf@seketeli.org> From: Giuliano Procida Date: Mon, 27 Jul 2020 11:55:13 +0100 Message-ID: Subject: Re: [PATCH 0/3] Type equality refactor and instrumentation To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com, =?UTF-8?Q?Matthias_M=C3=A4nnich?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-24.1 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 27 Jul 2020 10:55:52 -0000 Hi Dodji. On Mon, 27 Jul 2020 at 08:33, Dodji Seketeli wrote: > > Hello Giuliano, > > Giuliano Procida a =C3=A9crit: > > > Hi Dodji. > > > > This series refactors various operator=3D=3D methods making their commo= n > > functionality evident. > > > > The first patch is just a prelude to make the second smaller. > > > > The second patch does the refactoring. I'm not attached to the name > > 'equality_helper'. > > Thank you for working on this. > > I have looked into these first two patches and they look good to me in ge= neral. > I'll post their individual review as usual, shortly. > > > The third patch is not intended for direct inclusion in libabigail but > > builds on the refactoring to investigate how equality and canonical > > types work in practice. It identifies some potential discrepancies, > > but they may be entirely expected. > > In it's current form, I'd rather hold the inclusion of this one for now. > It's super intrusive, clutters the code quite a bit and I am not really > sure about its practical use at the moment. The code is absolutely not intended to be committed to master. On top of the obvious impact on the code, tests were still running after 60h with this turned on! > > In general, it can be risky to define operator=3D=3D in a way where > > reflexivity, symmetry and transitivity do not obviously hold or can be > > sensitive to small code changes or in way where equality, say for > > class_decl_sptr, can be affected by something like canonicalisation. > > More instrumentation could be added to check behaviour. > > The comparison code is tricky. The number one reason for this is that > it has to be fast. So yes, it's risky. > > One category of tests that are "simple" to perform and in practise are > quite powerful to detect and debug potential comparison issues are > "identity tests". That is, comparing a binary against itself and > requiring that the result be the empty set. This is why the option > "--abidiff" was added to abidw. > > So I tend to favor schemes that keep the code the less cluttered and the > most "debuggable" as possible, as opposed to adding a lot of > instrumentation. I guess some balance has to found there. This was intended for one-off (or occasional) instrumentation. Running with the instrumentation, I noticed various places where canonical and structura= l comparison differ. If the comparisons are happening during canonicalisation then things may be temporarily off. Or I may have a bug in the instrumentat= ion. At any rate, the commit adds code comments showing where certain discrepanc= ies were noted during "make check". For example, function_type nodes sometimes compare as canonically equal but not "structurally" and vice versa. "Structurally" is not really fully structurally as recursive calls attempt canonical compa= rison first. If there are any you think are of concern, let me know and I can investigat= e further and open a bug with findings. I've reworked my series to account for your version of the second commit. The instrumentation commit can be found amongst the changes in https://github.com/myxoid/libabigail/commits/type-equality-paranoia Regards, Giuliano. > Cheers, > > -- > Dodji