[Pharo-dev] ExternalAddress uniqueness (was Re: Call for help for stability and rewrite of FreeType)

Ben Coman btc at openinworld.com
Wed Nov 8 16:56:19 EST 2017


Thanks for your deep dive on this Nicolas.
I'm not familiar with that part of the system, and its interesting to read
about the interface constraints.

cheers -ben


On Wed, Nov 8, 2017 at 11:40 PM, Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com> wrote:

>
>
> 2017-11-08 16:10 GMT+01:00 Nicolas Cellier <nicolas.cellier.aka.nice@
> gmail.com>:
>
>>
>>
>> 2017-11-08 15:35 GMT+01:00 Nicolas Cellier <nicolas.cellier.aka.nice at gmai
>> l.com>:
>>
>>>
>>>
>>> 2017-11-08 14:53 GMT+01:00 Nicolas Cellier <
>>> nicolas.cellier.aka.nice at gmail.com>:
>>>
>>>>
>>>>
>>>> 2017-11-08 14:42 GMT+01:00 Nicolas Cellier <
>>>> nicolas.cellier.aka.nice at gmail.com>:
>>>>
>>>>>
>>>>> Ben,
>>>>>
>>>>> This is my fresh crash.dmp
>>>>> it sounds very related to your analysis!!!
>>>>>
>>>>> In fact we are not freeing by ourselves, but telling libgit2 to do
>>>>> it...
>>>>>
>>>>>
>>>> Oh worse than that, it sounds like git implemented its own mechanism of
>>>> counted pointers...
>>>> So we don't tell anything, he guesses by himself.
>>>> I would search for places where we #gcallocate: or manually #free a
>>>> pointer on a structure passed back by git...
>>>>
>>>>
>>> and of course, it's not gcallocate: because this was a very old wheel...
>>> It's rather somewhere in UFFI equivalent
>>> FFIExternalResourceExecutor <- FFIExternalResourceManager <-
>>> LGitExternalStructure autoRelease
>>>
>>> Among the senders, we see (tiens, tiens...):
>>> LGitTree>>...
>>>
>>> So this is where I would search the origin of my own crash dump...
>>>
>>> But also (tiens, tiens...):
>>> CairoFontFace>>initializeWithFreetypeFace:
>>>
>>> What if FreeType plugin was not the problem per se, but its usage in
>>> cairo was?
>>>
>>> cairo_font_face_destroy ()
>>> void                cairo_font_face_destroy
>>> (cairo_font_face_t *font_face);
>>> Decreases the reference count on font_face by one. If the result is
>>> zero, *then font_face and all associated resources are freed*. See
>>> cairo_font_face_reference().
>>> font_face :
>>> a cairo_font_face_t
>>>
>>> Since we pass a pointer to the free type font at creation:
>>>
>>> fromFreetypeFace: aFace
>>>     | handle cairoFace |
>>>     handle := aFace handle pointerAt: 1.
>>>      cairoFace := self primFtFace: handle loadFlags: ( LoadNoHinting |
>>> LoadTargetLCD | LoadNoAutohint | LoadNoBitmap).
>>>     ^ cairoFace initializeWithFreetypeFace: aFace
>>>
>>> Isn't it possible that we somehow double free the free type font too?
>>>
>>>
>> Hmm not the exact catch but it could well be related
>>
>> https://www.cairographics.org/manual/cairo-FreeType-Fonts.html tells how
>> to couple lifetime of the 2 data structures.
>> I see that CairoFontFace retains a pointer on the FT_Face thru a
>> dedicated ivar, so at least, we don't free the FT_Face twice, and we don't
>> free it until we free the cairo_ft_face
>>
>> When finalizatoin occurs, I'm not sure that the finalization order is
>> guaranteed but that does not matter.
>> What matters is that the cairo_ft_face could still be referenced
>> internally by cairo.
>>
>> So what can happen is that:
>> 1) we don't reference anymore the CairoFontFace from within Smalltalk
>> 2) finalization happens we call cairo_font_face_destroy ()
>> 3) there is no more pointer on the FTFace from within Smalltalk (because
>> we just reclaimed the CairoFontFace pointing on it)
>> 4) finalization happens and we call FT_Done_Face()
>>
>> BUT: cairo was still using the cairo_font_face internally, (the reference
>> count did not reach zero) and is now pointing on freed memory due to
>> FT_Done_Face()...
>>
>> We should have tested the status before invoking FT_Done():
>>
>> status = cairo_font_face_set_user_data (font_face, &key,
>>                                ft_face, (cairo_destroy_func_t) FT_Done_Face);
>>
>> That means that we would have to performa that status test in the
>> finalization, and if not ready, keep a reference to both cairo_font_face
>> handle  ft_face handle
>> But then there is no other mean than storing those reference in a safe
>> place and regularly poll for readiness
>> If my understanding is correct, this is absolutely garbage collector
>> unfriendly!
>>
>>
> No total misunderstanding from my side...
> by setting the user data and destroy function, we are asking to
> auto-release the ft_face
> In which case, it's bad, because the ft_face is not ref-counted and we
> might still have another pointer on it from within Smalltalk
>
> The only way is thus to poll thru:
>
> cairo_font_face_get_reference_count (*cairo_font_face_t <https://www.cairographics.org/manual/cairo-cairo-font-face-t.html#cairo-font-face-t> *font_face*);
>
> If the count is 1, we can safely proceed with finalization.
> Else, there is still an internal reference somewhere in cairo and bang!
>
> I'd suggest to instrument the code with this function:
>
> CairoFontFace class>>countReferences: handle
> "
> unsigned int  cairo_font_face_get_reference_count
> (cairo_font_face_t *font_face);
> "
>     <primitive: #primitiveNativeCall module: #NativeBoostPlugin error:
> errorCode>
>     ^ self nbCall: #( unsigned int cairo_font_face_get_reference_count
> (size_t handle))
>
>
> CairoFontFace class>>reallyFinalizeResourceData: handle
> "
> void                cairo_font_face_destroy
> (cairo_font_face_t *font_face);
> "
>     <primitive: #primitiveNativeCall module: #NativeBoostPlugin error:
> errorCode>
>     ^ self nbCall: #( void cairo_font_face_destroy (size_t handle))
>
> CairoFontFace class>>finalizeResourceData: handle
>       (self countReferences: handle) = 1 ifFalse: [self halt: 'Houston, we
> gonna have a pointer reference problem'].
>       ^self reallyFinalizeResourceData: handle
>
> If suspicion is confirmed, then we'll have to install the ref_count
> polling mechanism...
>
>
>
>>>
>>>>> Stack backtrace:
>>>>>     [7791E43E] RtlInitializeGenericTable + 0x196 in ntdll.dll
>>>>>     [7791E0A3] RtlGetCompressionWorkSpaceSize + 0x7e in ntdll.dll
>>>>>     [751F98CD] free + 0x39 in msvcrt.dll
>>>>>     [6CD60D43] git_tree_cache_write + 0x2ac in libgit2.dll
>>>>>     [6CD62073] git_tree__free + 0x53 in libgit2.dll
>>>>>     [6CD1A563] git_object__free + 0x52 in libgit2.dll
>>>>>     [6CCD0D78] git_cached_obj_decref + 0x4c in libgit2.dll
>>>>>     [6CD1A7D9] git_object_free + 0x17 in libgit2.dll
>>>>>     [6CD1B0D3] git_tree_free + 0x11 in libgit2.dll
>>>>>     [6CD0BE4F] git_iterator_for_nothing + 0x8aa in libgit2.dll
>>>>>     [6CD0C053] git_iterator_for_nothing + 0xaae in libgit2.dll
>>>>>     [6CCEADEF] git_diff_file_content__clear + 0x31d in libgit2.dll
>>>>>     [6CCECC3F] git_diff__oid_for_entry + 0xc29 in libgit2.dll
>>>>>     [6CCED2B2] git_diff__oid_for_entry + 0x129c in libgit2.dll
>>>>>     [6CCED495] git_diff__from_iterators + 0x1db in libgit2.dll
>>>>>     [6CCED6DE] git_diff_tree_to_tree + 0x1e3 in libgit2.dll
>>>>>     [004DE7C8] ??? + 0xde7c8 in Pharo.exe
>>>>>     [0044FE08] ??? + 0x4fe08 in Pharo.exe
>>>>>     [004516A7] ??? + 0x516a7 in Pharo.exe
>>>>>     [00446051] ??? + 0x46051 in Pharo.exe
>>>>>     [0049936E] ??? + 0x9936e in Pharo.exe
>>>>>
>>>>>
>>>>> Smalltalk stack dump:
>>>>>   0xafa86c I LGitDiff>diff_tree_to_tree:repo:old_tree:new_tree:opts:
>>>>> 0xe585410: a(n) LGitDiff
>>>>>   0xafa8a4 M [] in LGitDiff>diffTree:toTree:options: 0xe585410: a(n)
>>>>> LGitDiff
>>>>>   0xafa8bc M LGitDiff(LGitExternalObject)>withReturnHandlerDo:
>>>>> 0xe585410: a(n) LGitDiff
>>>>>   0xafc678 I LGitDiff>diffTree:toTree:options: 0xe585410: a(n)
>>>>> LGitDiff
>>>>>   0xafc6a4 I LGitDiff>diffTree:toTree: 0xe585410: a(n) LGitDiff
>>>>>   0xafc6d0 I LGitTree>diffTo: 0xe583e00: a(n) LGitTree
>>>>>   0xafc6fc M [] in IceLibgitLocalRepository>changedFilesBetween:and:
>>>>> 0x1055afc0: a(n) IceLibgitLocalRepository
>>>>>   0xafc720 M [] in IceLibgitLocalRepository>withRepoDo: 0x1055afc0:
>>>>> a(n) IceLibgitLocalRepository
>>>>>   0xafc73c M [] in LGitGlobal class>runSequence: 0xfb96188: a(n)
>>>>> LGitGlobal class
>>>>>   0xafc760 M [] in LGitActionSequence(DynamicVariable)>value:during:
>>>>> 0x102109f8: a(n) LGitActionSequence
>>>>>   0xafc780 M BlockClosure>ensure: 0xe582890: a(n) BlockClosure
>>>>>   0xafc7ac I LGitActionSequence(DynamicVariable)>value:during:
>>>>> 0x102109f8: a(n) LGitActionSequence
>>>>>   0xafc7cc M LGitActionSequence class(DynamicVariable
>>>>> class)>value:during: 0xfbb81e0: a(n) LGitActionSequence class
>>>>>   0xafc7f4 I LGitGlobal class>runSequence: 0xfb96188: a(n) LGitGlobal
>>>>> class
>>>>>   0xafc818 I IceLibgitLocalRepository>withRepoDo: 0x1055afc0: a(n)
>>>>> IceLibgitLocalRepository
>>>>>   0xafc840 I IceLibgitLocalRepository>changedFilesBetween:and:
>>>>> 0x1055afc0: a(n) IceLibgitLocalRepository
>>>>>   0xafc874 I IceCommitInfo>changedPackagesToCommitInfo: 0x113b80e0:
>>>>> a(n) IceCommitInfo
>>>>>   0xafc898 I IceCommitInfo>changedPackagesTo: 0x113b80e0: a(n)
>>>>> IceCommitInfo
>>>>>   0xafc8c0 I IceDiff>initialElements 0xe4c48f8: a(n) IceDiff
>>>>>   0xaf9664 I IceDiff(IceAbstractDiff)>elements 0xe4c48f8: a(n) IceDiff
>>>>>   0xaf9684 I IceDiffChangeTreeBuilder>elements 0xe4b9c80: a(n)
>>>>> IceDiffChangeTreeBuilder
>>>>>   0xaf969c M [] in IceDiffChangeTreeBuilder>buildOn: 0xe4b9c80: a(n)
>>>>> IceDiffChangeTreeBuilder
>>>>>
>>>>> Dimitris:
>>>>>
>>>>> I won't argument, I've learnt C in 1987, so it gave me enough time to
>>>>> learn my own limits.
>>>>> Working with pointers is like carrying a gun without engaging the
>>>>> safety catch.
>>>>> I came to think that shooting own foot was a feature ;)
>>>>>
>>>>> 2017-11-06 11:04 GMT+01:00 Dimitris Chloupis <kilon.alios at gmail.com>:
>>>>>
>>>>>> Its the usual case of not being able to have your cake and eat it too.
>>>>>>
>>>>>> If you want top performance you have to manage memory yourself plus
>>>>>> the abilitiy to access thousands of C libraries is not such a bad excuse
>>>>>> for a compromise. The FFI is not a problem is a solution to many problems
>>>>>> and people using it its not as if Smalltalk offers them any alternative
>>>>>> choice.
>>>>>>
>>>>>> Not to forget that Slang itself relies heavily on C, which is only
>>>>>> the core of the VM and the very core of the implementation.
>>>>>>
>>>>>> Understanding how to work with pointers in C is pretty much
>>>>>> understanding how to works with Objects in Smalltalk. Both are nuclear
>>>>>> weapons that those two languages are build around. If ones does not
>>>>>> understand their usage he will shoot his foot in the end.
>>>>>>
>>>>>> The important thing to remember is that C's goal is not the same as
>>>>>> of Smalltalk. Its not there to hold your hand and make coding easy for you.
>>>>>> C is there to offer low level access combined with top performance. It may
>>>>>> have started as a general purpose language decades ago when coding in
>>>>>> Assembly was still a pleasant experience. Nowdays C has completely replaced
>>>>>> Assembly as the top performance language for low level coding.
>>>>>>
>>>>>> C may appear as a problematic language to a Smalltalker but only
>>>>>> because he sees it from the Smalltalk point of view. The harsh reality of
>>>>>> the world is that as much as one may want to shoehorn it , not everything
>>>>>> can be elegantly mapped to a object. Smalltalk may be OO to the bone , but
>>>>>> the world we live in, cannot afford such simple structures to accomodate of
>>>>>> varied immense complexity.
>>>>>>
>>>>>> On the subject of pointers, the general rule of thumb is to keep
>>>>>> things as simple as possible and avoide trying to do weird "magic" with
>>>>>> them. There is a ton of things that C does under the hood to generate
>>>>>> highly optimised machine code that can fry the brain , as the usual case
>>>>>> with low level coding,  so keeping it simple is the way to go.
>>>>>>
>>>>>> Oh and dont try to shoehorn the Live coding enviroment in debugging C
>>>>>> code, as much as one may want to brag of Smalltalk's elegant debugger, C
>>>>>> development tools are light years ahead in dealing with C problems.
>>>>>>
>>>>>> May advice to people is that if you do it via FFI first, you do it
>>>>>> wrong.
>>>>>>
>>>>>> Do it always first with C with a powerful C IDE like Visual Studio,
>>>>>> make sure your code works there and then use the UFFI. Will make life
>>>>>> thousand times easier. I learned that the hard way when I was playing
>>>>>> around with Pharo and shared memory.
>>>>>>
>>>>>> So yes having a FFI that does not help you avoid coding in C first,
>>>>>> is a big plus, not a minus. Sometimes it makes sense to live outside the
>>>>>> image, this is an excellent case to prove why that is a great idea. .
>>>>>>
>>>>>> On Mon, Nov 6, 2017 at 11:10 AM Nicolas Cellier <
>>>>>> nicolas.cellier.aka.nice at gmail.com> wrote:
>>>>>>
>>>>>>> Hi Ben,
>>>>>>> It's a super bad idea to copy an ExternalAddress.
>>>>>>> It's common knowledge in C++ copy operator & copy constructors...
>>>>>>>
>>>>>>> But it's not obvious to me that you'll have double freeing (unless
>>>>>>> you explicitely free the pointer by yourself).
>>>>>>> If you use gcallocate: then only the original is registered for
>>>>>>> magical auto-deallocation at garbage collection...
>>>>>>>
>>>>>>> What you will have is more somthing like dangling pointer: continue
>>>>>>> to use pointer xa2->a1 when a1 was already freed.
>>>>>>>
>>>>>>> FFI is great, it introduces the problem of C in Smalltalk, augmented
>>>>>>> with the problems of wrapping C in Smalltalk.
>>>>>>>
>>>>>>>
>>>>>>> 2017-11-06 4:23 GMT+01:00 Ben Coman <btc at openinworld.com>:
>>>>>>>
>>>>>>>> My current employment work hours and roster have severely curtailed
>>>>>>>> the time I have hacking Pharo, so I've not dug enough to be sure of my
>>>>>>>> observations a few months ago, and this is from memory, but I was starting
>>>>>>>> to develop a suspicion about the uniqueness of ExternalAddress(s).
>>>>>>>>
>>>>>>>> A while ago, in order to fix some stability issues on Windows, a
>>>>>>>> guard was added somewhere that slowed down some operations.  Looking into
>>>>>>>> this and experimenting with removing the guard I seem to remember VM
>>>>>>>> crashes due to a double-free() of an address, due to there being two
>>>>>>>> ExternalAddresses holding the same external address.
>>>>>>>>
>>>>>>>> My intuition is that that somewhere an ExternalAddress(a1) pointing
>>>>>>>> at a particular external resource address "xa1" was being copied, so we end
>>>>>>>> up with ExternalAddress(a2) also pointing at "xa1", with and object b1
>>>>>>>> holding a1 and object b2 holding a2.  During finalization of b1,
>>>>>>>> ExternalAddress a1 free()d xa1, and a1 was flagged to avoid
>>>>>>>> double-free()ing.  But that didn't help when b2 was finalized, since a2 had
>>>>>>>> no indication that xa1 had been free()d.
>>>>>>>>
>>>>>>>> That is...
>>>>>>>> b1-->a1-->xa1
>>>>>>>> b2 := b1 copy.
>>>>>>>> b2-->a2-->xa1
>>>>>>>> b1 finalize a1 --> free(xa1)
>>>>>>>> b2 finalize a2 --> free(xa1) --> General Protection Fault
>>>>>>>>
>>>>>>>> It was hard to follow this through and I didn't succeed in tracking
>>>>>>>> down where such a copy might have been made, but the idea simmering in my
>>>>>>>> mind since then is to propose that...
>>>>>>>>
>>>>>>>>     ExternalAddresses be unique in the image and behave like
>>>>>>>> Symbols,
>>>>>>>>     such that trying to copy one returns the identical object.
>>>>>>>>
>>>>>>>> The idea being that when b2 is finalized, a1 would notice that xa1
>>>>>>>> had already been free()d and raise a Smalltalk exception rather than a
>>>>>>>> general protection fault.
>>>>>>>> b1-->a1-->xa1
>>>>>>>> b2 := b1 copy.
>>>>>>>> b2-->a1-->xa1
>>>>>>>>          ^^
>>>>>>>> b1 finalize a1 --> free(xa1)
>>>>>>>> b2 finalize a1 --> Smalltalk exception
>>>>>>>>
>>>>>>>>
>>>>>>>> I write now in response to Stef since I vaguely remember it being
>>>>>>>> Freetype related.  But I also remember the issue being FFI related and
>>>>>>>> Freetype is a plugin not FFI.  So I'm not sure my memory is clear and
>>>>>>>> perhaps I have the "wrong end of the stick" but anyway, rather than hold
>>>>>>>> back longer because of that, perhaps this can stimulate some discussion and
>>>>>>>> at least I learn something to clarify my understanding here.
>>>>>>>>
>>>>>>>> cheers -ben
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, Oct 28, 2017 at 4:48 PM, Stephane Ducasse <
>>>>>>>> stepharo.self at gmail.com> wrote:
>>>>>>>> >
>>>>>>>> > Hi all
>>>>>>>> >
>>>>>>>> > I'm and I guess many of you are fedup about the instability that
>>>>>>>> the
>>>>>>>> > FreeType plugin produces.
>>>>>>>> >
>>>>>>>> > So we need help because clement and esteban are fully booked.
>>>>>>>> >
>>>>>>>> > We have three options:
>>>>>>>> >
>>>>>>>> > - drop Freetype alltogether
>>>>>>>> > - rewrite the plugin
>>>>>>>> > - create a binding using raffaillac sketch
>>>>>>>> >
>>>>>>>> > Now we need help. Who is willing to help us?
>>>>>>>> > Should we try to set up a bounty?
>>>>>>>> >
>>>>>>>> > Stef
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/attachments/20171109/e2e3d4de/attachment-0002.html>


More information about the Pharo-dev mailing list