ImageMagick-6.9.3 reuses user lock after freed

[magick]

Thanks for your efforts. Threading issues are always difficult to debug and we want to ensure the problem is not embedded in ImageMagick.

[jhowarth]

The OpenMP developer confirmed that these failures are only seen on darwin.

[jhowarth]

It is interesting that these regressions occur in the Magick++ test suite and that a difference exists between master and ImageMagick-6 in that the latter has the following code in Magick++/lib/Functions.cpp...

Code: Select all

//
// Create a local wrapper around MagickCoreTerminus
//
namespace Magick
{
  extern "C" {
    void MagickPlusPlusDestroyMagick(void);
  }
}

void Magick::MagickPlusPlusDestroyMagick(void)
{
  if (magick_initialized)
    {
      magick_initialized=false;
      MagickCore::MagickCoreTerminus();
    }
}

//
// Cleanup class to ensure that ImageMagick singletons are destroyed
// so as to avoid any resemblence to a memory leak (which seems to
// confuse users)
//
namespace Magick
{
  class MagickCleanUp
  {
  public:

    MagickCleanUp(void);
    ~MagickCleanUp(void);
  };

  // The destructor for this object is invoked when the destructors for
  // static objects in this translation unit are invoked.
  static MagickCleanUp magickCleanUpGuard;
}

Magick::MagickCleanUp::MagickCleanUp(void)
{
  // Don't even think about invoking InitializeMagick here!
}

Magick::MagickCleanUp::~MagickCleanUp(void)
{
  MagickPlusPlusDestroyMagick();
}

Could master be less aggressive about calling MagickCoreTerminus than ImageMagick-6 because of those changes?

[jhowarth]

I am finding that a back port of the following change from master into ImageMagick-6 branch eliminates the test suite failures when built with clang/libomp 3.8svn on x86_64-apple-darwin15...

Code: Select all

--- ImageMagick-6.9.3-3/Magick++/lib/Functions.cpp      2016-01-23 05:17:00.000000000 -0500
+++ ImageMagick-master/Magick++/lib/Functions.cpp       2016-01-23 14:22:10.000000000 -0500
@@ -1,7 +1,7 @@
 // This may look like C code, but it is really -*- C++ -*-
 //
 // Copyright Bob Friesenhahn, 1999, 2002, 2003
-// Copyright Dirk Lemstra 2014
+// Copyright Dirk Lemstra 2014-2015
 //
 // Simple C++ function wrappers for ImageMagick equivalents
 //
@@ -19,7 +19,8 @@
 
 static bool magick_initialized=false;
 
-void Magick::CloneString(char **destination_,const std::string &source_)
+// Clone C++ string as allocated C string, de-allocating any existing string
+void Magick::CloneString(char **destination_, const std::string &source_)
 {
   MagickCore::CloneString(destination_,source_.c_str());
 }
@@ -62,51 +63,11 @@
   MagickCore::SetRandomSecretKey(seed);
 }
 
-//
-// Create a local wrapper around MagickCoreTerminus
-//
-namespace Magick
-{
-  extern "C" {
-    void MagickPlusPlusDestroyMagick(void);
-  }
-}
-
-void Magick::MagickPlusPlusDestroyMagick(void)
+MagickPPExport void Magick::TerminateMagick(void)
 {
   if (magick_initialized)
     {
       magick_initialized=false;
       MagickCore::MagickCoreTerminus();
     }
-}
-
-//
-// Cleanup class to ensure that ImageMagick singletons are destroyed
-// so as to avoid any resemblence to a memory leak (which seems to
-// confuse users)
-//
-namespace Magick
-{
-  class MagickCleanUp
-  {
-  public:
-
-    MagickCleanUp(void);
-    ~MagickCleanUp(void);
-  };
-
-  // The destructor for this object is invoked when the destructors for
-  // static objects in this translation unit are invoked.
-  static MagickCleanUp magickCleanUpGuard;
-}
-
-Magick::MagickCleanUp::MagickCleanUp(void)
-{
-  // Don't even think about invoking InitializeMagick here!
-}
-
-Magick::MagickCleanUp::~MagickCleanUp(void)
-{
-  MagickPlusPlusDestroyMagick();
-}
+}
\ No newline at end of file
--- ImageMagick-6.9.3-3/Magick++/lib/Magick++/Functions.h       2016-01-23 05:17:00.000000000 -0500
+++ ImageMagick-master/Magick++/lib/Magick++/Functions.h        2016-01-23 14:22:10.000000000 -0500
@@ -30,5 +30,8 @@
 
   // Seed a new sequence of pseudo-random numbers
   MagickPPExport void SetRandomSeed(const unsigned long seed);
+
+  // C library initialization routine
+  MagickPPExport void TerminateMagick();
 }
 #endif // Magick_Functions_header
 

[jhowarth]

Also confirmed on a second machine under x86_64-apple-darwin13 that back porting these changes in Magick++/lib/Functions.cpp and Magick++/lib/Magick++/Functions.h from master eliminates the test suite regressions in mageMagick-6 branch when it is built with clang/libomp 3.8svn.

[magick]

Thanks, we forwarded you analysis to the Magick++ maintainer. Give him a few days to respond.

[jhowarth]

Note for the current code in ImageMagick-6 branch , the OpenMP developer's analysis was...

Looks like the culprit to the finalization problem. Again, as a final indication that this is some odd Darwin phenomenon:

You can see that:
1) __kmp_cleanup_indirect_user_locks is called first (shutdown of libomp)
2) MagickCoreTerminus() is called second

