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

[BUG] ensure proper constexpr of string hashing fix fails to build on 32-bit archs #4212

Closed
brad0 opened this issue Mar 30, 2024 · 5 comments · Fixed by #4213
Closed

[BUG] ensure proper constexpr of string hashing fix fails to build on 32-bit archs #4212

brad0 opened this issue Mar 30, 2024 · 5 comments · Fixed by #4213

Comments

@brad0
Copy link
Contributor

brad0 commented Mar 30, 2024

The fix(strutil.h): ensure proper constexpr of string hashing (#3901) commit fails to build on 32-bit archs.

3fd70cf

We noticed this with 2.4 but applies to 2.5 and newer. Reverting this commit alone and we're good building beyond 2.4.12 (upgrading from 2.4.11).

FAILED: src/libutil/CMakeFiles/strutil_test.dir/strutil_test.cpp.o
/pobj/openimageio-2.4.17.0/bin/c++ -DOIIO_INTERNAL=1 -DUSE_BOOST_FILESYSTEM -DUSE_EXTERNAL_PUGIXML=1 -DUSE_FREETYPE=1 -DUSE_JPEG_TURBO=1 -DUSE_OCIO=1 -DUSE_OPENCOLORIO=1 -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -I/pobj/openimageio-2.4.17.0/build-i386/include/OpenImageIO -I/pobj/openimageio-2.4.17.0/build-i386/include -I/pobj/openimageio-2.4.17.0/build-i386/src/include -I/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include -isystem /usr/local/include -isystem /usr/local/include/Imath -O2 -pipe -DNDEBUG -std=c++14 -Wall -Wno-unused-function -Wno-overloaded-virtual -Wno-unneeded-internal-declaration -Wno-unused-private-field -Wno-tautological-compare -Qunused-arguments -Wunknown-warning-option -Wno-unused-local-typedefs -Wno-expansion-to-defined -fno-math-errno -MD -MT src/libutil/CMakeFiles/strutil_test.dir/strutil_test.cpp.o -MF src/libutil/CMakeFiles/strutil_test.dir/strutil_test.cpp.o.d -o src/libutil/CMakeFiles/strutil_test.dir/strutil_test.cpp.o -c /pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/libutil/strutil_test.cpp
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/libutil/strutil_test.cpp:354:22: error: constexpr variable 'hash' must be initialized by a constant expression
    constexpr size_t hash = Strutil::strhash("much longer string");
                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include/OpenImageIO/detail/farmhash.h:1207:24: note: cannot refer to element -4 of array of 19 elements in a constant expression
  uint32_t a = Fetch(s - 4 + (len >> 1));
                       ^
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include/OpenImageIO/detail/farmhash.h:1246:9: note: in call to 'Hash32Len13to24(&"much longer string"[0], 18, 0)'
        Hash32Len13to24(s, len);
        ^
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include/OpenImageIO/detail/farmhash.h:2070:7: note: in call to 'Hash32(&"much longer string"[0], 18)'
      farmhashmk::Hash32(s, len));
      ^
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include/OpenImageIO/detail/farmhash.h:2100:49: note: in call to 'Hash32(&"much longer string"[0], 18)'
  return sizeof(size_t) == 8 ? Hash64(s, len) : Hash32(s, len);
                                                ^
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include/OpenImageIO/strutil.h:377:12: note: in call to 'Hash(&"much longer string"[0], 18)'
    return OIIO::farmhash::inlined::Hash(s, len);
           ^
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/include/OpenImageIO/strutil.h:389:25: note: in call to 'strhash(18, &"much longer string"[0])'
    return s.length() ? strhash(s.length(), s.data()) : 0;
                        ^
/pobj/openimageio-2.4.17.0/OpenImageIO-2.4.17.0/src/libutil/strutil_test.cpp:354:29: note: in call to 'strhash({&"much longer string"[0], 18})'
    constexpr size_t hash = Strutil::strhash("much longer string");
                            ^
1 error generated.
@lgritz
Copy link
Collaborator

lgritz commented Mar 30, 2024

Proposed fix posted as #4213

We're no longer routinely updating the 2.4 series of releases. But if this is important for you and you can't upgrade to 2.5, I can backport it to 2.4. Let me know.

@brad0
Copy link
Contributor Author

brad0 commented Mar 30, 2024

Proposed fix posted as #4213

We're no longer routinely updating the 2.4 series of releases. But if this is important for you and you can't upgrade to 2.5, I can backport it to 2.4. Let me know.

We can upgrade. I was just pushing an update to 2.5.9 but then this was reported after my 2.4.17 update went in. I just want to at least get the root issue fixed first.

lgritz added a commit that referenced this issue Mar 31, 2024
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Mar 31, 2024
@brad0
Copy link
Contributor Author

brad0 commented Mar 31, 2024

At least backport this to 2.5.

@lgritz
Copy link
Collaborator

lgritz commented Mar 31, 2024

At least backport this to 2.5.

Done. It will be in Monday's scheduled release.

@brad0
Copy link
Contributor Author

brad0 commented Mar 31, 2024

Done. It will be in Monday's scheduled release.

Thanks.

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 a pull request may close this issue.

2 participants