Fix stale XFAILs that can now PASS

pythonbpf/expr/expr_pass.py

@@ -842,5 +842,13 @@ def handle_expr(
             call,
             local_sym_tab,
         )
+    elif any(isinstance(node, ast.Call) for node in ast.walk(call)):
+        # A docstring or a bare name has no effect and can be dropped; an
+        # expression with a call in it (`f() if c else g()`, `a() or b()`)
+        # does, and dropping it would silently lose the call.
+        raise NotImplementedError(
+            f"Unsupported expression statement on line {expr.lineno}: "
+            f"{type(call).__name__}; write it as an if statement or assignment"
+        )
     else:
-        logger.info("Unsupported expression type")
+        logger.debug(f"Dropping expression statement without effect: {ast.dump(call)}")

pythonbpf/expr/type_normalization.py

@@ -113,6 +113,14 @@ def convert_to_bool(builder, val):
     """Convert a value to boolean."""
     if val.type == ir.IntType(1):
         return val
+    if not isinstance(val.type, (ir.IntType, ir.PointerType)):
+        # C rejects `if (s)` on a struct, and Python would call any object
+        # true; either way it is never what was meant.
+        kind = "struct" if isinstance(val.type, ir.BaseStructType) else "value"
+        raise TypeError(
+            f"a {kind} of type {val.type} has no truth value: test one of its "
+            "fields instead"
+        )
     if isinstance(val.type, ir.PointerType):
         zero = ir.Constant(val.type, None)
     else:

pythonbpf/helper/helper_utils.py

@@ -67,6 +67,11 @@ def get_or_create_ptr_from_arg(
         else:
             # Stack space is already allocated
             ptr = get_var_ptr_from_name(arg.id, local_sym_tab)
+            if symbol is not None and isinstance(symbol.ir_type, ir.PointerType):
+                # The slot holds a pointer (a map lookup result, or a local
+                # rebound to point at a temporary): the helper wants that
+                # pointer, not the address of the slot it is kept in.
+                ptr = builder.load(ptr)
     elif isinstance(arg, ast.Constant) and isinstance(arg.value, int):
         int_width = 64  # Default to i64
         if expected_type and isinstance(expected_type, ir.IntType):

tests/c-form/map_update_from_lookup.bpf.c

@@ -0,0 +1,60 @@
+/* Reference for passing a map lookup result to a helper. `prev` is a local
+ * holding the pointer bpf_map_lookup_elem returned; bpf_map_update_elem gets
+ * that pointer (the map value), loaded from prev's slot. It never gets &prev,
+ * the slot's own address, which would make the map store a stack address.
+ * The clang -O0 IR for this file is the specification for the pointer depth
+ * that tests/test_signedness_ir.py checks:
+ *   copy()   mirrors passing_tests/helpers/map_update_from_lookup.py
+ *   rebind() mirrors passing_tests/vmlinux/named_arg.py, where `prev = prev + 1`
+ *            rebinds prev to a stack temporary holding the sum; the helper
+ *            still gets the pointer loaded from prev's slot. */
+#define SEC(name) __attribute__((section(name), used))
+#define __uint(name, val) int (*name)[val]
+#define __type(name, val) typeof(val) *name
+typedef long long __s64;
+
+#define BPF_MAP_TYPE_HASH 1
+#define BPF_ANY 0
+#define XDP_PASS 2
+
+static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *)1;
+static long (*bpf_map_update_elem)(void *map, const void *key,
+                   const void *value, __s64 flags) = (void *)2;
+
+struct {
+    __uint(type, BPF_MAP_TYPE_HASH);
+    __uint(max_entries, 2);
+    __type(key, __s64);
+    __type(value, __s64);
+} count SEC(".maps");
+
+SEC("tracepoint/syscalls/sys_enter_getpid")
+__s64 copy(void *ctx)
+{
+    __s64 key = 0;
+    __s64 *prev = bpf_map_lookup_elem(&count, &key);
+    if (prev) {
+        key = 1;
+        bpf_map_update_elem(&count, &key, prev, BPF_ANY);
+    }
+    return 0;
+}
+
+SEC("xdp")
+__s64 rebind(void *ctx)
+{
+    __s64 key = 0;
+    __s64 tmp;
+    __s64 *prev = bpf_map_lookup_elem(&count, &key);
+    if (prev) {
+        tmp = *prev + 1;
+        prev = &tmp;
+        bpf_map_update_elem(&count, &key, prev, BPF_ANY);
+        return XDP_PASS;
+    }
+    tmp = 1;
+    bpf_map_update_elem(&count, &key, &tmp, BPF_ANY);
+    return XDP_PASS;
+}
+
+char LICENSE[] SEC("license") = "GPL";

tests/failing_tests/conditionals/struct_ptr.py → tests/failing_tests/conditionals/struct_value.py

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

tests/failing_tests/assign/retype.py → tests/passing_tests/assign/retype.py

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

tests/failing_tests/conditionals/helper_cond.py → tests/passing_tests/conditionals/helper_cond.py

@@ -2,10 +2,8 @@
 from ctypes import c_void_p, c_int64, c_uint64
 from pythonbpf.maps import HashMap
 
-# NOTE: Decided against fixing this
-# as a workaround is assigning the result of lookup to a variable
-# and then using that variable in the if statement.
-# Might fix in future.
+# A map lookup used directly in a comparison: the value is null-checked and
+# dereferenced, and a missing key takes the else branch.
 
 
 @bpf

tests/passing_tests/helpers/map_update_from_lookup.py

@@ -0,0 +1,31 @@
+# A map lookup result passed straight to map.update(): the helper gets the
+# pointer the local holds (the map value), as bpf_map_update_elem(&m, &k, prev, 0)
+# would in C, not the address of the local's slot.
+
+from pythonbpf import bpf, map, section, bpfglobal, compile
+from pythonbpf.maps import HashMap
+from ctypes import c_void_p, c_int64
+
+
+@bpf
+@map
+def count() -> HashMap:
+    return HashMap(key=c_int64, value=c_int64, max_entries=2)
+
+
+@bpf
+@section("tracepoint/syscalls/sys_enter_getpid")
+def copy(ctx: c_void_p) -> c_int64:
+    prev = count.lookup(0)
+    if prev:
+        count.update(1, prev)
+    return c_int64(0)
+
+
+@bpf
+@bpfglobal
+def LICENSE() -> str:
+    return "GPL"
+
+
+compile()

tests/passing_tests/vmlinux/direct_assign.py

@@ -0,0 +1,37 @@
+from pythonbpf import bpf, map, section, bpfglobal, compile
+from vmlinux import XDP_PASS
+from pythonbpf.maps import HashMap
+
+from ctypes import c_void_p, c_int64
+
+# An expression (`prev + 1`) passed straight to map.update(): it is evaluated
+# into a scratch slot and the helper gets that slot's address. This was once
+# refused for want of a slot allocated up front, and now works.
+
+
+@bpf
+@map
+def count() -> HashMap:
+    return HashMap(key=c_int64, value=c_int64, max_entries=1)
+
+
+@bpf
+@section("xdp")
+def hello_world(ctx: c_void_p) -> c_int64:
+    prev = count.lookup(0)
+    if prev:
+        count.update(0, prev + 1)
+        return XDP_PASS
+    else:
+        count.update(0, 1)
+
+    return XDP_PASS
+
+
+@bpf
+@bpfglobal
+def LICENSE() -> str:
+    return "GPL"
+
+
+compile()

tests/failing_tests/vmlinux/named_arg.py → tests/passing_tests/vmlinux/named_arg.py

@@ -4,12 +4,11 @@
 
 from ctypes import c_void_p, c_int64
 
-# NOTE: This example exposes the problems with our typing system.
-# We can't do steps on line 25 and 27.
-# prev is of type i64**. For prev + 1, we deref it down to i64
-# To assign it back to prev, we need to go back to i64**.
-# We cannot allocate space for the intermediate type now.
-# We probably need to track the ref/deref chain for each variable.
+# `prev` starts as a map lookup result (a pointer) and is rebound to
+# `prev + 1`, which lives in a temporary that `prev` then points at. The update
+# must store the incremented value: the helper gets the pointer the local
+# holds, not the address of the local's own slot (that used to store a kernel
+# stack address into the map).
 
 
 @bpf

tests/passing_tests/vmlinux/xdp_pass.py

@@ -0,0 +1,41 @@
+# Counts packets in a one-entry map and lets every packet through. It used to
+# be an xfail, but the only thing failing was the old `count().lookup(...)`
+# spelling of a map call; `count.lookup(...)` is the current one.
+
+from pythonbpf import bpf, map, section, bpfglobal, compile
+from pythonbpf.maps import HashMap
+from vmlinux import XDP_PASS
+from vmlinux import struct_xdp_md
+from ctypes import c_int64
+
+
+@bpf
+@map
+def count() -> HashMap:
+    return HashMap(key=c_int64, value=c_int64, max_entries=1)
+
+
+@bpf
+@section("xdp")
+def hello_world(ctx: struct_xdp_md) -> c_int64:
+    key = 0
+    one = 1
+    prev = count.lookup(key)
+    if prev:
+        prevval = prev + 1
+        print(f"count: {prevval}")
+        count.update(key, prevval)
+        return XDP_PASS
+    else:
+        count.update(key, one)
+
+    return XDP_PASS
+
+
+@bpf
+@bpfglobal
+def LICENSE() -> str:
+    return "GPL"
+
+
+compile()

tests/test_config.toml

@@ -15,7 +15,9 @@
 
 [xfail]
 
-"failing_tests/conditionals/struct_ptr.py" = {reason = "Struct pointer used directly as boolean condition not supported", level = "ir"}
+"failing_tests/conditionals/struct_value.py" = {reason = "A struct value has no truth value (C rejects `if (s)`): a compile error naming the struct", level = "ir"}
+
+"failing_tests/conditionals/oneline.py" = {reason = "A conditional expression used as a statement (`f() if c else g()`) is not lowered: a compile error rather than silently dropping both calls; write it as an if statement", level = "ir"}
 
 "failing_tests/return_struct.py" = {reason = "Returning a struct value from an integer function is a type error (nothing to dereference to an integer)", level = "ir"}
 
@@ -26,7 +28,6 @@
 "failing_tests/vmlinux/args_test.py" = {reason = "struct_trace_event_raw_sys_enter args field access not supported", level = "ir"}
 
 
-"failing_tests/vmlinux/xdp_pass.py" = {reason = "XDP program using vmlinux structs (struct_xdp_md) and complex map/struct interaction not yet supported", level = "ir"}
 
 "failing_tests/globals_read_before_shadow.py" = {reason = "Reading a name above the assignment that makes it a local shadowing a global is UnboundLocalError in Python, and a compile error here", level = "ir"}