From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vs1-xe32.google.com (mail-vs1-xe32.google.com [IPv6:2607:f8b0:4864:20::e32]) by sourceware.org (Postfix) with ESMTPS id 302353857818 for ; Fri, 4 Dec 2020 14:42:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 302353857818 Received: by mail-vs1-xe32.google.com with SMTP id q5so3376800vsg.0 for ; Fri, 04 Dec 2020 06:42:25 -0800 (PST) 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; bh=Yl1i80otjH47DNUKtY570t3VKFowE9E81z5rLVE5pW0=; b=CRxOGhGj1rGMCa9fZ7Q8u4UkXLzlX0uCC70C6rMe0TRp8dJoKzWaaYcR3qq/j6r9UE bhNUiUx8ASwjPyvta8lwOxCVZ8VzlWUov3XFZnycVvrHxPm8jGCthVYhdwjCejNgfRlw hBWHOZmBEWV8QLGoXvovZGtvZfAZzCWH6yNr5L8nMdjFsUMX6FtMsZo5CMuylPewqNbB a16hAgqE9Ni6x4MROfxWdeZn+QIksnI6Jh6mHMqr5M6uOXBQl3Sg9hjqMxO4cGuYDdSv oj0g2T3zHCEtQYvqnWcAlO/SKG5PnOJWmN3OO3MmDb4AqQNpqnq1J+hNz2F73R86rVY1 wwEA== X-Gm-Message-State: AOAM531cxroYmLQvEQ/r9eQzTlB/XJS8UAH2+5v2QlKlK94rXjdROOAQ 8pksy7DNOKfEvUMQvOSM53qUdr70wc02uJmuHMUmww== X-Google-Smtp-Source: ABdhPJwEVDA7/BotzbstjH1HIaPIBsdxLHUmqFALoupsu6p1DoAWp0Dr4cd+tOe2fUHIOJjtKs5qWaa5rKiE/nxBc4s= X-Received: by 2002:a67:1b42:: with SMTP id b63mr3158528vsb.24.1607092944533; Fri, 04 Dec 2020 06:42:24 -0800 (PST) MIME-Version: 1.0 References: <20201203150916.3540551-1-gprocida@google.com> <865z5hq1eo.fsf@seketeli.org> In-Reply-To: <865z5hq1eo.fsf@seketeli.org> From: Giuliano Procida Date: Fri, 4 Dec 2020 14:41:46 +0000 Message-ID: Subject: Re: [PATCH] abidiff: improve treatment of array types in leaf changes mode To: Dodji Seketeli Cc: Giuliano Procida via Libabigail , kernel-team@android.com, =?UTF-8?Q?Matthias_M=C3=A4nnich?= X-Spam-Status: No, score=-29.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, HTML_MESSAGE, 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Fri, 04 Dec 2020 14:42:27 -0000 Hi. On Fri, 4 Dec 2020 at 11:02, Dodji Seketeli wrote: > Hello Giuliano, > > Giuliano Procida a =C3=A9crit: > > [...] > > > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > > index c6f7c13e..b0db9c39 100644 > > --- a/src/abg-ir.cc > > +++ b/src/abg-ir.cc > > @@ -23606,7 +23606,7 @@ types_have_similar_structure(const type_base* > first, > > || ty1->get_dimension_count() !=3D ty2->get_dimension_count() > > || !types_have_similar_structure(ty1->get_element_type(), > > ty2->get_element_type(), > > - indirect_type)) > > + true)) > > return false; > > > > return true; > > I think this change is correct, thanks for spotting that. > > However ... > > [...] > > > If an array's element type doesn't change name but has some other > > (local) change, then the change should not also be considered local to > > the array type. > > I find this comment confusing, even if I think I see what you mean. > > A change being local or not, is a concept that is not at the same > logical level as the concept of "type similarity" defined in the comment > of the function types_have_similar_structure. I would say that the type > similarity concept is at a lower logical level. So seeing this comment > that applies to a higher level concept for a change made to something > > But I agree that the comments of types_have_similar_structure are > confusing as well. > > So I am proposing two patches following this message, for this issue. > One patch amends the the comments for types_have_similar_structure, and > the second one is your patch, with amended comments. > > And we can discuss from there as you like. > > I'm happy with your patches and have replied to them. However, I'll be honest, I've found this aspect (local changes and types with similar structure) of abidiff really hard to reason about. I'm likely looking at things the wrong way, but here's roughly how I see things. We want to identify "leaf" changes. I think of them more as "root" (cause) changes, but that's just inverting the edges in some graph. These should be (user-defined) types that have not changed name but have changed in some way "directly" (indirectly would be if there were a more remote cause of the same sort). There are also direct changes to symbols' types. User-defined types with local changes have paths to the symbols whose types they affect - they are the impacted interfaces. Now with libabigail, there is the notion of local vs non-local change which might not correspond exactly with the notion sketched above, but it's certainly related. And then there are at least two more pieces: types_have_similar_structure which is used in decisions about locality and lastly the logic used to filter changes for consideration in the leaf report - they have to be local plus a lot of other conditions. The decision-making that goes into leaf reporting seems to be spread across several bits of code and I speculate that it was an inconsistency between two of these that resulted in diff nodes being selected for inclusion but actually have no local diff to report (by the represent helper function). If I knew what types_have_similar_structure had to be consistent with I could check all the cases or, more interestingly, see if the decisions being made in two different places could be made in just one place. Regards, Giuliano. > [...] > > Cheers, > > -- > Dodji >