On Fri, Oct 19, 2018 at 12:36:49PM -0700, Martin Lau wrote:
> On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote:
> > On 18/10/18 22:19, Martin Lau wrote:
> > > As I have mentioned earlier, it is also special to
> > > the kernel because the BTF verifier and bpf_prog_load()
> > > need to do different checks for FUNC and FUNC_PROTO to
> > > ensure they are sane.
> > >
> > > First, we need to agree that the kernel needs to verify
> > > them differently.
> > >
> > > and then we can proceed to discuss how to distinguish them.
> > > We picked the current way to avoid adding a
> > > new BTF function section and keep it
> > > strict forward to distinguish them w/o relying
> > > on other hints from 'struct btf_type'.
> > >
> > > Are you suggesting another way of doing it?
> > But you *do* have such a new section.
> > The patch comment talks about a 'FuncInfo Table' which appears to
> Note that the new section, which contains the FuncInfo Table,
> is in a new ELF section ".BTF.ext" instead of the ".BTF".
> It is not in the ".BTF" section because it is only useful during
> bpf_prog_load().
>
> I was meaning a new function section within ".BTF".
>
> > map (section, insn_idx) to type_id. (I think this gets added in
> > .BTF.ext per patch 9?) So when you're looking at a FUNC type
> > because you looked up a type_id from that table, you know it's
> > the signature of a subprogram, and you're checking it as such.
> > Whereas, if you were doing something with some other type and it
> > referenced a FUNC type (e.g., in the patch comment's example,
> > you're checking foo's first argument against the type bar) in
> > its type_id, you know you're using it as a formal type (a FUNC_
> > PROTO in your parlance) and not as a subprogram.
> > The context in which you are using a type entry tells you which
> > kind it is. And the verifier can and should be smart enough to
> > know what it's doing in this way.
> >
> > And it's entirely reasonable for the same type entry to get used
> > for both those cases; in my example, you'd have a FUNC type for
> > int foo(int), referenced both by the func_info entry for foo
> > and by the PTR type for bar. And if you had another subprogram
> > int baz(int), its func_info entry could reference the same
> > type_id, because the (reference to the) name of the function
> > should live in the func_info, not in the type.
> IIUC, I think what you are suggesting here is to use (type_id, name)
> to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}",
> "int foo3(int) {}" where type_id here is referring to the same
> DW_TAG_subroutine_type, and only define that _one_
> DW_TAG_subroutine_type in the BTF "type" section.
>
> That will require more manipulation/type-merging in the dwarf2btf
> process and it could get quite complicated.
>
> Note that CTF is also fully spelling out the return type
> and arg types for each DW_TAG_subprogram in a separate
> function section (still within the same ELF section).
> The only difference here is they are merged into the type
> section and FUNC_PROTO is added.
>
> If the concern is having both FUNC and FUNC_PROTO is confusing,
> we could go back to the CTF way which adds a new function section
> in ".BTF" and it is only for DW_TAG_subprogram.
> BTF_KIND_FUNC_PROTO is then no longer necessary.
> Some of new BTF verifier checkings may actually go away also.
> The down side is there will be two id spaces.
Discussed a bit offline with folks about the two id spaces
situation and it is not good for debugging purpose.
If we must get rid of FUNC_PROTO, it is better to use the
name_off==0 check instead of adding a new function section.
We will go for this path in the next respin.
>
> >
> > What you are proposing seems to be saying "if we have this
> > particular special btf_kind, then this BTF entry doesn't just
> > define a type, it declares a subprogram of that type". Oh,
> > and with the name of the type as the subprogram name. Which
> > just creates blurry confusion as to whether BTF entries define
> > types or declare objects; IMNSHO the correct approach is for
> > objects to be declared elsewhere and to reference BTF types by
> > their type_id.
> > Which is what the func_info table in patch 9 appears to do.
> >
> > (It also rather bothers me the way we are using special type
> > names to associate maps with their k/v types, rather than
> > extending the records in the maps section to include type_ids
> > referencing them. It's the same kind of weird implicitness,
> > and if I'd spotted it when it was going in I'd've nacked it,
> > but I suppose it's ABI now and too late to change.)
> >
> > -Ed
