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

Compilation error on 32-bit system #1277

Closed
Bolderaysky opened this issue Aug 23, 2024 · 6 comments · Fixed by #1287
Closed

Compilation error on 32-bit system #1277

Bolderaysky opened this issue Aug 23, 2024 · 6 comments · Fixed by #1287
Labels
bug Something isn't working

Comments

@Bolderaysky
Copy link
Contributor

ESP32, GCC 12.2 (32-bit target)

Compiling code that serializes structures with arrays/vectors results in errors like that:

glaze-src/include/glaze/util/dump.hpp:41:32: error: no matching function for call to 'max(std::__cxx11::basic_string<char>::size_type, const long unsigned int&)'
   41 |             b.resize((std::max)(b.size() * 2, k));
      |                      ~~~~~~~~~~^~~~~~~~~~~~~~~~~
In file included from include/c++/12.2.0/string:50,
                 from include/c++/12.2.0/bits/locale_classes.h:40,
                 from include/c++/12.2.0/bits/ios_base.h:41,
                 from include/c++/12.2.0/ios:42,
                 from include/c++/12.2.0/istream:38,
                 from include/c++/12.2.0/sstream:38,
                 from include/c++/12.2.0/chrono:41,
                 from base/traits/types.hpp:5,
                 from base/meta/runtime.hpp:3:
include/c++/12.2.0/bits/stl_algobase.h:254:5: note: candidate: 'template<class _Tp> constexpr const _Tp& std::max(const _Tp&, const _Tp&)'
  254 |     max(const _Tp& __a, const _Tp& __b)
      |     ^~~
include/c++/12.2.0/bits/stl_algobase.h:254:5: note:   template argument deduction/substitution failed:
glaze-src/include/glaze/util/dump.hpp:41:32: note:   deduced conflicting types for parameter 'const _Tp' ('unsigned int' and 'long unsigned int')
   41 |             b.resize((std::max)(b.size() * 2, k));
      |                      ~~~~~~~~~~^~~~~~~~~~~~~~~~~
include/c++/12.2.0/bits/stl_algobase.h:300:5: note: candidate: 'template<class _Tp, class _Compare> constexpr const _Tp& std::max(const _Tp&, const _Tp&, _Compare)'
  300 |     max(const _Tp& __a, const _Tp& __b, _Compare __comp)
      |     ^~~
include/c++/12.2.0/bits/stl_algobase.h:300:5: note:   template argument deduction/substitution failed:
glaze-src/include/glaze/util/dump.hpp:41:32: note:   deduced conflicting types for parameter 'const _Tp' ('unsigned int' and 'long unsigned int')
   41 |             b.resize((std::max)(b.size() * 2, k));
      |                      ~~~~~~~~~~^~~~~~~~~~~~~~~~~
In file included from include/c++/12.2.0/functional:64,
                 from glaze-src/include/glaze/util/type_traits.hpp:6,
                 from glaze-src/include/glaze/core/opts.hpp:8,
                 from glaze-src/include/glaze/binary/skip.hpp:7:
include/c++/12.2.0/bits/stl_algo.h:5746:5: note: candidate: 'template<class _Tp> constexpr _Tp std::max(initializer_list<_Tp>)'
 5746 |     max(initializer_list<_Tp> __l)
      |     ^~~
include/c++/12.2.0/bits/stl_algo.h:5746:5: note:   template argument deduction/substitution failed:
glaze-src/include/glaze/util/dump.hpp:41:32: note:   mismatched types 'std::initializer_list<_Tp>' and 'unsigned int'
   41 |             b.resize((std::max)(b.size() * 2, k));
      |                      ~~~~~~~~~~^~~~~~~~~~~~~~~~~
include/c++/12.2.0/bits/stl_algo.h:5756:5: note: candidate: 'template<class _Tp, class _Compare> constexpr _Tp std::max(initializer_list<_Tp>, _Compare)'
 5756 |     max(initializer_list<_Tp> __l, _Compare __comp)
      |     ^~~
include/c++/12.2.0/bits/stl_algo.h:5756:5: note:   template argument deduction/substitution failed:
glaze-src/include/glaze/util/dump.hpp:41:32: note:   mismatched types 'std::initializer_list<_Tp>' and 'unsigned int'
   41 |             b.resize((std::max)(b.size() * 2, k));

Casting to std::size_t resolves issue as std::max expects both values to be of the same type:

   template <uint32_t N, class B>
   GLZ_ALWAYS_INLINE void maybe_pad(B& b, auto& ix) noexcept
   {
      if constexpr (vector_like<B>) {
         if (const auto k = ix + N; k > b.size()) [[unlikely]] {
            b.resize((std::max)(std::size_t(b.size() * 2), std::size_t(k))); // cast values to std::size_t before passing to std::max
         }
      }
   }

While this may work for most use cases, it indicates deeper issue with glaze + 32-bit - most likely some fixes are needed to use std::size_t instead, as this would work nicely for both 32-bit and 64-bit systems?

@stephenberry
Copy link
Owner

Thanks for reporting this. I'm not sure how to set up a 32bit CI pipeline with GitHub, but I'll try to figure that out and then go through and more carefully use size_t

@Bolderaysky
Copy link
Contributor Author

Thanks for your hard work!

You are right, it seems to be tricky to setup 32-bit runners according to actions/runner#1181 and unfortunately this affects many projects...

@stephenberry stephenberry added the bug Something isn't working label Sep 3, 2024
@stephenberry
Copy link
Owner

Did making that one change to maybe_pad allow your code to compile, or was this just one example of many?

Do you think you could submit a pull request that adds a 32-bit runner? Or, does this seem impossible?

@Bolderaysky
Copy link
Contributor Author

Yeah, that single change allowed me to compile the code and it works just fine, at least in my limited use case.

It's possible to make such runner by using qemu or chroot. This runner based on alpine linux is tailored for chroot / emulation, so I will try this one. I will let you know how it's going and submit PR if everything is fine

@stephenberry
Copy link
Owner

I changed the maybe_pad function to explicitly take in a size_t in version 3.3.2, so either this should fix the issue or it should show up wherever maybe_pad is called. If you could give feedback on trying to build 3.3.2 that would be helpful.

@Bolderaysky
Copy link
Contributor Author

After some research, I found better way to make 32-bit builds - GCC can take -m32 as argument which compiles code for x86 architecture. maybe_pad indeed worked nicely, however, other errors showed up, but these seem to be not that difficult to fix and I will submit fixes together with the runner after I verify that everything works as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants