Message ID | CAGvU0HkcV=BL3BV1Vyu3yFG+e2qPs6DA223qtwR0MhiDArTZ8Q@mail.gmail.com |
---|---|
State | Rejected, archived |
Headers |
Return-Path: <libabigail-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0DF8F388E836; Thu, 18 Jun 2020 13:45:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0DF8F388E836 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1592487931; bh=fw8feX2kugnnEUAbwwANKHav8fQ+FUcWpvM0Xtpc+u0=; h=Date:Subject:To:List-Id:List-Unsubscribe:List-Archive:List-Help: List-Subscribe:From:Reply-To:Cc:From; b=VZRJC1kQhrvDBrmePooNO31DuqDuAheFSkqpjhyOd7cjI6PIBEn1MfvZaqXnr2C81 hmgsbKwa+ci/gXvixIcpqotDMF5kZt693v1bqFQfbS+yzzUdKvCE9PRhJWYb+kxw+K xMtl/i6UeOBqZazdo5/Q3cyp7yGrMVnidFgR5YQA= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mail-il1-x130.google.com (mail-il1-x130.google.com [IPv6:2607:f8b0:4864:20::130]) by sourceware.org (Postfix) with ESMTPS id 568A0387086A for <libabigail@sourceware.org>; Thu, 18 Jun 2020 13:45:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 568A0387086A Received: by mail-il1-x130.google.com with SMTP id z2so5800695ilq.0 for <libabigail@sourceware.org>; Thu, 18 Jun 2020 06:45:29 -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:from:date:message-id:subject:to:cc; bh=fw8feX2kugnnEUAbwwANKHav8fQ+FUcWpvM0Xtpc+u0=; b=FeptbvxlC73xF5L8xbeZeTAlPdS124SWQKREaHWh49304dCV9UOtvl+A9SqvpMeJ/E u/j055qiuqOCGHnCrLOXBckp9idgHxir+LmTX6tW8cQBh5HlIif0mTCPs0L9SY8u1X0u tI1KvgXFncZLsBGLEQ3LxpImeZtmsBOspit34E3wJpmAffGynR706C31OH0IFqb7/hz7 EkzQ+2KNstCFnYEER5Dls6ig5a7OkfZLjN5C+Us6+zuwH8A+5CQb7vyRI9OGW+HzaTIE 4k95lYp6ncEAA4zVahp0X8yqzNCZmFTzIypdjIJbzR8pjB2B30JQpn7YfgrGwrdsUrR1 zNnQ== X-Gm-Message-State: AOAM531cVuuaAL8MqIeToCPyJ716s8IresF33RE1eqvOQW9rojC+GSrl 7AA7tW0GtxHm4FKLnpAtHIjvNxR4lrxBoN4KRD4edVNhJkWZzw== X-Google-Smtp-Source: ABdhPJzXGpduYjE9DJPVP3h6Rr8OwIqE/hr/lm60PVoUY7ewZhNpuhaa6W9RH7aIv3RjDab9doH96x8cPwNTyvcWLbk= X-Received: by 2002:a05:6e02:1286:: with SMTP id y6mr4395705ilq.0.1592487928445; Thu, 18 Jun 2020 06:45:28 -0700 (PDT) MIME-Version: 1.0 Date: Thu, 18 Jun 2020 14:45:11 +0100 Message-ID: <CAGvU0HkcV=BL3BV1Vyu3yFG+e2qPs6DA223qtwR0MhiDArTZ8Q@mail.gmail.com> Subject: Linux kernel type equality To: Dodji Seketeli <dodji@seketeli.org> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-30.7 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, 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 <libabigail.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libabigail>, <mailto:libabigail-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libabigail/> List-Help: <mailto:libabigail-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libabigail>, <mailto:libabigail-request@sourceware.org?subject=subscribe> From: Giuliano Procida via Libabigail <libabigail@sourceware.org> Reply-To: Giuliano Procida <gprocida@google.com> Cc: libabigail@sourceware.org, =?utf-8?q?Matthias_M=C3=A4nnich?= <maennich@google.com> Errors-To: libabigail-bounces@sourceware.org Sender: "Libabigail" <libabigail-bounces@sourceware.org> |
Series |
Linux kernel type equality
|
|
Commit Message
Giuliano Procida
June 18, 2020, 1:45 p.m. UTC
Hi Dodji.
Users have reported issues with kernel ABI monitoring where there are
unexpected declaration-only vs definition diffs (between XML ABIs).
We're looking at one of these cases in some detail. The DWARF reader
does associate the struct definition with the declaration-only type,
so I'm taking a look at get_canonical_type_for.
I was checking the behaviour of
types_defined_same_linux_kernel_corpus_public (as it's described as an
optimisation) with the patch below (together with my
"get_canonical_type_for: restore environment better" patch).
The check triggers with existing tests. I think these are all typedef
struct { } foo.
$ build/tests/runtestreaddwarf
type equality discrepancy with struct {pgdval_t pgd;} and struct {pgdval_t pgd;}
type equality discrepancy with struct {s64 counter;} and struct {s64 counter;}
type equality discrepancy with struct {atomic64_t id; void* vdso;
unsigned long int flags;} and struct {atomic64_t id; void* vdso;
unsigned long int flags;}
type equality discrepancy with struct {unsigned long int sig[1];} and
struct {unsigned long int sig[1];}
type equality discrepancy with struct {uid_t val;} and struct {uid_t val;}
type equality discrepancy with struct {unsigned long int bits[1];} and
struct {unsigned long int bits[1];}
type equality discrepancy with struct {pteval_t pgprot;} and struct
{pteval_t pgprot;}
type equality discrepancy with struct {gid_t val;} and struct {gid_t val;}
My question: does this point to a problem with type equality elsewhere
in libabigail?
Giuliano.
From dd249b7dc83df3609c23ec237a7ddb94e29abf44 Mon Sep 17 00:00:00 2001
From: Giuliano Procida <gprocida@google.com>
Date: Thu, 18 Jun 2020 13:50:59 +0100
Subject: [PATCH] check linux optimisation
---
src/abg-ir.cc | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
// flags.
Comments
Hello Giuliano, Giuliano Procida <gprocida@google.com> a écrit: > Users have reported issues with kernel ABI monitoring where there are > unexpected declaration-only vs definition diffs (between XML ABIs). Ok. > > We're looking at one of these cases in some detail. The DWARF reader > does associate the struct definition with the declaration-only type, > so I'm taking a look at get_canonical_type_for. > > I was checking the behaviour of > types_defined_same_linux_kernel_corpus_public (as it's described as an > optimisation) with the patch below (together with my > "get_canonical_type_for: restore environment better" patch). > > The check triggers with existing tests. I think these are all typedef > struct { } foo. > > $ build/tests/runtestreaddwarf > type equality discrepancy with struct {pgdval_t pgd;} and struct {pgdval_t pgd;} > type equality discrepancy with struct {s64 counter;} and struct {s64 counter;} > type equality discrepancy with struct {atomic64_t id; void* vdso; > unsigned long int flags;} and struct {atomic64_t id; void* vdso; > unsigned long int flags;} > type equality discrepancy with struct {unsigned long int sig[1];} and > struct {unsigned long int sig[1];} > type equality discrepancy with struct {uid_t val;} and struct {uid_t val;} > type equality discrepancy with struct {unsigned long int bits[1];} and > struct {unsigned long int bits[1];} > type equality discrepancy with struct {pteval_t pgprot;} and struct > {pteval_t pgprot;} > type equality discrepancy with struct {gid_t val;} and struct {gid_t val;} > > My question: does this point to a problem with type equality elsewhere > in libabigail? I think it does, yes. As you have filed a problem report for this particular issue, I have responded to it at https://sourceware.org/bugzilla/show_bug.cgi?id=26135#c1. We can continue the discussion related to that problem report there, I guess. As for the initial unexpected diff between decl-only and its fully-defined counterpart that you talked about at the beginning of your message, I am not sure the issue raised in PR26135 is related. To say for sure, I guess I'd need to have a reproducer, probably as part of a problem report? Cheers,
On Wed, 24 Jun 2020 at 18:27, Dodji Seketeli <dodji@seketeli.org> wrote: > > Hello Giuliano, > > Giuliano Procida <gprocida@google.com> a écrit: > > > Users have reported issues with kernel ABI monitoring where there are > > unexpected declaration-only vs definition diffs (between XML ABIs). > > Ok. > > > > > We're looking at one of these cases in some detail. The DWARF reader > > does associate the struct definition with the declaration-only type, > > so I'm taking a look at get_canonical_type_for. > > > > I was checking the behaviour of > > types_defined_same_linux_kernel_corpus_public (as it's described as an > > optimisation) with the patch below (together with my > > "get_canonical_type_for: restore environment better" patch). > > > > The check triggers with existing tests. I think these are all typedef > > struct { } foo. > > > > $ build/tests/runtestreaddwarf > > type equality discrepancy with struct {pgdval_t pgd;} and struct {pgdval_t pgd;} > > type equality discrepancy with struct {s64 counter;} and struct {s64 counter;} > > type equality discrepancy with struct {atomic64_t id; void* vdso; > > unsigned long int flags;} and struct {atomic64_t id; void* vdso; > > unsigned long int flags;} > > type equality discrepancy with struct {unsigned long int sig[1];} and > > struct {unsigned long int sig[1];} > > type equality discrepancy with struct {uid_t val;} and struct {uid_t val;} > > type equality discrepancy with struct {unsigned long int bits[1];} and > > struct {unsigned long int bits[1];} > > type equality discrepancy with struct {pteval_t pgprot;} and struct > > {pteval_t pgprot;} > > type equality discrepancy with struct {gid_t val;} and struct {gid_t val;} > > > > My question: does this point to a problem with type equality elsewhere > > in libabigail? > > I think it does, yes. As you have filed a problem report for this > particular issue, I have responded to it at > https://sourceware.org/bugzilla/show_bug.cgi?id=26135#c1. We can > continue the discussion related to that problem report there, I guess. > > As for the initial unexpected diff between decl-only and its > fully-defined counterpart that you talked about at the beginning of your > message, I am not sure the issue raised in PR26135 is related. To say > for sure, I guess I'd need to have a reproducer, probably as part of a > problem report? > This appears to definitely be a separate issue. I'll continue working on isolating a smaller example. FYI, I analysed some recent kernel ABI XML diffs and used that to come up with the following ordering of "things which cause churn", most significant first. For a small change, the rough amount of churn is in [ ] below. 1 type-id renumbering - fixed by hash-based type-ids [~20000 line changes] 2 Types moving between translation units, particularly when translation units are added or removed [~400 line moves] 3 Type id hash collision linear probing (almost always due to anonymous types and internal name collisions; anonymous kernel unions in particular never seem to have naming typedefs) [~200 line changes] 4a Location (line and column) changes. [<20 line changes] 4b External name renumbering due to sequential anon type names. [<10 line changes] In terms of code review, line moves are worse than changes, so item 2 is worse than twice as bad as item 3. mjw had a patch out addressing item 2 by folding everything into one translation unit. This is what I'm currently looking at, in terms of XML diff sizes at least. Regards, Giuliano. > Cheers, > > -- > Dodji
Hi Dodji. On Thu, 25 Jun 2020 at 16:13, Giuliano Procida <gprocida@google.com> wrote: > > On Wed, 24 Jun 2020 at 18:27, Dodji Seketeli <dodji@seketeli.org> wrote: > > > > Hello Giuliano, > > > > Giuliano Procida <gprocida@google.com> a écrit: > > > > > Users have reported issues with kernel ABI monitoring where there are > > > unexpected declaration-only vs definition diffs (between XML ABIs). > > > > Ok. > > > > > > > > We're looking at one of these cases in some detail. The DWARF reader > > > does associate the struct definition with the declaration-only type, > > > so I'm taking a look at get_canonical_type_for. > > > > > > I was checking the behaviour of > > > types_defined_same_linux_kernel_corpus_public (as it's described as an > > > optimisation) with the patch below (together with my > > > "get_canonical_type_for: restore environment better" patch). > > > > > > The check triggers with existing tests. I think these are all typedef > > > struct { } foo. > > > > > > $ build/tests/runtestreaddwarf > > > type equality discrepancy with struct {pgdval_t pgd;} and struct {pgdval_t pgd;} > > > type equality discrepancy with struct {s64 counter;} and struct {s64 counter;} > > > type equality discrepancy with struct {atomic64_t id; void* vdso; > > > unsigned long int flags;} and struct {atomic64_t id; void* vdso; > > > unsigned long int flags;} > > > type equality discrepancy with struct {unsigned long int sig[1];} and > > > struct {unsigned long int sig[1];} > > > type equality discrepancy with struct {uid_t val;} and struct {uid_t val;} > > > type equality discrepancy with struct {unsigned long int bits[1];} and > > > struct {unsigned long int bits[1];} > > > type equality discrepancy with struct {pteval_t pgprot;} and struct > > > {pteval_t pgprot;} > > > type equality discrepancy with struct {gid_t val;} and struct {gid_t val;} > > > > > > My question: does this point to a problem with type equality elsewhere > > > in libabigail? > > > > I think it does, yes. As you have filed a problem report for this > > particular issue, I have responded to it at > > https://sourceware.org/bugzilla/show_bug.cgi?id=26135#c1. We can > > continue the discussion related to that problem report there, I guess. > > > > As for the initial unexpected diff between decl-only and its > > fully-defined counterpart that you talked about at the beginning of your > > message, I am not sure the issue raised in PR26135 is related. To say > > for sure, I guess I'd need to have a reproducer, probably as part of a > > problem report? > > > > This appears to definitely be a separate issue. I'll continue working > on isolating a smaller example. I've opened https://sourceware.org/bugzilla/show_bug.cgi?id=26182 for this. When it comes to the Linux kernel, small examples are still rather large. Regards, Giuliano. > FYI, I analysed some recent kernel ABI XML diffs and used that to come > up with the following ordering of "things which cause churn", most > significant first. For a small change, the rough amount of churn is in > [ ] below. > > 1 type-id renumbering - fixed by hash-based type-ids [~20000 line changes] > 2 Types moving between translation units, particularly when > translation units are added or removed [~400 line moves] > 3 Type id hash collision linear probing (almost always due to > anonymous types and internal name collisions; anonymous kernel unions > in particular never seem to have naming typedefs) [~200 line changes] > 4a Location (line and column) changes. [<20 line changes] > 4b External name renumbering due to sequential anon type names. [<10 > line changes] > > In terms of code review, line moves are worse than changes, so item 2 > is worse than twice as bad as item 3. > > mjw had a patch out addressing item 2 by folding everything into one > translation unit. This is what I'm currently looking at, in terms of > XML diff sizes at least. > > Regards, > Giuliano. > > > Cheers, > > > > -- > > Dodji
diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 41a8e3f9..a8096690 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -11904,8 +11904,18 @@ type_base::get_canonical_type_for(type_base_sptr t) // Compare types by considering that decl-only classes don't // equal their definition. env->decl_only_class_equals_definition(false); - bool equal = types_defined_same_linux_kernel_corpus_public(**it, *t) - || *it == t; + bool linux_eq = types_defined_same_linux_kernel_corpus_public(**it, *t); + bool plain_eq = *it == t; + // is it really just an optimisation? + if (linux_eq > plain_eq) + { + std::cerr << "type equality discrepancy with " + << t->get_pretty_representation() + << " and " + << (*it)->get_pretty_representation() + << std::endl; + } + bool equal = linux_eq || plain_eq; // Restore the state of the on-the-fly-canonicalization and // the decl-only-class-being-equal-to-a-matching-definition