Opened 8 years ago

Closed 10 months ago

#34231 closed defect (fixed)

ImageMagick: should not depend on pango + fix openMP

Reported by: devernay (Frédéric Devernay) Owned by: ryandesign (Ryan Schmidt)
Priority: Normal Milestone:
Component: ports Version: 2.0.4
Keywords: haspatch Cc:
Port: ImageMagick

Description (last modified by ryandesign (Ryan Schmidt))

this patch fixes two things:

  • if pango is installed, configure detects it and thus it becomes a "hidden dependency". TYhis patch makes the dependency exlicit with the +pango variant
  • OpenMP is broken in gcc 4.0 and 4.2, so disable it on these compilers.

Attachments (2)

ImageMagick-Portfile.patch (1.6 KB) - added by devernay (Frédéric Devernay) 8 years ago.
Portfile patch
test.cpp (1.0 KB) - added by devernay (Frédéric Devernay) 8 years ago.
test file from GCC bug 36242, exhibiting the OpenMP bug

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by devernay (Frédéric Devernay)

Attachment: ImageMagick-Portfile.patch added

Portfile patch

comment:1 in reply to:  description Changed 8 years ago by ryandesign (Ryan Schmidt)

Description: modified (diff)
Keywords: haspatch added
Owner: changed from macports-tickets@… to ryandesign@…

Replying to frederic.devernay@…:

  • if pango is installed, configure detects it and thus it becomes a "hidden dependency". TYhis patch makes the dependency exlicit with the +pango variant

