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.
>
> 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
