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 < [email protected]> 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@gmai >> l.com>: >> >>> >>> >>> 2017-11-08 14:53 GMT+01:00 Nicolas Cellier < >>> [email protected]>: >>> >>>> >>>> >>>> 2017-11-08 14:42 GMT+01:00 Nicolas Cellier < >>>> [email protected]>: >>>> >>>>> >>>>> 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 <[email protected]>: >>>>> >>>>>> 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 < >>>>>> [email protected]> 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 <[email protected]>: >>>>>>> >>>>>>>> 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 < >>>>>>>> [email protected]> 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 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>> >>> >> >
