Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Debugging mode for CPython universal #43

Closed
wants to merge 2 commits into from

Conversation

arigo
Copy link
Member

@arigo arigo commented Jun 20, 2020

Could be merged as it is, but probably only compiles on Linux. Also, there is no way to disable the extra checks, but they don't cost a lot.

@antocuni
Copy link
Collaborator

Are we sure this is the design we want to use?
If I understand it correctly, these changes merge the debug checks into the normal universal mode; even if these precise changes have a low overhead, it is likely that we will need to introduce a we_are_in_debug_mode flag to implement more expensive checks in the future.

An alternative design is to implement the debug mode as a wrapper around an existing HPyContext:

  • one minor advantage is that it will have 0 overhead when it's not used
  • the biggest advantage, IMHO, is that we will be able to use the very same debug mode on all interpreters. In particular, we will not need to implement it again on PyPy

@arigo
Copy link
Member Author

arigo commented Jun 26, 2020

While I see the long-term point, I have some doubts about the idea now. In "your" version, we need to wrap at least all functions of the API to check all handles. It doesn't exist so far because it is quite some work and it will add maintenance cost. It risks bugs of its own, because it needs to be complete and consistent. And it needs to be explicitly enabled by the user. By comparison this is a few lines of code, and adding the same ones inside PyPy is trivial.

@antocuni
Copy link
Collaborator

I see your point. My idea was to autogen all the wrappers, i.e. something like this:

HPy ctx_Long_FromLong(HPyContext ctx, long x)
{
    HPy res = ctx->original_ctx->ctx_Long_FromLong(ctx, x);
    ctx->_record_new_handle(res);
    return res;
}

If we do it this way, the only maintenance cost it adds is the autogen code (which, btw, it much nicer and easier to do in my WIP hpyfunc_autogen branch).
But, apart from handles, my main concern is about all the other debug checks that we will want to introduce. It would be nice to avoid redoing all the work twice, and moreover it would be nice if the debug mode on CPython and PyPy generate the very same warnings.

#define HPY_ASSERT(condition, errargs) \
do { \
if (!(condition)) \
_hpy_fatal errargs; \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably use __VA_ARGS__ to avoid the weird usage pattern in which you have to put parenthesis in the call?

@@ -12,6 +12,7 @@
'hpy/universal/src/ctx_module.c',
'hpy/universal/src/ctx_meth.c',
'hpy/universal/src/ctx_misc.c',
'hpy/universal/src/debugmode.c',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my idea, the "debug mode" be something different, i.e. a full wrapper around the ctx which does many extra checks, including potentially expensive ones, and will . So, maybe we should call this file differently?

@arigo
Copy link
Member Author

arigo commented Sep 1, 2020

Redone in PR #69.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants