[PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Mykyta Yatsenko]

From: Mykyta Yatsenko <yatsenko@meta.com>

To better verify some complex BPF programs we'd like to preset global
variables.
This patch introduces CLI argument `--set-global-vars` to veristat, that
allows presetting values to global variables defined in BPF program. For
example:

prog.c:
```
enum Enum { ELEMENT1 = 0, ELEMENT2 = 5 };
const volatile __s64 a = 5;
const volatile __u8 b = 5;
const volatile enum Enum c = ELEMENT2;
const volatile bool d = false;

char arr[4] = {0};

SEC("tp_btf/sched_switch")
int BPF_PROG(...)
{
	bpf_printk("%c\n", arr[a]);
	bpf_printk("%c\n", arr[b]);
	bpf_printk("%c\n", arr[c]);
	bpf_printk("%c\n", arr[d]);
	return 0;
}
```
By default verification of the program fails:
```
./veristat prog.bpf.o
```
By presetting global variables, we can make verification pass:
```
./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
```

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 tools/testing/selftests/bpf/veristat.c | 189 +++++++++++++++++++++++++
 1 file changed, 189 insertions(+)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 06af5029885b..65bb8a773d23 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -154,6 +154,11 @@ struct filter {
 	bool abs;
 };
 
+struct var_preset {
+	char *name;
+	long long value;
+};
+
 static struct env {
 	char **filenames;
 	int filename_cnt;
@@ -195,6 +200,8 @@ static struct env {
 	int progs_processed;
 	int progs_skipped;
 	int top_src_lines;
+	struct var_preset *presets;
+	int npresets;
 } env;
 
 static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va_list args)
@@ -246,12 +253,14 @@ static const struct argp_option opts[] = {
 	{ "test-reg-invariants", 'r', NULL, 0,
 	  "Force BPF verifier failure on register invariant violation (BPF_F_TEST_REG_INVARIANTS program flag)" },
 	{ "top-src-lines", 'S', "N", 0, "Emit N most frequent source code lines" },
+	{ "set-global-vars", 'g', "GLOBALS", 0, "Set global variables provided in the expression, for example \"var1 = 1; var2 = 2\"" },
 	{},
 };
 
 static int parse_stats(const char *stats_str, struct stat_specs *specs);
 static int append_filter(struct filter **filters, int *cnt, const char *str);
 static int append_filter_file(const char *path);
+static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size);
 
 static error_t parse_arg(int key, char *arg, struct argp_state *state)
 {
@@ -363,6 +372,17 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 			return -ENOMEM;
 		env.filename_cnt++;
 		break;
+	case 'g': {
+		char *expr = strdup(arg);
+
+		env.presets = calloc(64, sizeof(*env.presets));
+		if (parse_var_presets(expr, env.presets, 64, &env.npresets)) {
+			fprintf(stderr, "Could not parse global variables preset: %s\n", arg);
+			argp_usage(state);
+		}
+		free(expr);
+		break;
+	}
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
@@ -1292,6 +1312,169 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 	return 0;
 };
 
+static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size)
+{
+	char *state;
+	char *next;
+	int i = 0;
+
+	while ((next = strtok_r(i ? NULL : expr, ";", &state))) {
+		char *eq_ptr = strchr(next, '=');
+		char *name_ptr = next;
+		char *name_end = eq_ptr - 1;
+		char *val_ptr = eq_ptr + 1;
+
+		if (!eq_ptr)
+			continue;
+
+		if (i >= capacity) {
+			fprintf(stderr, "Too many global variable presets\n");
+			return -EINVAL;
+		}
+		while (isspace(*name_ptr))
+			++name_ptr;
+		while (isspace(*name_end))
+			--name_end;
+
+		*(name_end + 1) = '\0';
+		presets[i].name = strdup(name_ptr);
+		errno = 0;
+		presets[i].value = strtoll(val_ptr, NULL, 10);
+		if (errno == ERANGE) {
+			errno = 0;
+			presets[i].value = strtoull(val_ptr, NULL, 10);
+		}
+		if (errno) {
+			fprintf(stderr, "Could not parse integer value %s\n", val_ptr);
+			return -EINVAL;
+		}
+		++i;
+	}
+	*size = i;
+	return 0;
+}
+
+static bool is_signed_type(const struct btf_type *type)
+{
+	if (btf_is_int(type))
+		return btf_int_encoding(type) & BTF_INT_SIGNED;
+	return true;
+}
+
+static const struct btf_type *var_base_type(const struct btf *btf, const struct btf_type *type)
+{
+	switch (btf_kind(type)) {
+	case BTF_KIND_VAR:
+	case BTF_KIND_TYPE_TAG:
+	case BTF_KIND_CONST:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_RESTRICT:
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_DECL_TAG:
+		return var_base_type(btf, btf__type_by_id(btf, type->type));
+	}
+	return type;
+}
+
+static bool is_preset_supported(const struct btf_type *t)
+{
+	return btf_is_int(t) || btf_is_enum(t) || btf_is_enum64(t);
+}
+
+static int set_global_var(struct bpf_object *obj, struct btf *btf, const struct btf_type *t,
+			  struct bpf_map *map, struct btf_var_secinfo *sinfo, long long new_val)
+{
+	const struct btf_type *base_type;
+	void *ptr;
+	size_t size;
+
+	base_type = var_base_type(btf, t);
+	if (!is_preset_supported(base_type)) {
+		fprintf(stderr, "Setting global variable for btf kind %d is not supported\n",
+			btf_kind(base_type));
+		return -EINVAL;
+	}
+
+	/* Check if value fits into the target variable size */
+	if  (sinfo->size < sizeof(new_val)) {
+		bool is_signed = is_signed_type(base_type);
+		__u32 unsigned_bits = sinfo->size * 8 - (is_signed ? 1 : 0);
+		long long max_val = 1ll << unsigned_bits;
+
+		if (new_val >= max_val || new_val < -max_val) {
+			fprintf(stderr,
+				"Variable %s value %lld is out of range [%lld; %lld]\n",
+				btf__name_by_offset(btf, t->name_off), new_val,
+				is_signed ? -max_val : 0, max_val - 1);
+			return -EINVAL;
+		}
+	}
+
+	ptr = (void *)bpf_map__initial_value(map, &size);
+	if (!ptr || (sinfo->offset + sinfo->size > size))
+		return -EINVAL;
+
+	memcpy(ptr + sinfo->offset, &new_val, sinfo->size);
+	return 0;
+}
+
+static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, int npresets)
+{
+	struct btf_var_secinfo *sinfo;
+	const char *sec_name;
+	const struct btf_type *type;
+	struct bpf_map *map;
+	struct btf *btf;
+	int i, j, k, n, cnt, err, preset_cnt = 0;
+
+	if (npresets == 0)
+		return 0;
+
+	btf = bpf_object__btf(obj);
+	if (!btf)
+		return -EINVAL;
+
+	cnt = btf__type_cnt(btf);
+	for (i  = 0; i != cnt; ++i) {
+		type = btf__type_by_id(btf, i);
+
+		if (!btf_is_datasec(type))
+			continue;
+
+		sinfo = btf_var_secinfos(type);
+		sec_name = btf__name_by_offset(btf, type->name_off);
+		map = bpf_object__find_map_by_name(obj, sec_name);
+		if (!map)
+			continue;
+
+		n = btf_vlen(type);
+		for (j = 0; j < n; ++j, ++sinfo) {
+			const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
+			const char *var_name = btf__name_by_offset(btf, var_type->name_off);
+
+			if (!btf_is_var(var_type))
+				continue;
+
+			for (k = 0; k < npresets; ++k) {
+				if (strcmp(var_name, presets[k].name) != 0)
+					continue;
+
+				err = set_global_var(obj, btf, var_type, map, sinfo,
+						     presets[k].value);
+				if (err)
+					return err;
+
+				preset_cnt++;
+				break;
+			}
+		}
+	}
+	if (preset_cnt != npresets)
+		fprintf(stderr, "Some global variable presets have not been applied\n");
+
+	return 0;
+}
+
 static int process_obj(const char *filename)
 {
 	const char *base_filename = basename(strdupa(filename));
@@ -1338,6 +1521,12 @@ static int process_obj(const char *filename)
 		prog_cnt++;
 	}
 
+	err = set_global_vars(obj, env.presets, env.npresets);
+	if (err) {
+		fprintf(stderr, "Failed to set global variables\n");
+		goto cleanup;
+	}
+
 	if (prog_cnt == 1) {
 		prog = bpf_object__next_program(obj, NULL);
 		bpf_program__set_autoload(prog, true);
-- 
2.48.1

Re: [PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Eduard Zingerman]

On Mon, 2025-02-03 at 16:40 +0000, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> To better verify some complex BPF programs we'd like to preset global
> variables.
> This patch introduces CLI argument `--set-global-vars` to veristat, that
> allows presetting values to global variables defined in BPF program. For
> example:
> 
> prog.c:
> ```
> enum Enum { ELEMENT1 = 0, ELEMENT2 = 5 };
> const volatile __s64 a = 5;
> const volatile __u8 b = 5;
> const volatile enum Enum c = ELEMENT2;
> const volatile bool d = false;
> 
> char arr[4] = {0};
> 
> SEC("tp_btf/sched_switch")
> int BPF_PROG(...)
> {
> 	bpf_printk("%c\n", arr[a]);
> 	bpf_printk("%c\n", arr[b]);
> 	bpf_printk("%c\n", arr[c]);
> 	bpf_printk("%c\n", arr[d]);
> 	return 0;
> }
> ```
> By default verification of the program fails:
> ```
> ./veristat prog.bpf.o
> ```
> By presetting global variables, we can make verification pass:
> ```
> ./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
> ```
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---

This is super useful, thank you!
Maybe also add an ability to read variables list from a file?
(e.g. using -g @file-name syntax as in -f).

Worked fine for my small example, but failed to affect an object file
with multiple programs, see below.

Also, given that it is non-trivial to see if variable had indeed been set,
I think it would be useful to add a selftest that does
system("./veristat -l7 -v -g ...") and matches log output to check that
values are set correctly, e.g. I used the following simple test:

	const volatile u8  _u8  = 0;
	const volatile u16 _u16 = 0;
	const volatile u32 _u32 = 0;
	const volatile u64 _u64 = 0;
	const volatile s8  _s8  = 0;
	const volatile s16 _s16 = 0;
	const volatile s32 _s32 = 0;
	const volatile s64 _s64 = 0;

	SEC("socket")
	int test_globals(void *ctx)
	{
		volatile unsigned long cnt;
		cnt = _u8;
		cnt = _u16;
		cnt = _u32;
		cnt = _u64;
		cnt = _s8;
		cnt = _s16;
		cnt = _s32;
		cnt = _s64;
		return cnt;
	}

>  tools/testing/selftests/bpf/veristat.c | 189 +++++++++++++++++++++++++
>  1 file changed, 189 insertions(+)

[...]

> @@ -1292,6 +1312,169 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>  	return 0;
>  };
>  
> +static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size)
> +{
> +	char *state;
> +	char *next;
> +	int i = 0;
> +
> +	while ((next = strtok_r(i ? NULL : expr, ";", &state))) {
> +		char *eq_ptr = strchr(next, '=');
> +		char *name_ptr = next;
> +		char *name_end = eq_ptr - 1;
> +		char *val_ptr = eq_ptr + 1;
> +
> +		if (!eq_ptr)
> +			continue;

Nit: error message here?

> +
> +		if (i >= capacity) {
> +			fprintf(stderr, "Too many global variable presets\n");
> +			return -EINVAL;
> +		}
> +		while (isspace(*name_ptr))
> +			++name_ptr;
> +		while (isspace(*name_end))
> +			--name_end;
> +
> +		*(name_end + 1) = '\0';
> +		presets[i].name = strdup(name_ptr);
> +		errno = 0;
> +		presets[i].value = strtoll(val_ptr, NULL, 10);

Nit: using base of 0 would allow to specify values either as decimals or in hex
     (using '0x' prefix).

> +		if (errno == ERANGE) {
> +			errno = 0;
> +			presets[i].value = strtoull(val_ptr, NULL, 10);
> +		}
> +		if (errno) {
> +			fprintf(stderr, "Could not parse integer value %s\n", val_ptr);
> +			return -EINVAL;
> +		}
> +		++i;
> +	}
> +	*size = i;
> +	return 0;
> +}
> +
> +static bool is_signed_type(const struct btf_type *type)
> +{
> +	if (btf_is_int(type))

Nit: enums could be signed as well.

> +		return btf_int_encoding(type) & BTF_INT_SIGNED;
> +	return true;
> +}
> +
> +static const struct btf_type *var_base_type(const struct btf *btf, const struct btf_type *type)
> +{
> +	switch (btf_kind(type)) {
> +	case BTF_KIND_VAR:
> +	case BTF_KIND_TYPE_TAG:
> +	case BTF_KIND_CONST:
> +	case BTF_KIND_VOLATILE:
> +	case BTF_KIND_RESTRICT:
> +	case BTF_KIND_TYPEDEF:
> +	case BTF_KIND_DECL_TAG:
> +		return var_base_type(btf, btf__type_by_id(btf, type->type));
> +	}
> +	return type;
> +}
> +
> +static bool is_preset_supported(const struct btf_type *t)
> +{
> +	return btf_is_int(t) || btf_is_enum(t) || btf_is_enum64(t);
> +}
> +
> +static int set_global_var(struct bpf_object *obj, struct btf *btf, const struct btf_type *t,
> +			  struct bpf_map *map, struct btf_var_secinfo *sinfo, long long new_val)
> +{
> +	const struct btf_type *base_type;
> +	void *ptr;
> +	size_t size;
> +
> +	base_type = var_base_type(btf, t);
> +	if (!is_preset_supported(base_type)) {
> +		fprintf(stderr, "Setting global variable for btf kind %d is not supported\n",
> +			btf_kind(base_type));
> +		return -EINVAL;
> +	}
> +
> +	/* Check if value fits into the target variable size */
> +	if  (sinfo->size < sizeof(new_val)) {
> +		bool is_signed = is_signed_type(base_type);
> +		__u32 unsigned_bits = sinfo->size * 8 - (is_signed ? 1 : 0);
> +		long long max_val = 1ll << unsigned_bits;
> +
> +		if (new_val >= max_val || new_val < -max_val) {
> +			fprintf(stderr,
> +				"Variable %s value %lld is out of range [%lld; %lld]\n",
> +				btf__name_by_offset(btf, t->name_off), new_val,
> +				is_signed ? -max_val : 0, max_val - 1);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ptr = (void *)bpf_map__initial_value(map, &size);
> +	if (!ptr || (sinfo->offset + sinfo->size > size))
> +		return -EINVAL;
> +
> +	memcpy(ptr + sinfo->offset, &new_val, sinfo->size);

will this work for big endian?

> +	return 0;
> +}
> +
> +static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, int npresets)
> +{
> +	struct btf_var_secinfo *sinfo;
> +	const char *sec_name;
> +	const struct btf_type *type;
> +	struct bpf_map *map;
> +	struct btf *btf;
> +	int i, j, k, n, cnt, err, preset_cnt = 0;
> +
> +	if (npresets == 0)
> +		return 0;
> +
> +	btf = bpf_object__btf(obj);
> +	if (!btf)
> +		return -EINVAL;
> +
> +	cnt = btf__type_cnt(btf);
> +	for (i  = 0; i != cnt; ++i) {
> +		type = btf__type_by_id(btf, i);
> +
> +		if (!btf_is_datasec(type))
> +			continue;
> +
> +		sinfo = btf_var_secinfos(type);
> +		sec_name = btf__name_by_offset(btf, type->name_off);
> +		map = bpf_object__find_map_by_name(obj, sec_name);
> +		if (!map)
> +			continue;
> +
> +		n = btf_vlen(type);
> +		for (j = 0; j < n; ++j, ++sinfo) {
> +			const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
> +			const char *var_name = btf__name_by_offset(btf, var_type->name_off);
> +
> +			if (!btf_is_var(var_type))
> +				continue;
> +
> +			for (k = 0; k < npresets; ++k) {
> +				if (strcmp(var_name, presets[k].name) != 0)
> +					continue;
> +
> +				err = set_global_var(obj, btf, var_type, map, sinfo,
> +						     presets[k].value);
> +				if (err)
> +					return err;
> +
> +				preset_cnt++;
> +				break;
> +			}
> +		}
> +	}
> +	if (preset_cnt != npresets)
> +		fprintf(stderr, "Some global variable presets have not been applied\n");

Nit: it would be nice to print which ones were not set.

> +
> +	return 0;
> +}
> +
>  static int process_obj(const char *filename)
>  {
>  	const char *base_filename = basename(strdupa(filename));
> @@ -1338,6 +1521,12 @@ static int process_obj(const char *filename)
>  		prog_cnt++;
>  	}
>  
> +	err = set_global_vars(obj, env.presets, env.npresets);
> +	if (err) {
> +		fprintf(stderr, "Failed to set global variables\n");
> +		goto cleanup;
> +	}
> +
>  	if (prog_cnt == 1) {
>  		prog = bpf_object__next_program(obj, NULL);
>  		bpf_program__set_autoload(prog, true);

Same needs to happen for the loop below when prog_cnt != 1, e.g.:

@@ -1544,6 +1544,12 @@ static int process_obj(const char *filename)
                        goto cleanup;
                }
 
+               err = set_global_vars(tobj, env.presets, env.npresets);
+               if (err) {
+                       fprintf(stderr, "Failed to set global variables\n");
+                       goto cleanup;
+               }
+
                lprog = NULL;
                bpf_object__for_each_program(tprog, tobj) {
                        const char *tprog_name = bpf_program__name(tprog);

Or, better yet, get rid of the `prog_cnt == 1` special case.

Re: [PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Mykyta Yatsenko]

On 03/02/2025 22:56, Eduard Zingerman wrote:
> On Mon, 2025-02-03 at 16:40 +0000, Mykyta Yatsenko wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> To better verify some complex BPF programs we'd like to preset global
>> variables.
>> This patch introduces CLI argument `--set-global-vars` to veristat, that
>> allows presetting values to global variables defined in BPF program. For
>> example:
>>
>> prog.c:
>> ```
>> enum Enum { ELEMENT1 = 0, ELEMENT2 = 5 };
>> const volatile __s64 a = 5;
>> const volatile __u8 b = 5;
>> const volatile enum Enum c = ELEMENT2;
>> const volatile bool d = false;
>>
>> char arr[4] = {0};
>>
>> SEC("tp_btf/sched_switch")
>> int BPF_PROG(...)
>> {
>> 	bpf_printk("%c\n", arr[a]);
>> 	bpf_printk("%c\n", arr[b]);
>> 	bpf_printk("%c\n", arr[c]);
>> 	bpf_printk("%c\n", arr[d]);
>> 	return 0;
>> }
>> ```
>> By default verification of the program fails:
>> ```
>> ./veristat prog.bpf.o
>> ```
>> By presetting global variables, we can make verification pass:
>> ```
>> ./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
>> ```
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
> This is super useful, thank you!
> Maybe also add an ability to read variables list from a file?
> (e.g. using -g @file-name syntax as in -f).
>
> Worked fine for my small example, but failed to affect an object file
> with multiple programs, see below.
>
> Also, given that it is non-trivial to see if variable had indeed been set,
> I think it would be useful to add a selftest that does
> system("./veristat -l7 -v -g ...") and matches log output to check that
> values are set correctly, e.g. I used the following simple test:
>
> 	const volatile u8  _u8  = 0;
> 	const volatile u16 _u16 = 0;
> 	const volatile u32 _u32 = 0;
> 	const volatile u64 _u64 = 0;
> 	const volatile s8  _s8  = 0;
> 	const volatile s16 _s16 = 0;
> 	const volatile s32 _s32 = 0;
> 	const volatile s64 _s64 = 0;
>
> 	SEC("socket")
> 	int test_globals(void *ctx)
> 	{
> 		volatile unsigned long cnt;
> 		cnt = _u8;
> 		cnt = _u16;
> 		cnt = _u32;
> 		cnt = _u64;
> 		cnt = _s8;
> 		cnt = _s16;
> 		cnt = _s32;
> 		cnt = _s64;
> 		return cnt;
> 	}
>
>>   tools/testing/selftests/bpf/veristat.c | 189 +++++++++++++++++++++++++
>>   1 file changed, 189 insertions(+)
> [...]
>
>> @@ -1292,6 +1312,169 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>   	return 0;
>>   };
>>   
>> +static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size)
>> +{
>> +	char *state;
>> +	char *next;
>> +	int i = 0;
>> +
>> +	while ((next = strtok_r(i ? NULL : expr, ";", &state))) {
>> +		char *eq_ptr = strchr(next, '=');
>> +		char *name_ptr = next;
>> +		char *name_end = eq_ptr - 1;
>> +		char *val_ptr = eq_ptr + 1;
>> +
>> +		if (!eq_ptr)
>> +			continue;
> Nit: error message here?
>
>> +
>> +		if (i >= capacity) {
>> +			fprintf(stderr, "Too many global variable presets\n");
>> +			return -EINVAL;
>> +		}
>> +		while (isspace(*name_ptr))
>> +			++name_ptr;
>> +		while (isspace(*name_end))
>> +			--name_end;
>> +
>> +		*(name_end + 1) = '\0';
>> +		presets[i].name = strdup(name_ptr);
>> +		errno = 0;
>> +		presets[i].value = strtoll(val_ptr, NULL, 10);
> Nit: using base of 0 would allow to specify values either as decimals or in hex
>       (using '0x' prefix).
>
>> +		if (errno == ERANGE) {
>> +			errno = 0;
>> +			presets[i].value = strtoull(val_ptr, NULL, 10);
>> +		}
>> +		if (errno) {
>> +			fprintf(stderr, "Could not parse integer value %s\n", val_ptr);
>> +			return -EINVAL;
>> +		}
>> +		++i;
>> +	}
>> +	*size = i;
>> +	return 0;
>> +}
>> +
>> +static bool is_signed_type(const struct btf_type *type)
>> +{
>> +	if (btf_is_int(type))
> Nit: enums could be signed as well.
>
>> +		return btf_int_encoding(type) & BTF_INT_SIGNED;
>> +	return true;
>> +}
>> +
>> +static const struct btf_type *var_base_type(const struct btf *btf, const struct btf_type *type)
>> +{
>> +	switch (btf_kind(type)) {
>> +	case BTF_KIND_VAR:
>> +	case BTF_KIND_TYPE_TAG:
>> +	case BTF_KIND_CONST:
>> +	case BTF_KIND_VOLATILE:
>> +	case BTF_KIND_RESTRICT:
>> +	case BTF_KIND_TYPEDEF:
>> +	case BTF_KIND_DECL_TAG:
>> +		return var_base_type(btf, btf__type_by_id(btf, type->type));
>> +	}
>> +	return type;
>> +}
>> +
>> +static bool is_preset_supported(const struct btf_type *t)
>> +{
>> +	return btf_is_int(t) || btf_is_enum(t) || btf_is_enum64(t);
>> +}
>> +
>> +static int set_global_var(struct bpf_object *obj, struct btf *btf, const struct btf_type *t,
>> +			  struct bpf_map *map, struct btf_var_secinfo *sinfo, long long new_val)
>> +{
>> +	const struct btf_type *base_type;
>> +	void *ptr;
>> +	size_t size;
>> +
>> +	base_type = var_base_type(btf, t);
>> +	if (!is_preset_supported(base_type)) {
>> +		fprintf(stderr, "Setting global variable for btf kind %d is not supported\n",
>> +			btf_kind(base_type));
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Check if value fits into the target variable size */
>> +	if  (sinfo->size < sizeof(new_val)) {
>> +		bool is_signed = is_signed_type(base_type);
>> +		__u32 unsigned_bits = sinfo->size * 8 - (is_signed ? 1 : 0);
>> +		long long max_val = 1ll << unsigned_bits;
>> +
>> +		if (new_val >= max_val || new_val < -max_val) {
>> +			fprintf(stderr,
>> +				"Variable %s value %lld is out of range [%lld; %lld]\n",
>> +				btf__name_by_offset(btf, t->name_off), new_val,
>> +				is_signed ? -max_val : 0, max_val - 1);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	ptr = (void *)bpf_map__initial_value(map, &size);
>> +	if (!ptr || (sinfo->offset + sinfo->size > size))
>> +		return -EINVAL;
>> +
>> +	memcpy(ptr + sinfo->offset, &new_val, sinfo->size);
> will this work for big endian?
>
>> +	return 0;
>> +}
>> +
>> +static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, int npresets)
>> +{
>> +	struct btf_var_secinfo *sinfo;
>> +	const char *sec_name;
>> +	const struct btf_type *type;
>> +	struct bpf_map *map;
>> +	struct btf *btf;
>> +	int i, j, k, n, cnt, err, preset_cnt = 0;
>> +
>> +	if (npresets == 0)
>> +		return 0;
>> +
>> +	btf = bpf_object__btf(obj);
>> +	if (!btf)
>> +		return -EINVAL;
>> +
>> +	cnt = btf__type_cnt(btf);
>> +	for (i  = 0; i != cnt; ++i) {
>> +		type = btf__type_by_id(btf, i);
>> +
>> +		if (!btf_is_datasec(type))
>> +			continue;
>> +
>> +		sinfo = btf_var_secinfos(type);
>> +		sec_name = btf__name_by_offset(btf, type->name_off);
>> +		map = bpf_object__find_map_by_name(obj, sec_name);
>> +		if (!map)
>> +			continue;
>> +
>> +		n = btf_vlen(type);
>> +		for (j = 0; j < n; ++j, ++sinfo) {
>> +			const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
>> +			const char *var_name = btf__name_by_offset(btf, var_type->name_off);
>> +
>> +			if (!btf_is_var(var_type))
>> +				continue;
>> +
>> +			for (k = 0; k < npresets; ++k) {
>> +				if (strcmp(var_name, presets[k].name) != 0)
>> +					continue;
>> +
>> +				err = set_global_var(obj, btf, var_type, map, sinfo,
>> +						     presets[k].value);
>> +				if (err)
>> +					return err;
>> +
>> +				preset_cnt++;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	if (preset_cnt != npresets)
>> +		fprintf(stderr, "Some global variable presets have not been applied\n");
> Nit: it would be nice to print which ones were not set.
>
>> +
>> +	return 0;
>> +}
>> +
>>   static int process_obj(const char *filename)
>>   {
>>   	const char *base_filename = basename(strdupa(filename));
>> @@ -1338,6 +1521,12 @@ static int process_obj(const char *filename)
>>   		prog_cnt++;
>>   	}
>>   
>> +	err = set_global_vars(obj, env.presets, env.npresets);
>> +	if (err) {
>> +		fprintf(stderr, "Failed to set global variables\n");
>> +		goto cleanup;
>> +	}
>> +
>>   	if (prog_cnt == 1) {
>>   		prog = bpf_object__next_program(obj, NULL);
>>   		bpf_program__set_autoload(prog, true);
> Same needs to happen for the loop below when prog_cnt != 1, e.g.:
>
> @@ -1544,6 +1544,12 @@ static int process_obj(const char *filename)
>                          goto cleanup;
>                  }
>   
> +               err = set_global_vars(tobj, env.presets, env.npresets);
> +               if (err) {
> +                       fprintf(stderr, "Failed to set global variables\n");
> +                       goto cleanup;
> +               }
> +
>                  lprog = NULL;
>                  bpf_object__for_each_program(tprog, tobj) {
>                          const char *tprog_name = bpf_program__name(tprog);
>
> Or, better yet, get rid of the `prog_cnt == 1` special case.
>
Thanks for the suggestions, make sense, going to address in v2.

Re: [PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Andrii Nakryiko]

On Mon, Feb 3, 2025 at 8:41 AM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> From: Mykyta Yatsenko <yatsenko@meta.com>
>
> To better verify some complex BPF programs we'd like to preset global
> variables.
> This patch introduces CLI argument `--set-global-vars` to veristat, that
> allows presetting values to global variables defined in BPF program. For
> example:
>
> prog.c:
> ```
> enum Enum { ELEMENT1 = 0, ELEMENT2 = 5 };
> const volatile __s64 a = 5;
> const volatile __u8 b = 5;
> const volatile enum Enum c = ELEMENT2;
> const volatile bool d = false;
>
> char arr[4] = {0};
>
> SEC("tp_btf/sched_switch")
> int BPF_PROG(...)
> {
>         bpf_printk("%c\n", arr[a]);
>         bpf_printk("%c\n", arr[b]);
>         bpf_printk("%c\n", arr[c]);
>         bpf_printk("%c\n", arr[d]);
>         return 0;
> }
> ```
> By default verification of the program fails:
> ```
> ./veristat prog.bpf.o
> ```
> By presetting global variables, we can make verification pass:
> ```
> ./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
> ```
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
>  tools/testing/selftests/bpf/veristat.c | 189 +++++++++++++++++++++++++
>  1 file changed, 189 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 06af5029885b..65bb8a773d23 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -154,6 +154,11 @@ struct filter {
>         bool abs;
>  };
>
> +struct var_preset {
> +       char *name;
> +       long long value;
> +};
> +
>  static struct env {
>         char **filenames;
>         int filename_cnt;
> @@ -195,6 +200,8 @@ static struct env {
>         int progs_processed;
>         int progs_skipped;
>         int top_src_lines;
> +       struct var_preset *presets;
> +       int npresets;
>  } env;
>
>  static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va_list args)
> @@ -246,12 +253,14 @@ static const struct argp_option opts[] = {
>         { "test-reg-invariants", 'r', NULL, 0,
>           "Force BPF verifier failure on register invariant violation (BPF_F_TEST_REG_INVARIANTS program flag)" },
>         { "top-src-lines", 'S', "N", 0, "Emit N most frequent source code lines" },
> +       { "set-global-vars", 'g', "GLOBALS", 0, "Set global variables provided in the expression, for example \"var1 = 1; var2 = 2\"" },

nit: this is subjective, but I feel like -G instead of -g would be
better here to be more noticeable

but main point from me here would be to avoid parsing multiple values,
it's better to allow repeated -G uses and treat each value as strictly
single variable initialization. So instead of:

./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"

we'll have:

./veristat wq.bpf.o  -G "a = 0" -G "b = 1" -G "c = 2" -G "d = 3"

A touch more verbose for many variables, but not significantly so. On
the other hand, less parsing, and less arbitrary choices of what
separator (;) to use. WDYT?

pw-bot: cr

>         {},
>  };
>
>  static int parse_stats(const char *stats_str, struct stat_specs *specs);
>  static int append_filter(struct filter **filters, int *cnt, const char *str);
>  static int append_filter_file(const char *path);
> +static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size);
>
>  static error_t parse_arg(int key, char *arg, struct argp_state *state)
>  {
> @@ -363,6 +372,17 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>                         return -ENOMEM;
>                 env.filename_cnt++;
>                 break;
> +       case 'g': {
> +               char *expr = strdup(arg);
> +
> +               env.presets = calloc(64, sizeof(*env.presets));
> +               if (parse_var_presets(expr, env.presets, 64, &env.npresets)) {
> +                       fprintf(stderr, "Could not parse global variables preset: %s\n", arg);
> +                       argp_usage(state);
> +               }
> +               free(expr);
> +               break;
> +       }
>         default:
>                 return ARGP_ERR_UNKNOWN;
>         }
> @@ -1292,6 +1312,169 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>         return 0;
>  };
>
> +static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size)
> +{
> +       char *state;
> +       char *next;
> +       int i = 0;
> +
> +       while ((next = strtok_r(i ? NULL : expr, ";", &state))) {
> +               char *eq_ptr = strchr(next, '=');
> +               char *name_ptr = next;
> +               char *name_end = eq_ptr - 1;
> +               char *val_ptr = eq_ptr + 1;
> +
> +               if (!eq_ptr)
> +                       continue;
> +
> +               if (i >= capacity) {

why artificially hard-coding maximum capacity? we have malloc()

> +                       fprintf(stderr, "Too many global variable presets\n");
> +                       return -EINVAL;
> +               }
> +               while (isspace(*name_ptr))
> +                       ++name_ptr;
> +               while (isspace(*name_end))
> +                       --name_end;
> +
> +               *(name_end + 1) = '\0';
> +               presets[i].name = strdup(name_ptr);
> +               errno = 0;
> +               presets[i].value = strtoll(val_ptr, NULL, 10);
> +               if (errno == ERANGE) {
> +                       errno = 0;
> +                       presets[i].value = strtoull(val_ptr, NULL, 10);
> +               }
> +               if (errno) {
> +                       fprintf(stderr, "Could not parse integer value %s\n", val_ptr);
> +                       return -EINVAL;
> +               }
> +               ++i;
> +       }
> +       *size = i;
> +       return 0;
> +}
> +

it would be nice to be able to specify enums both by name and by value, WDYT?

> +static bool is_signed_type(const struct btf_type *type)
> +{
> +       if (btf_is_int(type))
> +               return btf_int_encoding(type) & BTF_INT_SIGNED;

enum can be signed as well, I think (but different way to specify
that, through kflag)

> +       return true;
> +}
> +
> +static const struct btf_type *var_base_type(const struct btf *btf, const struct btf_type *type)
> +{
> +       switch (btf_kind(type)) {
> +       case BTF_KIND_VAR:
> +       case BTF_KIND_TYPE_TAG:
> +       case BTF_KIND_CONST:
> +       case BTF_KIND_VOLATILE:
> +       case BTF_KIND_RESTRICT:
> +       case BTF_KIND_TYPEDEF:
> +       case BTF_KIND_DECL_TAG:
> +               return var_base_type(btf, btf__type_by_id(btf, type->type));

why recursion, just do a loop?

and libbpf actually has "btf__resolve_type()", see if you can just use that?

> +       }
> +       return type;
> +}
> +

[...]

Re: [PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Eduard Zingerman]

On Wed, 2025-02-05 at 17:06 -0800, Andrii Nakryiko wrote:

[...]

> but main point from me here would be to avoid parsing multiple values,
> it's better to allow repeated -G uses and treat each value as strictly
> single variable initialization. So instead of:
> 
> ./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
> 
> we'll have:
> 
> ./veristat wq.bpf.o  -G "a = 0" -G "b = 1" -G "c = 2" -G "d = 3"
> 
> A touch more verbose for many variables, but not significantly so. On
> the other hand, less parsing, and less arbitrary choices of what
> separator (;) to use. WDYT?
> 
> pw-bot: cr

In a sibling thread I asked about '-g @file' support, allowing to list
variables in files. This could be worked around as $(cat file) in
the command line, of course.

[...]

Re: [PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Andrii Nakryiko]

On Wed, Feb 5, 2025 at 5:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2025-02-05 at 17:06 -0800, Andrii Nakryiko wrote:
>
> [...]
>
> > but main point from me here would be to avoid parsing multiple values,
> > it's better to allow repeated -G uses and treat each value as strictly
> > single variable initialization. So instead of:
> >
> > ./veristat wq.bpf.o  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
> >
> > we'll have:
> >
> > ./veristat wq.bpf.o  -G "a = 0" -G "b = 1" -G "c = 2" -G "d = 3"
> >
> > A touch more verbose for many variables, but not significantly so. On
> > the other hand, less parsing, and less arbitrary choices of what
> > separator (;) to use. WDYT?
> >
> > pw-bot: cr
>
> In a sibling thread I asked about '-g @file' support, allowing to list
> variables in files. This could be worked around as $(cat file) in
> the command line, of course.

What I proposed doesn't interfere with this, it actually helps with
it: each line is treated as a separate `-G <line>` invocation, and
each line initializes a single variable.

>
> [...]
>