[poppler] XRef::fetch mutex lock

Albert Astals Cid aacid at kde.org
Sat Apr 6 09:25:49 PDT 2013 

El Dissabte, 6 d'abril de 2013, a les 18:03:55, Adam Reichold va escriure:
> Hello,
> 
> Am 06.04.2013 18:00, schrieb Adam Reichold:
> > Hello,
> > 
> > Am 06.04.2013 17:45, schrieb Adam Reichold:
> >> Hello again,
> >> 
> >> Am 06.04.2013 17:40, schrieb Adam Reichold:
> >>> Hello,
> >>> 
> >>> Am 06.04.2013 17:05, schrieb Albert Astals Cid:
> >>>> El Divendres, 5 d'abril de 2013, a les 17:30:49, Adam Reichold va 
escriure:
> >>>>> Hello,
> >>>>> 
> >>>>> Am 05.04.2013 00:38, schrieb Albert Astals Cid:
> >>>>>> El Dimecres, 3 d'abril de 2013, a les 19:32:23, Albert Astals Cid va
> >>>> 
> >>>> escriure:
> >>>>>>> El Dimecres, 3 d'abril de 2013, a les 08:00:34, Thomas Freitag va
> >>>> 
> >>>> escriure:
> >>>>>>>> Am 03.04.2013 00:01, schrieb Albert Astals Cid:
> >>>>>>>>> El Dimarts, 2 d'abril de 2013, a les 10:36:53, Thomas Freitag va
> >>>>>> 
> >>>>>> escriure:
> >>>>>>>>>> Am 02.04.2013 09:22, schrieb Thomas Freitag:
> >>>>>>>>>>> Hi Albert!
> >>>>>>>>>>> 
> >>>>>>>>>>> Am 27.03.2013 12:29, schrieb Thomas Freitag:
> >>>>>>>>>>>> Sorry, pushed the wrong button, here my answer to the list:
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Am 27.03.2013 11:41, schrieb Albert Astals Cid:
> >>>>>>>>>>>>> Why do we pass around the recursion integer around?
> >>>>>>>>>>>> 
> >>>>>>>>>>>> No idea: The recursive integer wasn't introduced by the thread
> >>>>>>>>>>>> safe
> >>>>>>>>>>>> patch, it was already there. I just used it and extend some
> >>>>>>>>>>>> functions
> >>>>>>>>>>>> which incorrectly ignore that parameter instead of passing it
> >>>>>>>>>>>> to the
> >>>>>>>>>>>> called functions. Probably I missed some more than only the one
> >>>>>>>>>>>> in
> >>>>>>>>>>>> DCTStream::DCTStream.
> >>>>>>>>>>> 
> >>>>>>>>>>> I just ran a regression test over my PDF files, but I didn't get
> >>>>>>>>>>> any
> >>>>>>>>>>> dead locks. So if You want me to solve that problem, You should
> >>>>>>>>>>> provide me that PDF. (Regardless if You prefer to use recursive
> >>>>>>>>>>> mutex:
> >>>>>>>>>>> the endless loop detection need to pass the recursive integer
> >>>>>>>>>>> everywhere!)
> >>>>>>>>>> 
> >>>>>>>>>> Attached a patch which corrects it. I made it in my sleep, 'cause
> >>>>>>>>>> the
> >>>>>>>>>> dead lock was probably caused by my patch for bug 61994 (even if
> >>>>>>>>>> that
> >>>>>>>>>> PDF doesn't cause any dead locks). This time I also respect the
> >>>>>>>>>> DCTStream usage if LIBJPEG is disabled :-)
> >>>>>>>>> 
> >>>>>>>>> Sure, this fixes the issue (and we should commit it to help with
> >>>>>>>>> the
> >>>>>>>>> infinite recursion issue), but what's your opinion on making our
> >>>>>>>>> mutexes
> >>>>>>>>> recursive?
> >>>>>>>> 
> >>>>>>>> I think, a quite good discussion about recursive and non recursive
> >>>>>>>> mutes
> >>>>>>>> was here:
> >>>>>>>> http://stackoverflow.com/questions/187761/recursive-lock-mutex-vs-n
> >>>>>>>> on-re
> >>>>>>>> cu
> >>>>>>>> rs ive-lock-mutex. This convinced me to vote for a usage of
> >>>>>>>> recursive
> >>>>>>>> mutex in poppler: 1. xpdf and therfore poppler wasn't really
> >>>>>>>> designed
> >>>>>>>> for
> >>>>>>>> the usage of threads. We have it now working, but if we cherish on
> >>>>>>>> the
> >>>>>>>> usage of non recursive mutex we will always run into dead lock
> >>>>>>>> problems
> >>>>>>>> with futur patches again, so testing patches will become more
> >>>>>>>> expensive.
> >>>>>>>> 2. Since poppler nowhere uses emergency exits with "throw", we
> >>>>>>>> don't
> >>>>>>>> have the problem that we have to keep track that a thread releases
> >>>>>>>> a
> >>>>>>>> mutex so often it aquires it.
> >>>>>>>> 3. The only (small) problem I see is the sentence "Not all systems
> >>>>>>>> supporting pthreads also support recursive mutexes, but if they
> >>>>>>>> want to
> >>>>>>>> be POSIX conform, they have to
> >>>>>>>> <http://www.opengroup.org/onlinepubs/009695399/functions/pthread_mu
> >>>>>>>> texat
> >>>>>>>> tr
> >>>>>>>> _g ettype.html>."
> >>>>>>> 
> >>>>>>> To be honest i don't think we should care much about the minority of
> >>>>>>> systems there that don't support recursive mutexes.
> >>>>>> 
> >>>>>> Here's a quick patch I've hacked together (still needs some work in
> >>>>>> the
> >>>>>> buildsystem side).
> >>>>> 
> >>>>> Not something that breaks either, but maybe a performance improvement:
> >>>>> According to the manual page of 'phtread_mutexattr_init', a single
> >>>>> mutex
> >>>>> attribute object can be used to initialize several mutexes, hence we
> >>>>> possibly don't need to keep it in the GooLock structure and could use
> >>>>> just a single instance that is initialized once. Not sure whether this
> >>>>> really hurts with the number of mutexes we have.
> >>>> 
> >>>> I tried doing that but then i needed a "global" mutex attribute that i
> >>>> needed to initialize, and that meant i needed to include a mutex to
> >>>> protect the initialization of the attribute or "force" all the mutexes
> >>>> to be created in the same thread. At the end i went the "easy" way of
> >>>> having one mutex attribute per mutex, it's not like we are going to
> >>>> use that much more memory and it's just easier to understand codewise.
> >>> 
> >>> I see and I agree that it is probably simpler that way. But then I think
> >>> there should be a call to 'pthread_mutexattr_destroy' in
> >>> 'gDestroyMutex', shouldn't there?
> >> 
> >> Sorry, just noticed that you already included it in the commit...
> > 
> > Again I am sorry (must be the weekend), but after reading the POSIX
> > programmer's manual page again, I think the 'mutexattr' object does not
> > need to survive the mutex itself, e.g.
> > 
> > typedef pthread_mutex_t GooMutex;
> > 
> > inline void gInitMutex(GooMutex *m) {
> > 
> >   pthread_mutexattr_t mutexattr;
> >   pthread_mutexattr_init(&mutexattr);
> >   pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_RECURSIVE);
> >   pthread_mutex_init(&m, &mutexattr);
> > 
> > }
> 
> Of course, include the destruction call, i.e.
> 
> inline void gInitMutex(GooMutex *m) {
>   pthread_mutexattr_t mutexattr;
>   pthread_mutexattr_init(&mutexattr);
>   pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_RECURSIVE);
>   pthread_mutex_init(&m, &mutexattr);
>   pthread_mutexattr_destroy(&mutexattr);
> }

