Message-ID: <dffb0db2-521c-0adb-ff3d-68a379d17b26@solarflare.com>
Date:   Thu, 1 Nov 2018 21:08:37 +0000
From:   Edward Cree <ecree@...arflare.com>
To:     Martin Lau <kafai@...com>
CC:     Yonghong Song <yhs@...com>, Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and
 BTF_KIND_FUNC_PROTO

I've spent a bit more time thinking about / sleeping on this, and I
 still think there's a major disagreement here.  Basically it seems
 like I'm saying "the design of BTF is wrong" and you're saying "but
 it's the design" (with the possible implication — I'm not entirely
 sure — of "but that's what DWARF does").
So let's back away from the details about FUNC/PROTO, and talk in
 more general terms about what a BTF record means.
There are two classes of things we might want to put in debug-info:
 * There exists a type T
 * I have an instance X (variable, subprogram, etc.) of type T
Both of these may need to reference other types, and have the same
 space of possible things T could be, but there the similarity ends:
 they are semantically different things.
Indeed, the only reason for any record of the first class is to
 define types referenced by records of the second class.  Some
 concrete examples of records of the second class are:
1) I have a map named "foo" with key-type T1 and value-type T2
2) I have a subprogram named "bar" with prototype T3
3) I am using stack slot fp-8 to store a value of type T4
4) I am casting ctx+8 to a pointer type T5 before dereferencing it
Currently we have (1) and this patch series adds (2), both done
 through records that look like they are just defining a type (i.e.
 the first class of record) but have 'magic' semantics (in the case
 of (1), special names of the form ____btf_map_foo.  How anyone
 thought that was a clean and tasteful design is beyond me.)
What IMHO the design *should* be, is that we have a 'types'
 subsection that *only* contains records of the first class, and
 then other subsections to hold records of the second class that
 reference records of the first class by ID.  So for (1) you'd have
 either additional fields in struct bpf_map_def (we've extended that
 several times before, after all), or you'd have a maps table in
 .BTF that links map names ("foo", not "____btf_map_foo"!) with type
 IDs for its key and value:
    struct btf_map_record {
        __u32 name_off; /* name of map */
        __u32 key_type_id; /* index in "types" table */
        __u32 value_type_id; /* ditto */
    }
(Note the absence of any meaningless struct type as created by
 BPF_ANNOTATE_KV_PAIR.  That kind of source-level hack should be
 converted by the compiler's BTF output module into something less
 magic, rather than baked into the format definition.)
Then for (2) you'd have a functions table in .BTF that links subprog
 names, start offsets, and signatures/prototypes:
    struct btf_func_record {
        __u32 name_off; /* name of function */
        __u16 subprog_secn; /* section index in which func appears */
        __u16 subprog_start; /* offset in section of func entry point */
        __u32 type_id; /* index in "types" table of func signature */
    }

I believe this is a much cleaner design, which will be easier to extend
 in the future to add things like (3) and (4) for source-line-level
 debug information.  I also believe that if someone had written
 documentation describing the original design, semantics of the various
 BTF records, etc., it would have been immediately obvious that the
 design was needlessly confusing and ad-hoc.

On 20/10/18 00:27, Martin Lau wrote:
> Like struct, the member's names of struct is part of the btf_type.
> A struct with the same member's types but different member's names
> is a different btf_type.
Yes, but that's not what I'm talking about.  I'm talking about structs
 with the same member names, but with different names of the structs.
As in the following C snippet:

struct foo {
    int i;
};

int main(void)
{
    struct foo x;
    struct foo y;

    x.i = 0
    y.i = x.i;

    return y.i;
}

We have one type 'struct foo' (name "foo"), but two _instances_ of
 that type (names "x", "y").  We *cannot* use a single BTF record to
 express both "x" and its type, because its type has a name of its
 own ("foo") and there is only room in struct btf_type for one name.
Thus we must have one record for the instance "x" and another record
 for the type "foo", with the former referencing the latter.

> Having two id spaces for debug-info is confusing.  They are
> all debug-info at the end.
But they have different semantics!  Just because you have a term,
 "debug-info", that's defined to cover both, doesn't mean that they
 are the same thing.  You might as well say that passport numbers and
 telephone numbers should be drawn from the same numbering space,
 because they're both "personal information", and never mind that one
 identifies a person and the other identifies a telephone.
It's having the _same_ id space for entities that are almost, but not
 quite, entirely unlike each other that's confusing.

-Ed

Powered by blists - more mailing lists