Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

btf: export DeclTag and TypeTag #1548

Merged
merged 2 commits into from
Oct 9, 2024
Merged

btf: export DeclTag and TypeTag #1548

merged 2 commits into from
Oct 9, 2024

Conversation

dylandreimerink
Copy link
Member

@dylandreimerink dylandreimerink commented Aug 20, 2024

This commit makes the TypeTag BTF type exported. It had not been
exported because it was lumped in with the DeclTag type. However,
no modification to the codebase seems to be needed to accommodate
the usage of TypeTags.
This commit makes the DeclTag BTF type exported. The DeclTag has been
unexported because its encoding is a bit tricky.

DeclTags on the wire refer to a type by its ID, and an component index.
Unlike other BTF types, the type ID isn't always the actual "target",
instead if the component index is not `-1` it targets a child object of
the type. This commit adds a `Tags` field to all types that can have
DeclTags including Members.

Exception is function parameters. Due to the func -> funcProto ->
parameter encoding, it is possible that BTF deduplication reuses the
same funcProto for multiple functions. The declTags point to the func to
annotate the arguments. Thus, adding a `Tags` field to Param would
cause inaccurate BTF marshalling. Instead, a `ParamTags` field is added
the func as `[][]string` to match the write format more closely to
avoid issues caused by BTF deduplication. This is a compromise between
technical feasibility and user experience. Users need to manually match
up the `ParamTags` with the `Params`. This shouldn't be a huge issue
in most cases however.

This means we now effectively have less "types" in our Go BTF
representation then we have on the wire since declTags become strings
on other types. This might leave "holes" in our BTF type numbering
if we were to remove them. So instead we leave the `declTag` types in
the BTF type list, but we hide them from the user during iteration.
Technically, users can still access them by ID if they know the ID, but
like before this commit, the returned value can only be printed.

Marshalling relies on the iteration of the BTF types. When go to load
a BTF blob from a collection, we iterate from a collection of root types
which are directly used. This causes us to load only BTF info that is
actually in use. This commit introduces a trick into the `children`
logic used during iteration. When we iterate over a type with declTags,
we create internal `declTag` types at-hoc with the correct component
index. So even though the tags are just strings on other types, when
iterating they appear as actual types. This causes only declTags that
are on types which are in use to be marshalled.

@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from 136c137 to b37d1e4 Compare August 20, 2024 12:13
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from b37d1e4 to 57d76e7 Compare August 22, 2024 15:00
@github-actions github-actions bot added the breaking-change Changes exported API label Aug 22, 2024
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from 57d76e7 to 5c9fca4 Compare August 22, 2024 15:08
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch 2 times, most recently from 95ec655 to 1c26ad8 Compare September 26, 2024 13:56
@dylandreimerink dylandreimerink marked this pull request as ready for review September 26, 2024 14:07
@dylandreimerink dylandreimerink requested a review from a team September 26, 2024 14:08
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from 1c26ad8 to 66cfe61 Compare September 26, 2024 14:37
btf/traversal.go Outdated Show resolved Hide resolved
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from 66cfe61 to 810a87b Compare September 27, 2024 08:56
btf/btf.go Outdated Show resolved Hide resolved
btf/types.go Outdated Show resolved Hide resolved
@mejedi
Copy link
Contributor

mejedi commented Sep 27, 2024

It looks like .copy methods on nodes should copy .Tags. Apparently, children function powers both marshalling and deep copying. In the context of deep copying, creating and yielding declTags is not strictly necessary.

@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch 2 times, most recently from a1d1e75 to db03777 Compare October 3, 2024 13:01
@dylandreimerink dylandreimerink requested a review from ti-mo October 3, 2024 13:01
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from db03777 to f19f0d9 Compare October 8, 2024 13:38
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Left a few comments, haven't looked at the tests in-depth.

btf/btf.go Outdated Show resolved Hide resolved
btf/testdata/tags.c Show resolved Hide resolved
btf/traversal.go Show resolved Hide resolved
btf/traversal.go Outdated Show resolved Hide resolved
btf/types.go Outdated Show resolved Hide resolved
btf/types.go Show resolved Hide resolved
btf/types.go Outdated Show resolved Hide resolved
@dylandreimerink dylandreimerink force-pushed the feature/btf-typetag-decltag branch from f19f0d9 to 74d069e Compare October 9, 2024 11:32
@dylandreimerink dylandreimerink requested a review from ti-mo October 9, 2024 11:33
This commit makes the TypeTag BTF type exported. It had not been
exported because it was lumped in with the DeclTag type. However,
no modification to the codebase seems to be needed to accommodate
the usage of TypeTags.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
This commit makes the DeclTag BTF type exported. The DeclTag has been
unexported because its encoding is a bit tricky.

DeclTags on the wire refer to a type by its ID, and an component index.
Unlike other BTF types, the type ID isn't always the actual "target",
instead if the component index is not `-1` it targets a child object of
the type. This commit adds a `Tags` field to all types that can have
DeclTags including Members.

Exception is function parameters. Due to the func -> funcProto ->
parameter encoding, it is possible that BTF deduplication reuses the
same funcProto for multiple functions. The declTags point to the func to
annotate the arguments. Thus, adding a `Tags` field to Param would
cause inaccurate BTF marshalling. Instead, a `ParamTags` field is added
the func as `[][]string` to match the write format more closely to
avoid issues caused by BTF deduplication. This is a compromise between
technical feasibility and user experience. Users need to manually match
up the `ParamTags` with the `Params`. This shouldn't be a huge issue
in most cases however.

This means we now effectively have less "types" in our Go BTF
representation then we have on the wire since declTags become strings
on other types. This might leave "holes" in our BTF type numbering
if we were to remove them. So instead we leave the `declTag` types in
the BTF type list, but we hide them from the user during iteration.
Technically, users can still access them by ID if they know the ID, but
like before this commit, the returned value can only be printed.

Marshalling relies on the iteration of the BTF types. When go to load
a BTF blob from a collection, we iterate from a collection of root types
which are directly used. This causes us to load only BTF info that is
actually in use. This commit introduces a trick into the `children`
logic used during iteration. When we iterate over a type with declTags,
we create internal `declTag` types at-hoc with the correct component
index. So even though the tags are just strings on other types, when
iterating they appear as actual types. This causes only declTags that
are on types which are in use to be marshalled.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@ti-mo ti-mo force-pushed the feature/btf-typetag-decltag branch from 74d069e to 2c0d9a6 Compare October 9, 2024 14:06
@ti-mo ti-mo changed the title Export DeclTag and TypeTag btf: export DeclTag and TypeTag Oct 9, 2024
@ti-mo ti-mo merged commit d4b2f8c into main Oct 9, 2024
16 checks passed
@ti-mo ti-mo deleted the feature/btf-typetag-decltag branch October 9, 2024 14:28
@mejedi
Copy link
Contributor

mejedi commented Oct 10, 2024

It looks like .copy methods on nodes should copy .Tags. Apparently, children function powers both marshalling and deep copying. In the context of deep copying, creating and yielding declTags is not strictly necessary.

Submitted a follow-up to handle .Tags in .copy: #1582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes exported API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants