Give and/or Python value semantics and deref map lookups (#89) - #100
Open
douglasmun wants to merge 3 commits into
Open
Give and/or Python value semantics and deref map lookups (#89)#100douglasmun wants to merge 3 commits into
and/or Python value semantics and deref map lookups (#89)#100douglasmun wants to merge 3 commits into
Conversation
added 3 commits
September 7, 2026 02:01
`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 pythonbpf#89
_prepare_bool_operand() only needs the pointer depth, not the base type. Keeps expr_pass.py at the same ruff error count as master.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #89.
The bug
compiles and loads cleanly, and the counter sticks at 1 instead of accumulating. The emitted IR on
master:Two separate things go wrong:
map.lookup()returns a pointer into the map (NULL when absent), andconvert_to_bool()emitsicmp != nullon it, so the stored count never enters the expression.orreturns i1 regardless of its operands. Even with the operand dereferenced,phi i1+sextmeans the addition operates on a 0/1 flag. Python'sorevaluates to an operand, not toTrue/False.Fixing only (1) is not enough — I tried that first, and the
sext i1was still there.The fix
_prepare_bool_operand()auto-dereferences pointer operands, mirroring whatget_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 rather than faulting — exactly the fallbackprev or 0asks for._handle_and_op/_handle_or_opnow carries operand values rather than a bool._widen_to).After:
Truthiness contexts are unaffected
if prev:goes throughfunctions_pass.py, which callsconvert_to_bool()directly, so pointer NULL-ness still means "is the key present?" there. The existingconditionals/or.pyandconditionals/map.pytests continue to pass.Note for reviewers
and/ornow returni64instead ofi1. Branch contexts re-convert, soif x or y:is unchanged, but this is a visible type change if anything downstream assumes an i1 result — worth a look from someone who knows the codebase better than I do.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.pyasserts on the emitted IR instead: thator.resultis not i1, and that the lookup is actually loaded. Both tests fail onmasterand pass with this change.tests/passing_tests/conditionals/map_or_default.pyadds the lookup-in-arithmetic pattern; the existingor.pyonly coversif x or y:nullness.Full suite before and after this change is identical (49 failed / 50 passed / 24 skipped / 5 xfailed on the IR + llc tiers in my container). The verifier tier needs a real kernel and is unaffected by this change. Note that llc in my test container rejects llvmlite's
captures(none)attribute on every test file includingmaster's, so the llc tier was not a useful signal here.How I verified
aarch64, Docker, Python 3.12,
pythonbpfat 926ce3f,pylibbpfstubbed (unused bycompile_to_ir).