Add the dev workflow skill, implement support for all global vars using @bpfglobal (both const and non-const) - #99
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
|
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
| elif isinstance(stmt.target, ast.Attribute) and isinstance( | ||
| stmt.target.value, ast.Name | ||
| ): | ||
| var_name, field_name = stmt.target.value.id, stmt.target.attr |
There was a problem hiding this comment.
Should add a NOTE or a TODO here that it only works for 1 level of struct dereferencing for now
| f"Failed to evaluate augmented-assignment value: {ast.dump(stmt.value)}" | ||
| ) | ||
| # Same width discipline as binary-op evaluation: compute in i64, narrow | ||
| # back to the slot's width on the way out. |
There was a problem hiding this comment.
I think this is handled better in another PR on top of this.
| or stmt.value.id not in compilation_context.bpf_globals | ||
| ) | ||
| ): | ||
| # The XDP fast path resolves names like XDP_PASS from the helper |
There was a problem hiding this comment.
Highly unlikely someone does this - if they do it they are likely just trying to test the limits of the compiler. But ok.
| raise SyntaxError("Augmented assignment not supported") | ||
| handle_aug_assign(func, compilation_context, builder, stmt, local_sym_tab) | ||
| elif isinstance(stmt, ast.Global): | ||
| # Declarations were collected by process_func_body; nothing to emit. |
There was a problem hiding this comment.
Is it worth being explicit for this case here then?
| logger.info(f"Copied string to char array {var_name}.{field_name}") | ||
| return | ||
|
|
||
| # Same implicit widening/truncation as assignment to a local: expressions |
There was a problem hiding this comment.
I hope this is being handles by the upcoming zext-sext fix PR
…tement Assigning a global's name without declaring it `global` was a compile error. Python makes it a local instead: the name is local for the whole function body, the global keeps its value, and only a `global` statement writes through to it. Do that, so the same program means the same thing in Python and in BPF. Python's scoping is function-wide and static, which the compiler gets for free: allocation runs as a pre-pass over the whole body, so a name bound anywhere is in local_sym_tab before the first statement is lowered, and reads resolve local-first. What does not come for free is the read above the binding, which Python answers with UnboundLocalError rather than the global's value. There is no runtime here in which to raise that, so the local records where its binding ends and a read above it is rejected, with the message Python would have given. Silence there would load an uninitialised stack slot through a name the author expected to be the global. `counter += 1` without `global counter` stays an error for the same reason: it binds a local and reads it in the same statement. Only its message changes, from a shadowing refusal to the UnboundLocalError it actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`global x` covers the whole function body, so a body that reads x above the declaration has two incompatible readings of that line. Python rejects it with "name 'x' is used prior to global declaration"; say the same thing rather than silently picking the global. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`x = a_global` logged three errors and emitted nothing: the Name source path in the allocation pass only looked in the local symbol table, so a read that needs no declaration failed anyway. Resolve local first, then the BPF globals, and give the copy the source's type as the local-source case already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three review notes from the globals PR, all about being explicit: - Augmented assignment on a struct field handles one level of dereference. Say so where the target is unpacked, and give `obj.inner.field += v` its own message instead of an ast.dump. - The ast.Global branch of process_stmt is a no-op for a reason (declarations are collected before anything is lowered, because the binding is function-wide). Spell that out, and log the names at debug level rather than leaving a bare pass. - process_stmt had no else, so any statement the compiler does not handle was dropped without a word, which makes a program mean something other than what it says. Warn, with the line number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
globals_shadowing.py asserted the old rule, so it moves to passing_tests/globals/shadowing.py and asserts the new one: the write without `global` binds a local, and a second program still reads the global's initializer. copy_to_local.py covers `x = a_global`, the read shape that used to fail in the allocation pass. globals_read_before_shadow.py takes over as the failing case, since a read above the binding is what Python rejects, and the augassign test keeps its xfail with the reason reworded to the UnboundLocalError it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
I think this should still be a part of the testsuite to see if shadowing works
| raise SyntaxError( | ||
| f"augmented assignment to '{name}' shadows the BPF global of " | ||
| f"the same name — add 'global {name}' to write to it" | ||
| f"local variable '{name}' referenced before assignment: " |
| elif isinstance(stmt, ast.Global): | ||
| # Declarations were collected by process_func_body; nothing to emit. | ||
| pass | ||
| # Nothing to emit: `global x` binds a name for the whole function body, |
There was a problem hiding this comment.
unnecessary comment
Levels 1 and 2 only prove the globals cases compile; what they are about is whether a name resolves to the @bpfglobal symbol or to a stack slot that shadows it. test_globals_ir.py reads the IR of each BPF function and counts loads and stores per operand, exactly: `global counter` is load/add/store on @counter with no slot, a read needs no declaration and hits the symbol, and a write without `global` binds a slot for the whole body of that function while a second function still reads the symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
49d43f7 to
dc212c3
Compare
Review on the previous round: the UnboundLocalError-shaped messages said in five lines what one says, and the ast.Global branch of process_stmt explained itself at length for a no-op. Both messages now name the problem and the fix and stop; the branch keeps its debug log only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Ok to merge now |
No description provided.