* [PATCH bpf-next 0/2] Regular expression support for test output matching
@ 2024-06-03 15:53 Cupertino Miranda
2024-06-03 15:53 ` [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression Cupertino Miranda
2024-06-03 15:53 ` [PATCH bpf-next 2/2] selftests/bpf: Match tests against " Cupertino Miranda
0 siblings, 2 replies; 13+ messages in thread
From: Cupertino Miranda @ 2024-06-03 15:53 UTC (permalink / raw)
To: bpf; +Cc: Cupertino Miranda
Hi everyone,
This patch is in the context of fixing all self-tests for GCC.
I have identified that many of the tests are checking for particular
output matching that is very tight to the code that CLANG generates.
This sort of approach for validation is very fragile and tight to
precise compiler expectations that cannot be guaranteed, even from
different versions of the same compiler.
This patch series introduces a regular expression based approach to
output matching, allowing to validate output without being so tight to
precise output.
Looking forward to your comments.
Best regards,
Cupertino
Cupertino Miranda (2):
selftests/bpf: Support checks against a regular expression.
selftests/bpf: Match tests against regular expression.
tools/testing/selftests/bpf/progs/bpf_misc.h | 11 +-
.../testing/selftests/bpf/progs/dynptr_fail.c | 6 +-
.../selftests/bpf/progs/exceptions_assert.c | 8 +-
.../testing/selftests/bpf/progs/rbtree_fail.c | 8 +-
.../bpf/progs/refcounted_kptr_fail.c | 4 +-
.../selftests/bpf/progs/verifier_sock.c | 4 +-
tools/testing/selftests/bpf/test_loader.c | 126 ++++++++++++++----
7 files changed, 120 insertions(+), 47 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression.
2024-06-03 15:53 [PATCH bpf-next 0/2] Regular expression support for test output matching Cupertino Miranda
@ 2024-06-03 15:53 ` Cupertino Miranda
2024-06-04 18:16 ` Andrii Nakryiko
2024-06-04 21:35 ` Eduard Zingerman
2024-06-03 15:53 ` [PATCH bpf-next 2/2] selftests/bpf: Match tests against " Cupertino Miranda
1 sibling, 2 replies; 13+ messages in thread
From: Cupertino Miranda @ 2024-06-03 15:53 UTC (permalink / raw)
To: bpf
Cc: Cupertino Miranda, jose.marchesi, david.faust, Yonghong Song,
Eduard Zingerman, Andrii Nakryiko
Add support for __regex and __regex_unpriv macros to check the test
execution output against a regular expression. This is similar to __msg
and __msg_unpriv, however those only allow to do full text matching.
Signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
Cc: jose.marchesi@oracle.com
Cc: david.faust@oracle.com
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
---
tools/testing/selftests/bpf/progs/bpf_misc.h | 11 +-
tools/testing/selftests/bpf/test_loader.c | 126 ++++++++++++++-----
2 files changed, 105 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index fb2f5513e29e..c0280bd2f340 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -7,9 +7,9 @@
*
* The test_loader sequentially loads each program in a skeleton.
* Programs could be loaded in privileged and unprivileged modes.
- * - __success, __failure, __msg imply privileged mode;
- * - __success_unpriv, __failure_unpriv, __msg_unpriv imply
- * unprivileged mode.
+ * - __success, __failure, __msg, __regex imply privileged mode;
+ * - __success_unpriv, __failure_unpriv, __msg_unpriv, __regex_unpriv
+ * imply unprivileged mode.
* If combination of privileged and unprivileged attributes is present
* both modes are used. If none are present privileged mode is implied.
*
@@ -24,6 +24,9 @@
* Multiple __msg attributes could be specified.
* __msg_unpriv Same as __msg but for unprivileged mode.
*
+ * __regex Same as __msg, but using a regular expression.
+ * __regex_unpriv Same as __msg_unpriv but using a regular expression.
+ *
* __success Expect program load success in privileged mode.
* __success_unpriv Expect program load success in unprivileged mode.
*
@@ -59,10 +62,12 @@
* __auxiliary_unpriv Same, but load program in unprivileged mode.
*/
#define __msg(msg) __attribute__((btf_decl_tag("comment:test_expect_msg=" msg)))
+#define __regex(regex) __attribute__((btf_decl_tag("comment:test_expect_regex=" regex)))
#define __failure __attribute__((btf_decl_tag("comment:test_expect_failure")))
#define __success __attribute__((btf_decl_tag("comment:test_expect_success")))
#define __description(desc) __attribute__((btf_decl_tag("comment:test_description=" desc)))
#define __msg_unpriv(msg) __attribute__((btf_decl_tag("comment:test_expect_msg_unpriv=" msg)))
+#define __regex_unpriv(regex) __attribute__((btf_decl_tag("comment:test_expect_regex_unpriv=" regex)))
#define __failure_unpriv __attribute__((btf_decl_tag("comment:test_expect_failure_unpriv")))
#define __success_unpriv __attribute__((btf_decl_tag("comment:test_expect_success_unpriv")))
#define __log_level(lvl) __attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 524c38e9cde4..c73fa04bca1b 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -2,6 +2,7 @@
/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
#include <linux/capability.h>
#include <stdlib.h>
+#include <regex.h>
#include <test_progs.h>
#include <bpf/btf.h>
@@ -17,9 +18,11 @@
#define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
#define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
#define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
+#define TEST_TAG_EXPECT_REGEX_PFX "comment:test_expect_regex="
#define TEST_TAG_EXPECT_FAILURE_UNPRIV "comment:test_expect_failure_unpriv"
#define TEST_TAG_EXPECT_SUCCESS_UNPRIV "comment:test_expect_success_unpriv"
#define TEST_TAG_EXPECT_MSG_PFX_UNPRIV "comment:test_expect_msg_unpriv="
+#define TEST_TAG_EXPECT_REGEX_PFX_UNPRIV "comment:test_expect_regex_unpriv="
#define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
#define TEST_TAG_DESCRIPTION_PFX "comment:test_description="
@@ -46,10 +49,15 @@ enum mode {
UNPRIV = 2
};
+struct expect_msg {
+ const char *msg;
+ regex_t *regex;
+};
+
struct test_subspec {
char *name;
bool expect_failure;
- const char **expect_msgs;
+ struct expect_msg *expect;
size_t expect_msg_cnt;
int retval;
bool execute;
@@ -91,27 +99,57 @@ static void free_test_spec(struct test_spec *spec)
{
free(spec->priv.name);
free(spec->unpriv.name);
- free(spec->priv.expect_msgs);
- free(spec->unpriv.expect_msgs);
+ free(spec->priv.expect);
+ free(spec->unpriv.expect);
spec->priv.name = NULL;
spec->unpriv.name = NULL;
- spec->priv.expect_msgs = NULL;
- spec->unpriv.expect_msgs = NULL;
+ spec->priv.expect = NULL;
+ spec->unpriv.expect = NULL;
}
static int push_msg(const char *msg, struct test_subspec *subspec)
{
void *tmp;
- tmp = realloc(subspec->expect_msgs, (1 + subspec->expect_msg_cnt) * sizeof(void *));
+ tmp = realloc(subspec->expect,
+ (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
if (!tmp) {
ASSERT_FAIL("failed to realloc memory for messages\n");
return -ENOMEM;
}
- subspec->expect_msgs = tmp;
- subspec->expect_msgs[subspec->expect_msg_cnt++] = msg;
+ subspec->expect = tmp;
+ subspec->expect[subspec->expect_msg_cnt].msg = msg;
+ subspec->expect[subspec->expect_msg_cnt].regex = NULL;
+ subspec->expect_msg_cnt += 1;
+ return 0;
+}
+
+static int push_regex(const char *regex_str, struct test_subspec *subspec)
+{
+ void *tmp;
+ int regcomp_res;
+
+ tmp = realloc(subspec->expect,
+ (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
+ if (!tmp) {
+ ASSERT_FAIL("failed to realloc memory for messages\n");
+ return -ENOMEM;
+ }
+ subspec->expect = tmp;
+
+ subspec->expect[subspec->expect_msg_cnt].regex = (regex_t *) malloc(sizeof(regex_t));
+ regcomp_res = regcomp (subspec->expect[subspec->expect_msg_cnt].regex,
+ regex_str, REG_EXTENDED|REG_NEWLINE);
+ if (regcomp_res != 0) {
+ fprintf(stderr, "Regexp: '%s'\n", regex_str);
+ ASSERT_FAIL("failed to compile regex\n");
+ return -EINVAL;
+ }
+
+ subspec->expect[subspec->expect_msg_cnt].msg = regex_str;
+ subspec->expect_msg_cnt += 1;
return 0;
}
@@ -243,6 +281,18 @@ static int parse_test_spec(struct test_loader *tester,
if (err)
goto cleanup;
spec->mode_mask |= UNPRIV;
+ } else if (str_has_pfx(s, TEST_TAG_EXPECT_REGEX_PFX)) {
+ msg = s + sizeof(TEST_TAG_EXPECT_REGEX_PFX) - 1;
+ err = push_regex(msg, &spec->priv);
+ if (err)
+ goto cleanup;
+ spec->mode_mask |= PRIV;
+ } else if (str_has_pfx(s, TEST_TAG_EXPECT_REGEX_PFX_UNPRIV)) {
+ msg = s + sizeof(TEST_TAG_EXPECT_REGEX_PFX_UNPRIV) - 1;
+ err = push_regex(msg, &spec->unpriv);
+ if (err)
+ goto cleanup;
+ spec->mode_mask |= UNPRIV;
} else if (str_has_pfx(s, TEST_TAG_RETVAL_PFX)) {
val = s + sizeof(TEST_TAG_RETVAL_PFX) - 1;
err = parse_retval(val, &spec->priv.retval, "__retval");
@@ -336,16 +386,16 @@ static int parse_test_spec(struct test_loader *tester,
spec->unpriv.execute = spec->priv.execute;
}
- if (!spec->unpriv.expect_msgs) {
- size_t sz = spec->priv.expect_msg_cnt * sizeof(void *);
+ if (!spec->unpriv.expect) {
+ size_t sz = spec->priv.expect_msg_cnt * sizeof(struct expect_msg);
- spec->unpriv.expect_msgs = malloc(sz);
- if (!spec->unpriv.expect_msgs) {
- PRINT_FAIL("failed to allocate memory for unpriv.expect_msgs\n");
+ spec->unpriv.expect = malloc(sz);
+ if (!spec->unpriv.expect) {
+ PRINT_FAIL("failed to allocate memory for unpriv.expect\n");
err = -ENOMEM;
goto cleanup;
}
- memcpy(spec->unpriv.expect_msgs, spec->priv.expect_msgs, sz);
+ memcpy(spec->unpriv.expect, spec->priv.expect, sz);
spec->unpriv.expect_msg_cnt = spec->priv.expect_msg_cnt;
}
}
@@ -403,26 +453,44 @@ static void validate_case(struct test_loader *tester,
int load_err)
{
int i, j;
+ const char *match;
for (i = 0; i < subspec->expect_msg_cnt; i++) {
- char *match;
const char *expect_msg;
+ regex_t *regex;
+ regmatch_t reg_match[1];
+
+ expect_msg = subspec->expect[i].msg;
+ regex = subspec->expect[i].regex;
+
+ if (regex == NULL) {
+ match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
+ if (!ASSERT_OK_PTR (match, "expect_msg")) {
+ /* if we are in verbose mode, we've already emitted log */
+ if (env.verbosity == VERBOSE_NONE)
+ emit_verifier_log(tester->log_buf, true /*force*/);
+ for (j = 0; j < i; j++)
+ fprintf(stderr,
+ "MATCHED MSG: '%s'\n", subspec->expect[j].msg);
+ fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
+ return;
+ }
+ tester->next_match_pos = match - tester->log_buf + strlen(expect_msg);
+ } else {
+ int match_size = regexec (regex, tester->log_buf + tester->next_match_pos, 1, reg_match, 0);
+ if (match_size != 1) {
+ /* if we are in verbose mode, we've already emitted log */
+ if (env.verbosity == VERBOSE_NONE)
+ emit_verifier_log(tester->log_buf, true /*force*/);
+ for (j = 0; j < i; j++)
+ fprintf(stderr,
+ "MATCHED REGEX: '%s'\n", subspec->expect[j].msg);
+ fprintf(stderr, "EXPECTED REGEX: '%s'\n", expect_msg);
+ return;
+ }
- expect_msg = subspec->expect_msgs[i];
-
- match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
- if (!ASSERT_OK_PTR(match, "expect_msg")) {
- /* if we are in verbose mode, we've already emitted log */
- if (env.verbosity == VERBOSE_NONE)
- emit_verifier_log(tester->log_buf, true /*force*/);
- for (j = 0; j < i; j++)
- fprintf(stderr,
- "MATCHED MSG: '%s'\n", subspec->expect_msgs[j]);
- fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
- return;
+ tester->next_match_pos += reg_match[0].rm_eo;
}
-
- tester->next_match_pos = match - tester->log_buf + strlen(expect_msg);
}
}
--
2.39.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-03 15:53 [PATCH bpf-next 0/2] Regular expression support for test output matching Cupertino Miranda
2024-06-03 15:53 ` [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression Cupertino Miranda
@ 2024-06-03 15:53 ` Cupertino Miranda
2024-06-04 18:16 ` Andrii Nakryiko
1 sibling, 1 reply; 13+ messages in thread
From: Cupertino Miranda @ 2024-06-03 15:53 UTC (permalink / raw)
To: bpf
Cc: Cupertino Miranda, jose.marchesi, david.faust, Yonghong Song,
Eduard Zingerman, Andrii Nakryiko
This patch changes a few tests to make use of regular expressions such
that the test validation would allow to properly verify the tests when
compiled with GCC.
signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
Cc: jose.marchesi@oracle.com
Cc: david.faust@oracle.com
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
---
tools/testing/selftests/bpf/progs/dynptr_fail.c | 6 +++---
tools/testing/selftests/bpf/progs/exceptions_assert.c | 8 ++++----
tools/testing/selftests/bpf/progs/rbtree_fail.c | 8 ++++----
tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
tools/testing/selftests/bpf/progs/verifier_sock.c | 4 ++--
5 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 66a60bfb5867..64cc9d936a13 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
* mem_or_null pointers.
*/
SEC("?raw_tp")
-__failure __msg("R1 type=scalar expected=percpu_ptr_")
+__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
int dynptr_invalidate_slice_or_null(void *ctx)
{
struct bpf_dynptr ptr;
@@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
/* Destruction of dynptr should also any slices obtained from it */
SEC("?raw_tp")
-__failure __msg("R7 invalid mem access 'scalar'")
+__failure __regex("R[0-9]+ invalid mem access 'scalar'")
int dynptr_invalidate_slice_failure(void *ctx)
{
struct bpf_dynptr ptr1;
@@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
/* bpf_dynptr_slice()s are read-only and cannot be written to */
SEC("?tc")
-__failure __msg("R0 cannot write into rdonly_mem")
+__failure __regex("R[0-9]+ cannot write into rdonly_mem")
int skb_invalid_slice_write(struct __sk_buff *skb)
{
struct bpf_dynptr ptr;
diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
index 5e0a1ca96d4e..deb67d198caf 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
@@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
SEC("?tc")
__log_level(2) __failure
-__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
+__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
int check_assert_range_s64(struct __sk_buff *ctx)
{
struct bpf_sock *sk = ctx->sk;
@@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
SEC("?tc")
__log_level(2) __failure
-__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
+__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
int check_assert_range_u64(struct __sk_buff *ctx)
{
u64 num = ctx->len;
@@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
SEC("?tc")
__log_level(2) __failure
-__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
+__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
int check_assert_single_range_s64(struct __sk_buff *ctx)
{
struct bpf_sock *sk = ctx->sk;
@@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
SEC("?tc")
__log_level(2) __failure
-__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
+__msg("R1=pkt(off=64,r=64)")
int check_assert_generic(struct __sk_buff *ctx)
{
u8 *data_end = (void *)(long)ctx->data_end;
diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
index 3fecf1c6dfe5..8399304eca72 100644
--- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
+++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
@@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
}
SEC("?tc")
-__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
+__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
long rbtree_api_nolock_add(void *ctx)
{
struct node_data *n;
@@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
}
SEC("?tc")
-__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
+__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
long rbtree_api_nolock_remove(void *ctx)
{
struct node_data *n;
@@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
}
SEC("?tc")
-__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
+__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
long rbtree_api_nolock_first(void *ctx)
{
bpf_rbtree_first(&groot);
@@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
}
SEC("?tc")
-__failure __msg("Unreleased reference id=3 alloc_insn=10")
+__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
long rbtree_api_remove_no_drop(void *ctx)
{
struct bpf_rb_node *res;
diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
index 1553b9c16aa7..f8d4b7cfcd68 100644
--- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
@@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
}
SEC("?tc")
-__failure __msg("Unreleased reference id=4 alloc_insn=21")
+__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
long rbtree_refcounted_node_ref_escapes(void *ctx)
{
struct node_acquire *n, *m;
@@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
}
SEC("?tc")
-__failure __msg("Unreleased reference id=3 alloc_insn=9")
+__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
{
struct node_acquire *n, *m;
diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
index ee76b51005ab..450b57933c79 100644
--- a/tools/testing/selftests/bpf/progs/verifier_sock.c
+++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
@@ -799,7 +799,7 @@ l0_%=: r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]); \
SEC("sk_skb")
__description("bpf_map_lookup_elem(sockmap, &key)")
-__failure __msg("Unreleased reference id=2 alloc_insn=6")
+__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
__naked void map_lookup_elem_sockmap_key(void)
{
asm volatile (" \
@@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
SEC("sk_skb")
__description("bpf_map_lookup_elem(sockhash, &key)")
-__failure __msg("Unreleased reference id=2 alloc_insn=6")
+__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
__naked void map_lookup_elem_sockhash_key(void)
{
asm volatile (" \
--
2.39.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression.
2024-06-03 15:53 ` [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression Cupertino Miranda
@ 2024-06-04 18:16 ` Andrii Nakryiko
2024-06-04 21:35 ` Eduard Zingerman
1 sibling, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-06-04 18:16 UTC (permalink / raw)
To: Cupertino Miranda
Cc: bpf, jose.marchesi, david.faust, Yonghong Song, Eduard Zingerman
On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
> Add support for __regex and __regex_unpriv macros to check the test
> execution output against a regular expression. This is similar to __msg
> and __msg_unpriv, however those only allow to do full text matching.
>
> Signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
> Cc: jose.marchesi@oracle.com
> Cc: david.faust@oracle.com
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> ---
> tools/testing/selftests/bpf/progs/bpf_misc.h | 11 +-
> tools/testing/selftests/bpf/test_loader.c | 126 ++++++++++++++-----
> 2 files changed, 105 insertions(+), 32 deletions(-)
>
This is useful, I have a few implementation/stylistical nits below.
pw-bot: cr
> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index fb2f5513e29e..c0280bd2f340 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -7,9 +7,9 @@
> *
> * The test_loader sequentially loads each program in a skeleton.
> * Programs could be loaded in privileged and unprivileged modes.
> - * - __success, __failure, __msg imply privileged mode;
> - * - __success_unpriv, __failure_unpriv, __msg_unpriv imply
> - * unprivileged mode.
> + * - __success, __failure, __msg, __regex imply privileged mode;
> + * - __success_unpriv, __failure_unpriv, __msg_unpriv, __regex_unpriv
> + * imply unprivileged mode.
> * If combination of privileged and unprivileged attributes is present
> * both modes are used. If none are present privileged mode is implied.
> *
> @@ -24,6 +24,9 @@
> * Multiple __msg attributes could be specified.
> * __msg_unpriv Same as __msg but for unprivileged mode.
> *
> + * __regex Same as __msg, but using a regular expression.
> + * __regex_unpriv Same as __msg_unpriv but using a regular expression.
> + *
> * __success Expect program load success in privileged mode.
> * __success_unpriv Expect program load success in unprivileged mode.
> *
> @@ -59,10 +62,12 @@
> * __auxiliary_unpriv Same, but load program in unprivileged mode.
> */
> #define __msg(msg) __attribute__((btf_decl_tag("comment:test_expect_msg=" msg)))
> +#define __regex(regex) __attribute__((btf_decl_tag("comment:test_expect_regex=" regex)))
> #define __failure __attribute__((btf_decl_tag("comment:test_expect_failure")))
> #define __success __attribute__((btf_decl_tag("comment:test_expect_success")))
> #define __description(desc) __attribute__((btf_decl_tag("comment:test_description=" desc)))
> #define __msg_unpriv(msg) __attribute__((btf_decl_tag("comment:test_expect_msg_unpriv=" msg)))
> +#define __regex_unpriv(regex) __attribute__((btf_decl_tag("comment:test_expect_regex_unpriv=" regex)))
> #define __failure_unpriv __attribute__((btf_decl_tag("comment:test_expect_failure_unpriv")))
> #define __success_unpriv __attribute__((btf_decl_tag("comment:test_expect_success_unpriv")))
> #define __log_level(lvl) __attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 524c38e9cde4..c73fa04bca1b 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -2,6 +2,7 @@
> /* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> #include <linux/capability.h>
> #include <stdlib.h>
> +#include <regex.h>
> #include <test_progs.h>
> #include <bpf/btf.h>
>
> @@ -17,9 +18,11 @@
> #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
> #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
> #define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
> +#define TEST_TAG_EXPECT_REGEX_PFX "comment:test_expect_regex="
> #define TEST_TAG_EXPECT_FAILURE_UNPRIV "comment:test_expect_failure_unpriv"
> #define TEST_TAG_EXPECT_SUCCESS_UNPRIV "comment:test_expect_success_unpriv"
> #define TEST_TAG_EXPECT_MSG_PFX_UNPRIV "comment:test_expect_msg_unpriv="
> +#define TEST_TAG_EXPECT_REGEX_PFX_UNPRIV "comment:test_expect_regex_unpriv="
> #define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
> #define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
> #define TEST_TAG_DESCRIPTION_PFX "comment:test_description="
> @@ -46,10 +49,15 @@ enum mode {
> UNPRIV = 2
> };
>
> +struct expect_msg {
> + const char *msg;
let's call this "str"? In both cases we match "message", it's just
whether it's a substring match or regex match that matters
> + regex_t *regex;
let's just have `regex_t regex` here, and avoid some more malloc/free dance.
I wouldn't reuse `msg` field to store original regex string, just add
another field, we are not concerned with saving a few bytes on this,
but keeping "regex_str" vs "str" separate makes everything simpler
> +};
> +
> struct test_subspec {
> char *name;
> bool expect_failure;
> - const char **expect_msgs;
> + struct expect_msg *expect;
I'd keep the name as expect_msgs (you can expect other things, potentially)
> size_t expect_msg_cnt;
> int retval;
> bool execute;
> @@ -91,27 +99,57 @@ static void free_test_spec(struct test_spec *spec)
> {
> free(spec->priv.name);
> free(spec->unpriv.name);
> - free(spec->priv.expect_msgs);
> - free(spec->unpriv.expect_msgs);
> + free(spec->priv.expect);
> + free(spec->unpriv.expect);
who's going to free regex instances? there has to be regfree() somewhere
>
> spec->priv.name = NULL;
> spec->unpriv.name = NULL;
> - spec->priv.expect_msgs = NULL;
> - spec->unpriv.expect_msgs = NULL;
> + spec->priv.expect = NULL;
> + spec->unpriv.expect = NULL;
> }
>
> static int push_msg(const char *msg, struct test_subspec *subspec)
let's have a single `push_exp_msg(struct test_subspec *subspec, const
char *str, const char *regex)` helper that will handle both substr and
regexp cases in one function. Let's not duplicate realloc logic so
much
> {
> void *tmp;
>
> - tmp = realloc(subspec->expect_msgs, (1 + subspec->expect_msg_cnt) * sizeof(void *));
> + tmp = realloc(subspec->expect,
> + (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
> if (!tmp) {
> ASSERT_FAIL("failed to realloc memory for messages\n");
> return -ENOMEM;
> }
> - subspec->expect_msgs = tmp;
> - subspec->expect_msgs[subspec->expect_msg_cnt++] = msg;
>
> + subspec->expect = tmp;
> + subspec->expect[subspec->expect_msg_cnt].msg = msg;
> + subspec->expect[subspec->expect_msg_cnt].regex = NULL;
> + subspec->expect_msg_cnt += 1;
we have named type now, let's have `struct expect_msg *tmp`, and then do
tmp = &subspec->expect[subspec->expect_msg_cnt];
tmp->msg = ...
tmp->regex = ...
> + return 0;
> +}
> +
> +static int push_regex(const char *regex_str, struct test_subspec *subspec)
> +{
> + void *tmp;
> + int regcomp_res;
> +
> + tmp = realloc(subspec->expect,
> + (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
> + if (!tmp) {
> + ASSERT_FAIL("failed to realloc memory for messages\n");
> + return -ENOMEM;
> + }
> + subspec->expect = tmp;
> +
> + subspec->expect[subspec->expect_msg_cnt].regex = (regex_t *) malloc(sizeof(regex_t));
> + regcomp_res = regcomp (subspec->expect[subspec->expect_msg_cnt].regex,
> + regex_str, REG_EXTENDED|REG_NEWLINE);
see above about tmp, we should shorten this (and combine with the above helper)
> + if (regcomp_res != 0) {
> + fprintf(stderr, "Regexp: '%s'\n", regex_str);
> + ASSERT_FAIL("failed to compile regex\n");
> + return -EINVAL;
> + }
> +
> + subspec->expect[subspec->expect_msg_cnt].msg = regex_str;
> + subspec->expect_msg_cnt += 1;
> return 0;
> }
>
> @@ -243,6 +281,18 @@ static int parse_test_spec(struct test_loader *tester,
> if (err)
> goto cleanup;
> spec->mode_mask |= UNPRIV;
> + } else if (str_has_pfx(s, TEST_TAG_EXPECT_REGEX_PFX)) {
> + msg = s + sizeof(TEST_TAG_EXPECT_REGEX_PFX) - 1;
> + err = push_regex(msg, &spec->priv);
> + if (err)
> + goto cleanup;
> + spec->mode_mask |= PRIV;
> + } else if (str_has_pfx(s, TEST_TAG_EXPECT_REGEX_PFX_UNPRIV)) {
> + msg = s + sizeof(TEST_TAG_EXPECT_REGEX_PFX_UNPRIV) - 1;
> + err = push_regex(msg, &spec->unpriv);
> + if (err)
> + goto cleanup;
> + spec->mode_mask |= UNPRIV;
> } else if (str_has_pfx(s, TEST_TAG_RETVAL_PFX)) {
> val = s + sizeof(TEST_TAG_RETVAL_PFX) - 1;
> err = parse_retval(val, &spec->priv.retval, "__retval");
> @@ -336,16 +386,16 @@ static int parse_test_spec(struct test_loader *tester,
> spec->unpriv.execute = spec->priv.execute;
> }
>
> - if (!spec->unpriv.expect_msgs) {
> - size_t sz = spec->priv.expect_msg_cnt * sizeof(void *);
> + if (!spec->unpriv.expect) {
> + size_t sz = spec->priv.expect_msg_cnt * sizeof(struct expect_msg);
>
> - spec->unpriv.expect_msgs = malloc(sz);
> - if (!spec->unpriv.expect_msgs) {
> - PRINT_FAIL("failed to allocate memory for unpriv.expect_msgs\n");
> + spec->unpriv.expect = malloc(sz);
> + if (!spec->unpriv.expect) {
> + PRINT_FAIL("failed to allocate memory for unpriv.expect\n");
> err = -ENOMEM;
> goto cleanup;
> }
> - memcpy(spec->unpriv.expect_msgs, spec->priv.expect_msgs, sz);
> + memcpy(spec->unpriv.expect, spec->priv.expect, sz);
> spec->unpriv.expect_msg_cnt = spec->priv.expect_msg_cnt;
> }
> }
> @@ -403,26 +453,44 @@ static void validate_case(struct test_loader *tester,
> int load_err)
> {
> int i, j;
> + const char *match;
>
> for (i = 0; i < subspec->expect_msg_cnt; i++) {
> - char *match;
> const char *expect_msg;
> + regex_t *regex;
> + regmatch_t reg_match[1];
> +
> + expect_msg = subspec->expect[i].msg;
> + regex = subspec->expect[i].regex;
> +
> + if (regex == NULL) {
if (!regex)
> + match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
> + if (!ASSERT_OK_PTR (match, "expect_msg")) {
> + /* if we are in verbose mode, we've already emitted log */
> + if (env.verbosity == VERBOSE_NONE)
> + emit_verifier_log(tester->log_buf, true /*force*/);
> + for (j = 0; j < i; j++)
> + fprintf(stderr,
> + "MATCHED MSG: '%s'\n", subspec->expect[j].msg);
> + fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
> + return;
> + }
> + tester->next_match_pos = match - tester->log_buf + strlen(expect_msg);
> + } else {
> + int match_size = regexec (regex, tester->log_buf + tester->next_match_pos, 1, reg_match, 0);
> + if (match_size != 1) {
ASSERT_EQ(match_size, 1) to stay similar to the substring case above
with ASSERT_OK_PTR?
> + /* if we are in verbose mode, we've already emitted log */
> + if (env.verbosity == VERBOSE_NONE)
> + emit_verifier_log(tester->log_buf, true /*force*/);
> + for (j = 0; j < i; j++)
> + fprintf(stderr,
> + "MATCHED REGEX: '%s'\n", subspec->expect[j].msg);
> + fprintf(stderr, "EXPECTED REGEX: '%s'\n", expect_msg);
> + return;
> + }
let's try to combine substring and regex case and keep verbosity and
error message output in one place?
>
> - expect_msg = subspec->expect_msgs[i];
> -
> - match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
> - if (!ASSERT_OK_PTR(match, "expect_msg")) {
> - /* if we are in verbose mode, we've already emitted log */
> - if (env.verbosity == VERBOSE_NONE)
> - emit_verifier_log(tester->log_buf, true /*force*/);
> - for (j = 0; j < i; j++)
> - fprintf(stderr,
> - "MATCHED MSG: '%s'\n", subspec->expect_msgs[j]);
> - fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
> - return;
> + tester->next_match_pos += reg_match[0].rm_eo;
> }
> -
> - tester->next_match_pos = match - tester->log_buf + strlen(expect_msg);
> }
> }
>
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-03 15:53 ` [PATCH bpf-next 2/2] selftests/bpf: Match tests against " Cupertino Miranda
@ 2024-06-04 18:16 ` Andrii Nakryiko
2024-06-06 10:50 ` Cupertino Miranda
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2024-06-04 18:16 UTC (permalink / raw)
To: Cupertino Miranda
Cc: bpf, jose.marchesi, david.faust, Yonghong Song, Eduard Zingerman
On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
> This patch changes a few tests to make use of regular expressions such
> that the test validation would allow to properly verify the tests when
> compiled with GCC.
>
> signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
> Cc: jose.marchesi@oracle.com
> Cc: david.faust@oracle.com
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> ---
> tools/testing/selftests/bpf/progs/dynptr_fail.c | 6 +++---
> tools/testing/selftests/bpf/progs/exceptions_assert.c | 8 ++++----
> tools/testing/selftests/bpf/progs/rbtree_fail.c | 8 ++++----
> tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
> tools/testing/selftests/bpf/progs/verifier_sock.c | 4 ++--
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> index 66a60bfb5867..64cc9d936a13 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> @@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
> * mem_or_null pointers.
> */
> SEC("?raw_tp")
> -__failure __msg("R1 type=scalar expected=percpu_ptr_")
> +__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
> int dynptr_invalidate_slice_or_null(void *ctx)
> {
> struct bpf_dynptr ptr;
> @@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
>
> /* Destruction of dynptr should also any slices obtained from it */
> SEC("?raw_tp")
> -__failure __msg("R7 invalid mem access 'scalar'")
> +__failure __regex("R[0-9]+ invalid mem access 'scalar'")
> int dynptr_invalidate_slice_failure(void *ctx)
> {
> struct bpf_dynptr ptr1;
> @@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
>
> /* bpf_dynptr_slice()s are read-only and cannot be written to */
> SEC("?tc")
> -__failure __msg("R0 cannot write into rdonly_mem")
> +__failure __regex("R[0-9]+ cannot write into rdonly_mem")
> int skb_invalid_slice_write(struct __sk_buff *skb)
> {
> struct bpf_dynptr ptr;
> diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
> index 5e0a1ca96d4e..deb67d198caf 100644
> --- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
> +++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
> @@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
>
> SEC("?tc")
> __log_level(2) __failure
> -__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
> +__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
curious, what R0 value do we end up with with GCC generated code?
> int check_assert_range_s64(struct __sk_buff *ctx)
> {
> struct bpf_sock *sk = ctx->sk;
> @@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
>
> SEC("?tc")
> __log_level(2) __failure
> -__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
> +__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
> int check_assert_range_u64(struct __sk_buff *ctx)
> {
> u64 num = ctx->len;
> @@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
>
> SEC("?tc")
> __log_level(2) __failure
> -__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
> +__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
> int check_assert_single_range_s64(struct __sk_buff *ctx)
> {
> struct bpf_sock *sk = ctx->sk;
> @@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
>
> SEC("?tc")
> __log_level(2) __failure
> -__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
> +__msg("R1=pkt(off=64,r=64)")
> int check_assert_generic(struct __sk_buff *ctx)
> {
> u8 *data_end = (void *)(long)ctx->data_end;
> diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
> index 3fecf1c6dfe5..8399304eca72 100644
> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
> @@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> }
>
> SEC("?tc")
> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> long rbtree_api_nolock_add(void *ctx)
> {
> struct node_data *n;
> @@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
> }
>
> SEC("?tc")
> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> long rbtree_api_nolock_remove(void *ctx)
> {
> struct node_data *n;
> @@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
> }
>
> SEC("?tc")
> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> long rbtree_api_nolock_first(void *ctx)
> {
> bpf_rbtree_first(&groot);
> @@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
> }
>
> SEC("?tc")
> -__failure __msg("Unreleased reference id=3 alloc_insn=10")
> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
this test definitely should have been written in BPF assembly if we
care to check alloc_insn... Otherwise we just care that there is
"Unreleased reference" message, we should match on that without
hard-coding id and alloc_insn?
> long rbtree_api_remove_no_drop(void *ctx)
> {
> struct bpf_rb_node *res;
> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> index 1553b9c16aa7..f8d4b7cfcd68 100644
> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> @@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> }
>
> SEC("?tc")
> -__failure __msg("Unreleased reference id=4 alloc_insn=21")
> +__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
same, relying on ID and alloc_insns in tests written in C is super fragile.
> long rbtree_refcounted_node_ref_escapes(void *ctx)
> {
> struct node_acquire *n, *m;
> @@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
> }
>
> SEC("?tc")
> -__failure __msg("Unreleased reference id=3 alloc_insn=9")
> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
> long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
ditto
> {
> struct node_acquire *n, *m;
> diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
> index ee76b51005ab..450b57933c79 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_sock.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
> @@ -799,7 +799,7 @@ l0_%=: r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]); \
>
> SEC("sk_skb")
> __description("bpf_map_lookup_elem(sockmap, &key)")
> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
same here and below
> __naked void map_lookup_elem_sockmap_key(void)
> {
> asm volatile (" \
> @@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
>
> SEC("sk_skb")
> __description("bpf_map_lookup_elem(sockhash, &key)")
> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
> __naked void map_lookup_elem_sockhash_key(void)
> {
> asm volatile (" \
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression.
2024-06-03 15:53 ` [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression Cupertino Miranda
2024-06-04 18:16 ` Andrii Nakryiko
@ 2024-06-04 21:35 ` Eduard Zingerman
1 sibling, 0 replies; 13+ messages in thread
From: Eduard Zingerman @ 2024-06-04 21:35 UTC (permalink / raw)
To: Cupertino Miranda, bpf
Cc: jose.marchesi, david.faust, Yonghong Song, Andrii Nakryiko
On Mon, 2024-06-03 at 16:53 +0100, Cupertino Miranda wrote:
I think this macro is a long overdue, thank you for working on this.
A few notes below.
[...]
> +static int push_regex(const char *regex_str, struct test_subspec *subspec)
> +{
> + void *tmp;
> + int regcomp_res;
> +
> + tmp = realloc(subspec->expect,
> + (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
> + if (!tmp) {
> + ASSERT_FAIL("failed to realloc memory for messages\n");
> + return -ENOMEM;
> + }
> + subspec->expect = tmp;
> +
> + subspec->expect[subspec->expect_msg_cnt].regex = (regex_t *) malloc(sizeof(regex_t));
> + regcomp_res = regcomp (subspec->expect[subspec->expect_msg_cnt].regex,
> + regex_str, REG_EXTENDED|REG_NEWLINE);
> + if (regcomp_res != 0) {
> + fprintf(stderr, "Regexp: '%s'\n", regex_str);
> + ASSERT_FAIL("failed to compile regex\n");
> + return -EINVAL;
> + }
Maybe also use a regerror() function that could be used to print
what's wrong with the regex.
Also, there is a ctx_rewrite.c:compile_regex, it might be interesting
to extract in from ctx_rewrite.c to testing_helpers.c and use it here.
> +
> + subspec->expect[subspec->expect_msg_cnt].msg = regex_str;
> + subspec->expect_msg_cnt += 1;
> return 0;
> }
[...]
> @@ -403,26 +453,44 @@ static void validate_case(struct test_loader *tester,
> int load_err)
> {
> int i, j;
> + const char *match;
>
> for (i = 0; i < subspec->expect_msg_cnt; i++) {
> - char *match;
> const char *expect_msg;
> + regex_t *regex;
> + regmatch_t reg_match[1];
> +
> + expect_msg = subspec->expect[i].msg;
> + regex = subspec->expect[i].regex;
> +
> + if (regex == NULL) {
> + match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
> + if (!ASSERT_OK_PTR (match, "expect_msg")) {
> + /* if we are in verbose mode, we've already emitted log */
> + if (env.verbosity == VERBOSE_NONE)
> + emit_verifier_log(tester->log_buf, true /*force*/);
> + for (j = 0; j < i; j++)
> + fprintf(stderr,
> + "MATCHED MSG: '%s'\n", subspec->expect[j].msg);
> + fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
> + return;
> + }
> + tester->next_match_pos = match - tester->log_buf + strlen(expect_msg);
> + } else {
> + int match_size = regexec (regex, tester->log_buf + tester->next_match_pos, 1, reg_match, 0);
^
Nit: |
I think scripts/checkpatch.pl complains about such spaces
[...]
There is no regfree() call in the patch-set,
could you please extend free_test_spec()?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-04 18:16 ` Andrii Nakryiko
@ 2024-06-06 10:50 ` Cupertino Miranda
2024-06-06 15:50 ` Alexei Starovoitov
2024-06-06 17:19 ` Andrii Nakryiko
0 siblings, 2 replies; 13+ messages in thread
From: Cupertino Miranda @ 2024-06-06 10:50 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, jose.marchesi, david.faust, Yonghong Song, Eduard Zingerman
Andrii Nakryiko writes:
> On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>> This patch changes a few tests to make use of regular expressions such
>> that the test validation would allow to properly verify the tests when
>> compiled with GCC.
>>
>> signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
>> Cc: jose.marchesi@oracle.com
>> Cc: david.faust@oracle.com
>> Cc: Yonghong Song <yonghong.song@linux.dev>
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> ---
>> tools/testing/selftests/bpf/progs/dynptr_fail.c | 6 +++---
>> tools/testing/selftests/bpf/progs/exceptions_assert.c | 8 ++++----
>> tools/testing/selftests/bpf/progs/rbtree_fail.c | 8 ++++----
>> tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
>> tools/testing/selftests/bpf/progs/verifier_sock.c | 4 ++--
>> 5 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> index 66a60bfb5867..64cc9d936a13 100644
>> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> @@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
>> * mem_or_null pointers.
>> */
>> SEC("?raw_tp")
>> -__failure __msg("R1 type=scalar expected=percpu_ptr_")
>> +__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
>> int dynptr_invalidate_slice_or_null(void *ctx)
>> {
>> struct bpf_dynptr ptr;
>> @@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
>>
>> /* Destruction of dynptr should also any slices obtained from it */
>> SEC("?raw_tp")
>> -__failure __msg("R7 invalid mem access 'scalar'")
>> +__failure __regex("R[0-9]+ invalid mem access 'scalar'")
>> int dynptr_invalidate_slice_failure(void *ctx)
>> {
>> struct bpf_dynptr ptr1;
>> @@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
>>
>> /* bpf_dynptr_slice()s are read-only and cannot be written to */
>> SEC("?tc")
>> -__failure __msg("R0 cannot write into rdonly_mem")
>> +__failure __regex("R[0-9]+ cannot write into rdonly_mem")
>> int skb_invalid_slice_write(struct __sk_buff *skb)
>> {
>> struct bpf_dynptr ptr;
>> diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> index 5e0a1ca96d4e..deb67d198caf 100644
>> --- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> +++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> @@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
>>
>> SEC("?tc")
>> __log_level(2) __failure
>> -__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
>> +__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
>
> curious, what R0 value do we end up with with GCC generated code?
Oups, this file should have not been committed. Those changes were just
for experimentation, nothing else. :(
>
>> int check_assert_range_s64(struct __sk_buff *ctx)
>> {
>> struct bpf_sock *sk = ctx->sk;
>> @@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
>>
>> SEC("?tc")
>> __log_level(2) __failure
>> -__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
>> +__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
>> int check_assert_range_u64(struct __sk_buff *ctx)
>> {
>> u64 num = ctx->len;
>> @@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
>>
>> SEC("?tc")
>> __log_level(2) __failure
>> -__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
>> +__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
>> int check_assert_single_range_s64(struct __sk_buff *ctx)
>> {
>> struct bpf_sock *sk = ctx->sk;
>> @@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
>>
>> SEC("?tc")
>> __log_level(2) __failure
>> -__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
>> +__msg("R1=pkt(off=64,r=64)")
>> int check_assert_generic(struct __sk_buff *ctx)
>> {
>> u8 *data_end = (void *)(long)ctx->data_end;
>> diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> index 3fecf1c6dfe5..8399304eca72 100644
>> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> @@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>> }
>>
>> SEC("?tc")
>> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> long rbtree_api_nolock_add(void *ctx)
>> {
>> struct node_data *n;
>> @@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
>> }
>>
>> SEC("?tc")
>> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> long rbtree_api_nolock_remove(void *ctx)
>> {
>> struct node_data *n;
>> @@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
>> }
>>
>> SEC("?tc")
>> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> long rbtree_api_nolock_first(void *ctx)
>> {
>> bpf_rbtree_first(&groot);
>> @@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
>> }
>>
>> SEC("?tc")
>> -__failure __msg("Unreleased reference id=3 alloc_insn=10")
>> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
>
> this test definitely should have been written in BPF assembly if we
> care to check alloc_insn... Otherwise we just care that there is
> "Unreleased reference" message, we should match on that without
> hard-coding id and alloc_insn?
I agree. Unfortunately I see a lot of tests that fall in this category.
I must admit, most of the time I do not know what is the proper approach
to correct it.
Also found some tests that made expectations on .bss section data
layout, expeting a particular variable order.
For example in prog_tests/core_reloc.c, when it maps .bss and assigns it
to data.
GCC will allocate variables in a different order then clang and when
comparing content is not where comparisson is expecting.
Some other test, would expect that struct fields would be in some
particular order, while GCC decides it would benefit from reordering
struct fields. For passing those tests I need to disable GCC
optimization that would make this reordering.
However reordering of the struct fields is a perfectly valid
optimization. Maybe disabling for this tests is acceptable, but in any
case the test itself is prune for any future optimizations that can be
added to GCC or CLANG.
This happened in progs/test_core_autosize.c for example.
Anyway, just a couple of examples of tests that were made very tight to
compiler.
>
>> long rbtree_api_remove_no_drop(void *ctx)
>> {
>> struct bpf_rb_node *res;
>> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> index 1553b9c16aa7..f8d4b7cfcd68 100644
>> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> @@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>> }
>>
>> SEC("?tc")
>> -__failure __msg("Unreleased reference id=4 alloc_insn=21")
>> +__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
>
> same, relying on ID and alloc_insns in tests written in C is super fragile.
>
>> long rbtree_refcounted_node_ref_escapes(void *ctx)
>> {
>> struct node_acquire *n, *m;
>> @@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
>> }
>>
>> SEC("?tc")
>> -__failure __msg("Unreleased reference id=3 alloc_insn=9")
>> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
>> long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
>
> ditto
>
>> {
>> struct node_acquire *n, *m;
>> diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
>> index ee76b51005ab..450b57933c79 100644
>> --- a/tools/testing/selftests/bpf/progs/verifier_sock.c
>> +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
>> @@ -799,7 +799,7 @@ l0_%=: r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]); \
>>
>> SEC("sk_skb")
>> __description("bpf_map_lookup_elem(sockmap, &key)")
>> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
>> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
>
> same here and below
>
>
>> __naked void map_lookup_elem_sockmap_key(void)
>> {
>> asm volatile (" \
>> @@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
>>
>> SEC("sk_skb")
>> __description("bpf_map_lookup_elem(sockhash, &key)")
>> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
>> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
>> __naked void map_lookup_elem_sockhash_key(void)
>> {
>> asm volatile (" \
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-06 10:50 ` Cupertino Miranda
@ 2024-06-06 15:50 ` Alexei Starovoitov
2024-06-06 18:07 ` Cupertino Miranda
2024-06-06 17:19 ` Andrii Nakryiko
1 sibling, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2024-06-06 15:50 UTC (permalink / raw)
To: Cupertino Miranda
Cc: Andrii Nakryiko, bpf, Jose E. Marchesi, David Faust,
Yonghong Song, Eduard Zingerman
On Thu, Jun 6, 2024 at 3:51 AM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
> GCC will allocate variables in a different order then clang and when
> comparing content is not where comparisson is expecting.
>
> Some other test, would expect that struct fields would be in some
> particular order, while GCC decides it would benefit from reordering
> struct fields. For passing those tests I need to disable GCC
> optimization that would make this reordering.
> However reordering of the struct fields is a perfectly valid
> optimization. Maybe disabling for this tests is acceptable, but in any
> case the test itself is prune for any future optimizations that can be
> added to GCC or CLANG.
Not really.
Allocating vars in different order within a section is fine,
but compilers are not allowed to reorder fields within structs.
There is a plugin for gcc that allows opt-in via
__attribute__((randomize_layout)).
But never by default.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-06 10:50 ` Cupertino Miranda
2024-06-06 15:50 ` Alexei Starovoitov
@ 2024-06-06 17:19 ` Andrii Nakryiko
2024-06-06 17:47 ` Eduard Zingerman
2024-06-06 18:35 ` Cupertino Miranda
1 sibling, 2 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-06-06 17:19 UTC (permalink / raw)
To: Cupertino Miranda
Cc: bpf, jose.marchesi, david.faust, Yonghong Song, Eduard Zingerman
On Thu, Jun 6, 2024 at 3:50 AM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
>
> Andrii Nakryiko writes:
>
> > On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
> > <cupertino.miranda@oracle.com> wrote:
> >>
> >> This patch changes a few tests to make use of regular expressions such
> >> that the test validation would allow to properly verify the tests when
> >> compiled with GCC.
> >>
> >> signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
> >> Cc: jose.marchesi@oracle.com
> >> Cc: david.faust@oracle.com
> >> Cc: Yonghong Song <yonghong.song@linux.dev>
> >> Cc: Eduard Zingerman <eddyz87@gmail.com>
> >> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> >> ---
> >> tools/testing/selftests/bpf/progs/dynptr_fail.c | 6 +++---
> >> tools/testing/selftests/bpf/progs/exceptions_assert.c | 8 ++++----
> >> tools/testing/selftests/bpf/progs/rbtree_fail.c | 8 ++++----
> >> tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
> >> tools/testing/selftests/bpf/progs/verifier_sock.c | 4 ++--
> >> 5 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> >> index 66a60bfb5867..64cc9d936a13 100644
> >> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> >> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> >> @@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
> >> * mem_or_null pointers.
> >> */
> >> SEC("?raw_tp")
> >> -__failure __msg("R1 type=scalar expected=percpu_ptr_")
> >> +__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
> >> int dynptr_invalidate_slice_or_null(void *ctx)
> >> {
> >> struct bpf_dynptr ptr;
> >> @@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
> >>
> >> /* Destruction of dynptr should also any slices obtained from it */
> >> SEC("?raw_tp")
> >> -__failure __msg("R7 invalid mem access 'scalar'")
> >> +__failure __regex("R[0-9]+ invalid mem access 'scalar'")
> >> int dynptr_invalidate_slice_failure(void *ctx)
> >> {
> >> struct bpf_dynptr ptr1;
> >> @@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
> >>
> >> /* bpf_dynptr_slice()s are read-only and cannot be written to */
> >> SEC("?tc")
> >> -__failure __msg("R0 cannot write into rdonly_mem")
> >> +__failure __regex("R[0-9]+ cannot write into rdonly_mem")
> >> int skb_invalid_slice_write(struct __sk_buff *skb)
> >> {
> >> struct bpf_dynptr ptr;
> >> diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
> >> index 5e0a1ca96d4e..deb67d198caf 100644
> >> --- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
> >> +++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
> >> @@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
> >> +__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
> >
> > curious, what R0 value do we end up with with GCC generated code?
> Oups, this file should have not been committed. Those changes were just
> for experimentation, nothing else. :(
>
> >
> >> int check_assert_range_s64(struct __sk_buff *ctx)
> >> {
> >> struct bpf_sock *sk = ctx->sk;
> >> @@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
> >> +__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
> >> int check_assert_range_u64(struct __sk_buff *ctx)
> >> {
> >> u64 num = ctx->len;
> >> @@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
> >> +__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
> >> int check_assert_single_range_s64(struct __sk_buff *ctx)
> >> {
> >> struct bpf_sock *sk = ctx->sk;
> >> @@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
> >> +__msg("R1=pkt(off=64,r=64)")
> >> int check_assert_generic(struct __sk_buff *ctx)
> >> {
> >> u8 *data_end = (void *)(long)ctx->data_end;
> >> diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
> >> index 3fecf1c6dfe5..8399304eca72 100644
> >> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
> >> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
> >> @@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> >> long rbtree_api_nolock_add(void *ctx)
> >> {
> >> struct node_data *n;
> >> @@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> >> long rbtree_api_nolock_remove(void *ctx)
> >> {
> >> struct node_data *n;
> >> @@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> >> long rbtree_api_nolock_first(void *ctx)
> >> {
> >> bpf_rbtree_first(&groot);
> >> @@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("Unreleased reference id=3 alloc_insn=10")
> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
> >
> > this test definitely should have been written in BPF assembly if we
> > care to check alloc_insn... Otherwise we just care that there is
> > "Unreleased reference" message, we should match on that without
> > hard-coding id and alloc_insn?
> I agree. Unfortunately I see a lot of tests that fall in this category.
> I must admit, most of the time I do not know what is the proper approach
> to correct it.
>
> Also found some tests that made expectations on .bss section data
> layout, expeting a particular variable order.
> For example in prog_tests/core_reloc.c, when it maps .bss and assigns it
> to data.
I haven't checked every single one, but I think most (if not all) of
these progs/test_core_reloc_*.c tests (which are what is being tested
in prog_tests/core_reloc.c) are structured with a singular variable in
.bss. And then the variable type is some well-defined struct type. As
Alexei pointed out, compiler is not allowed to just arbitrarily
reorder fields within a struct, unless randomization is enabled with
an extra attribute (which we do not use).
So if you have specific cases where something isn't correct, let's go
over them, but I think prog_tests/core_reloc.c should be fine.
> GCC will allocate variables in a different order then clang and when
> comparing content is not where comparisson is expecting.
>
> Some other test, would expect that struct fields would be in some
> particular order, while GCC decides it would benefit from reordering
> struct fields. For passing those tests I need to disable GCC
> optimization that would make this reordering.
> However reordering of the struct fields is a perfectly valid
Nope, it's not.
As mentioned, struct layout is effectively an ABI, so the compiler
cannot just reorder it. Lots and lots of things would be broken if
this was true for C programs.
> optimization. Maybe disabling for this tests is acceptable, but in any
> case the test itself is prune for any future optimizations that can be
> added to GCC or CLANG.
> This happened in progs/test_core_autosize.c for example.
We probably should rewrite such tests that have to deal with
.bss/.data to BPF skeletons, I think they were written before BPF
skeletons were available.
>
> Anyway, just a couple of examples of tests that were made very tight to
> compiler.
>
> >
> >> long rbtree_api_remove_no_drop(void *ctx)
> >> {
> >> struct bpf_rb_node *res;
> >> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> >> index 1553b9c16aa7..f8d4b7cfcd68 100644
> >> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> >> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> >> @@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("Unreleased reference id=4 alloc_insn=21")
> >> +__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
> >
> > same, relying on ID and alloc_insns in tests written in C is super fragile.
> >
> >> long rbtree_refcounted_node_ref_escapes(void *ctx)
> >> {
> >> struct node_acquire *n, *m;
> >> @@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("Unreleased reference id=3 alloc_insn=9")
> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
> >> long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
> >
> > ditto
> >
> >> {
> >> struct node_acquire *n, *m;
> >> diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
> >> index ee76b51005ab..450b57933c79 100644
> >> --- a/tools/testing/selftests/bpf/progs/verifier_sock.c
> >> +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
> >> @@ -799,7 +799,7 @@ l0_%=: r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]); \
> >>
> >> SEC("sk_skb")
> >> __description("bpf_map_lookup_elem(sockmap, &key)")
> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
> >
> > same here and below
> >
> >
> >> __naked void map_lookup_elem_sockmap_key(void)
> >> {
> >> asm volatile (" \
> >> @@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
> >>
> >> SEC("sk_skb")
> >> __description("bpf_map_lookup_elem(sockhash, &key)")
> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
> >> __naked void map_lookup_elem_sockhash_key(void)
> >> {
> >> asm volatile (" \
> >> --
> >> 2.39.2
> >>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-06 17:19 ` Andrii Nakryiko
@ 2024-06-06 17:47 ` Eduard Zingerman
2024-06-06 19:27 ` Jose E. Marchesi
2024-06-06 18:35 ` Cupertino Miranda
1 sibling, 1 reply; 13+ messages in thread
From: Eduard Zingerman @ 2024-06-06 17:47 UTC (permalink / raw)
To: Cupertino Miranda
Cc: bpf, jose.marchesi, david.faust, Yonghong Song, Andrii Nakryiko
On Thu, 2024-06-06 at 10:19 -0700, Andrii Nakryiko wrote:
[...]
> > Some other test, would expect that struct fields would be in some
> > particular order, while GCC decides it would benefit from reordering
> > struct fields. For passing those tests I need to disable GCC
> > optimization that would make this reordering.
> > However reordering of the struct fields is a perfectly valid
>
> Nope, it's not.
>
> As mentioned, struct layout is effectively an ABI, so the compiler
> cannot just reorder it. Lots and lots of things would be broken if
> this was true for C programs.
I'll chime in as well :)
Could you please show a few examples when GCC does reordering?
As Alexei and Andrii point out in general C language standard does not
allow reordering for fields, e.g. here is a wording from section
6.7.2.1, paragraph 17 of "WG 14/N 3088, Programming languages — C":
> Within a structure object, the non-bit-field members and the units
> in which bit-fields reside have addresses that increase in the order
> in which they are declared. A pointer to a structure object,
> suitably converted, points to its initial member (or if that member
> is a bit-field, then to the unit in which it resides), and vice
> versa. There may be unnamed padding within a structure object, but
> not at its beginning.
So, I'm curious what's happening.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-06 15:50 ` Alexei Starovoitov
@ 2024-06-06 18:07 ` Cupertino Miranda
0 siblings, 0 replies; 13+ messages in thread
From: Cupertino Miranda @ 2024-06-06 18:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, Jose E. Marchesi, David Faust,
Yonghong Song, Eduard Zingerman
Alexei Starovoitov writes:
> On Thu, Jun 6, 2024 at 3:51 AM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>> GCC will allocate variables in a different order then clang and when
>> comparing content is not where comparisson is expecting.
>>
>> Some other test, would expect that struct fields would be in some
>> particular order, while GCC decides it would benefit from reordering
>> struct fields. For passing those tests I need to disable GCC
>> optimization that would make this reordering.
>> However reordering of the struct fields is a perfectly valid
>> optimization. Maybe disabling for this tests is acceptable, but in any
>> case the test itself is prune for any future optimizations that can be
>> added to GCC or CLANG.
>
> Not really.
> Allocating vars in different order within a section is fine,
> but compilers are not allowed to reorder fields within structs.
> There is a plugin for gcc that allows opt-in via
> __attribute__((randomize_layout)).
> But never by default.
Apologies for the mess up. Indeed the reordering happens on variable
declarations. In prog/test_core_autosize.c, it declares variables and in
prog_tests/core_autosize.c it checks content with a "template" struct
that should map how the data is layout in memory.
Somehow I miss remembered the actual problem and got confused with what
was actually being reorderd.
In the end both examples are the same.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-06 17:19 ` Andrii Nakryiko
2024-06-06 17:47 ` Eduard Zingerman
@ 2024-06-06 18:35 ` Cupertino Miranda
1 sibling, 0 replies; 13+ messages in thread
From: Cupertino Miranda @ 2024-06-06 18:35 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, jose.marchesi, david.faust, Yonghong Song, Eduard Zingerman
Andrii Nakryiko writes:
> On Thu, Jun 6, 2024 at 3:50 AM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>>
>> Andrii Nakryiko writes:
>>
>> > On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
>> > <cupertino.miranda@oracle.com> wrote:
>> >>
>> >> This patch changes a few tests to make use of regular expressions such
>> >> that the test validation would allow to properly verify the tests when
>> >> compiled with GCC.
>> >>
>> >> signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
>> >> Cc: jose.marchesi@oracle.com
>> >> Cc: david.faust@oracle.com
>> >> Cc: Yonghong Song <yonghong.song@linux.dev>
>> >> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> >> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> >> ---
>> >> tools/testing/selftests/bpf/progs/dynptr_fail.c | 6 +++---
>> >> tools/testing/selftests/bpf/progs/exceptions_assert.c | 8 ++++----
>> >> tools/testing/selftests/bpf/progs/rbtree_fail.c | 8 ++++----
>> >> tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
>> >> tools/testing/selftests/bpf/progs/verifier_sock.c | 4 ++--
>> >> 5 files changed, 15 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> >> index 66a60bfb5867..64cc9d936a13 100644
>> >> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> >> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> >> @@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
>> >> * mem_or_null pointers.
>> >> */
>> >> SEC("?raw_tp")
>> >> -__failure __msg("R1 type=scalar expected=percpu_ptr_")
>> >> +__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
>> >> int dynptr_invalidate_slice_or_null(void *ctx)
>> >> {
>> >> struct bpf_dynptr ptr;
>> >> @@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
>> >>
>> >> /* Destruction of dynptr should also any slices obtained from it */
>> >> SEC("?raw_tp")
>> >> -__failure __msg("R7 invalid mem access 'scalar'")
>> >> +__failure __regex("R[0-9]+ invalid mem access 'scalar'")
>> >> int dynptr_invalidate_slice_failure(void *ctx)
>> >> {
>> >> struct bpf_dynptr ptr1;
>> >> @@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
>> >>
>> >> /* bpf_dynptr_slice()s are read-only and cannot be written to */
>> >> SEC("?tc")
>> >> -__failure __msg("R0 cannot write into rdonly_mem")
>> >> +__failure __regex("R[0-9]+ cannot write into rdonly_mem")
>> >> int skb_invalid_slice_write(struct __sk_buff *skb)
>> >> {
>> >> struct bpf_dynptr ptr;
>> >> diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> >> index 5e0a1ca96d4e..deb67d198caf 100644
>> >> --- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> >> +++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> >> @@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
>> >>
>> >> SEC("?tc")
>> >> __log_level(2) __failure
>> >> -__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
>> >> +__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
>> >
>> > curious, what R0 value do we end up with with GCC generated code?
>> Oups, this file should have not been committed. Those changes were just
>> for experimentation, nothing else. :(
>>
>> >
>> >> int check_assert_range_s64(struct __sk_buff *ctx)
>> >> {
>> >> struct bpf_sock *sk = ctx->sk;
>> >> @@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
>> >>
>> >> SEC("?tc")
>> >> __log_level(2) __failure
>> >> -__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
>> >> +__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
>> >> int check_assert_range_u64(struct __sk_buff *ctx)
>> >> {
>> >> u64 num = ctx->len;
>> >> @@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
>> >>
>> >> SEC("?tc")
>> >> __log_level(2) __failure
>> >> -__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
>> >> +__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
>> >> int check_assert_single_range_s64(struct __sk_buff *ctx)
>> >> {
>> >> struct bpf_sock *sk = ctx->sk;
>> >> @@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
>> >>
>> >> SEC("?tc")
>> >> __log_level(2) __failure
>> >> -__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
>> >> +__msg("R1=pkt(off=64,r=64)")
>> >> int check_assert_generic(struct __sk_buff *ctx)
>> >> {
>> >> u8 *data_end = (void *)(long)ctx->data_end;
>> >> diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> >> index 3fecf1c6dfe5..8399304eca72 100644
>> >> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> >> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> >> @@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>> >> }
>> >>
>> >> SEC("?tc")
>> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> >> long rbtree_api_nolock_add(void *ctx)
>> >> {
>> >> struct node_data *n;
>> >> @@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
>> >> }
>> >>
>> >> SEC("?tc")
>> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")q
>> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> >> long rbtree_api_nolock_remove(void *ctx)
>> >> {
>> >> struct node_data *n;
>> >> @@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
>> >> }
>> >>
>> >> SEC("?tc")
>> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> >> long rbtree_api_nolock_first(void *ctx)
>> >> {
>> >> bpf_rbtree_first(&groot);
>> >> @@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
>> >> }
>> >>
>> >> SEC("?tc")
>> >> -__failure __msg("Unreleased reference id=3 alloc_insn=10")
>> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
>> >
>> > this test definitely should have been written in BPF assembly if we
>> > care to check alloc_insn... Otherwise we just care that there is
>> > "Unreleased reference" message, we should match on that without
>> > hard-coding id and alloc_insn?
>> I agree. Unfortunately I see a lot of tests that fall in this category.
>> I must admit, most of the time I do not know what is the proper approach
>> to correct it.
>>
>> Also found some tests that made expectations on .bss section data
>> layout, expeting a particular variable order.
>> For example in prog_tests/core_reloc.c, when it maps .bss and assigns it
>> to data.
>
> I haven't checked every single one, but I think most (if not all) of
> these progs/test_core_reloc_*.c tests (which are what is being tested
> in prog_tests/core_reloc.c) are structured with a singular variable in
> .bss. And then the variable type is some well-defined struct type. As
> Alexei pointed out, compiler is not allowed to just arbitrarily
> reorder fields within a struct, unless randomization is enabled with
> an extra attribute (which we do not use).
>
> So if you have specific cases where something isn't correct, let's go
> over them, but I think prog_tests/core_reloc.c should be fine.
>
I think it was in progs/test_core_reloc_type_id.c where you also define:
/* preserve types even if Clang doesn't support built-in */
struct a_struct t1 = {};
union a_union t2 = {};
enum an_enum t3 = 0;
named_struct_typedef t4 = {};
func_proto_typedef t5 = 0;
arr_typedef t6 = {};
>> GCC will allocate variables in a different order then clang and when
>> comparing content is not where comparisson is expecting.
>>
>> Some other test, would expect that struct fields would be in some
>> particular order, while GCC decides it would benefit from reordering
>> struct fields. For passing those tests I need to disable GCC
>> optimization that would make this reordering.
>> However reordering of the struct fields is a perfectly valid
>
> Nope, it's not.
>
> As mentioned, struct layout is effectively an ABI, so the compiler
> cannot just reorder it. Lots and lots of things would be broken if
> this was true for C programs.
>
>> optimization. Maybe disabling for this tests is acceptable, but in any
>> case the test itself is prune for any future optimizations that can be
>> added to GCC or CLANG.
>> This happened in progs/test_core_autosize.c for example.
>
> We probably should rewrite such tests that have to deal with
> .bss/.data to BPF skeletons, I think they were written before BPF
> skeletons were available.
>
>>
>> Anyway, just a couple of examples of tests that were made very tight to
>> compiler.
>>
>> >
>> >> long rbtree_api_remove_no_drop(void *ctx)
>> >> {
>> >> struct bpf_rb_node *res;
>> >> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> >> index 1553b9c16aa7..f8d4b7cfcd68 100644
>> >> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> >> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> >> @@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>> >> }
>> >>
>> >> SEC("?tc")
>> >> -__failure __msg("Unreleased reference id=4 alloc_insn=21")
>> >> +__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
>> >
>> > same, relying on ID and alloc_insns in tests written in C is super fragile.
>> >
>> >> long rbtree_refcounted_node_ref_escapes(void *ctx)
>> >> {
>> >> struct node_acquire *n, *m;
>> >> @@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
>> >> }
>> >>
>> >> SEC("?tc")
>> >> -__failure __msg("Unreleased reference id=3 alloc_insn=9")
>> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
>> >> long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
>> >
>> > ditto
>> >
>> >> {
>> >> struct node_acquire *n, *m;
>> >> diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
>> >> index ee76b51005ab..450b57933c79 100644
>> >> --- a/tools/testing/selftests/bpf/progs/verifier_sock.c
>> >> +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
>> >> @@ -799,7 +799,7 @@ l0_%=: r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]); \
>> >>
>> >> SEC("sk_skb")
>> >> __description("bpf_map_lookup_elem(sockmap, &key)")
>> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
>> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
>> >
>> > same here and below
>> >
>> >
>> >> __naked void map_lookup_elem_sockmap_key(void)
>> >> {
>> >> asm volatile (" \
>> >> @@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
>> >>
>> >> SEC("sk_skb")
>> >> __description("bpf_map_lookup_elem(sockhash, &key)")
>> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
>> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
>> >> __naked void map_lookup_elem_sockhash_key(void)
>> >> {
>> >> asm volatile (" \
>> >> --
>> >> 2.39.2
>> >>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.
2024-06-06 17:47 ` Eduard Zingerman
@ 2024-06-06 19:27 ` Jose E. Marchesi
0 siblings, 0 replies; 13+ messages in thread
From: Jose E. Marchesi @ 2024-06-06 19:27 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Cupertino Miranda, bpf, david.faust, Yonghong Song,
Andrii Nakryiko
> On Thu, 2024-06-06 at 10:19 -0700, Andrii Nakryiko wrote:
>
> [...]
>
>> > Some other test, would expect that struct fields would be in some
>> > particular order, while GCC decides it would benefit from reordering
>> > struct fields. For passing those tests I need to disable GCC
>> > optimization that would make this reordering.
>> > However reordering of the struct fields is a perfectly valid
>>
>> Nope, it's not.
>>
>> As mentioned, struct layout is effectively an ABI, so the compiler
>> cannot just reorder it. Lots and lots of things would be broken if
>> this was true for C programs.
>
> I'll chime in as well :)
> Could you please show a few examples when GCC does reordering?
> As Alexei and Andrii point out in general C language standard does not
> allow reordering for fields, e.g. here is a wording from section
> 6.7.2.1, paragraph 17 of "WG 14/N 3088, Programming languages — C":
GCC does not reorder struct fields.
The option -ftoplevel-reorder enables reordering of data declarations.
>> Within a structure object, the non-bit-field members and the units
>> in which bit-fields reside have addresses that increase in the order
>> in which they are declared. A pointer to a structure object,
>> suitably converted, points to its initial member (or if that member
>> is a bit-field, then to the unit in which it resides), and vice
>> versa. There may be unnamed padding within a structure object, but
>> not at its beginning.
>
> So, I'm curious what's happening.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-06-06 19:28 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-03 15:53 [PATCH bpf-next 0/2] Regular expression support for test output matching Cupertino Miranda
2024-06-03 15:53 ` [PATCH bpf-next 1/2] selftests/bpf: Support checks against a regular expression Cupertino Miranda
2024-06-04 18:16 ` Andrii Nakryiko
2024-06-04 21:35 ` Eduard Zingerman
2024-06-03 15:53 ` [PATCH bpf-next 2/2] selftests/bpf: Match tests against " Cupertino Miranda
2024-06-04 18:16 ` Andrii Nakryiko
2024-06-06 10:50 ` Cupertino Miranda
2024-06-06 15:50 ` Alexei Starovoitov
2024-06-06 18:07 ` Cupertino Miranda
2024-06-06 17:19 ` Andrii Nakryiko
2024-06-06 17:47 ` Eduard Zingerman
2024-06-06 19:27 ` Jose E. Marchesi
2024-06-06 18:35 ` Cupertino Miranda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox