Add the dev workflow skill, implement support for all global vars using @bpfglobal (both const and non-const) - #99
Open
r41k0u wants to merge 18 commits into
Open
Add the dev workflow skill, implement support for all global vars using @bpfglobal (both const and non-const)#99r41k0u wants to merge 18 commits into
r41k0u wants to merge 18 commits into
Conversation
Captures the development loop this project has always used for new compiler features: write a minimal C eBPF program exercising only the feature, compile it to LLVM IR and read that as the specification, diff against what PythonBPF currently emits, stop for a human decision on the Python syntax, implement against the reference, and test at the right tier. Recorded as a project skill so agents follow it too; the syntax step is an explicit hard stop. tests/c-form/global_vars.bpf.c is the loop's step 1 for global variables: the four C global classes and nothing else. Its IR established that every global is an independent symbol accessed by plain load/store, that llc manufactures the BTF VAR/DATASEC entries from DIGlobalVariable metadata, and that clang folds const but not const volatile -- the facts the globals design rests on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
globals_processing emitted each global and forgot it, which is why nothing could ever read one back. Each integer-scalar global now lands in CompilationContext.bpf_globals as a BpfGlobalSymbol; non-scalar globals get a clear NotImplementedError instead of emitting something unusable. LICENSE stays special-cased and unaffected. Alignment becomes natural (width/8) instead of a blanket 8, matching what clang emits for the same declarations in tests/c-form/global_vars.bpf.c -- llc derives the BTF DATASEC layout from these symbols. Also adds current_func_globals to the context for the upcoming write support. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Name resolution tried local_sym_tab, then vmlinux enums, then failed. Globals now resolve between the two, in _handle_name_expr, get_operand_value, and the printk f-string formatter, each emitting the plain 'load i64, ptr @counter' form the C reference produces. A global therefore works in conditions, binops, print arguments and as a helper scalar argument with no further changes -- those paths all funnel through eval_expr/get_operand_value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Python's own scoping marks the write: 'global cg_id' declares that assignments
to cg_id in this function mean the BPF global, and the assignment then emits
the plain 'store i64 %v, ptr @cg_id' of the C reference, with the same implicit
widening/truncation rules as local assignments.
Without the declaration, an assignment to a global's name is refused:
SyntaxError: assignment to 'cg_id' shadows the BPF global of the same
name -- add 'global cg_id' to write to it
That is deliberate. In real Python such an assignment creates a shadowing
local; silently compiling it as either a local or a global store would be wrong
in one direction or the other, so it is a loud error instead. 'global' naming
something that is not a @bpfglobal is also a compile error.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
AugAssign previously raised 'not supported' outright. Desugaring to an ordinary assignment with a BinOp reproduces its Python semantics for Name and Attribute targets and reuses the whole existing assignment path -- locals, struct fields, and the new globals all work, so 'counter += 1' (the canonical global idiom in the kernel selftest corpus) compiles to load/add/store on @counter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Each integer global gets a DIGlobalVariableExpression with a C-named basic
type (matching signedness and width), attached via !dbg -- the same mechanism
map_debug_info.py already uses. llc's BPF backend manufactures the BTF VAR and
DATASEC '.bss'/'.data' entries from it:
[2] VAR 'expected_pid' type_id=1, linkage=global
[3] VAR 'cg_id' type_id=1, linkage=global
[4] DATASEC '.bss' size=0 vlen=2
Verified against clang's output for the same shapes: the in-object DATASEC
var offsets read 0 in both (libbpf patches them from the ELF symbol table at
load), and the ELF symbols carry the real layout, identical to the reference
(expected_pid at .bss+0, cg_id at .bss+8).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Five passing tests: read in condition and print; the get_cgroup_id_kern.c read-one-write-one shape; the counter += 1 idiom; two programs sharing global state (the test_autoattach.c shape); and mixed .bss/.data placement with mixed widths and signedness. Three strict expected failures documenting deliberate errors: assignment without the global statement (shadowing), a global statement naming a non-global, and a non-integer global type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Collaborator
Author
|
Fixes #48 |
handle_return consulted the hardcoded XDP action table before any symbol table, so with a local XDP_PASS = 55 in scope, "return XDP_PASS" silently returned 2. clang treats the equivalent C (a local shadowing an enum constant) as the local winning, and every other resolution site in the compiler already resolves local, then global, then vmlinux -- the return fast path was the one place with the order inverted. It now applies only when the name is bound in neither table. Also warn when a @bpfglobal shadows a vmlinux enum constant. C rejects that outright as a redefinition; PythonBPF follows the rebinding semantics the Python file itself has (the global wins, consistently), but the collision is worth a compile-time warning rather than silence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Two tests locking the shadowing semantics: a local named XDP_PASS returned from a program (the exact case the return fast path used to hijack), and a vmlinux-importing program where a local shadows one enum constant while a @bpfglobal shadows another, read in expressions and f-strings. The vmlinux test needs noqa on its import and both collision sites: ruff rightly flags unused imports and redefinitions in normal Python, and this file is deliberately made of them. These pin compilation; the value-level proof (ret of 55, not 2) was verified against the emitted IR and the clang reference for the same collision shapes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
The first version synthesised an Assign(BinOp(...)) node and fed it back through handle_assign. That is a standard compiler move, but it is not how this compiler works: every pass walks the tree the user wrote, and a node invented mid-codegen is invisible to the allocation pass that already ran, carries no source location for diagnostics, and shows up in logs as a statement nobody typed. handle_aug_assign now resolves the target slot (declared global, local, or struct field), loads it, evaluates the right-hand side, applies the operator, and stores back. Semantic agreement with "x = x op v" no longer rests on shared AST but on shared value-level helpers: the RHS goes through get_operand_value like any other read, and the operator table is extracted from binary-op evaluation into apply_binop so it exists exactly once. Two behavioural refinements fall out: augmented assignment to a global name without a global statement is now the same loud SyntaxError plain assignment gives (previously it slipped past the allocation pass, which only walks Assign), and struct-field targets compute their GEP once rather than twice. Target read precedes RHS evaluation, matching Python. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
handle_struct_field_assignment stored the evaluated value as-is, and since expressions evaluate in i64 that meant any assignment to a sub-64-bit field (dat.hits = 1 with hits: c_uint32) died in llvmlite with "cannot store i64 to i32*". Apply the same implicit widening/truncation the local-assignment path already has. Surfaced by the augmented-assignment struct-field test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
A struct-field test with a non-add operator and a sub-64-bit field (the narrow-on-store path), and a strict expected failure for counter += 1 without a global statement, which now fails as loudly as plain assignment does. Ruff flags that line as F823 (referenced before assignment) -- the same UnboundLocalError Python itself would raise, which is exactly the semantics the global statement requirement mirrors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Captures the trade-off behind reworking augmented assignment: desugaring into synthetic AST is a legitimate compiler technique with real benefits, and it is still the wrong shape for this compiler, whose passes communicate through the source tree. Records both sides so the next person tempted by it declines for the right reasons, and states the idiom to reach for instead -- shared value-level helpers, not shared AST. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1PX8EuP9C3o3veWGA84RF
Operator knowledge was spread across three files: the binary-op table (an op_map dict rebuilt inline on every _handle_binary_op_impl call, then extracted to ir_ops.apply_binop for augmented assignment), COMPARISON_OPS in type_normalization.py, and the unary/boolean operators as isinstance chains in expr_pass.py. Anyone asking "which operators does PythonBPF support" had to read all three. They now live in one module. BINOP_METHODS and COMPARISON_OPS are the tables themselves; UNARY_OPS and BOOL_OPS list the operators whose lowering is structural (convert_to_bool, short-circuit control flow) so the registry is complete even where the emission stays in expr_pass. apply_binop moves here from ir_ops, which goes back to being purely about IR operations. No behaviour change: 133 passed, 19 xfailed before and after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BSDVsZH5NtoASyxB8FCtGU
…parameter Reads resolve a name local-first (_handle_name_expr, get_operand_value, the printk formatter), but both write paths checked the declared-global set before local_sym_tab. Normally harmless because the two tables are disjoint by construction -- except for a parameter whose name matches a @bpfglobal. With 'global ctx' in a function taking ctx, the write path stored to the global while a read of the same name picked the parameter and crashed on its None slot: the same name resolving to different storage depending on which side of an assignment it sat. Python forbids the construct outright, and now so does the compiler, with Python's own wording: SyntaxError: name 'ctx' is parameter and global. Both write paths now resolve local-first like the reads, so all name lookups read the same way; and an augmented assignment to the context parameter itself gets a clear error instead of a store to None. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BSDVsZH5NtoASyxB8FCtGU
…n set A 'global x' statement is a fact about one function's scope, but it was tracked as current_func_globals on the compilation-wide context -- per-function state living at the wrong level, which had to be set before each body and reset after, and which every read and write site then had to consult in a specific order relative to local_sym_tab (the source of the parameter collision fixed in f4ec12d). The declaration now does what it means: it binds the name in local_sym_tab to the @bpfglobal's storage, as a LocalSymbol whose var is the GlobalVariable and whose declared_global flag records why. A GlobalVariable is a pointer to storage exactly as an alloca is, so every existing path that loads from or stores through a symbol's var works unchanged. That deletes the dedicated global-store branch in handle_variable_assignment, the global branch in handle_aug_assign, the allocation-pass skip, and the reset -- one table, one lookup, no order to get wrong. Reads of an undeclared global (legal in Python) still fall back to bpf_globals after local_sym_tab. Python's rules fall out naturally: a parameter is already in the table when the declarations are processed, so 'global ctx' on a parameter is detected by the same membership check. LocalSymbol.__iter__ deliberately still yields three fields, since several sites tuple-unpack a symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BSDVsZH5NtoASyxB8FCtGU
LocalSymbol, BpfGlobalSymbol and MapSymbol each named the storage behind a name differently (var, var, sym) and shared no type. They now all derive from symbols.Symbol, which carries what every symbol has -- the storage pointer (alloca, GlobalVariable, or None for the context parameter) and its IR type -- and each subclass adds only what its kind needs: LocalSymbol its metadata and declared_global flag, BpfGlobalSymbol its ctypes name, MapSymbol its map type and params. MapSymbol.sym becomes var to match. local_symbol.py is folded into the new symbols.py. LocalSymbol keeps its three-field __iter__ for the call sites that tuple-unpack a symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BSDVsZH5NtoASyxB8FCtGU
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.
No description provided.