pothosware / PothosCore

The Pothos data-flow framework
https://github.com/pothosware/PothosCore/wiki
Boost Software License 1.0
302 stars 48 forks source link

self-test failure on 32-bit arm (zynq) #164

Closed wluker closed 5 years ago

wluker commented 5 years ago

After installing PothosComms I ran PothosUtil --self-tests and got the following error.

Testing /comms/tests/test_rotate... FAIL!
 +-------------------------------------------------------------------------+
 | Testing /comms/tests/test_rotate...                                     |
 | Testing rotate with type complex_float64, phase 0*pi                    |
 | Testing rotate with type complex_float32, phase 0*pi                    |
 | Testing rotate with type complex_int64, phase 0*pi                      |
 | Exception: Pothos::Testing TestRotate.cpp:52                            |
 |   assert close 10-20j ~= 10+4294967276j                                 |
 |   statement "(std::abs((pOut[i]) - (expected)) <= (1))" evaluated false |
 +-------------------------------------------------------------------------+

Failed 1 out of 129 tests
  FAIL: /comms/tests/test_rotate

Specifically the failing test in TestRotate.cpp is testRotateTmpl<int64_t>(phase);. If I comment out the int64_t test the other integer tests pass, so this is probably something to do with a 32-bit system.

guruofquality commented 5 years ago

Thats special, the first test is actually phase 0, and it looks like the actual processing block got the correct answer 10-20j and 10+4294967276j is the complex double logic approximating it, which is totally off.

        const auto input = std::complex<double>(pIn[i].real(), pIn[i].imag());
        const auto expected = std::complex<Type>(input * std::polar(1.0, phase));

Given that the double version of the block actually worked, I would have to guess that somehow const auto input = std::complex<double>(pIn[i].real(), pIn[i].imag()); failed. But thats crazy, its just converting the numbers 10 and 20 as long longs to doubles.

Anyway, not really sure what went wrong, can you print some intermediate values from the lines above and see where things went wrong?

wluker commented 5 years ago

I think the issue is size_t i in:

    for (size_t i = 0; i < buffIn.elements(); i++)
    {
        pIn[i] = std::complex<Type>(Type(10*i), Type(-20*i));
    }

This program:

#include <iostream>

int main() {
  size_t i = 1;
  int64_t q = int64_t(-20*i); 
  std::cout << q << std::endl;
  return 0;
}

prints : 4294967276 and when I use int i=1 I get the expected -20. Somehow mixing unsigned and signed types doesn't work as expected for int64_t on a 32-bit platform?

guruofquality commented 5 years ago

This post Talks about what happens when multiplying signed and unsigned integers. Short answer is, as long as they are the same rank (size), a signed is implicitly typecast to unsigned.

So it looks like -20 (an int) times 1 (an unsigned long) probably becomes an unsigned 32-bit type. In this case the contents of the resulting type is not -20, but 4294967276 which is the unsigned equivalent. So typecasting this unsigned 32-bit type to a signed 64-bit type basically stores the value as-is which fits into the 64-bit type and does not sign extend it, because its unsigned.

On a 64-bit platform, the size_t is already 64-bits, so its just fine.

So good find, I will just be more explicit with the type.

guruofquality commented 5 years ago

Pushed a fix, this is in PothosComms project: https://github.com/pothosware/PothosComms/commit/17d9cda18083e098890846427b4643ea1a015c65