Bug 196014 - [patch] unbreak graphics/OpenEXR build when CPUTYPE is set when building with gcc on i386
Summary: [patch] unbreak graphics/OpenEXR build when CPUTYPE is set when building with...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Matthias Andree
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-16 08:29 UTC by Don Lewis
Modified: 2015-01-19 01:46 UTC (History)
1 user (show)

See Also:
mandree: maintainer-feedback+


Attachments
patch to unbreak graphics/OpenEXR build when CPUTYPE is set (1.39 KB, patch)
2014-12-16 08:29 UTC, Don Lewis
no flags Details | Diff
patch to unbreak graphics/OpenEXR build when CPUTYPE is set and unbreak regression test compilation (2.75 KB, patch)
2015-01-06 01:48 UTC, Don Lewis
no flags Details | Diff
.tar.gz archive of poudriere build logs and regression results on FreeBSD 8.4 and 10.0, and both amd64 and i386 (36.59 KB, application/octet-stream)
2015-01-06 02:00 UTC, Don Lewis
no flags Details
updated regression test results on FreeBSD 8.4 i386 (61.61 KB, application/octet-stream)
2015-01-15 18:41 UTC, Don Lewis
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Don Lewis freebsd_committer 2014-12-16 08:29:19 UTC
Created attachment 150626 [details]
patch to unbreak graphics/OpenEXR build when CPUTYPE is set

If CPUTYPE is set in make.conf to a value that enables SSE2 when building graphics/OpenEXR with gcc on i386, the build fails with this error:

gcc48 -MT ImfSystemSpecific.lo -MD -MP -MF .deps/ImfSystemSpecific.Tpo -c ImfSystemSpecific.cpp  -fPIC -DPIC -o .libs/ImfSystemSpecific.o
libtool: compile:  g++48 -DHAVE_CONFIG_H -I. -I../config -I.. -I../config -pthread -I/usr/local/include/OpenEXR -I. -I../IlmImf -pipe -O2 -pipe -march=pentium-m -Wl,-rpath=/usr/local/lib/gcc48 -fno-strict-aliasing -Wl,-rpath=/usr/local/lib/gcc48 -MT ImfCompositeDeepScanLine.lo -MD -MP -MF .deps/ImfCompositeDeepScanLine.Tpo -c ImfCompositeDeepScanLine.cpp -o ImfCompositeDeepScanLine.o >/dev/null 2>&1
ImfSystemSpecific.cpp: In constructor 'Imf_2_2::CpuId::CpuId()':
ImfSystemSpecific.cpp:51:29: error: inconsistent operand constraints in an 'asm'
             : /* Clobber */);
                             ^
ImfSystemSpecific.cpp:51:29: error: inconsistent operand constraints in an 'asm'
             : /* Clobber */);
                             ^
Makefile:669: recipe for target 'ImfSystemSpecific.lo' failed
gmake[2]: *** [ImfSystemSpecific.lo] Error 1


The problem is in this block of code:

#if defined(IMF_HAVE_SSE2) &&  defined(__GNUC__)

    // Helper functions for gcc + SSE enabled
    void cpuid(int n, int &eax, int &ebx, int &ecx, int &edx)
    {
        __asm__ __volatile__ (
            "cpuid"
            : /* Output  */ "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx)
            : /* Input   */ "a"(n)
            : /* Clobber */);
    }

#else // IMF_HAVE_SSE2 && __GNUC__

The problem is that this code is clobbering the ebx register which is reserved for the global offset table when compiling -fPIC on i386.

The attached patch reimplments the cpuid() function using __cpuid() from the gcc include file <cpuid.h> in place of the inline asm.

See also: <https://github.com/openexr/openexr/issues/128>
Comment 1 Bugzilla Automation freebsd_committer 2014-12-16 08:29:19 UTC
Auto-assigned to maintainer mandree@FreeBSD.org
Comment 2 Matthias Andree freebsd_committer 2014-12-27 01:22:38 UTC
If you can successfully build a package with the port and OpenEXR run-time tests succeed for you, permission granted to commit.

Feel free to reassign back to me if you want me to run tests or handle the commit.
Comment 3 Don Lewis freebsd_committer 2015-01-06 01:48:29 UTC
Created attachment 151374 [details]
patch to unbreak graphics/OpenEXR build when CPUTYPE is set and unbreak regression test compilation

Same CPUTYPE fix as in the previous patch.

The regression test does not compile on FreeBSD.  Include <unidstd.h> as is done for Linux and Darwin to unbreak it.

Remove an extraneous blank line in the Makefile, which is now a portlint fatal error.
Comment 4 Don Lewis freebsd_committer 2015-01-06 02:00:37 UTC
Created attachment 151375 [details]
.tar.gz archive of poudriere build logs and regression results on FreeBSD 8.4 and 10.0, and both amd64 and i386

