Re: [PATCH bpf-next v1 4/4] selftests/bpf: Check that sorting preserves types in bpftool dump

On Thu, 2026-08-20 at 13:58 -0700, Ihor Solodrai wrote:
> On 2026-08-20 1:42 p.m., Eduard Zingerman wrote:
> > On Wed, 2026-08-19 at 17:06 -0700, Ihor Solodrai wrote:
> > 
> > ...
> > 
> > > @@ -87,6 +90,36 @@ static struct btf *mk_btf(void)
> > >   	if (!ASSERT_OK(err, "holey_tail"))
> > >   		goto err_out;
> > >   
> > > +	for (i = 0; i < 3; i++) {
> > 
> > Testing this locally, a single enum suffices to showcase the bug,
> > why the complication?
> 
> Hi Eduard, thank you for reviewing so quickly.
> 
> For the bug that was fixed, yes. But in general, I thought it's good to 
> have a couple of different things in the test sample.

We have a good set of tests for libbpf's BTF dump logic (not bpftool).
Given that, I think it is better to keep tests focused.

> 
> > 
> > > +		char name[16];
> > > +
> > > +		snprintf(name, sizeof(name), "E%d", i);
> > > +
> > > +		id = btf__add_enum(btf, NULL, 4);
> > > +		if (!ASSERT_GT(id, 0, "anon_enum"))
> > > +			goto err_out;
> > > +
> > > +		err = btf__add_enum_value(btf, name, i + 1);
> > > +		if (!ASSERT_OK(err, "enum_val"))
> > > +			goto err_out;
> > > +	}
> > > +
> > > +	id = btf__add_struct(btf, "s", 4);
> > > +	if (!ASSERT_GT(id, 0, "struct_s"))
> > > +		goto err_out;
> > > +
> > > +	err = btf__add_field(btf, "f", 1, 0, 0);
> > > +	if (!ASSERT_OK(err, "s_field"))
> > > +		goto err_out;
> > > +
> > > +	id = btf__add_union(btf, "u", 4);
> > > +	if (!ASSERT_GT(id, 0, "union_u"))
> > > +		goto err_out;
> > > +
> > > +	err = btf__add_field(btf, "f", 1, 0, 0);
> > > +	if (!ASSERT_OK(err, "u_field"))
> > > +		goto err_out;
> > > +
> > >   	btf__set_pointer_size(btf, 4);
> > >   
> > >   	return btf;
> > > @@ -95,6 +128,42 @@ static struct btf *mk_btf(void)
> > 
> > ...
> > 
> > > +static void test_sort_preserves_types(const char *path)
> > > +{
> > > +	char *sorted = NULL, *unsorted = NULL;
> > > +	int n_sorted, n_unsorted;
> > > +
> > > +	sorted = dump_c(path, true);
> > > +	unsorted = dump_c(path, false);
> > > +	if (!sorted || !unsorted)
> > > +		goto out;
> > > +
> > > +	/*
> > > +	 * Count definitions by the closing brace in their first column. Counting
> > > +	 * "struct"/"union"/"enum" openers instead would also count forward
> > > +	 * declarations, and which types need one depends on emission order.
> > > +	 */
> > > +	n_unsorted = count_substr(unsorted, "\n}");
> > > +	n_sorted = count_substr(sorted, "\n}");
> > 
> > This does not include e.g. typedef. I'd just match for presence of enum.
> > Or better yet, compare full expected output with actual one and tailor
> > several BTF definitions, each for a specific sub-test.
> 
> I wanted to avoid committing a sample generated header. I think the
> exact string check will also be too sensitive to the changes in libbpf
> dumper or bpftool.
> 
> Presence of enum only tests the concrete bug, and I wanted some simple
> proxy to a more generic sorted vs unsorted output check.
> That is, "did we drop anything while sorting?" check.
> 
> Ideally we'd need to parse the headers and compare the full set of
> emitted types.. An overkill here, I think.
> 
> We can just add counting typedefs as well.
> That's an easy improvement. wdyt?

I don't think bpftool output changes all that match.
I won't block on this, but working with text is simpler in case if the
test fails. One can get the 'diff' output directly pointing at what
went wrong (and it would be nice to have a utility that shows the diff
in case of mismatch, btf dump tests do that, but it is not abstracted).

> 
> > 
> > > +	ASSERT_GT(n_unsorted, 0, "types_emitted");
> > > +	ASSERT_EQ(n_sorted, n_unsorted, "same_type_count");
> > > +
> > > +out:
> > > +	free(sorted);
> > > +	free(unsorted);
> > > +}
> > > +
> > >   /*
> > >    * Check only what the selftests build cannot:
> > >    *   - bpf_helpers.h defines __ksym and __weak as well, and no program uses
> > 
> > ...