Thanks; I had overlooked that. Committed that part in r92396 along with a minor version update (since a revision increase would have been needed otherwise, and we might as well instead pull in some upstream bug fixes while we're at it). Note that I rewrote the pango dependency so that pango-devel could satisfy it.

This also means we have to close #20923 as wontfix, which I'm quite happy to do.

  • OpenMP is broken in gcc 4.0 and 4.2, so disable it on these compilers.

That's news to me. You should probably report that to the developers of ImageMagick so that they can fix their use of OpenMP on gcc 4.2. My understanding is that ImageMagick did not attempt to use OpenMP automatically on gcc earlier than 4.2, but that it could still be added. See also #15945 and #24944 and #32642.

comment:2 Changed 8 years ago by devernay (Frédéric Devernay)

Here's the proper reference for the gcc 4.2/4.3 bug with OpenMP: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36242#c4

to cite Jakup, GCC OpenMP guru: "Jakub Jelinek 2008-06-11 20:29:19 UTC

Using OpenMP pragmas in multiple pthread_create created threads concurrently isn't supported in 4.2/4.3. There is a support for it on the trunk since a few days ago though."

gcc 4.2 is for old geezers anyway (gcc 4.2.1 was released almost five years ago!)

here's another report about that OpenMP bug in another context: http://lists.apple.com/archives/coreaudio-api/2009/May/msg00035.html

comment:3 in reply to:  2 Changed 8 years ago by ryandesign (Ryan Schmidt)

Replying to frederic.devernay@…:

Here's the proper reference for the gcc 4.2/4.3 bug with OpenMP: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36242#c4

to cite Jakup, GCC OpenMP guru:
"Jakub Jelinek 2008-06-11 20:29:19 UTC

Using OpenMP pragmas in multiple pthread_create created threads concurrently isn't supported in 4.2/4.3. There is a support for it on the trunk since a few days ago though."

And does ImageMagick use OpenMP pragmas in multiple pthread_create-created threads concurrently? What is the consequence—is there a crash? If so, can you attach a crash log? How can I reproduce the problem on my own system? I am on Snow Leopard with Xcode 3.2.6 so my ImageMagick is compiled with Apple gcc 4.2.1 so it should exhibit the problem.

gcc 4.2 is for old geezers anyway (gcc 4.2.1 was released almost five years ago!)

Apple's version of gcc 4.2.1 is the default compiler in Xcode 3.2.6, the last free version of Xcode for Snow Leopard. It's also the last version of gcc Apple will ever ship in Xcode. For these reasons it's still a relevant version of gcc for OS X users.

here's another report about that OpenMP bug in another context: http://lists.apple.com/archives/coreaudio-api/2009/May/msg00035.html

I don't think that's the same problem. They're talking about problems using OpenMP from a "time constraint thread" which "is very different from a normal thread context" and the guess is "that OpenMP has some baseline incompatibility when running in this context". The thread also says gcc 4.4 still exhibits the problem, whereas the problem you're describing should be fixed by that version.

If there is really a problem with OpenMP when compiled by gcc 4.2.1 and earlier, then in light of the requests in #15945 and #24944 to provide options to enable or disable OpenMP at install time, what we should do is add an openmp variant, make it on by default when the compiler is clang or llvm-gcc-4.2, and off by default otherwise. That should enable OpenMP on Lion and later, and on Snow Leopard with Xcode 4, and disable it otherwise. Furthermore perhaps we could offer Xcode 3 users on Leopard and Snow Leopard the option of turning on OpenMP support by changing the compiler to llvm-gcc-4.2 or clang, if that works—the version of clang in Xcode 3 is old so I'm a little skeptical of that anyway.

comment:4 Changed 8 years ago by devernay (Frédéric Devernay)

IM doesn't create separate threads by itself, so any single threaded IM test will run OK with the OpenMP version. The ImageMagick API doesn't forbid you to call ImageMagick from a different thread than the main thread. I did that, and it crashed EXACTLY as described in the GCC bug report. Sorry, but that happened one or two years ago and I don't have time to reproduce the bug. This is very annoying, and I had a hard time fuguring it out, because GCC 4.2 has not been maintained for several years. Believe me, it always crashes, others have reported this, and the *official* GCC bug says that GCC 4.2 and 4.3 have buggy OpenMP implementation. I thought it was enough to prevent using OpenMP with these compilers.

Anybody using OpenMP from the main thread (which is how ALL OpenMP samples are designed) won't even nptice the bug. Everybody using OpenMP from a secondary thread will get a crash.

#15945 suggests to enable it for gcc 4.3, which is still wrong. gcc 4.4 (or llvm-gcc or clang) is the minimum version. Check out the official gcc bug mentioned above.

Nobody should be able to enable openmp on gcc 4.0, 4.2 or 4.3, because this leads to a buggy IM which will may other ports inconsistently. IMHO adding a variant to be able to enable it on on a buggy gcc is not a good thing, because it may cause bug reports for other ports.

And, but the way, IM doesn't really benefit from using OpenMP (do you do intensive image processing using ImageMagick? I don't, although image processing and computer vision is my day job).Turning it off by default on buggy systems, and on by default on working systems is the best option. Why mess things up with variants? It just works or does not work. There's no extra functionnality.

fred

comment:5 in reply to:  4 Changed 8 years ago by ryandesign (Ryan Schmidt)

