Skip to content

Conversation

@EarlMilktea
Copy link
Member

This PR replaces cdef public with cdef extern.

@leofang leofang self-requested a review November 13, 2024 05:35
@leofang
Copy link
Member

leofang commented Nov 13, 2024

Hi @EarlMilktea I am not sure if this makes sense, but perhaps I can explain the intention from #8442 first, and then you can tell me if I missed anything 🙂

First of all, we own these functions; they are not defined somewhere else, so technically cdef extern is a bit confusing. But more importantly, we need them to be exposed with C linkage, so that we can load these symbols via ctypes or other means (ex: dlopen) by (unmangled) name, ex:

cupy/tests/conftest.py

Lines 93 to 96 in 0714d17

_calloc_ = ctypes.addressof(lib._calloc)
_malloc_ = ctypes.addressof(lib._malloc)
_realloc_ = ctypes.addressof(lib._realloc)
_free_ = ctypes.addressof(lib._free)

In order to ensure we have C instead of C++ linkage, we used CYTHON_EXTERN_C, which I am not sure if it'd still work after changing these to cdef extern.

Maybe we can start by confirming the symbols are still exposed under the same names after this change, and then add a simple test coverage? If this change can still expose the symbols just fine, I agree we should not need to generate/package a header.

@asi1024 asi1024 added cat:enhancement Improvements to existing features prio:medium labels Nov 14, 2024
@EarlMilktea
Copy link
Member Author

@leofang (CC: @kmaehashi )

Thank you for pointing out the problems!
How about closing this PR and adding the file to .gitignore for better DX for now, as the role of the file is much more important and complex than I expected?

@kmaehashi
Copy link
Member

I thought cdef extern can be used to expose symbols but will double check.

@kmaehashi
Copy link
Member

I thought cdef extern can be used to expose symbols but will double check.

The symbol is exported using cdef extern but as C++ linkage, which is not what we want. I'll add it to gitignore in #8813.

@kmaehashi kmaehashi closed this Dec 14, 2024
@EarlMilktea EarlMilktea deleted the use-extern branch December 14, 2024 11:58
@kmaehashi kmaehashi added this to the Closed PRs milestone Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:enhancement Improvements to existing features prio:medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants