You can cheat in FFI as long as you don't get caught...
I was trying to inquire about my latest vm crash in libgit2 https://pharo.fogbugz.com/f/cases/20655/vm-crash-in-libgit-git_tree_free-thr... when I found this bizarre prototype: ^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #() While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/git_diff_tree_to_tree int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts); The fact that it does not differs for opts, made me think of a bug... But no. opts is allocated on external c heap with: callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version)) int git_diff_init_options(git_diff_options *opts, unsigned int version); What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be. For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer). As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance. That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to. But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down. I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
yeah, but this is because LGitDiff, LGitRepository and LGitTree (and many others) are defined as FFIExternalObjects while they should be FFIOpaqueObjects. Then, the problem is that external objects are *already* pointers (they have arity 1). this are remaining of old nativeboost and we created the opaque objects to not need this âcheatingâ⦠just, I wanted to change them but is a lot of work and I didnât have the time. I agree it stinks. Esteban
On 8 Nov 2017, at 19:06, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com <mailto:nicolas.cellier.aka.nice@gmail.com>> wrote:
I was trying to inquire about my latest vm crash in libgit2
https://pharo.fogbugz.com/f/cases/20655/vm-crash-in-libgit-git_tree_free-thr... <https://pharo.fogbugz.com/f/cases/20655/vm-crash-in-libgit-git_tree_free-thr...>
when I found this bizarre prototype:
^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #()
While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/git_diff_tree_to_tree <https://libgit2.github.com/libgit2/#v0.19.0/group/diff/git_diff_tree_to_tree>
int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts);
The fact that it does not differs for opts, made me think of a bug... But no.
opts is allocated on external c heap with:
callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version))
int git_diff_init_options(git_diff_options *opts, unsigned int version);
What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be.
For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer).
As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance.
That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to.
But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down.
I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
On 9 November 2017 at 12:03, Esteban Lorenzano <estebanlm@gmail.com> wrote:
yeah, but this is because LGitDiff, LGitRepository and LGitTree (and many others) are defined as FFIExternalObjects while they should be FFIOpaqueObjects. Then, the problem is that external objects are *already* pointers (they have arity 1).
hmm but the external objects were already designed with thought that it holding some opaque handle to some external object. it does not neccessary should be a pointer .. it could be some handle, that doesn't points directly to some memory location (like in windows api etc). they only difference is only in what function(s) expecting, e.g. if it takes a handle value - you should pass it by value (the handle itself), and when it asks for pointer to the handle (to fill a return value , for instance) - you passing a pointer to it.
this are remaining of old nativeboost and we created the opaque objects to not need this âcheatingâ⦠just, I wanted to change them but is a lot of work and I didnât have the time.
I agree it stinks.
Esteban
On 8 Nov 2017, at 19:06, Nicolas Cellier <nicolas.cellier.aka.nice@ gmail.com> wrote:
I was trying to inquire about my latest vm crash in libgit2
https://pharo.fogbugz.com/f/cases/20655/vm-crash-in- libgit-git_tree_free-throw-suspiscion-on-potential-double-free-problem
when I found this bizarre prototype:
^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #()
While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/ git_diff_tree_to_tree
int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts);
The fact that it does not differs for opts, made me think of a bug... But no.
opts is allocated on external c heap with:
callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version))
int git_diff_init_options(git_diff_options *opts, unsigned int version);
What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be.
For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer).
As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance.
That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to.
But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down.
I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
-- Best regards, Igor Stasenko.
2017-11-09 12:24 GMT+01:00 Igor Stasenko <siguctua@gmail.com>:
On 9 November 2017 at 12:03, Esteban Lorenzano <estebanlm@gmail.com> wrote:
yeah, but this is because LGitDiff, LGitRepository and LGitTree (and many others) are defined as FFIExternalObjects while they should be FFIOpaqueObjects. Then, the problem is that external objects are *already* pointers (they have arity 1).
hmm but the external objects were already designed with thought that it holding some opaque handle to some external object. it does not neccessary should be a pointer .. it could be some handle, that doesn't points directly to some memory location (like in windows api etc). they only difference is only in what function(s) expecting, e.g. if it takes a handle value - you should pass it by value (the handle itself), and when it asks for pointer to the handle (to fill a return value , for instance) - you passing a pointer to it.
Agree that should not make a difference, a handle can be anything, a pointer, an int, ... Technically, the git_tree are not opaque, they are struct. BUT we don't care at all of their layout/fields from within Pharo and want to treat them as Opaque. - First reason: declaring fields would make the Pharo code fragile versus future evolution of libgit2 because any change in the layout would create a dangerous mismatch. - Second reason: we never have to allocate space for those struct, the library cares for us. - Third reason: libgit2 APi will never deal with a git_tree, only git_tree* or git_tree** So I think that you made a good choice to use a sort-of-handle (git_tree*). I am now certain that it was the exact intention of FFIExternalObject Maybe we could rename superclass to FFIExternalObjectPointer to make explicit that it adds a pointer arity? We must also provide examples in class comment (or link to a Help page...) Example: I have an external object bar from library foo (a foo_bar). C signature: void foo_create_bar(foo_bar **bar); FFIExternalObjectPointer subclass: FooBar. a Smalltalk FooBar now represents a pointer to a foo_bar (a foo_bar *). And the library will be invoked with: self call: #(void foo_create_bar(FooBar *self)) If the library function return a pointer to a foo_bar C signature: foo_bar *foo_bar_alloc(size_t size); then we interface it like that: handle pointerAt: 1 put: (self call: #(FooBar foo_bar_alloc( size_t size))). Note that the handle has one more indirection (it is equivalent to a foo_bar**) And when wanting to use a the foo_bar: C signature: void foo_use_bar(foo_bar *bar); self call: #(void foo_use_bar(FooBar self)). Still it's not obvious to distinguish why LGitTree has different pointer arity than LGitDiffOption at first sight... Naming them LGitTreeHandle or LGitTreePointer is maybe too much? this are remaining of old nativeboost and we created the opaque objects to
not need this âcheatingâ⦠just, I wanted to change them but is a lot of work and I didnât have the time.
I agree it stinks.
Esteban
On 8 Nov 2017, at 19:06, Nicolas Cellier <nicolas.cellier.aka.nice@gmai l.com> wrote:
I was trying to inquire about my latest vm crash in libgit2
https://pharo.fogbugz.com/f/cases/20655/vm-crash-in-libgit- git_tree_free-throw-suspiscion-on-potential-double-free-problem
when I found this bizarre prototype:
^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #()
While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/git_ diff_tree_to_tree
int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts);
The fact that it does not differs for opts, made me think of a bug... But no.
opts is allocated on external c heap with:
callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version))
int git_diff_init_options(git_diff_options *opts, unsigned int version);
What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be.
For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer).
As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance.
That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to.
But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down.
I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
-- Best regards, Igor Stasenko.
On 9 November 2017 at 16:28, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
2017-11-09 12:24 GMT+01:00 Igor Stasenko <siguctua@gmail.com>:
On 9 November 2017 at 12:03, Esteban Lorenzano <estebanlm@gmail.com> wrote:
yeah, but this is because LGitDiff, LGitRepository and LGitTree (and many others) are defined as FFIExternalObjects while they should be FFIOpaqueObjects. Then, the problem is that external objects are *already* pointers (they have arity 1).
hmm but the external objects were already designed with thought that it holding some opaque handle to some external object. it does not neccessary should be a pointer .. it could be some handle, that doesn't points directly to some memory location (like in windows api etc). they only difference is only in what function(s) expecting, e.g. if it takes a handle value - you should pass it by value (the handle itself), and when it asks for pointer to the handle (to fill a return value , for instance) - you passing a pointer to it.
Agree that should not make a difference, a handle can be anything, a pointer, an int, ...
Technically, the git_tree are not opaque, they are struct. BUT we don't care at all of their layout/fields from within Pharo and want to treat them as Opaque. - First reason: declaring fields would make the Pharo code fragile versus future evolution of libgit2 because any change in the layout would create a dangerous mismatch. - Second reason: we never have to allocate space for those struct, the library cares for us. - Third reason: libgit2 APi will never deal with a git_tree, only git_tree* or git_tree**
So I think that you made a good choice to use a sort-of-handle (git_tree*).
Not me. I have nothing to do with libgit bindings.. But usually, for any who knows C it should be quite simple to follow a pattern.. it is like typedef foo_bar* FooBarObject; so, then you use FooBarObject instead of foo_bar* everywhere. I am now certain that it was the exact intention of FFIExternalObject
Maybe we could rename superclass to FFIExternalObjectPointer to make explicit that it adds a pointer arity? We must also provide examples in class comment (or link to a Help page...)
IIRC, FFIExternalObject intentionally was not allowing arity more than one. So, you can use it for passing it as a return-value argument to function, but you should not use it where function expecting an array/list of pointers of given type. Example: I have an external object bar from library foo (a foo_bar).
C signature: void foo_create_bar(foo_bar **bar); FFIExternalObjectPointer subclass: FooBar.
a Smalltalk FooBar now represents a pointer to a foo_bar (a foo_bar *). And the library will be invoked with:
self call: #(void foo_create_bar(FooBar *self))
If the library function return a pointer to a foo_bar
yes, if it using argument as a return-value.
C signature: foo_bar *foo_bar_alloc(size_t size);
then we interface it like that:
handle pointerAt: 1 put: (self call: #(FooBar foo_bar_alloc( size_t size))).
? why?
it should return an object already suitable for use..- an instance of FooBar. mybar := self call: #(FooBar foo_bar_alloc( size_t size). self assert: (mybar class == FooBar) At least it was like that in NB.
Note that the handle has one more indirection (it is equivalent to a foo_bar**) And when wanting to use a the foo_bar:
C signature: void foo_use_bar(foo_bar *bar); self call: #(void foo_use_bar(FooBar self)).
yep
Still it's not obvious to distinguish why LGitTree has different pointer arity than LGitDiffOption at first sight... Naming them LGitTreeHandle or LGitTreePointer is maybe too much?
i think it expecting a list of options, and if not, then there's a mistake. i don't know much about libgit..
this are remaining of old nativeboost and we created the opaque objects to
not need this âcheatingâ⦠just, I wanted to change them but is a lot of work and I didnât have the time.
I agree it stinks.
Esteban
On 8 Nov 2017, at 19:06, Nicolas Cellier <nicolas.cellier.aka.nice@gmai l.com> wrote:
I was trying to inquire about my latest vm crash in libgit2
https://pharo.fogbugz.com/f/cases/20655/vm-crash-in-libgit-g it_tree_free-throw-suspiscion-on-potential-double-free-problem
when I found this bizarre prototype:
^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #()
While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/git_d iff_tree_to_tree
int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts);
The fact that it does not differs for opts, made me think of a bug... But no.
opts is allocated on external c heap with:
callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version))
int git_diff_init_options(git_diff_options *opts, unsigned int version);
What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be.
For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer).
As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance.
That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to.
But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down.
I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
-- Best regards, Igor Stasenko.
-- Best regards, Igor Stasenko.
2017-11-09 17:53 GMT+01:00 Igor Stasenko <siguctua@gmail.com>:
On 9 November 2017 at 16:28, Nicolas Cellier <nicolas.cellier.aka.nice@ gmail.com> wrote:
2017-11-09 12:24 GMT+01:00 Igor Stasenko <siguctua@gmail.com>:
On 9 November 2017 at 12:03, Esteban Lorenzano <estebanlm@gmail.com> wrote:
yeah, but this is because LGitDiff, LGitRepository and LGitTree (and many others) are defined as FFIExternalObjects while they should be FFIOpaqueObjects. Then, the problem is that external objects are *already* pointers (they have arity 1).
hmm but the external objects were already designed with thought that it holding some opaque handle to some external object. it does not neccessary should be a pointer .. it could be some handle, that doesn't points directly to some memory location (like in windows api etc). they only difference is only in what function(s) expecting, e.g. if it takes a handle value - you should pass it by value (the handle itself), and when it asks for pointer to the handle (to fill a return value , for instance) - you passing a pointer to it.
Agree that should not make a difference, a handle can be anything, a pointer, an int, ...
Technically, the git_tree are not opaque, they are struct. BUT we don't care at all of their layout/fields from within Pharo and want to treat them as Opaque. - First reason: declaring fields would make the Pharo code fragile versus future evolution of libgit2 because any change in the layout would create a dangerous mismatch. - Second reason: we never have to allocate space for those struct, the library cares for us. - Third reason: libgit2 APi will never deal with a git_tree, only git_tree* or git_tree**
So I think that you made a good choice to use a sort-of-handle (git_tree*).
Not me. I have nothing to do with libgit bindings..
Sorry I answerd to both you and Esteban (and other developpers) But usually, for any who knows C it should be quite simple to follow a
pattern.. it is like
typedef foo_bar* FooBarObject;
As long as the names tells, my whole point is that typedef foo_bar* FooBar; Is already a questionable naming per se And having at the same time typedef foo_opt FooOpt; is troublesome so, then you use FooBarObject instead of foo_bar* everywhere.
I am now certain that it was the exact intention of FFIExternalObject
Maybe we could rename superclass to FFIExternalObjectPointer to make explicit that it adds a pointer arity? We must also provide examples in class comment (or link to a Help page...)
IIRC, FFIExternalObject intentionally was not allowing arity more than one. So, you can use it for passing it as a return-value argument to function, but you should not use it where function expecting an array/list of pointers of given type.
Example: I have an external object bar from library foo (a foo_bar).
C signature: void foo_create_bar(foo_bar **bar); FFIExternalObjectPointer subclass: FooBar.
a Smalltalk FooBar now represents a pointer to a foo_bar (a foo_bar *). And the library will be invoked with:
self call: #(void foo_create_bar(FooBar *self))
If the library function return a pointer to a foo_bar
yes, if it using argument as a return-value.
C signature: foo_bar *foo_bar_alloc(size_t size);
then we interface it like that:
handle pointerAt: 1 put: (self call: #(FooBar foo_bar_alloc( size_t size))).
? why?
it should return an object already suitable for use..- an instance of FooBar.
mybar := self call: #(FooBar foo_bar_alloc( size_t size).
self assert: (mybar class == FooBar)
At least it was like that in NB.
Ah, maybe i was thinking at a much too low level then I will try and understand what happens in UFFI.
Note that the handle has one more indirection (it is equivalent to a foo_bar**) And when wanting to use a the foo_bar:
C signature: void foo_use_bar(foo_bar *bar); self call: #(void foo_use_bar(FooBar self)).
yep
Still it's not obvious to distinguish why LGitTree has different pointer arity than LGitDiffOption at first sight... Naming them LGitTreeHandle or LGitTreePointer is maybe too much?
i think it expecting a list of options, and if not, then there's a mistake. i don't know much about libgit..
Libgit2 git_options is just another struct like every other ones git_tree etc... The problem is that the API does not alllocate those git_options for us. Instead they just initialize directly from literal values via macro: https://github.com/libgit2/libgit2/search?utf8=%E2%9C%93&q=git_diff_options&... #define GIT_DIFF_OPTIONS_INIT \ {GIT_DIFF_OPTIONS_VERSION, 0, GIT_SUBMODULE_IGNORE_UNSPECIFIED, {NULL,0}, NULL, NULL, NULL, 3} So we have to allocate the options in Smalltalk by ourself and can't handle the structure as Opaque unlike the other structures...
this are remaining of old nativeboost and we created the opaque objects
to not need this âcheatingâ⦠just, I wanted to change them but is a lot of work and I didnât have the time.
I agree it stinks.
Esteban
On 8 Nov 2017, at 19:06, Nicolas Cellier <nicolas.cellier.aka.nice@gmai l.com> wrote:
I was trying to inquire about my latest vm crash in libgit2
https://pharo.fogbugz.com/f/cases/20655/vm-crash-in-libgit-g it_tree_free-throw-suspiscion-on-potential-double-free-problem
when I found this bizarre prototype:
^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #()
While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/git_d iff_tree_to_tree
int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts);
The fact that it does not differs for opts, made me think of a bug... But no.
opts is allocated on external c heap with:
callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version))
int git_diff_init_options(git_diff_options *opts, unsigned int version);
What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be.
For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer).
As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance.
That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to.
But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down.
I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
-- Best regards, Igor Stasenko.
-- Best regards, Igor Stasenko.
Yes. Signatures of C functions often tells little about correct use of arguments, when there are pointers. And true, without checking the docs & use example, you cannot really make it work correctly, if you will rely only on signature when designing the call to such function(s). It is, in fact, orthogonal to FFI .. it is the way it works in C.. But what is good from FFI part, is that you can design a convenient interface to API that having such functions, in a way, that your users don't need to scratch their heads and stumble upon the code, asking , - hey.. wtf. That's why i said many times, that yes, it is cool to have automatic FFI wrapper generators, but they will never be as good as well thought and designed manually written interfaces to libraries, providing features of a library without burden of strange, bizarre and inconvenient way to pass data between user and library. On 9 November 2017 at 00:06, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
I was trying to inquire about my latest vm crash in libgit2
https://pharo.fogbugz.com/f/cases/20655/vm-crash-in- libgit-git_tree_free-throw-suspiscion-on-potential-double-free-problem
when I found this bizarre prototype:
^ self callUnchecked: #(LGitReturnCodeEnum git_diff_tree_to_tree #(LGitDiff * diff , LGitRepository repo , LGitTree old_tree , LGitTree new_tree , LGitDiffOptions * opts)) options: #()
While the libgit2 signature differs in indirection levels: https://libgit2.github.com/libgit2/#v0.19.0/group/diff/ git_diff_tree_to_tree
int git_diff_tree_to_tree(git_diff_list **diff, git_repository *repo, git_tree *old_tree, git_tree *new_tree, const git_diff_options *opts);
The fact that it does not differs for opts, made me think of a bug... But no.
opts is allocated on external c heap with:
callUnchecked: #(LGitReturnCodeEnum git_diff_init_options(LGitDiffOptions * self, LGitOptionsVersionsEnum version))
int git_diff_init_options(git_diff_options *opts, unsigned int version);
What you see this time is that signatures match... So the LGitDiffOptions handle will be an ExternalAddress that will really point on a git_diff_options In other words, the handle is a git_diff_options *... IOW, our LGitDiffOptions object points to an external it_diff_options so it is what it promises to be.
For the other structures not so. We are lying on the level of indirection, so our LGitTree handle are not really a git_tree *. They are a git_tree **. (an ExternalAddress on a pointer).
As long as we lie consistently, it's OK, because it avoids too much packToArity: unpackToArity: dance.
That's the "beauty" of C pointers. We can cast them to whatever or pretend they are handle_for_anything * in the intermediate level, as long as original producer and final consumer agree on what it really points to.
But from code quality perspective, it really stinks. Anyone like me opening a page to get the exact signature of the function will be scratching head and loose precious time. Especially when tracking vm crash down.
I'm not sure how well it's documented, I presume it's a well known conscious hack from the original developpers, but such practice really ought to be discussed here.
-- Best regards, Igor Stasenko.
participants (3)
-
Esteban Lorenzano -
Igor Stasenko -
Nicolas Cellier