Repository navigation
Fix stale XFAILs that can now PASS - #111
Merged
Merged
Conversation
A local bound to a map lookup holds a pointer, and get_or_create_ptr_from_arg handed a helper the address of that local's stack slot instead. So count.update(k, prev) stored the kernel address of the map value, not the value, and after prev = prev + 1 it stored the address of the temporary. The verifier accepts both when loading as root, so nothing failed: the failing_tests/vmlinux/named_arg.py example has been passing all along while storing the wrong thing. The slot is now loaded when it holds a pointer. named_arg.py moves to passing_tests, and both it and a new map_update_from_lookup.py are pinned by IR assertions. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`if dat:` on a local struct built Constant(struct_type, 0) in convert_to_bool, and llvmlite failed later, while printing the module, with "'int' object is not iterable". A struct has no truth value (C rejects `if (s)`; Python would call any object true), so it is now a TypeError that says to test a field. A pointer to a struct is unaffected. The xfail was named struct_ptr but tests a struct value; it is renamed to struct_value.py and becomes a negative test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
handle_expr lowered a statement only when it was a call, and dropped any other expression with an INFO log. So `print(a) if c else print(b)` compiled to nothing at all, and failing_tests/conditionals/oneline.py passed. An expression statement that contains a call is now a compile error; one without (a docstring, a bare name) is still dropped. oneline.py becomes an IR-level xfail. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
retype, helper_cond, direct_assign and globals had no xfail entry and have been passing at every level; their NOTE headers describe limits that have since been lifted, so they are rewritten. xdp_pass was an xfail only because it used the old count().lookup() spelling of a map call; in the current syntax it compiles. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new pointer-lowering IR expectations need the repository-required minimal C reference before approval.
1 open finding
What changed in this PR
Updates stale expected-failure tests while correcting pointer argument lowering and unsupported-expression diagnostics.
Changes:
- Promotes now-supported map/XDP cases to passing tests.
- Correctly loads pointer-valued helper arguments.
- Adds explicit errors for invalid struct conditions and unsupported call expressions.
| File | Description |
|---|---|
pythonbpf/helper/helper_utils.py |
Loads pointer-valued helper arguments from local slots. |
pythonbpf/expr/type_normalization.py |
Rejects unsupported truth-value conversions. |
pythonbpf/expr/expr_pass.py |
Rejects effectful unsupported expression statements. |
tests/test_signedness_ir.py |
Adds pointer-depth IR assertions. |
tests/test_config.toml |
Updates strict XFAIL entries. |
tests/passing_tests/helpers/map_update_from_lookup.py |
Covers lookup pointers passed to updates. |
tests/passing_tests/vmlinux/direct_assign.py |
Promotes direct update expressions. |
tests/passing_tests/vmlinux/xdp_pass.py |
Promotes the corrected XDP example. |
tests/passing_tests/vmlinux/named_arg.py |
Updates pointer-rebinding documentation. |
tests/passing_tests/globals/several_globals.py |
Adds a multi-global compilation case. |
tests/passing_tests/conditionals/helper_cond.py |
Documents supported helper comparisons. |
tests/passing_tests/assign/retype.py |
Documents supported pointer-to-integer rebinding. |
tests/failing_tests/conditionals/struct_value.py |
Clarifies the negative struct-condition case. |
tests/failing_tests/vmlinux/xdp_pass.py |
Removes the obsolete failing fixture. |
tests/failing_tests/vmlinux/direct_assign.py |
Removes the obsolete failing fixture. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+115
to
+119
| # A local holding a pointer is passed to a helper as that pointer, loaded | ||
| # from its slot; the slot's own address (an i64**) would make the map store | ||
| # a kernel address instead of the value. | ||
| "helpers/map_update_from_lookup.py": ( | ||
| [r'load i64\*, i64\*\* %"prev"'], |
tests/c-form/map_update_from_lookup.bpf.c mirrors the two programs the check covers. At -O0 clang passes bpf_map_update_elem the result of `load ptr, ptr <prev's slot>`, in both copy() and rebind(). The old expected pattern only asked for some load of prev, and `if prev` already does one, so it matched the buggy IR too. The check now requires the value loaded from prev's slot to be the helper's value argument. It fails on the code before dde720f and passes on this branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
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.
No description provided.