From 32cd7731cd81bcbe7b079c6493d1056367531ad2 Mon Sep 17 00:00:00 2001 From: Douglas Mun Date: Mon, 7 Sep 2026 02:01:56 +0800 Subject: [PATCH 1/3] Give `and`/`or` Python value semantics and deref map lookups `prev = map.lookup(k)` returns a pointer into the map (NULL when the key is absent), and the boolean-operand path converted that pointer straight to i1 via convert_to_bool(). Two things went wrong in `map.update(k, (prev or 0) + 1)`: 1. the pointer was tested for NULL rather than dereferenced, so the stored count never took part in the expression; and 2. `or` returned i1 regardless of its operands, so the result was sign-extended and the addition operated on a 0/1 flag. The counter therefore stuck at 1 instead of accumulating -- silently, as the program compiles and loads cleanly. Fix both halves. `_prepare_bool_operand()` auto-dereferences pointer operands, mirroring what `get_operand_value()` already does on the binary -operator path; `deref_to_depth()` emits a null-checked load, so an absent key yields a zero-valued pointee, which is the fallback `prev or 0` asks for. The phi in `_handle_and_op`/`_handle_or_op` now carries the operand values instead of a bool, matching Python, where `a or b` evaluates to an operand and not to True/False. Operands that cannot yield an integer keep their previous truth-value behaviour. Truthiness contexts are unaffected: `if prev:` goes through functions_pass.py, which calls convert_to_bool() directly, so pointer NULL-ness still means "is the key present?" there. Regression tests: the existing IR and llc tiers only assert that compilation succeeds, so a wrong-value miscompile passes them both, which is how this survived. tests/test_boolop_semantics.py asserts on the emitted IR instead -- that or.result is not i1 and that the lookup is actually loaded. Both fail before this change and pass after. Refs #89 --- pythonbpf/expr/expr_pass.py | 93 ++++++++++++++----- .../conditionals/map_or_default.py | 33 +++++++ tests/test_boolop_semantics.py | 40 ++++++++ 3 files changed, 142 insertions(+), 24 deletions(-) create mode 100644 tests/passing_tests/conditionals/map_or_default.py create mode 100644 tests/test_boolop_semantics.py diff --git a/pythonbpf/expr/expr_pass.py b/pythonbpf/expr/expr_pass.py index 2270fdbb..631de1ee 100644 --- a/pythonbpf/expr/expr_pass.py +++ b/pythonbpf/expr/expr_pass.py @@ -401,14 +401,52 @@ def _handle_unary_op( # ============================================================================ +def _widen_to(builder, val, result_type, val_bool): + """Widen an operand to the phi's result type. + + Anything that is not an integer (a pointer that did not dereference to + one) cannot carry a meaningful value, so its truth value is used + instead -- preserving the old behaviour for those operands. + """ + if val.type == result_type: + return val + if isinstance(val.type, ir.IntType): + return builder.zext(val, result_type) + return builder.zext(val_bool, result_type) + + +def _prepare_bool_operand(func, builder, val): + """Dereference a pointer operand so its *value* takes part in the expression. + + `map.lookup()` returns a pointer into the map (NULL when the key is + absent). A bare truthiness test on that pointer asks "is the key + present?", which is what `if prev:` wants, but not what `prev or 0` + means: there the operand's stored value is the result of the expression. + The binary-operator path already auto-dereferences pointer results (see + `get_operand_value`); this mirrors it for boolean operands. + + `deref_to_depth` emits a null-checked load, so an absent key yields a + zero-valued pointee rather than faulting -- exactly the fallback that + `prev or 0` asks for. + """ + base_type, depth = get_base_type_and_depth(val.type) + if depth > 0: + deref = deref_to_depth(func, builder, val, depth) + if deref is not None: + return deref + return val + + def _handle_and_op(func, builder, expr, local_sym_tab, compilation_context): """Handle `and` boolean operations.""" logger.debug(f"Handling 'and' operator with {len(expr.values)} operands") merge_block = func.append_basic_block(name="and.merge") - false_block = func.append_basic_block(name="and.false") + # Python's `and` evaluates to the operand itself, not to a bool (see the + # note in _handle_or_op). + result_type = ir.IntType(64) incoming_values = [] for i, value in enumerate(expr.values): @@ -423,35 +461,37 @@ def _handle_and_op(func, builder, expr, local_sym_tab, compilation_context): return None operand_val, operand_type = operand_result + operand_val = _prepare_bool_operand(func, builder, operand_val) # Convert to boolean if needed operand_bool = convert_to_bool(builder, operand_val) + operand_val = _widen_to(builder, operand_val, result_type, operand_bool) current_block = builder.block if is_last: # Last operand: result is this value builder.branch(merge_block) - incoming_values.append((operand_bool, current_block)) + incoming_values.append((operand_val, current_block)) else: - # Not last: check if true, continue or short-circuit + # Not last: short-circuit with this operand's value if it is falsy next_check = func.append_basic_block(name=f"and.check_{i + 1}") - builder.cbranch(operand_bool, next_check, false_block) - builder.position_at_end(next_check) + short_circuit = func.append_basic_block(name=f"and.value_{i}") + builder.cbranch(operand_bool, next_check, short_circuit) - # False block: short-circuit with false - builder.position_at_end(false_block) - builder.branch(merge_block) - false_value = ir.Constant(ir.IntType(1), 0) - incoming_values.append((false_value, false_block)) + builder.position_at_end(short_circuit) + builder.branch(merge_block) + incoming_values.append((operand_val, short_circuit)) + + builder.position_at_end(next_check) # Merge block: phi node builder.position_at_end(merge_block) - phi = builder.phi(ir.IntType(1), name="and.result") + phi = builder.phi(result_type, name="and.result") for val, block in incoming_values: phi.add_incoming(val, block) logger.debug(f"Generated 'and' with {len(incoming_values)} incoming values") - return phi, ir.IntType(1) + return phi, result_type def _handle_or_op(func, builder, expr, local_sym_tab, compilation_context): @@ -460,8 +500,11 @@ def _handle_or_op(func, builder, expr, local_sym_tab, compilation_context): logger.debug(f"Handling 'or' operator with {len(expr.values)} operands") merge_block = func.append_basic_block(name="or.merge") - true_block = func.append_basic_block(name="or.true") + # Python's `or` evaluates to the operand itself, not to a bool, so the phi + # carries values. i64 covers every integer the frontend produces and keeps + # a map-lookup result (i64) exact. + result_type = ir.IntType(64) incoming_values = [] for i, value in enumerate(expr.values): @@ -476,35 +519,37 @@ def _handle_or_op(func, builder, expr, local_sym_tab, compilation_context): return None operand_val, operand_type = operand_result + operand_val = _prepare_bool_operand(func, builder, operand_val) # Convert to boolean if needed operand_bool = convert_to_bool(builder, operand_val) + operand_val = _widen_to(builder, operand_val, result_type, operand_bool) current_block = builder.block if is_last: # Last operand: result is this value builder.branch(merge_block) - incoming_values.append((operand_bool, current_block)) + incoming_values.append((operand_val, current_block)) else: - # Not last: check if false, continue or short-circuit + # Not last: short-circuit with this operand's value if it is truthy next_check = func.append_basic_block(name=f"or.check_{i + 1}") - builder.cbranch(operand_bool, true_block, next_check) - builder.position_at_end(next_check) + short_circuit = func.append_basic_block(name=f"or.value_{i}") + builder.cbranch(operand_bool, short_circuit, next_check) + + builder.position_at_end(short_circuit) + builder.branch(merge_block) + incoming_values.append((operand_val, short_circuit)) - # True block: short-circuit with true - builder.position_at_end(true_block) - builder.branch(merge_block) - true_value = ir.Constant(ir.IntType(1), 1) - incoming_values.append((true_value, true_block)) + builder.position_at_end(next_check) # Merge block: phi node builder.position_at_end(merge_block) - phi = builder.phi(ir.IntType(1), name="or.result") + phi = builder.phi(result_type, name="or.result") for val, block in incoming_values: phi.add_incoming(val, block) logger.debug(f"Generated 'or' with {len(incoming_values)} incoming values") - return phi, ir.IntType(1) + return phi, result_type def _handle_boolean_op( diff --git a/tests/passing_tests/conditionals/map_or_default.py b/tests/passing_tests/conditionals/map_or_default.py new file mode 100644 index 00000000..eaed878d --- /dev/null +++ b/tests/passing_tests/conditionals/map_or_default.py @@ -0,0 +1,33 @@ +# A map lookup used as a value in `or`, not just as a nullness test. +# +# `last.lookup(0)` returns a pointer into the map (NULL when the key is +# absent), so `prev or 0` must evaluate to the *stored value* when the key +# is present and to 0 when it is not. Compiling it as a truthiness test on +# the pointer makes the counter add a 0/1 flag instead of the stored count, +# so it sticks at 1 (or 2) forever. +from pythonbpf import bpf, map, section, bpfglobal, compile +from ctypes import c_void_p, c_int64, c_uint64 +from pythonbpf.maps import HashMap + + +@bpf +@map +def last() -> HashMap: + return HashMap(key=c_uint64, value=c_uint64, max_entries=3) + + +@bpf +@section("tracepoint/syscalls/sys_enter_execve") +def hello_world(ctx: c_void_p) -> c_int64: + prev = last.lookup(0) + last.update(0, (prev or 0) + 1) + return c_int64(0) + + +@bpf +@bpfglobal +def LICENSE() -> str: + return "GPL" + + +compile() diff --git a/tests/test_boolop_semantics.py b/tests/test_boolop_semantics.py new file mode 100644 index 00000000..711d9795 --- /dev/null +++ b/tests/test_boolop_semantics.py @@ -0,0 +1,40 @@ +"""Semantic tests for `and` / `or` result values. + +The generic IR-generation and llc tiers only check that compilation +succeeds, so a boolean operator that compiles cleanly but yields the wrong +*value* passes them both. `(prev or 0) + 1` did exactly that: the map +pointer was converted to i1 and sign-extended, so the expression added a +0/1 flag instead of the stored count. +""" + +import re +from pathlib import Path + +from tests.framework.compiler import run_ir_generation + +SOURCE = Path(__file__).parent / "passing_tests" / "conditionals" / "map_or_default.py" + + +def _compile(tmp_path): + ll_path = tmp_path / "output.ll" + run_ir_generation(SOURCE, ll_path) + return ll_path.read_text() + + +def test_or_result_is_not_a_truncated_bool(tmp_path): + """`prev or 0` must not collapse to i1 before the addition.""" + ir = _compile(tmp_path) + phi = re.search(r'%"or\.result" = phi\s+(\S+)', ir) + assert phi, "no or.result phi in emitted IR" + assert phi.group(1) != "i1", ( + "`or` produced an i1: the operand value is lost, so arithmetic on it " + "adds a 0/1 flag instead of the stored value" + ) + + +def test_or_dereferences_the_map_lookup(tmp_path): + """The stored value must be loaded, not just tested for NULL.""" + ir = _compile(tmp_path) + assert re.search(r"load i64, i64\*", ir) or re.search(r"load i64, ptr", ir), ( + "map lookup result was never dereferenced; only its NULL-ness was used" + ) From 3b8c49a738e6872a91b40c6b44102a6a5f6ba53d Mon Sep 17 00:00:00 2001 From: Douglas Mun Date: Mon, 7 Sep 2026 02:08:00 +0800 Subject: [PATCH 2/3] Drop an unused unpack flagged by ruff _prepare_bool_operand() only needs the pointer depth, not the base type. Keeps expr_pass.py at the same ruff error count as master. --- pythonbpf/expr/expr_pass.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pythonbpf/expr/expr_pass.py b/pythonbpf/expr/expr_pass.py index 631de1ee..5cd5cdf0 100644 --- a/pythonbpf/expr/expr_pass.py +++ b/pythonbpf/expr/expr_pass.py @@ -429,7 +429,7 @@ def _prepare_bool_operand(func, builder, val): zero-valued pointee rather than faulting -- exactly the fallback that `prev or 0` asks for. """ - base_type, depth = get_base_type_and_depth(val.type) + _, depth = get_base_type_and_depth(val.type) if depth > 0: deref = deref_to_depth(func, builder, val, depth) if deref is not None: From 04ae866ddd3bb73fe6904ca56fb24bee09358008 Mon Sep 17 00:00:00 2001 From: Douglas Mun Date: Mon, 7 Sep 2026 02:09:35 +0800 Subject: [PATCH 3/3] Sort imports in the new tests to satisfy ruff The sibling tests in conditionals/ use an unsorted import block, but the repo's own pre-commit config runs ruff, so follow the linter here. --- tests/passing_tests/conditionals/map_or_default.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/passing_tests/conditionals/map_or_default.py b/tests/passing_tests/conditionals/map_or_default.py index eaed878d..1e90b4af 100644 --- a/tests/passing_tests/conditionals/map_or_default.py +++ b/tests/passing_tests/conditionals/map_or_default.py @@ -5,8 +5,9 @@ # is present and to 0 when it is not. Compiling it as a truthiness test on # the pointer makes the counter add a 0/1 flag instead of the stored count, # so it sticks at 1 (or 2) forever. -from pythonbpf import bpf, map, section, bpfglobal, compile -from ctypes import c_void_p, c_int64, c_uint64 +from ctypes import c_int64, c_uint64, c_void_p + +from pythonbpf import bpf, bpfglobal, compile, map, section from pythonbpf.maps import HashMap