t/vector fails on 32bit platforms at ASSERT_FALSE(vec.resize(INT64_MAX))

[cheese]

Are 32bit platforms still supported?

Tested 1.1.19.1 on 32bit CentOS7 altarch, and t/vector failed during make test.
The same on 32bit ARM.

The only failing assertion is ASSERT_FALSE(vec.resize(INT64_MAX)).

This Dockerfile can reproduce the issue:

FROM i386/centos:7

MAINTAINER gearmand

ARG GEARMAN_REPO=https://github.com/gearman/gearmand

LABEL version="${version}" description="Gearman SSL Job Server Image"

# Configure environment
ENV HOME=/root

# Install packages
RUN yum update -y
RUN yum install -y rpm-build make
RUN yum install -y boost-devel boost-thread gcc-c++ gperf  gperftools-devel  hiredis-devel  libevent-devel  libmemcached-devel  libpq-devel \
  libuuid-devel  mariadb-connector-c-devel  memcached  openssl-devel  sqlite-devel  systemd  tokyocabinet-devel  zlib-devel

# Retrieve the source code and bootstrap
COPY gearmand-1.1.19.1.tar.gz /tmp/
RUN cd /tmp && tar fx gearmand-1.1.19.1.tar.gz
WORKDIR /tmp/gearmand-1.1.19.1

RUN ./configure --enable-ssl 2>&1 | tee ./configure.log
RUN make 2>&1 | tee ./build.log
RUN make test 2>&1 | tee ./test.log

[p-alik]

Tested 1.1.19.1 on 32bit CentOS7 altarch, and t/vector failed during make test.
The same on 32bit ARM.

There are {U,}INT64 type checks

gearmand/configure.ac

Lines 188 to 205 in a6bd168

	# Checks for typedefs, structures, and compiler characteristics.
	AC_CHECK_TYPES([ptrdiff_t])
	AC_C_CONST
	AC_C_INLINE
	AC_C_VOLATILE
	AC_HEADER_STDBOOL
	AC_HEADER_TIME
	AC_TYPE_INT32_T
	AC_TYPE_INT64_T
	AC_TYPE_OFF_T
	AC_TYPE_PID_T
	AC_TYPE_SIZE_T
	AC_TYPE_SSIZE_T
	AC_TYPE_UID_T
	AC_TYPE_UINT16_T
	AC_TYPE_UINT32_T
	AC_TYPE_UINT64_T
	AC_TYPE_UINT8_T

Shouldn't it cause an exception on 32bit system?

The only failing assertion is ASSERT_FALSE(vec.resize(INT64_MAX)).

The assertion has a long history 255137a.

[esabol]

This is not specific to 1.1.19, right? This applies to 1.1.18, 1.1.17, etc., too?

[p-alik]

yes

[esabol]

The only failing assertion is ASSERT_FALSE(vec.resize(INT64_MAX)).

If this is the only thing that fails, then that is easily fixed, right?

Is there any reason not to support 32-bit architectures?

[SpamapS]

Quoting Ed Sabol (2020-02-22 17:07:05)

The only failing assertion is ASSERT_FALSE(vec.resize(INT64_MAX)). If this is the only thing that fails, then that is easily fixed, right? Is there any reason not to support 32-bit architectures?

There will be relevant 32-bit arches for many years to come. This is an easy fix with a conditional define. I don't have time, but yes, somebody should fix this.

…

— You are receiving this because you are subscribed to this thread. Reply to this email directly, [1]view it on GitHub, or [2]unsubscribe. References 1. #279?email_source=notifications&email_token=AADS6YDIOOY4PNGWLDCB2E3REHD3TA5CNFSM4KXUL7K2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMVOUDI#issuecomment-590014989 2. https://github.com/notifications/unsubscribe-auth/AADS6YC7QB6HGXM65WS2TOTREHD3TANCNFSM4KXUL7KQ

[esabol]

So I think this works on both 32-bit and 64-bit systems:

    // SIZE_MAX is ((size_t)(-1)), but we need to subtract 1 because the
    // gearman_vector_st implementation adds 1 to the capacity.
    ASSERT_FALSE(vec.resize(((size_t)(-1)) -1));

Anyone see a problem with that?

Otherwise, things get complicated:

#if defined(__LP64__) || defined(_WIN64) || (defined(__x86_64__) && !defined(__ILP32__)) || defined(_M_X64) || defined(__ia64) || defined (_M_IA64) || defined(__aarch64__) || defined(__powerpc64__)
    ASSERT_FALSE(vec.resize(INT64_MAX));
#else
    // SIZE_MAX is ((size_t)(-1)), but we need to subtract 1 because the
    // gearman_vector_st implementation adds 1 to the capacity.
    ASSERT_FALSE(vec.resize(((size_t)(-1)) -1));
#endif

References:
https://stackoverflow.com/a/40033930/5153779
https://stackoverflow.com/a/44401992/5153779

Should we bother to rename INT64_MAX_resize_TEST to something else?

[p-alik]

Anyone see a problem with that?

LGTM. My worry is, that we haven't 32 CI environment for testing.

Should we bother to rename INT64_MAX_resize_TEST to something else?

Wouldn't it be appropriate to cut off INT64 prefix?

[cheese]

Anyone see a problem with that?

In C++ style, it would be better to use std::numeric_limits<size_t>::max() instead of ((size_t)(-1)).

Otherwise, things get complicated:

#if defined(__LP64__) || defined(_WIN64) || (defined(__x86_64__) && !defined(__ILP32__)) || defined(_M_X64) || defined(__ia64) || defined (_M_IA64) || defined(__aarch64__) || defined(__powerpc64__)
    ASSERT_FALSE(vec.resize(INT64_MAX));
#else
    // SIZE_MAX is ((size_t)(-1)), but we need to subtract 1 because the
    // gearman_vector_st implementation adds 1 to the capacity.
    ASSERT_FALSE(vec.resize(((size_t)(-1)) -1));
#endif

What's the real intention of using INT64_MAX? The generic of INT64_MAX seems std::numeric_limits<ssize_t>::max() instead of std::numeric_limits<size_t>::max().

[esabol]

In reply to @p-alik's comment:

My worry is, that we haven't 32 CI environment for testing.

Yeah. I've tested it with a Docker i386/centos7 image similar to what @cheese posted above, fwiw. I think it is possible to add Docker builds to Travis CI, but I don't know how. I think it's not worth the effort.

Should we bother to rename INT64_MAX_resize_TEST to something else?

Wouldn't it be appropriate to cut off INT64 prefix?

Sure, that would be appropriate.

In reply to @cheese's comment:

In C++ style, it would be better to use std::numeric_limits<size_t>::max() instead of ((size_t)(-1)).

Interesting. I'll test that (maybe later today) and report back.

What's the real intention of using INT64_MAX?

I'm not sure any of us can answer that question since I'm guessing these tests were written before any of us got involved with gearmand. I think the intent is to just test resize() with the largest possible number it can take.

The generic of INT64_MAX seems std::numeric_limits<ssize_t>::max() instead of std::numeric_limits<size_t>::max().

Is that a significant distinction?

[cheese]

The generic of INT64_MAX seems std::numeric_limits<ssize_t>::max() instead of std::numeric_limits<size_t>::max().

Is that a significant distinction?

If your guess of the test intent is right, the argument should be std::numeric_limits<size_t>::max() - 1. But the original used INT64_MAX, which is much less than std::numeric_limits<size_t>::max() - 1 on x86_64. The original author may just use a large enough argument instead of the largest possible one.

[esabol]

So this issue has languished. Rereading the thread here, I'm still not clear what the solution is. I think I'm still inclined to go with @cheese's suggestion of std::numeric_limits<size_t>::max() - 1. Anyone else have an opinion?

[p-alik]

I'm of the same opinion because it is most appropriate for the purpose of test.

gearmand/libgearman/vector.hpp

Line 89 in 36c1e23

bool resize(const size_t);

[esabol]

Merged commit addresses this issue, so I’m going to close this.