(lldb) b __kmp_cleanup_indirect_user_locks
Breakpoint 2: where = libiomp5.dylib`__kmp_cleanup_indirect_user_locks, address = 0x00000001009b4b90
(lldb) b MagickCoreTerminus
Breakpoint 3: where = libMagickCore-6.Q16.2.dylib`MagickCoreTerminus + 1 at semaphore-private.h:54, address = 0x000000010045d511
(lldb) c
Process 58875 resuming
Process 58875 stopped
* thread #1: tid = 0x170edbfc, 0x00000001009b4b90 libiomp5.dylib`__kmp_cleanup_indirect_user_locks, queue = 'com.apple.main-thread, stop reason = breakpoint 2.1
frame #0: 0x00000001009b4b90 libiomp5.dylib`__kmp_cleanup_indirect_user_locks
libiomp5.dylib`__kmp_cleanup_indirect_user_locks:
-> 0x1009b4b90: pushq %r14
0x1009b4b92: pushq %r15
0x1009b4b94: pushq %rbx
0x1009b4b95: pushq %rbp
(lldb) c
Process 58875 resuming
Process 58875 stopped
* thread #1: tid = 0x170edbfc, 0x000000010045d511 libMagickCore-6.Q16.2.dylib`MagickCoreTerminus + 1 at semaphore-private.h:54, queue = 'com.apple.main-thread, stop reason = breakpoint 3.1
frame #0: 0x000000010045d511 libMagickCore-6.Q16.2.dylib`MagickCoreTerminus + 1 at semaphore-private.h:54
51 static inline void InitializeMagickMutex(void)
52 {
53 #if defined(MAGICKCORE_OPENMP_SUPPORT)
-> 54 if (active_mutex == MagickFalse)
55 omp_init_lock(&semaphore_mutex);
56 #endif
57 active_mutex=MagickTrue;
(lldb) c
Process 58875 resuming
Process 58875 stopped
* thread #1: tid = 0x170edbfc, 0x00000001009ae9ed libiomp5.dylib`__kmp_acquire_ticket_lock + 13, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
frame #0: 0x00000001009ae9ed libiomp5.dylib`__kmp_acquire_ticket_lock + 13
libiomp5.dylib`__kmp_acquire_ticket_lock + 13:
-> 0x1009ae9ed: lock
0x1009ae9ee: xaddl %ecx, (%rdi)
0x1009ae9f1: movl 20(%rbp), %eax
0x1009ae9f4: cmpl %ecx, %eax
(lldb)

Here is the exact way __kmp_cleanup is being called (using debug library):

(lldb) bt
* thread #1: tid = 0x17133ab9, 0x00000001009b0de6 libiomp5.dylib`__kmp_cleanup + 8 at kmp_runtime.c:7414, queue = 'com.apple.main-thread, stop reason = breakpoint 2.1
frame #0: 0x00000001009b0de6 libiomp5.dylib`__kmp_cleanup + 8 at kmp_runtime.c:7414
frame #1: 0x00000001009adf44 libiomp5.dylib`__kmp_internal_end() + 773 at kmp_runtime.c:6019
frame #2: 0x00000001009ae2a1 libiomp5.dylib`__kmp_internal_end_library(gtid_req=-1) + 859 at kmp_runtime.c:6111
frame #3: 0x00000001009ad8cf libiomp5.dylib`__kmp_internal_end_atexit + 50 at kmp_runtime.c:5778
frame #4: 0x00000001009ad890 libiomp5.dylib`__kmp_internal_end_dtor + 9 at kmp_runtime.c:5741
frame #5: 0x00007fff5fc11ef5 dyld`ImageLoaderMachO::doTermination(ImageLoader::LinkContext const&) + 215
frame #6: 0x00007fff5fc02279 dyld`dyld::runTerminators(void*) + 69
frame #7: 0x00007fff940ea7a1 libsystem_c.dylib`__cxa_finalize + 177
frame #8: 0x00007fff940eaa4c libsystem_c.dylib`exit + 22
frame #9: 0x00007fff97e3c604 libdyld.dylib`start + 8

It is being shut down by the application. I can't help with this problem because it isn't a bug in libomp. Or, I'm not convinced it is a bug in libomp. The initial finding that ImageMagick is using omp_locks in its finalization code after libomp has been shut down is still the issue.

The only pthread_create() calls are coming from within libomp so I don't think there are sibling threads. Even when I forget to set SRCDIR to the input file the test needs (appendImages) and it just says "Can't find input file" and exits, it STILL segfaults (no threads created at all) because of the initial finding.

It almost seems like a dynamic linker issue where the library that is depended on (libomp) is being shut down before the library that depends on it (ImageMagick).

[jhowarth]

Also confirmed that back porting the changes in Magick++/lib/Functions.cpp and Magick++/lib/Magick++/Functions.h from master eliminates the test suite regressions in mageMagick-6 branch when it is built with clang/libomp 3.9svn (llvm trunk) on x86_64-apple-darwin15.

[jhowarth]

Also opened https://github.com/ImageMagick/ImageMagick/issues/100 as I assume that is what the ImageMagick developers use in place of bugzilla for tracking bugs.

[jhowarth]

The llvm openmp developer also confirmed that back porting the changes in Magick++/lib/Functions.cpp and Magick++/lib/Magick++/Functions.h from master eliminates the test suite regressions for him as well on darwin.

[magick]

Well done. Look for a patch within the next day or two.

[fmw42]

Hello jhowarth,

I am trying to help someone install Imagemagick on OSX El Capitan with OpenMP enabled. But OpenMP never gets enabled. I understand that this may be due to APPLE changing its compiler from gcc to llvm. Have you had success installing Imagemagick with OpenMP enabled? If so, could you provide some pointers how to do that. I have installed all needed delegates using MacPorts and then install Imagemagick from source according to viewtopic.php?f=1&t=21502&p=88202&hilit ... rts#p88202

Thanks for any information.

Fred