Thank you for the additional information. While I do of course want to "do the right thing" (or the best thing) by default, users have requested the ability to turn openmp support on when it was off by default (#15945) or off when it was on by default (#24944). These users apparently feel there is value to being able to select this, so I would like to accommodate their wishes, just as I would like to accommodate the wishes you've expressed in this ticket. Although you say "IM doesn't really benefit from using OpenMP", clearly the developers of ImageMagick must disagree with you or else they wouldn't have added OpenMP support to ImageMagick, so I am trying to come up with a solution that gives users the choice if possible.

It's not really important that later versions of gcc would work with openmp, since there is no supported circumstance in which ImageMagick would ever be compiled with a later version of gcc in MacPorts. The only compilers ImageMagick might be compiled with today are the ones that come with Xcode; by default those are: gcc 4.0 on Tiger and Leopard, gcc 4.2 on Snow Leopard with Xcode 3, llvm-gcc-4.2 on Xcode 4.0 and 4.1, and clang on Xcode 4.2 and up.

I was not suggesting that we should allow users to compile ImageMagick with gcc and with openmp on. Rather, I was suggesting that Xcode 3 and up includes versions of llvm-gcc-4.2 and clang. Although the default compiler for Xcode 3 is a version of gcc, we could set ImageMagick to compile with llvm-gcc-4.2 or clang instead, thereby allowing openmp to function properly. Assuming the old versions of llvm-gcc-4.2 or clang in Xcode 3 work well enough to compile ImageMagick at all, we could either switch the compiler unconditionally, or conditionally only when openmp support is requested. If necessary we could even make ImageMagick install the clang-3.0 port and use that to compile ImageMagick, though since that's a rather heavy dependency it would be best to avoid that if possible.

Changed 8 years ago by devernay (Frédéric Devernay)

Attachment: test.cpp added

test file from GCC bug 36242, exhibiting the OpenMP bug

comment:6 Changed 8 years ago by devernay (Frédéric Devernay)

OK I just attached the test file from the gcc bug attachment:test.cpp . Compilation instructions are in the comments of the file. This file is very short, and very easy to understand: launch a secondary thread, do some openmp stuff in it. This situation will happen in IM whenever you call an IM function that may do some OpenMP from a secondary thread (I always do that, and most image processing software with a GUI will do that, because the main thread usually handles the GUI).

It consistently crashes in the same function (gomp_resolve_num_threads) with: gcc-4.2 from XCode 3.2.6 on x86_64 gcc-4.2 from XCode 3.1.4 on PowerPC llvm-g++-4.2 from XCode 4.3.1 on x86_64 (which means that llvm-g++ is not a good option either) you can add many different combinations to this list: anything based on gcc-4.2 will just crashed, because OpenMP was buggy in GCC 4.2 and 4.3, as I explained. Shouldn't we avoid compiler bugs?

It doesn't crash with clang 3.1 from XCode 4.3.1 on x86_64 I don't know about the clang from XCode 3.2.6, but XCode 3.1.4 doesn't have clang

You say "The only compilers ImageMagick might be compiled with today are the ones that come with Xcode", but port that rely on recent compiler functionality, such as R, build by default with recent gcc versions (macports-gcc-4.5 in the case of R).

I think there are two options for the IM port:

  • build without OpenMP by default if the compiler is gcc-4.2 or llvm-gcc-4.2, and if the openmp variant is requested build with a more recent gcc (or add a default "+gcc45" variant to do so, which can be desactivated: if the user really wants a buggy IM he/she can do a "sudo port install ImageMagick +openmp -gcc45")
  • build with openmp by default but add a +gcc45 variant if clang is not available (the openmp and gcc45 variants can still be disactivated anyway).

In the above, replace gcc45 with you preferred gcc version, as long as it's >= 4.4

Last edited 10 months ago by ryandesign (Ryan Schmidt) (previous) (diff)

comment:7 Changed 10 months ago by jmroot (Joshua Root)

Is there anything that can or should be done about this at this point?

comment:8 Changed 10 months ago by kencu (Ken)

Once an Xcode-supplied clang supports openmp then there will be a reasonable push on to figure out the whole openmp setup properly.

Basically, software that uses libraries built with openmp support needs to be built with compilers that support openmp as well. It will need something like the cxx11 1.1 PG approach to make sure a proper compiler is selected, flags enabled, link libraries passed in, etc.

And some software, like ImageMagick, is just broken when trying to use openmp and clang (see many other tickets over the past six months where this was worked on).

comment:9 Changed 10 months ago by ryandesign (Ryan Schmidt)

Resolution: fixed
Status: newclosed

Then let's close this ticket. I'll mark it "fixed" since I did add the pango variant, and other tickets can track the OpenMP issues.

Note: See TracTickets for help on using tickets.