gnuradio / pmt

pmt
GNU Lesser General Public License v3.0
11 stars 11 forks source link

Add support of std::size_t for Emscripten with serialization to uint64_t #101

Closed drslebedev closed 1 year ago

drslebedev commented 1 year ago

This PR introduces std:size_t support for Emscripten.

First check if std::size_t has the same type as uint16_t or uint32_t or uint64_t. If it has the same type, then there is no need to add std::size_t the supported types. Otherwise, std::size_t is added to the supported types. This can happen, for example, if one builds using Emscripten where std::size_t is defined as unsigned long and uint32_t and uint64_t are defined as unsigned int and unsigned long long, respectively.

The problem can be illustrated with this code:

static constexpr std::variant<uint32_t, uint64_t> var = static_cast<std::size_t>(32); 

It does not compile for Emscripten with the error:

error: no viable conversion from 'std::size_t' (aka 'unsigned long') to 'const std::variant<uint32_t, uint64_t>' (aka 'const variant<unsigned int, unsigned long long>')

With this PR the above code compiles and one can create pmt from std::size_t without additional cast:

pmt x = static_cast<std::size_t>(42);

Important Notice that std:size_t is always serialized to uint64_t, no matter what exact type it has.

jsallay commented 1 year ago

Do you believe that this will work on a system with 32-bit size_t? I think that serialization would be broken because in that case. I could probably be okay with that because the space of 32-bit applications is constantly diminishing.

drslebedev commented 1 year ago

Here is a test case for std::size_t. I tested it also with Emscripten where std::size_t is 32 bits and everything works.

jsallay commented 1 year ago

I've been trying to work out all the possibilities here. I went onto godbolt and ran the following code:

#include <cstdint>
#include <iostream>

int main() {
    if constexpr(std::is_same<size_t, unsigned long>()) std::cout << "size_t is long\n";
    if constexpr(std::is_same<size_t, unsigned long long>()) std::cout << "size_t is long long\n";
    if constexpr(std::is_same<size_t, unsigned>()) std::cout << "size_t is int\n";
    if constexpr(std::is_same<uint64_t, unsigned long>()) std::cout << "uint64_t is long\n";
    if constexpr(std::is_same<uint64_t, unsigned long long>()) std::cout << "uin64_t is long long\n";
    return 0;
}
I ran it for LP64 Linux gcc, ILP32 Linux gcc (-m32 compiler flag), LLP64 windows MSVC, and ILP32 Windows MSVC. Note that I've left out the unsigned in the types in the below table for brevity. Model size_t type uint64_t type
LP64 Linux long long
ILP32 Linux int long long
LLP64 Windows long long long long
ILP32 Windows int long long

Since windows and linux use the same types with the ILP32 model, we have 3 cases to worry about (LP64, LLP64, and ILP32). I looked at what happens if we serialize in one model and receive in another. Does the underlying type change? If we always serialize a size_t like a uint64_t then we have the following table ( I am going to use uint64_t/uint32_t for the serialized types):

From Model To Model Serial Type Rx Data Type Desired Type Breaking Type Change?
LP64 LP64 uint64_t unsigned long unsigned long no
LP64 LLP64 uint64_t unsigned long long unsigned long long no
LP64 ILP32 uint64_t unsigned long long unsigned int yes
LLP64 LP64 uint64_t unsigned long unsigned long no
LLP64 LLP64 uint64_t unsigned long long unsigned long long no
LLP64 ILP32 uint64_t unsigned long long unsigned int yes
ILP32 LP64 uint32_t unsigned int unsigned long yes
ILP32 LLP64 uint32_t unsigned int unsigned long long yes
ILP32 ILP32 uint32_t unsigned int unsigned int no

It looks like with the code that you have; we can serialize and receive size_t just fine (maintaining the correct underlying type) as long as we don't go between 32 and 64 bit systems. I think that is acceptable.

I can open a separate issue for this, but we could add the data model to the serialized data. On the receive side, I could detect if the data is crossing a 32/64 bit boundary and print a warning to the user that things may not work properly for certain types.

drslebedev commented 1 year ago

Thank a lot for sharing your results.

The underline type is always uint64_t which was already supported before in pmt and your results proved to me that everything works as expected.

I agree that 64->32 bits problem should be addressed in a separate issue since it is not directly related to support of std::size but in general to int64_t and uint64_t support. It is possible to detect if type size was changed and print some warning to the user.