Skip to content

Commit f2e6fba

Browse files
authored
Merge pull request #111 from pythonbpf/fix/trivial-xfails
Fix stale XFAILs that can now PASS
2 parents d093516 + 37fe5d7 commit f2e6fba

16 files changed

Lines changed: 229 additions & 123 deletions

File tree

‎pythonbpf/expr/expr_pass.py‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -842,5 +842,13 @@ def handle_expr(
842842
call,
843843
local_sym_tab,
844844
)
845+
elif any(isinstance(node, ast.Call) for node in ast.walk(call)):
846+
# A docstring or a bare name has no effect and can be dropped; an
847+
# expression with a call in it (`f() if c else g()`, `a() or b()`)
848+
# does, and dropping it would silently lose the call.
849+
raise NotImplementedError(
850+
f"Unsupported expression statement on line {expr.lineno}: "
851+
f"{type(call).__name__}; write it as an if statement or assignment"
852+
)
845853
else:
846-
logger.info("Unsupported expression type")
854+
logger.debug(f"Dropping expression statement without effect: {ast.dump(call)}")

‎pythonbpf/expr/type_normalization.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,14 @@ def convert_to_bool(builder, val):
113113
"""Convert a value to boolean."""
114114
if val.type == ir.IntType(1):
115115
return val
116+
if not isinstance(val.type, (ir.IntType, ir.PointerType)):
117+
# C rejects `if (s)` on a struct, and Python would call any object
118+
# true; either way it is never what was meant.
119+
kind = "struct" if isinstance(val.type, ir.BaseStructType) else "value"
120+
raise TypeError(
121+
f"a {kind} of type {val.type} has no truth value: test one of its "
122+
"fields instead"
123+
)
116124
if isinstance(val.type, ir.PointerType):
117125
zero = ir.Constant(val.type, None)
118126
else:

‎pythonbpf/helper/helper_utils.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,11 @@ def get_or_create_ptr_from_arg(
6767
else:
6868
# Stack space is already allocated
6969
ptr = get_var_ptr_from_name(arg.id, local_sym_tab)
70+
if symbol is not None and isinstance(symbol.ir_type, ir.PointerType):
71+
# The slot holds a pointer (a map lookup result, or a local
72+
# rebound to point at a temporary): the helper wants that
73+
# pointer, not the address of the slot it is kept in.
74+
ptr = builder.load(ptr)
7075
elif isinstance(arg, ast.Constant) and isinstance(arg.value, int):
7176
int_width = 64 # Default to i64
7277
if expected_type and isinstance(expected_type, ir.IntType):
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/* Reference for passing a map lookup result to a helper. `prev` is a local
2+
* holding the pointer bpf_map_lookup_elem returned; bpf_map_update_elem gets
3+
* that pointer (the map value), loaded from prev's slot. It never gets &prev,
4+
* the slot's own address, which would make the map store a stack address.
5+
* The clang -O0 IR for this file is the specification for the pointer depth
6+
* that tests/test_signedness_ir.py checks:
7+
* copy() mirrors passing_tests/helpers/map_update_from_lookup.py
8+
* rebind() mirrors passing_tests/vmlinux/named_arg.py, where `prev = prev + 1`
9+
* rebinds prev to a stack temporary holding the sum; the helper
10+
* still gets the pointer loaded from prev's slot. */
11+
#define SEC(name) __attribute__((section(name), used))
12+
#define __uint(name, val) int (*name)[val]
13+
#define __type(name, val) typeof(val) *name
14+
typedef long long __s64;
15+
16+
#define BPF_MAP_TYPE_HASH 1
17+
#define BPF_ANY 0
18+
#define XDP_PASS 2
19+
20+
static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *)1;
21+
static long (*bpf_map_update_elem)(void *map, const void *key,
22+
const void *value, __s64 flags) = (void *)2;
23+
24+
struct {
25+
__uint(type, BPF_MAP_TYPE_HASH);
26+
__uint(max_entries, 2);
27+
__type(key, __s64);
28+
__type(value, __s64);
29+
} count SEC(".maps");
30+
31+
SEC("tracepoint/syscalls/sys_enter_getpid")
32+
__s64 copy(void *ctx)
33+
{
34+
__s64 key = 0;
35+
__s64 *prev = bpf_map_lookup_elem(&count, &key);
36+
if (prev) {
37+
key = 1;
38+
bpf_map_update_elem(&count, &key, prev, BPF_ANY);
39+
}
40+
return 0;
41+
}
42+
43+
SEC("xdp")
44+
__s64 rebind(void *ctx)
45+
{
46+
__s64 key = 0;
47+
__s64 tmp;
48+
__s64 *prev = bpf_map_lookup_elem(&count, &key);
49+
if (prev) {
50+
tmp = *prev + 1;
51+
prev = &tmp;
52+
bpf_map_update_elem(&count, &key, prev, BPF_ANY);
53+
return XDP_PASS;
54+
}
55+
tmp = 1;
56+
bpf_map_update_elem(&count, &key, &tmp, BPF_ANY);
57+
return XDP_PASS;
58+
}
59+
60+
char LICENSE[] SEC("license") = "GPL";

tests/failing_tests/conditionals/struct_ptr.py renamed to tests/failing_tests/conditionals/struct_value.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
from pythonbpf import bpf, struct, section, bpfglobal, compile
22
from ctypes import c_void_p, c_int64, c_uint64
33

4-
# NOTE: Decided against fixing this
5-
# as one workaround is to just check any field of the struct
6-
# in the if statement. Ugly but works.
7-
# Might fix in future.
4+
# Negative test: `dat` is a struct value, and a struct has no truth value
5+
# (C rejects `if (s)`; Python would call any object true). It is a compile
6+
# error that says to test one of the struct's fields instead. A *pointer* to
7+
# a struct, such as a map lookup result, is a valid condition (non-null).
88

99

1010
@bpf

‎tests/failing_tests/vmlinux/direct_assign.py‎

Lines changed: 0 additions & 45 deletions
This file was deleted.

‎tests/failing_tests/vmlinux/xdp_pass.py‎

Lines changed: 0 additions & 54 deletions
This file was deleted.
Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,9 @@
33
from pythonbpf.maps import HashMap
44

55

6-
# NOTE: This example tries to reinterpret the variable `x` to a different type.
7-
# We do not allow this for now, as stack allocations are typed and have to be
8-
# done in the first basic block. Allowing re-interpretation would require
9-
# re-allocation of stack space (possibly in a new basic block), which is not
10-
# supported in eBPF yet.
11-
# We can allow bitcasts in cases where the width of the types is the same in
12-
# the future. But for now, we do not allow any re-interpretation of variables.
6+
# `x` is first bound to a map lookup (a pointer) and then rebound to an integer,
7+
# as Python allows; the comparison sees the integer. This was once refused
8+
# because stack slots are typed, and now works.
139

1410

1511
@bpf

tests/failing_tests/conditionals/helper_cond.py renamed to tests/passing_tests/conditionals/helper_cond.py

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@
22
from ctypes import c_void_p, c_int64, c_uint64
33
from pythonbpf.maps import HashMap
44

5-
# NOTE: Decided against fixing this
6-
# as a workaround is assigning the result of lookup to a variable
7-
# and then using that variable in the if statement.
8-
# Might fix in future.
5+
# A map lookup used directly in a comparison: the value is null-checked and
6+
# dereferenced, and a missing key takes the else branch.
97

108

119
@bpf
File renamed without changes.

0 commit comments

Comments
 (0)