Azure / azure-uamqp-c

AMQP library for C
Other
58 stars 63 forks source link

amqpvalue_encode - AMQP_TYPE_FLOAT missing #73

Open mario19911 opened 7 years ago

mario19911 commented 7 years ago

Hey guys,

I had some problems when I tried to encode a float with amqpvalue_create_float and figured out that the case statement for AMQP_TYPE_FLOAT is missing in the switch of the function amqpvalue_encode. Is this intended or an issue?

Best regards, Mario

dcristoloveanu commented 7 years ago

Hi @mario19911,

Thanks for reporting it, it is a miss, we'll definitely fix it.

Thanks, /Dan

dcristoloveanu commented 7 years ago

Hi @mario19911

PR #76 has been created to fix this and add float and double support. It should get checked in to master soon.

Thanks, /Dan

mario19911 commented 7 years ago

Hi @dcristoloveanu

depending on the platform, a float can be represented as little or big endian. Shouldn't there be a check about that to be platform independent? I got an example here:

static bool f_float32_litte;

void check_float (void) { unsigned char * p_tmp; float test;

test = 1; p_tmp = (unsigned char *)&test;

if ((p_tmp[0] == 0x00) && (p_tmp[1] == 0x00) && (p_tmp[2] == 0x80) && (p_tmp[3] == 0x3F)) { f_float32_little = true; } else { f_float32_little = true; }

}

Best regards, Mario

dcristoloveanu commented 7 years ago

Hi @mario19911

AMQP uses network byte order so that machines that different endianess can talk to each other. By casting to uint32_t* and putting the high byte of the uint32_t value first the network byte order is achieved, so that should be fine.

The PR is now merged, could you please run this against your test to make sure the issue is fixed for you? If not, of course I'll help fix it :-).

Thanks, /Dan

mario19911 commented 7 years ago

Hi @dcristoloveanu

I don't think that the problem of the endianess is solved with this implementation. When the float is internally represented as Big Endian, you still have the problem of a wrong byte order. In that case the order needs to be changed. Either you need a define for the endianess or you can check the order of the float bytes with the example above. Example 1 as float in Big Endian is 0x3F800000 in Little Endian its 0x0000803F.

Best regards, Mario

dcristoloveanu commented 7 years ago

@mario19911

The endianess applies to both uint32_t and float/double. So by casting to (uint32_t*) and using the 4 bytes as a 32 bit integer makes sure that the high byte of the uint32_t is the high byte of the float. The code will put/get on the wire always the high byte of the uint32_t first (which is the high byte of the float).

To alleviate your concerns I made a rather simple test. I fired up a QEMU Debian Wheezy image for PowerPC. QEMU can be found here: http://www.qemu-project.org/download/#windows. You can use it on Linux or Windows. Debian Wheezy image link is in this post: http://create.stephan-brumme.com/big-endian/

Mind you this is not HW virtualization so do expect this to be quite slow if you give it a try. Compiling cmake from sources for example took .... forever. But once setup is a nice test bed.

I modified the local_client and local_server samples to receive/send a float and a double as key/value pair in the application properties of the messages. The code is in the float_test. Replace the IP of course with your own.

Here is a screenshot with the result: image

The server was run on my x64 machine. The client is run on the QEMU PowerPC.

As you can see on the Big endian PowerPC machine the bytes in memory are in opposite order than on my local x64 machine which is little endian.

Nevertheless they are properly sent/received by the client/server sample.

Given this I will close this issue. Do let me know if you have any more concern around it though.

Cheers! /Dan

mario19911 commented 7 years ago

@dcristoloveanu

in that case it worked since at the Big-Endian machine the float is also Big-Endian and on the Little-Endian machine it is Little-Endian. There are some processors (e.g. the ARM 7) where the endianess of the platform and the endianess of the float are different. There you would need to check separately, how the float is encoded before sending.

dcristoloveanu commented 7 years ago

@mario19911

Interesting, I reopened the issue, let me look at the ARM7 case.

Cheers, /Dan

dcristoloveanu commented 7 years ago

Hi @mario19911

You are right that there are some cases where float endianess is different than integer endianess. Apparently there are some cases also where the 64bit double has the endianess of integers, but the 2 32bit values are stored in reverse order.

According to Wikipedia though these are very rare cases (https://en.wikipedia.org/wiki/Endianness#Floating_point):

"Floating point[edit] Although the ubiquitous x86 processors of today use little-endian storage for all types of data (integer, floating point, BCD), there are a few historical machines where floating point numbers are represented in big-endian form while integers are represented in little-endian form.[17] There are old ARM processors that have half little-endian, half big-endian floating point representation for double-precision numbers: both 32-bit words are stored in little-endian like integer registers, but the most significant one first. Because there have been many floating point formats with no "network" standard representation for them, the XDR standard uses big-endian IEEE 754 as its representation. It may therefore appear strange that the widespread IEEE 754 floating point standard does not specify endianness.[18] Theoretically, this means that even standard IEEE floating point data written by one machine might not be readable by another. However, on modern standard computers (i.e., implementing IEEE 754), one may in practice safely assume that the endianness is the same for floating point numbers as for integers, making the conversion straightforward regardless of data type. (Small embedded systems using special floating point formats may be another matter however.)"

I can detect the endianess by having -1.0 checked in memory, this would be an extra float op (not horrible though).

Before doing that I wanted to check whether you are actually hitting this on your platform or is it a theoretical case?

Cheers, /Dan

mario19911 commented 7 years ago

Hi @dcristoloveanu

with out platform there is no such problem, it is more a theoretical case. So for the moment it will work fine. We just wanted to mention this case if the stack needs to be implemented by somebody on such a machine.

Thanks, Mario

dcristoloveanu commented 7 years ago

@mario19911 Thanks for clarifying. I will mark this as an enhancement and keep the issue open as a rainy day project.

Cheers, /Dan