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

HPyGlobal: debug mode should check registration in HPyModuleDef->globals #462

Open
mattip opened this issue Dec 6, 2023 · 4 comments
Open
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@mattip
Copy link
Contributor

mattip commented Dec 6, 2023

I don't see tests for setting the globals field in HPyModuleDef, and the only reference I could find in the cpython backend is here for error checking.

The NumPy port does use this, but I think that is redundant, since there the objects are globally saved and retrieved but not from the module. Using .global would require being able to fish out the module handles, which is discussed elsewhere.

@fangerer
Copy link
Contributor

fangerer commented Dec 6, 2023

You are right. The idea of HPyModuleDef.globals is that one needs to register all HPyGlobal variables such that the interpreter can initialize the C-level value of those and do any internal preparation to be able to use the globals.

For CPython, we don't actually need that because HPyGlobal variables are just C global variables of type PyObject * and those are guaranteed to be initialized to NULL.

But I agree with you: since the contract is that one must register the globals, we should enforce this in the debug mode.
We could do that by writing some marker value to the HPyGlobal.

@steve-s steve-s changed the title Is HPyModuleDef->globals used anywhere? HPyGlobal: debug mode should check registration in HPyModuleDef->globals Dec 6, 2023
@steve-s
Copy link
Contributor

steve-s commented Dec 6, 2023

I though I already had an issue opened for that. I took the liberty and renamed this issue. I think we are using that field in GraalPy (or at least planned to use it) to preallocate our internal array with the actual values for globals.

@mattip
Copy link
Contributor Author

mattip commented Dec 6, 2023

A test would be nice (and that test would fail on CPython since it does not register them).

@fangerer fangerer added this to the ABI version 2 milestone Dec 7, 2023
@fangerer fangerer self-assigned this Dec 7, 2023
@fangerer fangerer added the enhancement New feature or request label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants