Debug mode: detect HPy_Close/return of context constants #303
Conversation
| @@ -75,7 +85,7 @@ void DHPy_invalid_handle(HPyContext *dctx, DHPy dh) | |||
| { | |||
| HPyDebugInfo *info = get_info(dctx); | |||
| HPyContext *uctx = info->uctx; | |||
| assert(as_DebugHandle(dh)->is_closed); | |||
| assert(as_DebugHandle(dh)->is_closed || as_DebugHandle(dh)->is_context_constant); | |||
| if (HPy_IsNull(info->uh_on_invalid_handle)) { | |||
| // default behavior: print an error and abort | |||
| HPy_FatalError(uctx, "Invalid usage of already closed handle"); | |||
There was a problem hiding this comment.
The error messages could be more specific (also for other scenarios) and we could pass some ID of the error to the user provided hook, but I am leaving that for followup work
steve-s
force-pushed
the
ss/debug-check-ctx-constants
branch
from
5de4bbe
to
4f96eab
Compare
|
Note: rebased on master, it seems the infer error was fixed on master meanwhile... |
| @@ -15,6 +15,8 @@ HPyDef_METH(new_generation, "new_generation", new_generation_impl, HPyFunc_NOARG | |||
| static UHPy new_generation_impl(HPyContext *uctx, UHPy self) | |||
| { | |||
| HPyContext *dctx = hpy_debug_get_ctx(uctx); | |||
| if (dctx == NULL) | |||
There was a problem hiding this comment.
These checks were added after error report from infer. https://github.com/hpyproject/hpy/actions/runs/1968620957/jobs/2813414188
There was a problem hiding this comment.
Maybe we should really abort in hpy_debug_ctx_get instead of returning NULL:
{
HPyContext *dctx = &g_debug_ctx;
if (uctx == dctx) {
HPy_FatalError(uctx, "hpy_debug_get_ctx: expected an universal ctx, "
"got a debug ctx");
}
if (hpy_debug_ctx_init(dctx, uctx) < 0) {
HPyErr_SetString(uctx, uctx->h_SystemError, "Could not create debug context");
return NULL;
}
There was a problem hiding this comment.
the current solution has the nice property is that if you import a module and for some reason the debug ctx cannot be created, you get a nice Python exception, but I see why it causes problems with infer.
Maybe we could switch to a different scheme in which you have a clearly separate init phase (which can raise) and then an "usage phase" (in which we assume that the dctx has been successfully created and thus we can assert that it's not NULL.
That said, I don't think it's important to do it in this PR, but i'ts good to keep in mind for the future
steve-s
force-pushed
the
ss/debug-check-ctx-constants
branch
from
46db47e
to
3604e54
Compare
|
Rebased on master. @antocuni can we take a look at reviewing this? :-) |
| @@ -15,6 +15,8 @@ HPyDef_METH(new_generation, "new_generation", new_generation_impl, HPyFunc_NOARG | |||
| static UHPy new_generation_impl(HPyContext *uctx, UHPy self) | |||
| { | |||
| HPyContext *dctx = hpy_debug_get_ctx(uctx); | |||
| if (dctx == NULL) | |||
There was a problem hiding this comment.
the current solution has the nice property is that if you import a module and for some reason the debug ctx cannot be created, you get a nice Python exception, but I see why it causes problems with infer.
Maybe we could switch to a different scheme in which you have a clearly separate init phase (which can raise) and then an "usage phase" (in which we assume that the dctx has been successfully created and thus we can assert that it's not NULL.
That said, I don't think it's important to do it in this PR, but i'ts good to keep in mind for the future
hpy/tools/autogen/debug.py
Outdated
| @@ -47,7 +47,7 @@ def generate(self): | |||
| w('{') | |||
| for var in self.api.variables: | |||
| name = var.name | |||
| w(f' dctx->{name} = DHPy_open(dctx, uctx->{name});') | |||
| w(f' dctx->{name} = DHPy_open_context_constant(dctx, uctx->{name});') | |||
There was a problem hiding this comment.
I really like the idea, but context_constant is a bit weird IMHO. What about DHPy_open_immortal?
Seemingly unrelated issue reported by infer.
steve-s
force-pushed
the
ss/debug-check-ctx-constants
branch
from
3604e54
to
8aa5219
Compare
|
Rebased on master and changed the name to "immortal" |
Tags handles that are used as context constants (e.g.,
h_None) and adds checks that guards againstHPy_Close(ctx, ctx->h_None)or againstreturn ctx->h_None.