Re: [PATCH v2 bpf-next 03/18] libbpf: Support moving permuted BTF types into split BTF

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4502 bytes --]

> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 9449f6f50f18..30f8c426d145 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -2147,16 +2147,9 @@ int btf__find_str(struct btf *btf, const char *s)
>   *   - > 0 offset into string section, on success;
>   *   - < 0, on error.
>   */
> -int btf__add_str(struct btf *btf, const char *s)
> +static int btf_add_local_str(struct btf *btf, const char *s)
>  {
> -	int off;
> -	int err;
> -
> -	if (btf->base_btf) {
> -		off = btf__find_str(btf->base_btf, s);
> -		if (off != -ENOENT)
> -			return off;
> -	}
> +	int off, err;
>
>  	err = btf_ensure_modifiable(btf);
>  	if (err)
> @@ -2167,10 +2160,24 @@ int btf__add_str(struct btf *btf, const char *s)
>  		return libbpf_err(off);
>
>  	btf->hdr.str_len = strset__data_size(btf->strs_set);
> +	btf->strs_deduped = false;

The refactoring left the API doc comment attached to the new static
helper btf_add_local_str(), so the public btf__add_str() (btf.c:2168) now
has no documentation at all, and its declaration at tools/lib/bpf/btf.h:202
is bare without doxygen. The comment is also no longer descriptive of what
it now documents: the distinguishing property of btf_add_local_str() versus
btf__add_str() is that it always adds to this BTF's own string section and
never reuses a string already present in base_btf, which is exactly what
makes the new transfer mode able to keep split-only strings out of the base.

Should the comment be moved back onto btf__add_str() and btf_add_local_str()
given a one-liner about bypassing the base string table?

The new btf->strs_deduped = false sits in btf_add_local_str(), which is on
the path of every public btf__add_str() call (and therefore of btf__add_int,
btf__add_var, btf__add_field, btf__add_enum_value, btf__add_func_param,
btf__add_datasec, btf__add_locsec, btf_add_composite, btf_add_decl_tag,
btf_add_ref_kind, btf_add_enum_common, and more), not just the new transfer
path.

Before this commit nothing in tools/lib/bpf ever cleared strs_deduped. The
complete set of writers post-commit is: btf_ensure_modifiable() sets it true
when hdr.str_len == 0 or when !base_btf && str_len == 1, end of
btf_dedup_strings() sets it true, plus the two new clears here and in
btf_compact_strings().

For BTF built from scratch, btf__new_empty() (str_len == 1) and
btf__new_empty_split() (str_len == 0) both make btf_ensure_modifiable() set
strs_deduped = true, so pre-commit btf_dedup_strings() returned 0 at its
early-return check and the string table was never rebuilt. Post-commit the
first added string clears the flag, so btf__dedup() now always rebuilds the
string table for such BTF: strings are reordered and unreferenced strings
are dropped. Any offset a caller previously obtained from the public
btf__add_str() and stashed outside a BTF type's name field is therefore
silently invalidated across btf__dedup().

This is not needed by the commit: both btf_compact_strings() call sites
already force btf->strs_deduped = false themselves at btf.c:4536, so the
transfer path does not depend on btf_add_local_str() clearing it.

All in-tree consumers were checked and are safe: bpf_linker's
finalize_btf() (tools/lib/bpf/linker.c:2860) runs finalize_btf_ext() first
and then passes opts.btf_ext = linker->btf_ext to btf__dedup(), so the
offsets produced at linker.c:1968, 2697, 2703, 2740 and 2952 all get
remapped; bpftool merge_btf_files() (tools/bpf/bpftool/btf.c:880) and
resolve_btfids finalize_btf() (tools/bpf/resolve_btfids/main.c:1578) hold no
external string offsets. So there is no in-tree breakage, but this is an
unadvertised behaviour change to a public API plus a full string-table
rebuild on every static link, and the commit message does not mention
btf__add_str() or strs_deduped at all.

Is the clearing of strs_deduped here intentional for non-transfer use of
btf__add_str(), or should it only be cleared in btf_compact_strings() where
the transfer path already sets it?

>
>  	return btf->start_str_off + off;
>  }
>
> +int btf__add_str(struct btf *btf, const char *s)
> +{
> +	int off;
> +
> +	if (btf->base_btf) {
> +		off = btf__find_str(btf->base_btf, s);
> +		if (off != -ENOENT)
> +			return off;
> +	}
> +
> +	return btf_add_local_str(btf, s);
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33537080133