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

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

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

cary-ilm
Copy link
Member

@cary-ilm cary-ilm commented Aug 29, 2023

  • MSVCRT doesn't recognize printf("%l") so PRIu64 doesn't work, it expects "%I64u".
  • Add to the cases that disable a strncpy warning
  • Use "%zu" for printing size_t

* 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>
Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we had a solution that for the strncpy that doesn't trigger warnings, such as writing out own strncpy variant, but I'm fine with punting a deeper think about that for later in order to clean up warnings now.

@@ -195,13 +195,18 @@ exr_attr_string_set_with_length (
#ifdef _MSC_VER
# pragma warning(push)
# pragma warning(disable : 4996)
#elif __MSVCRT__ && __GNUC__
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wstringop-truncation"
#endif
if (d)
strncpy (sstr, d, (size_t) len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the MSVC warning go away if the str[len]='\0' line is copied immediately after the strncpy? I wonder if the compiler is smart enough to see that.

Copy link

@kmilos kmilos Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, __MSVCRT__ seems to be defined even in more modern/compliant UCRT environments like UCRT64 and CLANG64, where I didn't see this warning before... Perhaps this should be checking for && !defined(_UCRT) in addition?

This is from my _mingw.h:

#if !defined(_UCRT) && ((__MSVCRT_VERSION__ >= 0x1400) || (__MSVCRT_VERSION__ >= 0xE00 && __MSVCRT_VERSION__ < 0x1000))
/* Allow both 0x1400 and 0xE00 to identify UCRT */
#define _UCRT
#endif
  • On UCRT64 and CLANG64 (using UCRT), __MSVCRT_VERSION__ is 0xE00 so _UCRT is defined.
  • On recent MINGW64 (using MSVCRT), __MSVCRT_VERSION__ is 0x700, so it is not defined.
  • On older Debian/Ubuntu mingw cross toolchains targeting MSVCRT, either the statement is not in the old _mingw.h so _UCRT is not defined anyway, or __MSVCRT_VERSION__ is also at a lower version.

@kmilos
Copy link

kmilos commented Aug 31, 2023

FWIW, I don't get any print format or string warnings on either main or 3.2.0 tag using the native and up to date MSYS2 MINGW64 environment. (I'm only seeing deprecation warnings.)

P.S. Again, adding MSYS2 builds (e.g. MINGW64 + CLANG64 as minimum) to your CI would be beneficial and easy ;)

Signed-off-by: Cary Phillips <cary@ilm.com>
@cary-ilm
Copy link
Member Author

I reverted the blank line, but otherwise I'm inclined to leave strncpy alone for now (with the further pragmas). There's no suggestion that the code is incorrect, just that come compilers are being extra persnickety. Conceivably we could replace these strncpy's with memcpy's with the proper length calculations, but we'll leave that for another day.

@cary-ilm cary-ilm merged commit 1ee0ffe into AcademySoftwareFoundation:main Aug 31, 2023
cary-ilm added a commit to cary-ilm/openexr that referenced this pull request Sep 23, 2023
…cademySoftwareFoundation#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>
cary-ilm added a commit that referenced this pull request 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

Successfully merging this pull request may close these issues.

3 participants