search

SciNim / flambeau

Nim bindings to libtorch
85 stars 3 forks source link

To `lent` or not to `lent` #26

Open HugoGranstrom opened 3 years ago

HugoGranstrom commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 👍