Makes sense.

Pushed.

Cheers,
  Albert

> 
> > should work as well.
> > 
> > Best regards, Adam.
> > 
> > P.S.: At least this is how I would interpret
> > 
> > After a mutex attributes object has been used to initialize one or more
> > mutexes,  any  function  affecting  the  attributes  object  (including
> > destruction) shall not affect any previously initialized mutexes.
> > 
> > from the manual page.
> > 
> >>>> Cheers,
> >>>> 
> >>>>   Albert
> >>>> 
> >>>> P.S: I had to add some -lpthread to the cmake buildsystem but not to
> >>>> the
> >>>> automake one, find it a bit weird to be honest but it's what i needed
> >>>> to make it compile in both.
> >>> 
> >>> Yes, sounds strange. Maybe the 'ACX_PTHREAD' macro has some side effects
> >>> w.r.t. that?
> >>> 
> >>> Best regards, Adam.
> >>> 
> >>>>> Best regards, Adam.
> >>>>> 
> >>>>>> I'll finish it properly tomorrow but meanwhile can you guys check if
> >>>>>> it
> >>>>>> breaks something related to the multithreading side?
> >>>>>> 
> >>>>>> I haven't touched the windows side since somewhere i've read that
> >>>>>> CriticalSections are already recursive. Can anyone confirm?
> >>>>>> 
> >>>>>> Cheers,
> >>>>>> 
> >>>>>>   Albert
> >>>>>>> 
> >>>>>>> Cheers,
> >>>>>>> 
> >>>>>>>   Albert
> >>>>>>>> 
> >>>>>>>> Cheers,
> >>>>>>>> Thomas
> >>>>>>>> 
> >>>>>>>>> It should help us making the code simpler since we could just drop
> >>>>>>>>> all
> >>>>>>>>> the
> >>>>>>>>> DoNotLockMutex cases, no?
> >>>>>>>>> 
> >>>>>>>>> Cheers,
> >>>>>>>>> 
> >>>>>>>>>    Albert
> >>>>>>>>>> 
> >>>>>>>>>> Cheers,
> >>>>>>>>>> Thomas
> >>>>>>>>>> 
> >>>>>>>>>>> Cheers,
> >>>>>>>>>>> Thomas
> >>>>>>>>>>> 
> >>>>>>>>>>>>> Wouldn't a real recursive mutex be enough?
> >>>>>>>>>>>> 
> >>>>>>>>>>>> No idea: I used in the thread safe patch just the pthread_lock
> >>>>>>>>>>>> which
> >>>>>>>>>>>> was defined. I'm not knowing what happens when we change it
> >>>>>>>>>>>> globally
> >>>>>>>>>>>> i.e. if it affects other usages of the GooMutex. Just feel free
> >>>>>>>>>>>> to
> >>>>>>>>>>>> change it, You should have the better overview of usages than I
> >>>>>>>>>>>> have.
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Cheers,
> >>>>>>>>>>>> Thomas
> >>>>>>>>>>>> 
> >>>>>>>>>>>>> I'm asking because i have a document here (that sadly i can't
> >>>>>>>>>>>>> share)
> >>>>>>>>>>>>> that is deadlocking
> >>>>>>>>>>>>> itself there because we are not passing the recursion integer
> >>>>>>>>>>>>> everywhere (we lose it in DCTStream::DCTStream)
> >>>>>>>>>>>>> as shown in this backtrace
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> #0  __lll_lock_wait () at
> >>>>>>>>>>>>> ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> >>>>>>>>>>>>> #1  0x00007ffff599217c in _L_lock_982 () from
> >>>>>>>>>>>>> /lib/x86_64-linux-gnu/libpthread.so.0
> >>>>>>>>>>>>> #2  0x00007ffff5991fcb in __GI___pthread_mutex_lock
> >>>>>>>>>>>>> (mutex=0x71dc10)
> >>>>>>>>>>>>> at pthread_mutex_lock.c:64
> >>>>>>>>>>>>> #3  0x00007ffff5f02f73 in MutexLocker::MutexLocker
> >>>>>>>>>>>>> (this=0x7fffffffcfb0, mutexA=0x71dc10, modeA=DoLockMutex) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/goo/GooMutex.h:72
> >>>>>>>>>>>>> #4  0x00007ffff5fac8e2 in XRef::fetch (this=0x71db50, num=5,
> >>>>>>>>>>>>> gen=0,
> >>>>>>>>>>>>> obj=0x7fffffffd0b0, recursion=0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1137
> >>>>>>>>>>>>> #5  0x00007ffff5f8a013 in Object::fetch (this=0x71fe08,
> >>>>>>>>>>>>> xref=0x71db50, obj=0x7fffffffd0b0, recursion=0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122
> >>>>>>>>>>>>> #6  0x00007ffff5f19de4 in Dict::lookup (this=0x71fb60,
> >>>>>>>>>>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0)
> >>>>>>>>>>>>> at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256
> >>>>>>>>>>>>> #7  0x00007ffff5f032e9 in Object::dictLookup
> >>>>>>>>>>>>> (this=0x7fffffffd5a0,
> >>>>>>>>>>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0)
> >>>>>>>>>>>>> at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315
> >>>>>>>>>>>>> #8  0x00007ffff601f8a3 in DCTStream::DCTStream (this=0x7832b0,
> >>>>>>>>>>>>> strA=0x720780, colorXformA=-1, dict=0x7fffffffd5a0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/DCTStream.cc:72
> >>>>>>>>>>>>> #9  0x00007ffff5f9bf6d in Stream::makeFilter (this=0x720780,
> >>>>>>>>>>>>> name=0x71fef0 "DCTDecode", str=0x720780,
> >>>>>>>>>>>>> params=0x7fffffffd1e0,
> >>>>>>>>>>>>> recursion=1, dict=0x7fffffffd5a0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:311
> >>>>>>>>>>>>> #10 0x00007ffff5f9b71a in Stream::addFilters (this=0x720780,
> >>>>>>>>>>>>> dict=0x7fffffffd5a0, recursion=1) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:184
> >>>>>>>>>>>>> #11 0x00007ffff5f9204b in Parser::makeStream (this=0x71fcd0,
> >>>>>>>>>>>>> dict=0x7fffffffd5a0, fileKey=0x0, encAlgorithm=cryptRC4,
> >>>>>>>>>>>>> keyLength=1146103040, objNum=3, objGen=0, recursion=1,
> >>>>>>>>>>>>> strict=false)
> >>>>>>>>>>>>> at /home/tsdgeos/devel/poppler/poppler/Parser.cc:280
> >>>>>>>>>>>>> #12 0x00007ffff5f918e6 in Parser::getObj (this=0x71fcd0,
> >>>>>>>>>>>>> obj=0x7fffffffd5a0, simpleOnly=false, fileKey=0x0,
> >>>>>>>>>>>>> encAlgorithm=cryptRC4, keyLength=1146103040, objNum=3,
> >>>>>>>>>>>>> objGen=0,
> >>>>>>>>>>>>> recursion=0, strict=false) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Parser.cc:131
> >>>>>>>>>>>>> #13 0x00007ffff5facdd3 in XRef::fetch (this=0x71db50, num=3,
> >>>>>>>>>>>>> gen=0,
> >>>>>>>>>>>>> obj=0x7fffffffd5a0, recursion=0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1197
> >>>>>>>>>>>>> #14 0x00007ffff5f8a013 in Object::fetch (this=0x71e128,
> >>>>>>>>>>>>> xref=0x71db50, obj=0x7fffffffd5a0, recursion=0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122
> >>>>>>>>>>>>> #15 0x00007ffff5f19de4 in Dict::lookup (this=0x71e660,
> >>>>>>>>>>>>> key=0x7206e0
> >>>>>>>>>>>>> "Im1", obj=0x7fffffffd5a0, recursion=0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256
> >>>>>>>>>>>>> #16 0x00007ffff5f032e9 in Object::dictLookup (this=0x71e598,
> >>>>>>>>>>>>> key=0x7206e0 "Im1", obj=0x7fffffffd5a0, recursion=0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315
> >>>>>>>>>>>>> #17 0x00007ffff5f2a647 in GfxResources::lookupXObject
> >>>>>>>>>>>>> (this=0x71e590, name=0x7206e0 "Im1", obj=0x7fffffffd5a0) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:411
> >>>>>>>>>>>>> #18 0x00007ffff5f3f41e in Gfx::opXObject (this=0x71e440,
> >>>>>>>>>>>>> args=0x7fffffffd720, numArgs=1) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:4114
> >>>>>>>>>>>>> #19 0x00007ffff5f2bfd6 in Gfx::execOp (this=0x71e440,
> >>>>>>>>>>>>> cmd=0x7fffffffd6e0, args=0x7fffffffd720, numArgs=1) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:858
> >>>>>>>>>>>>> #20 0x00007ffff5f2b82b in Gfx::go (this=0x71e440,
> >>>>>>>>>>>>> topLevel=true) at
> >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:717
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>>     Albert
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> _______________________________________________
> >>>>>>>>>>>>> poppler mailing list
> >>>>>>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> .
> >>>>>>>>>>>> 
> >>>>>>>>>>>> _______________________________________________
> >>>>>>>>>>>> poppler mailing list
> >>>>>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>>>>>>> 
> >>>>>>>>>>>> .
> >>>>>>>>>>> 
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> poppler mailing list
> >>>>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>>>>>> 
> >>>>>>>>>>> .
> >>>>>>>>> 
> >>>>>>>>> _______________________________________________
> >>>>>>>>> poppler mailing list
> >>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>>>> 
> >>>>>>>>> .
> >>>>>>> 
> >>>>>>> _______________________________________________
> >>>>>>> poppler mailing list
> >>>>>>> poppler at lists.freedesktop.org
> >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>> 
> >>>>>>> 
> >>>>>>> _______________________________________________
> >>>>>>> poppler mailing list
> >>>>>>> poppler at lists.freedesktop.org
> >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>> 
> >>>>> _______________________________________________
> >>>>> poppler mailing list
> >>>>> poppler at lists.freedesktop.org
> >>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>> 
> >>>> _______________________________________________
> >>>> poppler mailing list
> >>>> poppler at lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>> 
> >>> _______________________________________________
> >>> poppler mailing list
> >>> poppler at lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >> 
> >> _______________________________________________
> >> poppler mailing list
> >> poppler at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/poppler
> > 
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler

More information about the poppler mailing list