Propagate exceptions raised inside a Work.derive - #83
Conversation
A derive is invoked through a first-class FunctionType call. numba chooses how to lower that call from the jit_addr slot of the function data model: a populated slot selects the numba calling convention and unwinds normally, an empty one selects a C wrapper that numba documents as not supporting exceptions. numba fills the slot only for a dispatcher, so the compile result that cres produces always took the discarding path. calculate returned normally, data was left zero-filled, and derived was set anyway, so the wrong value was cached permanently. For unicode_type data the zeroed string struct has a NULL data pointer, so reading it back from Python segfaulted the interpreter. derive_wap adds DeriveWAP, which captures the calling convention entry point of its compile result, and DeriveFunctionType, which fills jit_addr from it on both unboxing and constant lowering. cres now mints those, so an exception raised in a derive reaches the caller with its original type and message, data is untouched and derived stays unset, leaving the node calculable once the cause is addressed. Every registration uses numba's public extension API; no numba internals are patched. _call_derive never consulted jit_addr, reading struct slot 0 unconditionally, so the discard on the Work path came from numbox rather than from numba's lowering. It now selects the propagating convention outright for a DeriveFunctionType field and tests jit_addr at runtime for a plain FunctionType one, which also covers an njit dispatcher passed as a FunctionType-typed argument. A null slot still emits the previous call unchanged. make_work gains a Python front that upgrades a foreign CompileResultWAP through its compile result, since the check is on the object's class and a jitted body cannot see it. Jitted callers reach an overload that takes the value as given. function_struct_size moves to configurations, because importing lowlevel compiles a cached proxy and both cres and derive_wap need the constant on paths that must stay free of compilation side effects. Gating on it keeps numba 0.60, which has no jit_addr slot, on the previous behaviour. Any.get_as accepts a stored DeriveFunctionType where the matching FunctionType is requested. Both use numba's FunctionModel, so the erased payload is identical. The type codes stay distinct rather than aliased, because they also feed content-addressed cache names where collapsing two types onto one hash would let a stale binary load.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reasoning DetailsWe are asked to review the diff, focusing on defects. The diff introduces changes to several files. Let's analyze each change for potential bugs, security vulnerabilities, performance issues, data loss, concurrency/resource bugs, etc. We'll need to look at the added/modified lines carefully.Scope: Let's look at each file. First,
Potential defect: The But is the logic correct? The purpose: if the instance type is But what if the instance type is a Now, potential defect: The Now, is there a risk that Potential bug: The condition But note: However, consider the case where One potential issue: What if Now, But is there a possibility that Now,
Potential defect: But a possible defect: The assert But Now, Imports: many from numba. The file defines First, The class
Let's verify: In the test file class CompileResultWAP(Hashable):
def __init__(self, cres):
self._cres = cres
self._dispatcher = cres.entry_point # This is the cfunc wrapper
@property
def signature(self):
return self._cres.signature
@property
def cres(self):
return self._cres
def __wrapper_address__(self):
return self._dispatcher.addressSo class CompileResultWAP:
def __init__(self, cres):
self._cres = cres
self._dispatcher = cres.entry_point
@property
def signature(self):
return self._cres.signatureSo it's a property. So But wait, maybe the Now, other parts of
The Potential issue: But note: Now Now class CompileResultWAP:
"""A wrapper around a CompileResult that provides the
signature and the address of the wrapper function for a
FunctionType.
"""
def __init__(self, cres):
self._cres = cres
self._dispatcher = cres.entry_point
@property
def signature(self):
return self._cres.signature
@property
def cres(self):
return self._cres
def __wrapper_address__(self):
return self._dispatcher.addressThat's what I remember. So it's a property. However, maybe in a newer version they might have changed it? The diff's But wait, the Now, also in Now Now, The diff modifies First, import of In The
Then the logic: if not jit_addr_supported():
return emit_c_call()
jit_addr = builder.extract_value(derive_struct, JIT_ADDR_SLOT)
if isinstance(derive_ty, DeriveFunctionType):
return emit_propagating_call(jit_addr)
res_slot = cgutils.alloca_once(builder, context.get_value_type(fsig.return_type))
with builder.if_else(cgutils.is_null(builder, jit_addr), likely=False) as (null, populated):
with null:
builder.store(emit_c_call(), res_slot)
with populated:
builder.store(emit_propagating_call(jit_addr), res_slot)
return builder.load(res_slot)Potential issues:
Now, there's a potential performance issue: For Now, what about the case where Now, the Potential bug: In But note: Now, the But there's a potential bug: they allocate Now, the Now, the import of Now, But note: There is an Now, Now,
Potential bug: The But note: In Now,
Now, potential issues with the Now, the test files: we can ignore defects in tests unless they are obviously wrong. But there might be a bug in test assertions? The test Now, let's look at other potential bugs. In Now, consider thread safety: Now, resource leak: Now, Now, potential bug: In Now, the Now, there's a subtle issue: In Now, consider the possibility that Now, any security vulnerabilities? No user input, just internal types. No auth, deserialization, etc. Now, data loss: The previous behavior could silently corrupt data (zero-filled). The fix addresses that. So no data loss. Now, performance: The branching in Now, one more potential bug: In Now, what about the case where the stored type is a Now, a potential issue: In Now, check Now, Now, the code: Now, the inner function Now, I think the main defect is the So it's a property. So it's a bug. However, the test code might still compile and maybe not call that code path because the Now, are there any other defects? The One more potential issue: In Now, look at Now, consider the interplay: In Now, in the overload for Now, is there any issue with the Now, check Now, any memory leak in Now, consider the Now, a potential bug: In Now, the Now, the Now, the Now, Now, a thought: The Now, look at the Now, one more possible defect: In Now, check Now, check Now, the test code in Now, I'll summarize defects:
Now, I'll also check the line Now, check if there's any issue with Now, overall, the only defect I'm confident about is the Let's phrase the finding. Severity: Critical (causes a crash when trying to infer type of a DeriveWAP or lower a constant DeriveWAP). File: In the diff, the So I'll cite both lines. Problem: Fix: Replace That's the defect. Now, I'll consider if there are other issues. Could the Now, what about the Now, consider that Now, I'll produce the final output with the single finding. I need to ensure the format: Now, I'll double-check the line numbers in the diff: Now, I'll output that. Critical —
|
MiniMax Code ReviewThis is a substantial pull request that introduces a new mechanism for handling exceptions in `derive` functions within the numbox library. Let me analyze the changes carefully.Summary of ChangesThe main goal of this PR is to fix a long-standing issue where exceptions raised inside Key Components:
AnalysisStrengths:
Concerns:
Recommendations:The solution is well-implemented overall. Key areas worth examining more closely: the memory management around The docstring modifications need careful review to ensure accuracy. Additionally, I should verify that the constant-lowering path for derived values works as intended, and check how Code Review: Exception Propagation for Derive FunctionsThis is a substantial and well-architected PR that solves a critical issue: exceptions raised inside Overall AssessmentStrong implementation with good design decisions. The approach of creating Detailed Review1. Architecture -
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing correctness/safety issue where exceptions raised inside Work.derive could be silently discarded at the first-class FunctionType boundary, leaving data zero-filled and derived incorrectly set (including a possible Python segfault for unicode_type). It does so by introducing a numbox-owned DeriveWAP/DeriveFunctionType pair that can reliably populate and consume the jit_addr entry point when available (numba ≥ 0.61), and by updating the derive call path to propagate exceptions.
Changes:
- Add
DeriveWAPandDeriveFunctionTypeto carry/populatejit_addrand enable exception propagation through first-class calls (numba ≥ 0.61), with explicit gating for 0.60. - Update
Workconstruction and_call_derivelowering to use the propagating calling convention when possible, and to preserve the “old behavior” only where unavoidable. - Add focused tests + docs updates covering propagation, node integrity on failure, and upgrade behavior for foreign
CompileResultWAP.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/test_lowlevel.py | Updates FunctionModel tuple expectations to assert cres-minted derives carry a populated jit_addr. |
| test/utils/test_highlevel.py | Adjusts cacheability/type-identity expectations to accept DeriveFunctionType (or FunctionType on numba 0.60). |
| test/core/test_derive_wap.py | Adds a comprehensive regression suite for exception propagation and node integrity. |
| numbox/utils/lowlevel.py | Moves numba_version / function_struct_size sourcing to core.configurations to avoid import side effects. |
| numbox/utils/highlevel.py | Updates cres to mint DeriveWAP when jit_addr is supported. |
| numbox/core/work/work.py | Introduces Python-level make_work wrapper for rewrapping derives + updates _call_derive to propagate exceptions. |
| numbox/core/work/derive_wap.py | New extension types + boxing/unboxing/constant lowering to populate jit_addr. |
| numbox/core/configurations.py | Centralizes numba_version and function_struct_size. |
| numbox/core/any/any_type.py | Allows decoding stored DeriveFunctionType values when callers request the matching FunctionType. |
| docs/numbox.core.work.rst | Updates Work exception-handling documentation and adds module docs for derive_wap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| fn = context.declare_function(builder.module, pyval.cres.fndesc) | ||
| sfunc.jit_addr = builder.bitcast( | ||
| fn, context.get_value_type(types.voidptr)) | ||
| context.active_code_library.add_linking_library(pyval.cres.library) | ||
| except Exception: | ||
| sfunc.jit_addr = context.add_dynamic_addr( | ||
| builder, pyval.jit_address, info="jit_addr:" + str(typ)) | ||
| return sfunc._getvalue() |
There was a problem hiding this comment.
From the fake Slim Shady:
Agreed, and the fallback is gone in cff2ba3. A DeriveFunctionType value takes the propagating arm with no null test, so a jit_addr that failed to resolve would be called unconditionally. Failing the compilation is the only honest outcome.
One correction on the failure mode. The cached-invalid-address case could not arise, but not because the type is uncacheable. Forcing the old fallback, by making declare_function raise inside this function, gives has_dynamic_globals: True, Cannot cache compiled function "uses_const" as it uses dynamic globals, and zero cache files written. numba refuses to cache a baked address rather than storing one that goes stale.
The symbolic path is the opposite, which is worth stating because it is new behaviour rather than a restored guarantee. With jit_addr a constant symbol the dead c_addr/py_addr globals are eliminated before numba scans the final module, so an @njit(cache=True) that only calls the derive reports has_dynamic_globals: False, writes .nbi/.nbc, and cache-hits in a second process while still propagating. The same module on main is refused the cache and swallows. The cost is that the caller's cached binary binds the derive's code: with the derive in another module, changing it from x + 1.0 to x + 100.0 still returned 11.0 on the cache hit. numba does this for any cross-module cached callee, but this pattern was never cached at all before.
The docstring sentence I deleted was right about the outcome and loose about the mechanism, and dropping it lost the property a reader needs. Restored with both consequences in 69e6442.
| pyapi = context.get_python_api(builder) | ||
| modname = context.insert_const_string(builder.module, __name__) | ||
| mod = pyapi.import_module(modname) | ||
| fn = pyapi.object_getattr_string(mod, "_get_derive_jit_address") | ||
| pyapi.decref(mod) | ||
| sig_obj = pyapi.unserialize(pyapi.serialize_object(sig)) | ||
| addr = pyapi.call_function_objargs(fn, (func, sig_obj)) | ||
| with builder.if_then(cgutils.is_null(builder, addr), likely=False): | ||
| builder.ret(pyapi.get_null_object()) | ||
| return addr |
There was a problem hiding this comment.
From the fake Slim Shady:
Correct on both objects, fixed in cff2ba3. Measured over 1000 unboxing calls on the pre-fix tree: _get_derive_jit_address module attribute +1000, memoized Signature +2000 (one per unserialize site), numba's _get_wrapper_address attribute +1000. Four references per unbox, two of them this function's. After the fix its attribute delta is 0 and the total drops to two.
The remaining two are numba's, from the lower_get_wrapper_address call numbox makes for c_addr: _lower_get_address decrefs numba_mod only and never releases numba_func or sig_obj.
Both decrefs sit after call_function_objargs and before the null branch that returns early, and addr does not borrow from either.
An exception escaping a numba parallel region surfaces differently per platform: Linux wraps it in SystemError with the original as __cause__, macOS raises nothing at all. Neither is something a derive can influence, and the case is already documented as a limitation rather than a guarantee. Assert the invariant that does hold everywhere instead: whatever the caller saw, the node keeps its data and its unset derived, so the failure is not cached and a later calculate outside the parallel region raises normally.
Reasoning DetailsWe are to review the diff and report only defects on lines that were added or modified. The diff includes several files. We need to identify any bugs, security vulns, performance problems, data loss, etc., strictly from the added/modified lines. The review must cite file:line for each finding.Let's examine each changed file.
Overall, I don't see any clear defect in the production code. The only potential defect I saw was the version parsing in configurations.py, but I'm not confident it's a real bug (IndexError). The But wait, there is a potential defect in Another potential defect: In The But there might be a subtle issue: In the But there is a potential bug: The Another thing: The old Now, any performance issues? For example, the One possible defect: In Now, is there any concurrency or resource leak? The unboxing does Now, regarding Now, the One potential data loss issue: The comment in the docstring of Now, any security vulnerability? None apparent. The use of Thus, I see no defects. I'll respond with "No issues found." No issues found. |
…s fallback The unboxing helper took new references to the module attribute and to the serialized signature and released neither, leaking one reference per call. Unboxing runs on every call for a derive passed as an argument, so this accumulated: measured at 2000 leaked references over 2000 calls, and zero after releasing both before the null branch, which returns early. Constant lowering fell back to baking the process-local entry point when the symbolic declaration failed. A value of this type always takes the propagating call, so a jit_addr that failed to resolve would then be called unconditionally. Failing the compilation is the only honest outcome, so the fallback is gone. Its stated rationale was wrong as well: c_addr and py_addr already go through add_dynamic_addr, which disables caching for the module either way, so the symbolic path is about resolving the entry point as a symbol and linking the compile result's library, not about cacheability.
Reasoning DetailsWe need to review the diff for defects. The rules: only defects (bug, security, performance, data loss, concurrency) on changed lines. Report severity, file:line, problem, fix. No style, no suggestions. Only >80% sure.We'll go through each diff chunk. First, any_type.py:
Potential defect: The check But what if But there's a subtlety: The Potential bug: Another thing: The comment says "Both use numba's Potential defect: I don't see a clear defect here. Now configurations.py: added import, assert, constant function_struct_size. The assert derive_wap.py: many lines, new file.
Should we report it? The rule: only defects >80% sure are real. I'm >80% sure that
Potential issue: In But there's a subtle bug: The Now, for the jit_addr, they set
Now, work.py:
Potential defect: In Potential bug: In Now, lowlevel.py: they removed import of Now, highlevel.py: added import of DeriveWAP and jit_addr_supported (deferred). They now return DeriveWAP if supported, else CompileResultWAP. That's fine. Test files: no defects introduced. Now, potential defect I identified: Another potential defect: In However, what if someone stores a plain FunctionType value (from an njit dispatcher) in an Any, and later tries to get_as as FunctionType? The stored self.t would be str(FunctionType(sig)). When calling get_as with FunctionType, instance_ty is FunctionType, type(...) is True, so stored_codes includes ty_code (FunctionType) and str(DeriveFunctionType(sig)). So self.t (FunctionType) will be in stored_codes (the first element), so match. So okay. Thus, no defect in any_type.py. Now, regarding configurations.py: they set Potential defect: In derive_wap.py, Another thing: In work.py, Now, there's a possible issue: In One more: In Potential defect: In Now, one more thing: In the new file derive_wap.py, the Also, in But note: In Now, I found a potential defect in Now, another potential defect: In So far, I don't see a clearcut defect. However, there is one potential performance issue: In Now, consider the change in One more subtle defect: In Now, the potential defect about can_convert_to I think is not a real bug because of type equality. I'll skip. Another possible defect: In derive_wap.py, the Now, consider the Now, potential concurrency/race: The Now, resource leak: None. Now, data loss: The whole purpose is to prevent data loss by not setting derived and not zeroing data on exception. No introduced data loss. Given the above, I don't see any clear defect meeting the criteria. Possibly a defect in the type conversion for DeriveFunctionType, but it's borderline. Since the instructions say only >80% sure, I can skip. But re-reading the diff, there's a potential issue in However, note: But there's a subtlety: The condition Now, the problem: If Now, I'm going to consider the possibility that the Now, there's another issue: In Let's examine the import line: But wait: The diff includes the import statement: Another potential: In Now, think about the Now, lowlevel.py they removed the local version computation and import. They added a noqa for "noqa: F401 re-exported for existing importers". That's fine. Now, work.py: They import Now, the However, I note one more potential defect: In But there's a subtle bug: In the Now, the Now, the big change: In the new All seems okay. Given the high bar, I might not find any defect. But I need to be sure. Let's examine the But there's a possible issue in the test Another possible defect: In Now, I'm going to consider if there's any defect with the "stored_codes" change in any_type.py: They check Now, what about the case where the stored any was created with a Now, the crucial part: The Thus, no defect. Now, I need to report exactly "No issues found." if none. But wait, I recalled a potential resource leak or refcounting issue: In But after returning addr, the caller sets But there's a subtle refcounting: But the Now, another potential defect: In Now, consider the test But I'm going to look for a real bug: In Now, a potential bug: In Now, I think there is no defect. But I need to check one more thing: In Thus, I'll output "No issues found." But wait, the rule says: "Report only defects from the list above." I found none. I'll output exactly: "No issues found." However, I recall the potential defect about the Thus, answer is "No issues found." No issues found. |
make_work upgraded a foreign CompileResultWAP by minting a DeriveWAP per call and passing it straight into jitted scope. Unboxing records the derive's address in py_addr without taking a reference, so that wrapper was freed as soon as the call returned and every Work built from it held a dangling pointer. Reading the derive back out then increfs released memory: measured as a segmentation fault, where the same program on the base commit round-trips the object it was given. The upgrade is now memoized onto the object it upgrades, which ties its lifetime to the one the caller already holds, and is the lifetime the stored address assumed. Repeated upgrades of the same derive return the same wrapper. One user-visible consequence, documented: the node's derive attribute reads back as the upgraded wrapper rather than as the object passed in. It wraps the same compile result and is called identically; only the identity differs. The guard is a weakref on the upgrade, which fails cleanly rather than taking the interpreter down with it; mutation-verified by restoring the per-call mint.
Reasoning DetailsWe need to review the diff for defects according to the strict criteria: bugs, security vulnerabilities, performance issues, data loss/corruption, breaking API/contract changes, concurrency/resource-leak bugs. Only report findings from lines that were added or modified, >80% confidence, with severity and location.Let's examine each file change carefully. File: numbox/core/any/any_type.py Added/modified lines: The diff shows new imports and modified function The changed code: @overload_method(AnyTypeClass, "get_as", strict=False, jit_options=jit_options)
def ol_get_as(self_ty, ty_ref: TypeRef):
from numba.core.types.function_type import FunctionType
from numbox.core.work.derive_wap import DeriveFunctionType
instance_ty = ty_ref.instance_type
ty_code = str(instance_ty)
# A derive compiled by `cres` stores as `DeriveFunctionType`, which callers
# ask back as the plain `FunctionType` of the same signature. ...
stored_codes = (ty_code,)
if type(instance_ty) is FunctionType:
stored_codes = (ty_code, str(DeriveFunctionType(instance_ty.signature)))
def _(self, ty):
if self.t not in stored_codes:
raise NumbaError(f"Any stored type {self.t}, cannot decode as {ty_code}")
return _deref_payload(self.p, ty)
return _Potential defect: On line with But is there a problem with Potential bug: If the user asks with a The imports inside the function: File: numbox/core/configurations.py Added lines: from importlib.metadata import version
numba_version = int(version("numba").split(".")[1])
assert numba_version >= 60, numba_version
function_struct_size = 3 if numba_version >= 61 else 2Potential defect: Now, in No defect here. File: numbox/core/work/derive_wap.py (new file) This is large. We need to check for defects.
Let's check if the Could there be other defects? The File: numbox/core/work/work.py Changes to if not jit_addr_supported():
return emit_c_call()
jit_addr = builder.extract_value(derive_struct, JIT_ADDR_SLOT)
if isinstance(derive_ty, DeriveFunctionType):
return emit_propagating_call(jit_addr)
res_slot = cgutils.alloca_once(builder, context.get_value_type(fsig.return_type))
with builder.if_else(cgutils.is_null(builder, jit_addr), likely=False) as (null, populated):
with null:
builder.store(emit_c_call(), res_slot)
with populated:
builder.store(emit_propagating_call(jit_addr), res_slot)
return builder.load(res_slot)Potential issues:
Now, def make_work(name, data, sources=(), derive=None):
return _make_work_jit(name, data, sources, rewrap_derive(derive))
@overload(make_work, strict=False, jit_options=jit_options)
def ol_make_work(name, data, sources=(), derive=None):
def _(name, data, sources=(), derive=None):
return ll_make_work(name, data, sources, derive)
return _The overloaded version is for jit-callers, and it does not rewrap, as expected. The Python version uses Potential issue: File: numbox/utils/highlevel.py Changes: from numbox.core.work.derive_wap import DeriveWAP, jit_addr_supported
# in cres:
if jit_addr_supported():
return DeriveWAP(func_cres)
return CompileResultWAP(func_cres)This imports inside function. ok. File: numbox/utils/lowlevel.py Moved Test files Not part of the review for defects in the code itself, but we are not to report test coverage or style. Only report defects in the code changes. However, if a test demonstrates a defect, it's still the code under test that's defective. But the test may be flawed? The prompt says "Review the diff against the rules ... report only defects on lines that were added or modified." The test file is part of the diff, but it's a new file. The rules are about defects in the code being reviewed. So if the test contains a bug (like a false positive test that doesn't actually test correctly), we could report it as a defect in the test code. But the prompt likely expects us to review the changes to the application/library code, not the tests, unless there's a defect in the test code that could cause problems (e.g., test that passes but masks a bug). But the instruction: "Your sole job is to surface defects in the diff." and the diff includes test files. So we should look for defects there too. However, the defects must be from the list (bug, security, perf, data loss, etc.). A test that incorrectly passes could be a defect (masking a bug), but that's a test logic issue. Could there be a defect in So I'll focus on the dangling pointer issue in Let's verify the likelihood: In numba's first-class function model, the struct stores py_addr, which is a raw pointer. The Python object must be kept alive. Typically, Numba's Dispatcher holds a reference to the underlying Python function when it's a dispatcher. For a CompileResultWAP, it's expected that the user keeps it alive. If the Work stores a reference to the derive object, it would keep it alive. But does the Work store a reference? The Thus, I'll report this as a defect at But wait: The memoization on the original derive is designed so that the wrapper is kept alive as long as the original derive is alive. In typical usage, the original derive (foreign) might be kept alive by the user, but not guaranteed. The Now, what about the import of Another defect: In Could there be a performance issue? In Now, I'll structure findings. One finding: dangling pointer in rewrap_derive, high severity. Location: Check if there's any other defect. I'll review Potential issue: In I think that's it. So I'll output a single finding. But let's confirm: The dangling pointer is a defect, >80% sure. Yes. It's a crash bug. Medium -> High severity? I'd say Critical because it can crash the interpreter. So Critical. Now, format: Critical — Dangling pointer in rewrap_derive can cause use-after-free crashFile: Now produce output. But wait: Could there be a defect in the test file? No. So only one finding. I'll output that. Critical — Dangling pointer in
|
The rewrite that dropped the baked-address fallback also dropped the only sentence describing what the symbolic path means for caching, which is the property a reader of this function actually needs. Resolving the entry point as a symbol leaves the c_addr and py_addr globals dead, so they are eliminated before numba scans the final module and a caller that only calls the derive is cacheable: measured has_dynamic_globals False, .nbi and .nbc written, and a cache hit in a second process that still propagates. A baked address keeps them live, and numba then refuses to cache the caller at all rather than storing an address that is randomized per process. The same change makes a stale binary reachable. The caller's cached object code binds the derive's code, so editing the derive's body in another module serves the old binary on the cache hit: a caller returned 11.0 after its derive changed from x + 1.0 to x + 100.0. numba behaves this way for any cross-module cached callee, but on main this pattern was refused the cache outright, so nothing here could go stale before.
Reasoning DetailsWe are asked to review the diff and report only defects on added/modified lines. We must adhere strictly to the defect categories: bug, security, performance, data loss/corruption, or concurrency/resource-leak. Only report if >80% confident. Cite file:line.Let's examine the diff systematically. First, file: Is there a bug in One potential defect: importing Move to Now The class
The
Unboxing: Boxing: uses
Now Potential defect in Now The new
Potential defects:
Potential bug: The Another potential bug: In the But what about the One more thing: In the else branch where it branches on null, the Now, a potential defect in Now,
Now, look at the test file Now, is there any defect regarding the But what about Potential defect: In One more thing: In I'll look for any security vulnerability: maybe the assertion Data loss: The whole point is to prevent data loss by zeroed data on exception. The code fixes that. No new data loss. Performance: No obvious performance issue. Concurrency/resource leak: The Another potential issue: In But there is a subtle point: In the One more: In Potential defect: The Now, the most suspect area: However, there is a potential defect: The But the bigger issue: The code uses But I see a potential bug: The condition Now, check the Is there any chance that Thus, no obvious defect in I'll re-examine Potential performance defect: In Now, But I notice a discrepancy: Now, is there any risk of Potential defect: In Is there any issue with the Now, I'll consider the fact that in Potential defect: The Now, one more critical defect: The code in But there is a bigger issue: In Now, consider if But wait, in Thus, the Now, the test So far, no defects of high confidence. Let's consider the possibility of a type confusion in Another potential: In Now, Potential defect: The inner function Now, consider the assertion in Now, I'm thinking about Is there a chance that numba 0.61 changed the struct ordering? Now, I'll double-check the test expectation in Given the rules, if no defects, we output "No issues found." But we must be certain. I'll think if there's any critical bug that could cause a crash or data corruption. The key fix is to use However, the comment in So they assume All right, I think there is no defect. We should output "No issues found." But double-check the rule: "Omit Medium findings entirely unless they cause incorrect behavior." So no defect. I'll output exactly "No issues found." No issues found. |
Mutation over the whole suite showed both of these claims were asserted by nothing: removing either decref from the unbox helper, and reverting the constant lowering to a baked address, each left all tests passing. The refcount test replaces the module attribute the unboxer resolves at run time with a delegating spy, which both intercepts the Signature numba unserializes and gives the attribute probe an object nothing else holds. The two probes are deliberately asymmetric. Nothing but the unboxer touches the spy, so its release is pinned exactly at zero. The Signature is one memoized object that numba also hands to lower_get_wrapper_address, which numbox calls for c_addr and which never releases it, so one reference per call is a floor numba imposes; dropping the numbox-side release doubles it, and the ceiling is what catches that. The cache test runs one probe twice against a shared cache directory. A baked address is a dynamic global, so numba refuses to cache the caller and writes nothing; declaring the entry point symbolically leaves c_addr and py_addr dead and they are eliminated before numba scans the final module, so the caller caches. Both processes are asserted because a caller that recompiled every time would still propagate and still report no dynamic globals, and the index file is checked so the hit count cannot be vacuous.
The remaining two claims that mutation showed nothing asserted. A derive field typed as a plain FunctionType reaches _call_derive's runtime test of the jit_addr slot. rewrap_derive can only upgrade from Python scope, so a jitted caller building its own Work fixes the field at the type it declared, and numba's unboxing populates the slot for an njit dispatcher passed as a FunctionType-typed argument. That is the configuration the branch exists for, and deleting the branch left the whole suite passing because every other _call_derive typing in it is a DeriveFunctionType, so the branch was never even compiled. The test drives both outcomes through one compiled caller, since the declared argument type is the plain FunctionType rather than either dispatcher. typeof on a cres derive now yields the numbox-owned type, so an explicitly typed njit naming FunctionType would stop accepting it without can_convert_to, which would break code predating this change. Disabling the conversion passes the whole suite while breaking exactly that call.
Reasoning DetailsWe need to review the diff for defects: bugs, security vulnerabilities, performance problems, data loss, concurrency/resource-leak, breaking changes. Only report findings >80% sure.We'll examine each changed file for potential issues. File: any_type.pyChanges in def ol_get_as(self_ty, ty_ref: TypeRef):
from numba.core.types.function_type import FunctionType
from numbox.core.work.derive_wap import DeriveFunctionType
instance_ty = ty_ref.instance_type
ty_code = str(instance_ty)
# A derive compiled by `cres` stores as `DeriveFunctionType`, which callers
# ask back as the plain `FunctionType` of the same signature. ...
stored_codes = (ty_code,)
if type(instance_ty) is FunctionType:
stored_codes = (ty_code, str(DeriveFunctionType(instance_ty.signature)))
def _(self, ty):
if self.t not in stored_codes:
raise NumbaError(f"Any stored type {self.t}, cannot decode as {ty_code}")
return _deref_payload(self.p, ty)
return _The logic: For Potential issues: One concern: Edge: What if But is there a risk of a mismatch? For example, the stored type could be But there is a potential defect: The Is there a bug: The code checks Potential issue: The condition I don't see an immediate bug here. However, there is a nuanced performance issue: The creation of One defect: In But there might be a type error: Perhaps Wait, there's a potential defect: In the closure, So no defect in any_type.py. File: configurations.pyAdded: numba_version = int(version("numba").split(".")[1])
assert numba_version >= 60, numba_version
function_struct_size = 3 if numba_version >= 61 else 2Potential issues: Potential issue: import of Any potential performance? No. Defect? None. File: work.pyChanges:
Let's examine The intrinsic signature: The new code: def codegen(context, builder, signature, arguments):
derive_struct, sources = arguments
derive_args = []
for source_ind, source_ty in enumerate(sources_ty):
source = builder.extract_value(sources, source_ind)
data = extract_struct_member(context, builder, source_ty, source, "data")
derive_args.append(data)
def emit_propagating_call(jit_addr):
func_ty = context.call_conv.get_function_type(fsig.return_type, fsig.args)
derive_p = builder.bitcast(jit_addr, func_ty.as_pointer())
status, res = context.call_conv.call_function(
builder, derive_p, fsig.return_type, fsig.args, derive_args
)
with cgutils.if_unlikely(builder, status.is_error):
context.call_conv.return_status_propagate(builder, status)
return res
def emit_c_call():
derive_p_raw = get_func_p_from_func_struct(builder, derive_struct)
derive_ty_ll = get_ll_func_sig(context, derive_ty)
derive_p = builder.bitcast(derive_p_raw, derive_ty_ll.as_pointer())
return builder.call(derive_p, derive_args)
if not jit_addr_supported():
return emit_c_call()
jit_addr = builder.extract_value(derive_struct, JIT_ADDR_SLOT)
if isinstance(derive_ty, DeriveFunctionType):
return emit_propagating_call(jit_addr)
res_slot = cgutils.alloca_once(builder, context.get_value_type(fsig.return_type))
with builder.if_else(cgutils.is_null(builder, jit_addr), likely=False) as (null, populated):
with null:
builder.store(emit_c_call(), res_slot)
with populated:
builder.store(emit_propagating_call(jit_addr), res_slot)
return builder.load(res_slot)Potential issues:
Potential bug: In But wait, the intrinsic Now, potential defect: The But there might be a problem: One possible issue: In Now, a more subtle defect: The But note: Potential issue: In the Now, look at the def rewrap_derive(derive):
if not jit_addr_supported():
return derive
if not isinstance(derive, CompileResultWAP) or isinstance(derive, DeriveWAP):
return derive
upgraded = getattr(derive, _UPGRADED_ATTR, None)
if upgraded is None:
upgraded = DeriveWAP(derive.cres)
setattr(derive, _UPGRADED_ATTR, upgraded)
return upgradedThis is called from But is there any scenario where Potential defect: What if the original derive is a Now, note: In Now, any other defects in work.py? The Potential issue: The Now, consider the Potential bug: For Now, one more thing: In the Potential performance problem: The branching in Now, check Potential issues in
However, there is a potential leak: They do: sig_obj = pyapi.unserialize(pyapi.serialize_object(sig))
addr = pyapi.call_function_objargs(fn, (func, sig_obj))
pyapi.decref(fn)
pyapi.decref(sig_obj)
Let's check the exact code in sig_obj = pyapi.unserialize(pyapi.serialize_object(sig))
Let's see if the I'll confirm by checking numba source (mentally). Is it high severity? It's a memory leak per unboxing call, which could accumulate. If unboxing happens frequently (e.g., in a loop), it could be significant. So I'd report Medium? Or High? The rules: "a real performance problem (accidentally quadratic, N+1 query, unbounded allocation)" but a leak is a resource leak, which is covered under "unclosed handle, deadlock, lost wakeup". Actually resource leak is part of concurrency/resource-leak bug. So it's a defect. I'd rate it High because it's an unbounded leak. Now, other defects: In But the lack of decref on serialized object is a leak. Also, in sfunc.c_addr = pyapi.long_as_voidptr(addr)
pyapi.decref(addr)
...
sfunc.jit_addr = pyapi.long_as_voidptr(addr)
pyapi.decref(addr)The Potential issue: Now, in upgraded = getattr(derive, _UPGRADED_ATTR, None)
if upgraded is None:
upgraded = DeriveWAP(derive.cres)
setattr(derive, _UPGRADED_ATTR, upgraded)Potential problem: Check def lower_constant_derive_function_type(context, builder, typ, pyval):
typ = typ.get_precise()
assert typ.check_signature(pyval.signature())
sfunc = cgutils.create_struct_proxy(typ)(context, builder)
sfunc.c_addr = context.add_dynamic_addr(builder, pyval.__wrapper_address__(), info=str(typ))
sfunc.py_addr = context.add_dynamic_addr(builder, id(pyval), info=type(pyval).__name__)
fn = context.declare_function(builder.module, pyval.cres.fndesc)
sfunc.jit_addr = builder.bitcast(fn, context.get_value_type(types.voidptr))
context.active_code_library.add_linking_library(pyval.cres.library)
return sfunc._getvalue()Potential issue: But note: They use Potential bug: In But there is a potential issue: They don't call Now, the file Finally, the test Now, any other bugs? Check the interaction between One more potential issue: In Now, overall defect list:
We'll report the leak as High severity. Check if the leak might cause a crash? No, just memory usage. So High. Now, confirm the line: sig_obj = pyapi.unserialize(pyapi.serialize_object(sig))I'll cite In the full file, we can deduce from the structure: the function is after I'll need to look at the attached diff content. In the provided diff for The line with I'll report: Better to use the line number as it would be in the new file. The diff starts with line numbers for the new file: I'll count lines from I'll estimate line numbers by counting from the start of the diff (the The diff for derive_wap.py starts: I'll count line numbers: I'll report: Now, any other defect? Check Now, let's consider the test So I have one finding. Double-check if def serialize_object(self, obj):
...
return self.call_method(obj, "__reduce__")But I'm not 100% sure. However, typical pattern: any PyObject* returned from Python C-API that is not borrowed must be decref'd. Thus, report it. Now, check if any other leak exists in the function. They do Thus the only leak is the serialized bytes. I'll report it as High. Now, final answer: only that one finding. But wait: In One more potential defect: In But consider the case where the derive is a Thus, the only defect is the reference leak. I'll format the output accordingly. High — Reference leak in
|
Four prose corrections, each verified against a running interpreter rather than argued. Work.derive's docstring stated the propagating contract with no version gate, while pyproject declares numba>=0.60.0 and nothing propagates on 0.60. It is the one place a user reaches through help(Work) and the rendered API page, and the only one of the four that lacked the caveat. It now names both cases that still discard the exception. "Everything uses numba's public extension API" was overstated in the module docstring and flatly false in the rst. numba.extending exposes none of FunctionModel, CompileResultWAP, Conversion, box_function_type or lower_get_wrapper_address, JIT_ADDR_SLOT hardcodes the data model's field order, and the constant lowering drives declare_function and active_code_library directly. The half that is true, that no numba internals are patched, is kept and said plainly. The live model was checked to be (c_addr, py_addr, jit_addr), so the slot index is right today, which is what makes hardcoding it worth disclosing rather than fixing. Two cache caveats had no home in the docs at all. Upgrading numbox does not invalidate numba's on-disk cache: a user module compiled with cache=True against the older library keeps cache-hitting after the upgrade and goes on discarding the exception, because numba keys on the user's source rather than on the compiling library. Measured across three runs sharing one cache directory: old library cold gives data 0.0, new library over that cache gives a cache hit and still 0.0, new library cold raises. A cres derive re-keys on its own because its type name changes. Downgrading is worse than stale. A cached compile that saw a derive pickles the numbox-owned type into numba's index, and the unpickle at caching.py precedes the freshness check, so the load fails with ModuleNotFoundError rather than recompiling. Editing the user source and moving its mtime does not clear it, verified; deleting the cache directory does, also verified.
A container mixing a cres derive with a differently typed function value fails during numba's type unification. Four combinations that worked before this change now do not: a cres with a plain CompileResultWAP, with an njit dispatcher, with a cfunc, and the same pair reversed. A tuple of two cres derives is unaffected. The underlying defect is not this change's. unified_function_type requires every function type it meets to equal the first through a bare assert, so the failure carries no message, and two plain CompileResultWAP values of different signatures but the same argument count fail identically with numbox uninvolved, on this branch and on main alike. What changes here is how easily the case is reached, because DeriveFunctionType is a distinct type and numba compares function types by class. Making the two types compare equal is not available as a fix, which is why this is documented rather than repaired. numba interns types in a cache keyed by a weak reference, and a weak reference's equality is the referent's, so two equal types collapse onto whichever was interned first. Measured both ways: with the plain type first, asking for the derive type hands back the plain one and every derive silently returns to discarding its exception; with the derive type first, asking for the plain type hands back the derive one and every plain function value fails to unbox. The outcome depends on import order, so the distinctness is now pinned by a test rather than left to be rediscovered. Declaring the container type explicitly does not help either; the unification runs before any conversion applies.
Reasoning DetailsWe need to review the diff. The system prompt says: "Only report defects on lines that were added or modified." We must check for bugs, security vulnerabilities, performance issues, data loss/corruption, concurrency/resource leak bugs.We need to output only defects, strict markdown format. If no defects found, output "No issues found.". We'll go through each file change. Changes Overview
We must focus on defects in the changed lines. The changes are pretty large but we need to find concrete bugs. Let's examine each file for potential defects.
|
Work.deriveis invoked through a first-classFunctionTypecall. numba chooses how to lower thatcall from the
jit_addrslot of the function data model: a populated slot selects the numba callingconvention and unwinds normally, an empty one selects a C wrapper that does not carry exceptions.
numba fills the slot only for a
Dispatcher, so the compile resultcresproduces always took thediscarding path.
None of the consequences were visible to calling code.
calculatereturned normally,datawas leftzero-filled, and
derivedwas set anyway, so the wrong value was cached permanently. Forunicode_typedata the zeroed string struct has a NULL data pointer, so reading it back from Pythonsegfaulted the interpreter.
What changes
numbox/core/work/derive_wap.pyaddsDeriveWAP, which captures the calling convention entry pointof its compile result, and
DeriveFunctionType, which fillsjit_addrfrom it on both unboxing andconstant lowering.
cresmints those. An exception raised inside a derive now reaches the caller withits original type and message,
datais untouched, andderivedstays unset, so the node calculatesagain once the cause is addressed. The registrations go through numba's public extension API, though
the data model, wrapper protocol and conversion types they build on sit outside
numba.extending,and
JIT_ADDR_SLOThardcodes the data model's field order, verified as(c_addr, py_addr, jit_addr)on every supported numba. No numba internals are patched: nothing replaces numbabehaviour, it only registers against it.
_call_derivenever consultedjit_addr, reading struct slot 0 unconditionally, so the discard onthe
Workpath came from numbox rather than from numba's lowering. It now selects the propagatingconvention outright for a
DeriveFunctionTypefield, and testsjit_addrat runtime for a plainFunctionTypeone. The runtime test is what covers an njit dispatcher passed as an explicitlyFunctionType-typed argument, whose slot numba populates itself. A null slot emits the previous callunchanged.
make_workgains a Python front that upgrades a foreignCompileResultWAPthrough its compileresult, since the check is on the object's class and a jitted body cannot see it. Jitted callers reach
an overload that takes the value as given.
function_struct_sizemoves toconfigurations. Importinglowlevelcompiles a cached@proxy, andboth
cresandderive_wapneed the constant on paths that must stay free of compilation sideeffects; two
compile_kerneltests assert that a cres-formula kernel writes zero cache files andcatch exactly this.
Any.get_asaccepts a storedDeriveFunctionTypewhere the matchingFunctionTypeis requested.Both use numba's
FunctionModel, so the erased payload is identical. The type codes stay distinctrather than aliased, because they also feed content-addressed cache names, where collapsing two types
onto one hash would let a stale binary load.
What it does not cover
jit_addrslot.cresreturns a plainCompileResultWAPthere and the previousbehaviour is kept, gated on
function_struct_size.cresand reached from jitted scope,where its class is no longer visible to upgrade.
cresderive with a differently typed function value: a tuple holding acresalongside a plain
CompileResultWAP, an njit dispatcher or acfunc. numba unifies the elementtypes before any conversion applies, and
unified_function_typerequires every function type itmeets to equal the first through a bare
assert, so the failure carries no message. Homogeneouscontainers are unaffected, a tuple of two
cresderives included. This is not specific to numbox:two plain
CompileResultWAPvalues of different signatures but the same argument count failidentically with numbox uninvolved. Equality is not an available fix, because numba interns types in
a cache keyed by a weak reference, whose equality is the referent's, so equal types collapse onto
whichever was interned first, either erasing the propagating behaviour or making every plain
function value fail to unbox.
except ... as eand every typedexceptclause other than
Exception, so code reacting to a specific failure in jitted scope still has toencode it in the returned value.
calculatefrom inside aprangebody, where what escapes the parallel regionvaries by platform: Linux gives numba's
SystemError: ... returned a result with an exception setwith the original as
__cause__, and macOS raises nothing at all, so the loop completes and thecaller reads the node's previous
data. A plain jitted function raising insideprangebehavesthe same way with numbox uninvolved. The node keeps its
dataand its unsetderivedon everyplatform, so the failure is not cached and a later
calculateoutside the parallel region raisesnormally. A raise is not a reliable failure signal inside a
prangebody.Cache interactions
Three consequences of numba's on-disk cache. None is fixable here; all were measured.
compiled with
cache=Trueagainst the previous numbox keeps cache-hitting after the upgrade, andwhere it takes a plain
FunctionTypederive it goes on discarding the exception, because numba keysthe entry on your source rather than on the library that compiled it. Clear
NUMBA_CACHE_DIRafterupgrading. Measured over one shared cache directory: previous numbox cold gives
data 0.0, thisbranch over that cache gives a cache hit and still
0.0, this branch cold raises. Acresderivere-keys on its own, because its type name changes.
that saw a derive pickles the numbox-owned type into numba's index, and the unpickle precedes the
freshness check, so the load fails with
ModuleNotFoundError: No module named 'numbox.core.work.derive_wap'instead of recompiling. Editing your own source and moving its mtimedoes not clear it; deleting the cache directory does.
cresderive as a compile-time constant is now cacheable, and its cachedbinary binds the derive's code. Lowering
jit_addras a symbol leavesc_addrandpy_addrdead,so they are eliminated before numba scans the module and the caller no longer reports dynamic
globals. Editing the derive's body in another module then serves the stale binary on the cache hit.
numba does this for any cross-module cached callee, but on
mainthis pattern was refused the cacheoutright, so nothing could go stale.
Version boundaries
Both sides of the gate were exercised locally, since CI cannot show the difference in behaviour on its
own. On numba 0.61.0, the first release carrying the slot, the new tests run and pass. On 0.60.0 they
skip and a raising derive still returns a zero-filled
datawithderivedset, matching the previousbehaviour exactly, including the unraisable report on stderr.
The remaining known defect is not specific to this change: an exception propagating through the graph
leaks the NRT references owned by the frames it unwinds through, proportional to the number of nodes
walked. It belongs to propagation itself rather than to this design, and is worth raising upstream.