This attachment is a .tar.xz archive of the poudriere build logs and the results of the port regression tests for FreeBSD 8.4 (using gcc from ports) and FreeBSD 10.0 (using clang).  All were built with CPUTYPE=athlon64.  Only the 8.4/i386 version should be affected by the CPUTYPE patch.

In all cases, the regression test fails because the file comp_dwaa_v1.exr is missing.  The tests up to that point seem to pass.  On FreeBSD 10.0, both i386 and amd64 give nearly identical results.  On FreeBSD 8.4, both i386 and amd64 give nearly identical results.
Comment 5 Don Lewis freebsd_committer 2015-01-15 18:41:40 UTC
Created attachment 151695 [details]
updated regression test results on FreeBSD 8.4 i386

The previous regression test failures were caused by a number of .exr files that were ommitted from the source tarball, as mentioned in this thread:
<https://www.mail-archive.com/openexr-devel@nongnu.org/msg01877.html>

After downloading the missing .exr files and copying them to the appropriate directory, the regression tests run further until I hit the NaN vs. NaN comparison issue mentioned in this thread:
<https://lists.gnu.org/archive/html/openexr-devel/2014-08/msg00030.html>.
The workaround of adding either -fno-inline or -ffloat-store to CFLAGS when building the test suite did not work, so I make this change instead:

 --- IlmImfTest/testOptimizedInterleavePatterns.cpp.orig	2014-08-09 19:03:49.000000000 -0700
+++ IlmImfTest/testOptimizedInterleavePatterns.cpp	2015-01-15 00:52:18.000000000 -0800
@@ -226,7 +226,8 @@
                     writtenHalf=half(i.slice().fillValue);
                 }
 
-                if (writtenHalf.bits()!=readHalf.bits())
+                if (writtenHalf.bits()!=readHalf.bits() &&
+		    !(writtenHalf.isNan() && readHalf.isNan()))
                 {
                     if (nonfatal)
                     {
@@ -235,7 +236,8 @@
                     else
                     {
                         cout << "\n\nerror reading back channel " << i.name() << " pixel " << x << ',' << y << " got " << readHalf << " expected " << writtenHalf << endl;
-                        assert(writtenHalf.bits()==readHalf.bits());
+                        assert(writtenHalf.bits()==readHalf.bits() ||
+			 (writtenHalf.isNan() && readHalf.isNan()));
                         exit(1);
                     }
                 }             

That allowed the library test suite to run to completion.  The utility test suite then failed to compile because it had not been patched to #include <unistd.h>.

In any case, I believe this shows that the patched library is functional.
Comment 6 Matthias Andree freebsd_committer 2015-01-18 11:10:33 UTC
I am currently in the process of testing things on 9.3 i386 (bare metal) and amd64 (poudriere), and moving the regression test fixes into the port as well.

Currently looking good, I could reproduce the problem on 9.3 with CPUTYPE?=k8-sse3 and saw it go away with the patch.

Don, this has been an enormous help - thank you very much. 

Would you like to take over maintainership for OpenEXR and ilmbase, you seem to be using it to some more extent than I do.
Comment 7 Matthias Andree freebsd_committer 2015-01-18 11:11:21 UTC
(That is not to say I want to get rid of it, I am only asking if there is someone better suited to maintaining it than I am; I am also happy to keep it.)
Comment 8 Don Lewis freebsd_committer 2015-01-18 17:50:59 UTC
(In reply to Matthias Andree from comment #7)

I don't really use this port.  It's just required by some other ports that I do use.
Comment 9 Matthias Andree freebsd_committer 2015-01-19 01:46:42 UTC
Fixed in r377376. 9.3 amd64 and i386 now pass.
Comment 10 commit-hook freebsd_committer 2015-01-19 01:46:57 UTC
A commit references this bug:

Author: mandree
Date: Mon Jan 19 01:46:17 UTC 2015
New revision: 377376
URL: https://svnweb.freebsd.org/changeset/ports/377376

Log:
  Fix compilation if SSE2 is enabled on i386.

  While here, fix regression tests and see that things are fine.
  (Some regression-test issues remain for largestack, the default build
  passes make regression now on i386 and amd64).

  (No revision bump because these are build fixes, and the regression test
  stuff does not become part of the installed material.)

  Assisted by:	truckman@

  PR:		196014
  Submitted by:	truckman@

Changes:
  head/graphics/OpenEXR/Makefile
  head/graphics/OpenEXR/distinfo
  head/graphics/OpenEXR/files/patch-IlmImfTest__main.cpp
  head/graphics/OpenEXR/files/patch-IlmImfTest_testOptimizedInterleavePatterns.cpp
  head/graphics/OpenEXR/files/patch-IlmImfUtilTest_main.cpp
  head/graphics/OpenEXR/files/patch-IlmImf__ImfSystemSpecific.cpp