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

To lent or not to lent #26

Open
HugoGranstrom opened this issue Apr 30, 2021 · 4 comments
Open

To lent or not to lent #26

HugoGranstrom opened this issue Apr 30, 2021 · 4 comments

Comments

@HugoGranstrom
Copy link
Member

Right now we use lent UncheckedArray instead of ptr UncheckedArray in some places. And at the moment lent is handled behind the scenes with pointers but from this conversation with @Clyybber it isn't guaranteed to stay that way in the future. Should we just change all functions returning lent UncheckedArray to ptr UncheckedArray to be sure it won't break in the future?

@mratsim
Copy link
Collaborator

mratsim commented May 3, 2021

The advantage of lent is that it avoids people escaping with a ptr and then introducing use-after-free bugs which are hard to debug.

What we can do is write a test that ensures we don't miss the transition and then use lent ptr UncheckedArray.

@HugoGranstrom
Copy link
Member Author

Ok that's a good point 👍

What does it mean to lend a ptr though? Doesn't it just mean that the pointer itself won't be released but whatever it points to could have been released? It's better than nothing regardless 😄

One other problem, if we have a lent Uncheckedarray and want to pass it to a proc but the proc wants a ptr Uncheckedarray. Is there a way to keep the lent part? Ie is there an alternative to passing in the unsafeAddr of the Uncheckedarray?

@Clonkk
Copy link
Member

Clonkk commented May 5, 2021

This just regards the ArrayRef wrapped type since for Tensor & RawTensor data_ptr already returns a ptr UncheckedArray[T], right ?

I think I would go with a pt UncheckedArray for internal use.

For public API, we should try to mostly expose openArray / seq and do the conversion internally.

For the few that may remains, it will mainly involve seq/openArray with low number of element to ArrayRef[T] to pass as parameters so even exposing a copyMem based API could be enough.

@HugoGranstrom
Copy link
Member Author

Yes that seems to be the case, although it may very well be that I changed them to ptr in the past because I was frustrated with how they didn't cooperate with me 🤣

Yes, that seems like a reasonable thing to do. The user shouldn't be exposed to these things unless specifically asking for them (data_ptr etc). Openarray should be the standard type we expose to the user in my opinion 👍

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

No branches or pull requests

3 participants