TestPacketDataStream fails on ARM64

[davidebeatrici]

FAIL!  : TestPacketDataStream::space() 'out.isValid()' returned FALSE. ()
   Loc: [TestPacketDataStream.cpp(124)]

mumble/src/tests/TestPacketDataStream/TestPacketDataStream.cpp

Line 124 in bafcf47

QVERIFY(out.isValid());

Once this is fixed as per #3846 the travis build make check should be enabled.

[Kissaki]

Added reference to missing make check mentioned in #3846; should be enabled once this is fixed.

[Krzmbrzl]

As I see it, the test is back in place...

[davidebeatrici]

It's not, tests are not run for the ARM64 build.

[carlwgeorge]

In addition to arm64 (aka aarch64), I see the same failure with s390x and ppc64le. Only x86_64 passes as far as I can tell.

[Krzmbrzl]

Presumably this class relies on something like the size of an integer being exactly x bytes or something like this 🤔

[thesamesam]

All of the arches which fail have unsigned char, so that's it I think. Either use a different type, cast it, or -fsigned-char should be added to the build system.

[Krzmbrzl]

I fail to see why this would be a problem other than perhaps causing compiler warnings. But to my understanding signed and unsigned char should be of the exact same byte size 🤔

[davidebeatrici]

The function used in the test behaves differently depending on whether the variable is signed:

mumble/src/PacketDataStream.h

Lines 140 to 192 in 73d8a4d

	PacketDataStream &operator<<(const quint64 value) {
	quint64 i = value;
	if ((i & 0x8000000000000000LL) && (~i < 0x100000000LL)) {
	// Signed number.
	i = ~i;
	if (i <= 0x3) {
	// Shortcase for -1 to -4
	append(0xFC | i);
	return *this;
	} else {
	append(0xF8);
	}
	}
	if (i < 0x80) {
	// Need top bit clear
	append(i);
	} else if (i < 0x4000) {
	// Need top two bits clear
	append((i >> 8) | 0x80);
	append(i & 0xFF);
	} else if (i < 0x200000) {
	// Need top three bits clear
	append((i >> 16) | 0xC0);
	append((i >> 8) & 0xFF);
	append(i & 0xFF);
	} else if (i < 0x10000000) {
	// Need top four bits clear
	append((i >> 24) | 0xE0);
	append((i >> 16) & 0xFF);
	append((i >> 8) & 0xFF);
	append(i & 0xFF);
	} else if (i < 0x100000000LL) {
	// It's a full 32-bit integer.
	append(0xF0);
	append((i >> 24) & 0xFF);
	append((i >> 16) & 0xFF);
	append((i >> 8) & 0xFF);
	append(i & 0xFF);
	} else {
	// It's a 64-bit value.
	append(0xF4);
	append((i >> 56) & 0xFF);
	append((i >> 48) & 0xFF);
	append((i >> 40) & 0xFF);
	append((i >> 32) & 0xFF);
	append((i >> 24) & 0xFF);
	append((i >> 16) & 0xFF);
	append((i >> 8) & 0xFF);
	append(i & 0xFF);
	}
	return *this;
	}

mumble/src/PacketDataStream.h

Lines 40 to 50 in 73d8a4d

	void append(const quint64 v) {
	#ifndef QT_NO_DEBUG
	Q_ASSERT(v <= 0xff);
	#endif
	if (offset < maxsize)
	data[offset++] = static_cast< unsigned char >(v);
	else {
	ok = false;
	overshoot++;
	}
	};

mumble/src/PacketDataStream.h

Line 34 in 73d8a4d

bool isValid() const { return ok; }

[Krzmbrzl]

How does

mumble/src/PacketDataStream.h

Line 143 in 73d8a4d

if ((i & 0x8000000000000000LL) && (~i < 0x100000000LL)) {

really check for the type of the variable passed? I would have interpreted as if it was merely checking whether the first bit is set, which afaik indicates a negative number, if the respective type is signed and has no special meaning for unsigned types 🤔
At least that's what I believe the first part does. What the second part with the inverted bits is trying to accomplish, I don't quite get 👀

[thesamesam]

If you want to avoid having to mess with the existing logic, you can build with -fsigned-char for now.

[davidebeatrici]

We can use int8_t, no problem. Would you like to create a pull request for it?

[Krzmbrzl]

I still fail to see how the issue can be in the variable's type. We're only testing its binary layout (the set bits) and afaik that doesn't change between signed and unsigned types 👀

[sirjeannot]

In the meantime, would be possible to republish the last working version for aarch64 (for the docker image).
mumble-voip/mumble-docker#3
thanks a lot

[Krzmbrzl]

From #5943 (comment):

TestPacketDataStream::space() fails on aarch64, due to the fact that by default 'char' is unsigned on arm unlike i386 where 'char' is signed.

Using gcc's '-fsigned-char' makes the test work.

FWIW this is enough to run the server. The client does not seem to work, it can not connect to any server, "Invalid Server" appears on the shell.

[boletus-edulis]

I still fail to see how the issue can be in the variable's type. We're only testing its binary layout (the set bits) and afaik that doesn't change between signed and unsigned types eyes

I do not know much C++, but in C the following test.c illustrates the behavior:

#include <stdio.h>

int
main(){
  char c = -2;

  printf("%hhx\n", c);

  // promoting a datatype retains sign
  printf("%hx\n", c);
  printf("%x\n", c);

  return 0;
}

x86_64 $ gcc -o test test.c
x86_64 $ ./test
fe
fffe
fffffffe

aarch64 $ gcc -o test test.c
aarch64 $ ./test
fe
fe
fe

x86_64 $ gcc -fsigned-char -o test test.c
x86_64 $ ./test
fe
fffe
fffffffe

aarch64 $ gcc -fsigned-char -o test test.c
aarch64 $ ./test
fe
fffe
fffffffe

To be more clear, this is happening in PacketDataStream.h:

PacketDataStream &operator<<(const quint64 value) {
PacketDataStream &operator>>(quint64 &i) {

looking at the test, we see the following:

	PacketDataStream out(buff, 1);

	char val = -2;

	out << val;

On x86_64, signed char is converted quint64, which includes the conversion from signed to unsigned.
On aarch64, unsigned char in converted to quint64, which basically only makes the allocation larger.

[Krzmbrzl]

@boletus-edulis thanks for the illustration! I think that finally cleared things up for me. Especially in addition with the chat I had with @davidebeatrici about this this morning, where I/we realized that even though PacketDataStream::operator<< takes its argument as an unsigned 64-bit integer, the code actually manually checks for the most significant bit and if that is set, it interprets the number as negative:

mumble/src/PacketDataStream.h

Lines 143 to 144 in 73d8a4d

	if ((i & 0x8000000000000000LL) && (~i < 0x100000000LL)) {
	// Signed number.

Combined with the fact that the conversion from signed to unsigned makes the difference that it makes (in particular whether we convert the signed value to unsigned char or then a step later to unsigned int64, I think I now understand what is going on.
Let's see if I can get it fixed :D

[Krzmbrzl]

TODO: Make use of https://github.com/Krzmbrzl/cmake-compiler-flags to set the compiler flags to ensure signed chars on all platforms.
(This seems most robust as our code might depend on char-signedness in different places as well)