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

Debug mode: check correct HPyContext usage #363

Merged
merged 5 commits into from
Feb 6, 2023

Conversation

steve-s
Copy link
Contributor

@steve-s steve-s commented Sep 23, 2022

Based on WIP in #350 done with @nirit100 at the HPy sprint

  • the private data of debug context now contains a flag is_valid and all the context functions check this flag as the first thing.
  • the private data of debug context is split to "shared" part and the is_valid flag
  • we have a circular buffer of multiple copies of the debug context
  • the trampoline (debug_ctx_CallRealFunctionFromTrampoline) takes next context from the buffer, sets is_valid=false for the previous context and is_valid=true for the context to be used (after the call the flags are reverted back)

@steve-s steve-s mentioned this pull request Sep 23, 2022
@steve-s steve-s force-pushed the ss/debug_context_check branch 2 times, most recently from 1027249 to a8a4b8e Compare September 23, 2022 17:47
@steve-s steve-s force-pushed the ss/debug_context_check branch from a8a4b8e to 8784bea Compare October 4, 2022 07:00
@mattip
Copy link
Contributor

mattip commented Nov 24, 2022

This has a merge conflict. Is it ready for review?

@steve-s steve-s force-pushed the ss/debug_context_check branch 2 times, most recently from 3550d56 to 5c2e64c Compare November 29, 2022 14:14
@steve-s
Copy link
Contributor Author

steve-s commented Nov 29, 2022

I've rebased the PR and it's ready for a review now.

Copy link
Contributor

@fangerer fangerer left a comment

Choose a reason for hiding this comment

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

Besides my concern about code duplication (see comment below), looks like a very good debug mode feature.

DHPy dh_result = f(dctx, dh_self);
DHPy_close_and_check(dctx, dh_self);

HPyContext *next_dctx = hpy_debug_get_next_dctx_from_cache(dctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a lot of code duplication. I think we could pull the temporary debug context setup and tear down code out of the switch if we use break instead of return at the end of the cases.
We then only need to use next_dctx for opening and closing the argument handles like dh_self.
Or do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. We cannot really pull out the whole thing, because in a->result variable a is a different struct every time, but I pulled as much as I could. Also used the new helpers in the autogenerated code.

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

@steve-s steve-s force-pushed the ss/debug_context_check branch from 8ce9712 to 76fef11 Compare December 6, 2022 11:02
@steve-s
Copy link
Contributor Author

steve-s commented Dec 12, 2022

@antocuni would you have some spare cycles for a review :-)?

@mattip
Copy link
Contributor

mattip commented Jan 31, 2023

This has conflicts.

@steve-s steve-s force-pushed the ss/debug_context_check branch from 76fef11 to 96fe058 Compare February 3, 2023 10:56
@steve-s
Copy link
Contributor Author

steve-s commented Feb 3, 2023

Proactively rebased on top of #402 to streamline merging of both PRs

@mattip
Copy link
Contributor

mattip commented Feb 5, 2023

Could you describe under what cases the checks might fail? What is the scenario that would cause someone to get the wrong context?

@steve-s
Copy link
Contributor Author

steve-s commented Feb 6, 2023

Could you describe under what cases the checks might fail? What is the scenario that would cause someone to get the wrong context?

What this aims to prevent is a situation when someone misunderstands the contract of HPy and stashes context into some global variable to read it later in a context of another new Python->HPy call. For example, in the test:

https://github.com/hpyproject/hpy/pull/363/files#diff-885e9f36d2a0124e4a50c595a7fac468a1ac9af3c5dea852646c1d52d5694346R37

on this line it uses global variable keep as HPyContext, but it should use the ctx argument.

@mattip mattip merged commit 628acaf into hpyproject:master Feb 6, 2023
@mattip
Copy link
Contributor

mattip commented Feb 6, 2023

Thanks @steve-s

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.

4 participants