[PATCH bpf-next] selftests/bpf: don't count skipped reg_bounds cases as skipped tests

[Vineet Gupta <vineet.gupta@linux.dev>]

test_progs-bpf_gcc reports an order of magnitude more skipped tests than it
lists.  A recent bpf-next run ends with

  Summary: 720/6383 PASSED, 8392 SKIPPED, 105/368 FAILED

but the log for that same run names only 684 skipped subtests which are
all accounted for; the other ~7700 are just ghosts.

Nothing here is specific to BPF_GCC; reg_bounds builds its programs
from hand-written instructions at run time, so the BPF compiler is not
involved.  That build is just where the inflated total was noticed.

The total is not reproducible either.  Four runs over an identical
tree reported 8240, 8261, 8405 and 8424 SKIPPED; two of those had
byte-identical verdicts for every test.

The extra skips come from reg_bounds.  Counting the increments by
test, every heavy contributor is one of its random tests:

  632 reg_bounds_rand_ranges_s64_u32    576 reg_bounds_rand_consts_s64_u32
  624 reg_bounds_rand_ranges_u64_u32    564 reg_bounds_rand_consts_u64_u32
  600 reg_bounds_rand_consts_u64_s32    552 reg_bounds_rand_ranges_u64_s64

The path, for one of them:

  test_reg_bounds_rand_consts_u64_u32()
  `- validate_rand_ranges()                     100 random cases
     `- verify_case_opt(..., false /* !is_subtest */)
        |    if (is_subtest && !test__start_subtest(sb->buf))
        |            return 0;       <-- no subtest is opened, so
        |                                env.subtest_state stays NULL
        `- for (op = OP_LT .. OP_NE)             x6 -> 1200 cases
           `- verify_case_op()
              `- if (reg_state_needs_two_arcs(...))
                    test__skip()
                    `- else env.test_state->skip_cnt++   <-- once per case

skip_cnt feeds the SKIPPED total, so it grows once per case rather than
once per test.  The variance has the same source: how many cases are
unrepresentable depends on the seed.

Report the skip up to verify_case_opt() instead, the one frame that
knows whether a subtest exists:

  err = verify_case_op(init_t, cond_t, x, y, sub.op, &skipped);
  if (skipped) {
          if (is_subtest)
                  test__skip();
          else
                  ctx->skip_case_cnt++;
  }

with the tally printed once at the end of the test.  The subtest path
keeps its old behaviour: test__skip() only sets a bool there.

After the change, a run of reg_bounds_rand_consts_u64_u32 -- the test
contributing 564 above -- reports 0 SKIPPED and logs

  SKIPPED 480/1200 cases needing two range arcs

The two counts differ because the seeds do.

This also stops the rand_* tests reporting themselves as skipped: with
no subtest open, any non-zero skip_cnt made print_test_result() call
the whole test SKIP, so a test that ran 1200 cases and checked them all
was indistinguishable from one that never ran.  18 of the 32 report OK
instead; the rest were already OK, their type combinations never
producing an unrepresentable case.

Fixes: bbc631085503 ("bpf: replace min/max fields with struct cnum{32,64}")
Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
---
 .../selftests/bpf/prog_tests/reg_bounds.c     | 25 ++++++++++++++++---
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
index 7f170a69d1d8..6231258b26cc 100644
--- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
+++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
@@ -1504,7 +1504,7 @@ struct ctx {
 	struct range *uranges, *sranges;
 	struct range *usubranges, *ssubranges;
 	int max_failure_cnt, cur_failure_cnt;
-	int total_case_cnt, case_cnt;
+	int total_case_cnt, case_cnt, skip_case_cnt;
 	int rand_case_cnt;
 	unsigned rand_seed;
 	__u64 start_ns;
@@ -1541,7 +1541,8 @@ static void subtest_case_str(struct strbuf *sb, struct subtest_case *t, bool use
  * performed)
  */
 static int verify_case_op(enum num_t init_t, enum num_t cond_t,
-			  struct range x, struct range y, enum op op)
+			  struct range x, struct range y, enum op op,
+			  bool *skipped)
 {
 	char log_buf[256 * 1024];
 	size_t log_sz = sizeof(log_buf);
@@ -1581,7 +1582,7 @@ static int verify_case_op(enum num_t init_t, enum num_t cond_t,
 	    !assert_reg_state_eq(&tr2, &te2, "true_reg2")) {
 		if (reg_state_needs_two_arcs(&fe1) || reg_state_needs_two_arcs(&fe2) ||
 		    reg_state_needs_two_arcs(&te1) || reg_state_needs_two_arcs(&te2)) {
-			test__skip();
+			*skipped = true;
 			return 0;
 		}
 		failed = true;
@@ -1629,13 +1630,26 @@ static int verify_case_opt(struct ctx *ctx, enum num_t init_t, enum num_t cond_t
 		return 0;
 
 	for (sub.op = first_op; sub.op <= last_op; sub.op++) {
+		bool skipped = false;
+
 		sb->pos = 0; /* reset position in strbuf */
 		subtest_case_str(sb, &sub, true /* print op */);
 
 		if (env.verbosity >= VERBOSE_NORMAL) /* this speeds up debugging */
 			printf("TEST CASE: %s\n", sb->buf);
 
-		err = verify_case_op(init_t, cond_t, x, y, sub.op);
+		err = verify_case_op(init_t, cond_t, x, y, sub.op, &skipped);
+		if (skipped) {
+			/*
+			 * Outside a subtest test__skip() bumps the test's skip
+			 * counter, which feeds the SKIPPED total; tally these
+			 * locally instead of reporting each as a skipped test.
+			 */
+			if (is_subtest)
+				test__skip();
+			else
+				ctx->skip_case_cnt++;
+		}
 		if (err || env.verbosity >= VERBOSE_NORMAL)
 			ASSERT_OK(err, sb->buf);
 		if (err) {
@@ -2167,6 +2181,9 @@ static void validate_rand_ranges(enum num_t init_t, enum num_t cond_t, bool cons
 	}
 
 cleanup:
+	if (ctx.skip_case_cnt)
+		printf("SKIPPED %d/%d cases needing two range arcs\n",
+		       ctx.skip_case_cnt, ctx.case_cnt);
 	/* make sure we report random seed for reproducing */
 	ASSERT_TRUE(true, ctx.progress_ctx);
 	cleanup_ctx(&ctx);
-- 
2.53.0-Meta