Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINGW : bad use of __cpuid() #1445

Open
claudeha opened this issue Jun 7, 2023 · 10 comments
Open

MINGW : bad use of __cpuid() #1445

claudeha opened this issue Jun 7, 2023 · 10 comments

Comments

@claudeha
Copy link

claudeha commented Jun 7, 2023

When cross-compiling OpenEXR-3.1.8 from the release tarball for Windows from Debian Bookworm (current testing distribution) using x86_64-w64-mingw32-g++ (GCC) 12-posix, the build fails because __cpuid() is called with 2 arguments instead of 5. This small patch seems to fix it:

diff --git a/openexr-3.1.8.old/src/lib/OpenEXRCore/internal_cpuid.h b/openexr-3.1.8.new/src/lib/OpenEXRCore/internal_cpuid.h
in
dex 9eb89d0..777a61f 100644
--- a/openexr-3.1.8.old/src/lib/OpenEXRCore/internal_cpuid.h
+++ b/openexr-3.1.8.new/src/lib/OpenEXRCore/internal_cpuid.h
@@ -35,7 +35,7 @@ static inline void check_for_x86_simd (int *f16c, int *avx, int *sse2)
     *avx = 1;
     *sse2 = 1;
 #        else
-#            ifdef _WIN32
+#            if defined(_MSC_VER) && defined(_WIN32)
     int regs[4], osxsave;
 
     __cpuid (regs, 0);
@kmilos
Copy link

kmilos commented Jun 7, 2023

No problems w/ MinGW GCC/Clang when not cross-compiling (e.g. using MSYS2), so restricting to _MSC_VER maybe not the best fix?

@kmilos
Copy link

kmilos commented Jun 7, 2023

Does it work w/ the win32 toolchain (which I guess is closer to native) rather than the posix one?

@claudeha
Copy link
Author

claudeha commented Jun 7, 2023

I don't have Microsoft Windows so I can't check non-cross-compiling. Good to know that this patch may be bogus, even though it works for me.

I haven't tried win32 toolchain recently, it caused me too many issues with threading etc in the past. I have no time to try again soon unfortunately.

@kmilos
Copy link

kmilos commented Jun 18, 2023

Perhaps the real/better fix is to remove/change _MSC_VER here and here (i.e. just check for _WIN32):

# if defined(_MSC_VER) && defined(_WIN32)

#if defined(_MSC_VER)

This is what I found in the MinGW shipped intrin.h:

/* Undefine the GCC one taking 5 parameters to prefer the mingw-w64 one. */
#undef __cpuid

This has popped up before IIRC - gotta keep in mind Windows as a platforms is not just MSVC... ;)

There might be other places where _MSC_VER is wrongly used as a catchall "Windows" alias.

@kdt3rd
Copy link
Contributor

kdt3rd commented Jun 26, 2023

at least in the OpenEXRCore library, _MSC_VER should only be used to truly detect msvc, not the win32 api vs. not - although at this level of builtin functions vs. not, this may fall over occasionally. I believe I had actually addressed this already in #1467 (noticing that disconnect). Once that is merged, do let us know if there are other related issues. Thanks for noticing this

@cary-ilm
Copy link
Member

@claudeha, could you confirm whether this is still and issue with your build configuration on the main branch? Hopefully, #1467 fixes it.

@claudeha
Copy link
Author

claudeha commented Jul 12, 2023 via email

@claudeha
Copy link
Author

for me there is still an issue with the tests with openexr-3.2.0-rc:

claude@eiskaffee:~/opt/windows/posix/x86_64/src/openexr-3.2.0-rc/build$ make -k >/dev/null 2>/dev/null && make
[  2%] Built target Iex
[  5%] Built target IlmThread
[ 17%] Built target OpenEXRCore
[ 49%] Built target OpenEXR
[ 54%] Built target OpenEXRUtil
[ 55%] Built target exr2aces
[ 56%] Built target exrheader
[ 57%] Built target exrinfo
[ 58%] Built target exrmaketiled
[ 59%] Built target exrstdattr
[ 60%] Built target exrmakepreview
[ 63%] Built target exrenvmap
[ 64%] Built target exrmultiview
[ 65%] Built target exrmultipart
[ 65%] Built target exrcheck
[ 69%] Built target OpenEXRExamples
[ 71%] Built target IexTest
[ 71%] Building CXX object src/test/OpenEXRCoreTest/CMakeFiles/OpenEXRCoreTest.dir/base_units.cpp.obj
In file included from /usr/share/mingw-w64/include/intrin.h:41,
                 from /home/claude/opt/windows/posix/x86_64/include/Imath/half.h:183,
                 from /home/claude/opt/windows/posix/x86_64/src/openexr-3.2.0-rc/src/test/OpenEXRCoreTest/../../lib/OpenEXRCore/internal_coding.h:24,
                 from /home/claude/opt/windows/posix/x86_64/src/openexr-3.2.0-rc/src/test/OpenEXRCoreTest/base_units.cpp:24:
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2013:42: error: macro "__cpuid" requires 5 arguments, but only 2 given
 2013 | void __cpuid(int CPUInfo[4], int InfoType);
      |                                          ^
In file included from /home/claude/opt/windows/posix/x86_64/src/openexr-3.2.0-rc/src/test/OpenEXRCoreTest/../../lib/OpenEXRCore/internal_cpuid.h:21,
                 from /home/claude/opt/windows/posix/x86_64/src/openexr-3.2.0-rc/src/test/OpenEXRCoreTest/base_units.cpp:23:
/usr/lib/gcc/x86_64-w64-mingw32/12-posix/include/cpuid.h:223: note: macro "__cpuid" defined here
  223 | #define __cpuid(level, a, b, c, d)                                      \
      | 
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2016:42: error: macro "__cpuid" requires 5 arguments, but only 2 given
 2016 | void __cpuid(int CPUInfo[4], int InfoType) {
      |                                          ^
/usr/lib/gcc/x86_64-w64-mingw32/12-posix/include/cpuid.h:223: note: macro "__cpuid" defined here
  223 | #define __cpuid(level, a, b, c, d)                                      \
      | 
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2013:6: error: variable or field ‘__cpuid’ declared void
 2013 | void __cpuid(int CPUInfo[4], int InfoType);
      |      ^~~~~~~
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2016:6: error: variable or field ‘__cpuid’ declared void
 2016 | void __cpuid(int CPUInfo[4], int InfoType) {
      |      ^~~~~~~
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2017:4: error: expected primary-expression before ‘__asm__’
 2017 |    __asm__ __volatile__ (
      |    ^~~~~~~
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2017:4: error: expected ‘}’ before ‘__asm__’
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2016:44: note: to match this ‘{’
 2016 | void __cpuid(int CPUInfo[4], int InfoType) {
      |                                            ^
/usr/share/mingw-w64/include/psdk_inc/intrin-impl.h:2280:1: error: expected declaration before ‘}’ token
 2280 | }
      | ^
make[2]: *** [src/test/OpenEXRCoreTest/CMakeFiles/OpenEXRCoreTest.dir/build.make:77: src/test/OpenEXRCoreTest/CMakeFiles/OpenEXRCoreTest.dir/base_units.cpp.obj] Error 1
make[2]: Target 'src/test/OpenEXRCoreTest/CMakeFiles/OpenEXRCoreTest.dir/build' not remade because of errors.
make[1]: *** [CMakeFiles/Makefile2:1823: src/test/OpenEXRCoreTest/CMakeFiles/OpenEXRCoreTest.dir/all] Error 2

@meshula
Copy link
Contributor

meshula commented Aug 21, 2023

I notice this

/home/claude/opt/windows/posix/x86_64/include/Imath/half.h

It makes me wonder if you have a very old system installed half.h, and if using a new imath would fix the issue.
Can you try reconfiguring your build, using the OPENEXR_FORCE_INTERNAL_IMATH flag?

@claudeha
Copy link
Author

claudeha commented Aug 22, 2023 via email

cary-ilm added a commit to cary-ilm/openexr that referenced this issue Aug 29, 2023
As noted in AcademySoftwareFoundation#1445, _MSC_VER should only be used to detect msvc, not
for Windows. The _WIN32 check should be sufficient. The extra check
for _MSC_VER fails when cross-compiling from Linux to Windows.

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit that referenced this issue Aug 30, 2023
As noted in #1445, _MSC_VER should only be used to detect msvc, not
for Windows. The _WIN32 check should be sufficient. The extra check
for _MSC_VER fails when cross-compiling from Linux to Windows.

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit to cary-ilm/openexr that referenced this issue Sep 23, 2023
…ion#1528)

As noted in AcademySoftwareFoundation#1445, _MSC_VER should only be used to detect msvc, not
for Windows. The _WIN32 check should be sufficient. The extra check
for _MSC_VER fails when cross-compiling from Linux to Windows.

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit that referenced this issue Sep 25, 2023
* Propagate OPENEXR_INSTALL_PKG_CONFIG to internal Imath (#1531)

* Propagate OPENEXR_INSTALL_PKG_CONFIG to internal Imath

If OpenEXR is installing a pkg-config file, then the internal Imath
build (if there is one) should install it, too.

Also, add an explicit release version variable to the .pc file.

This is in preparation for the python wheel build to pick up the
version settings from the pkg-config files.

Signed-off-by: Cary Phillips <cary@ilm.com>

* remove version= from OpenEXR.pc

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* Remove check for _MSC_VER in internal_cpuid.h (#1528)

As noted in #1445, _MSC_VER should only be used to detect msvc, not
for Windows. The _WIN32 check should be sufficient. The extra check
for _MSC_VER fails when cross-compiling from Linux to Windows.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Fix warnings in multipartExamples.cpp (#1533)

* Fix warnings in multipartExamples.cpp

Signed-off-by: Cary Phillips <cary@ilm.com>

* fix indentation

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* Fix warnings from cross-compiling with x86_64-w64-mingw32-gcc-posix (#1534)

* Fix compiler warnings from x86_64-w64-mingw32-g++-posix

* MSVCRT doesn't recognize printf("%l") so PRIu64 doesn't work, it
  expects "%I64u".

* Add to the cases that disable a strncpy warning

Signed-off-by: Cary Phillips <cary@ilm.com>

* %zu for size_t

Signed-off-by: Cary Phillips <cary@ilm.com>

* revert attempt to fix PRIu64 warnings

Signed-off-by: Cary Phillips <cary@ilm.com>

* revert blank line

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* Fix OPENEXR_VERSION_HEX (#1539)

* Fix OPENEXR_VERSION_HEX

* fix syntax error
* remove unnecessary uint32_t cast
* add validation test

Signed-off-by: Cary Phillips <cary@ilm.com>

* Add #if statement with OPENEXR_VERSION_HEX

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* OPENEXR_INSTALL_PKG_CONFIG is on by default, even on Windows (#1541)

There seems to be no downside to generating the OpenEXR.pc file even
on Windows, so for consistency, apply the setting to all platforms.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Remove SPDX license identifier in LICENSE.md

Signed-off-by: Cary Phillips <cary@ilm.com>

* Python wheel setup gets version from OpenEXR.pc/Imath.pc (#1536)

* Python wheel setup gets version from OpenEXR.pc/Imath.pc

OpenEXR and Imath have indepdenent versions that may not match. The
.pc files appear to be the simplest way to extract the versions and
library suffixes.

Running pkg-config itself doesn't work on Windows, hence the manual
file parsing.

Signed-off-by: Cary Phillips <cary@ilm.com>

* =

Signed-off-by: Cary Phillips <cary@ilm.com>

* Remove -DOPENEXR_INSTALL_PKG_CONFIG=ON since it's now on by default

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* Default value for chromaticities attribute constructor in exrstdattr (#1540)

Signed-off-by: Cary Phillips <cary@ilm.com>

* Update openexr_deps.bzl (#1565)

Signed-off-by: Vertexwahn <julian.amann@tum.de>

* Bazel: Improve module (#1562)

Signed-off-by: Vertexwahn <julian.amann@tum.de>

* Set build-shared:OFF for Static build (#1557)

Looks like this somehow we've been building shared when we meant to be
building static.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Omit OPENEXR_IMAGES_TAG from test image url if empty (#1560)

This allows the url for the test images to reference a local file:

  cmake -DOPENEXR_IMAGES_REPO=file:///my/clone/of/openexr-images -DOPENEXR_IMAGES_TAG=""

which avoids the remote downloading of the test images at cmake time.

Also, mention the test images in the install docs.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Clean up handling of libdeflate when linking static  (#1561)

* Set EXR_DEFLATE_LIB properly for static linking

This leads OpenEXRCore/CMakeLists.txt to do:

    target_link_libraries(OpenEXRCore PUBLIC libdeflate)

instead of:

    target_link_libraries(OpenEXRCore PUBLIC PkgConfig::deflate)

which is not recognized. With a target link library of
`PkgConfig::deflate`, the static build of the OpenEXR libraries builds
successfully, but configuring an application against those static
libraries fails, saying it can't find `PkgConfig::deflate`.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Add -ldeflate to pkgconfig for static builds

When static linking and referencing an external libdeflate (i.e. not
fetching and building internally), OpenEXR.pc needs -ldeflate.

This also adds a test for this condition to the CI's validation script.

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* Don't trigger ci/bazel/ossfuzz builds on pushes/PRs to src/wrappers (#1532)

No need to rebuild/test OpenEXR on changes to the python wheels.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Set minimal permissions for workflow python-wheels.yml (#1530)

The exact same changes done on other workflows through the PR #1417

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* Release notes for 3.2.1

Signed-off-by: Cary Phillips <cary@ilm.com>

* Fix Imf/Iex/IlmThread namespaces in python bindings and website code (#1568)

* Fix Imf/Iex/IlmThread namespaces in python bindings and website code

Use OPENEXR_NAMESPACE, IEX_NAMESPACE, ILMTHREAD_NAMESPACE instead of
Imf, Iex, IlmThread, to support custom settings.

Signed-off-by: Cary Phillips <cary@ilm.com>

* Use Iex:: namespace in example code instead of IEX_NAMESPACE

Requires this in all.cpp:

  namespace Iex = IEX_NAMESPACE;

Signed-off-by: Cary Phillips <cary@ilm.com>

* use #define Iex

Signed-off-by: Cary Phillips <cary@ilm.com>

* remove explicit Iex namespace

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>

* Bump version to 3.2.1

Signed-off-by: Cary Phillips <cary@ilm.com>

---------

Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Vertexwahn <julian.amann@tum.de>
Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
Co-authored-by: Vertexwahn <julian.amann@tum.de>
Co-authored-by: Diogo Teles Sant'Anna <diogoteles@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants