unresolved external symbol "unsigned char __cdecl sbepp::detail::byteswap<unsigned char>(unsigned char)"

[ujos]

When I build my application based on sbepp v1.3.0 with Visual Studio 2019/2022, I get a link time error:

error LNK2001: unresolved external symbol "unsigned char __cdecl sbepp::detail::byteswap<unsigned char>(unsigned char)" (??$byteswap@E@detail@sbepp@@YAEE@Z)

Looks like the problem is caused by the following declaration without the definition:

sbepp\sbepp.hpp:(464)

// because `if((E == endian::native) || (sizeof(T) == 1))` is not a constexpr-if
// this function has to be declared for single byte types even if it will never
// be used
template<typename T>
T byteswap(T) noexcept;

g++ 11/12 compiles the code just fine.

Please let me know if you need a full source code to reproduce the issue.

[OleksandrKvl]

Interesting, seems the issue is in this section: https://github.com/OleksandrKvl/sbepp/blob/main/sbepp/src/sbepp/sbepp.hpp#L569-L576. MSVC somehow can't figure out from that if-condition that byteswap is never used for single-byte types and that it's always known at compile-time. One solution might be to define that function, although it's never used. I will think if there's a better way.

[ujos]

Try constexpr:

    if constexpr((E != endian::native) && (sizeof(T) != 1))
    {
        value = from_unsigned<T>{}(byteswap(to_unsigned(value)));
    }

At least that might be a workaround for our case as we've migrated to C++17

[OleksandrKvl]

It will work for C++17 but not for C++11/14.

[OleksandrKvl]

Can you please check if the fix from PR works for you? I don't have access to MSVC compiler at the moment.

[ujos]

As far as you have the template specialization for the 1-byte variables, do you still need that if inside the set_primitive?

[OleksandrKvl]

Removing sizeof(T) == 1 part from if((E == endian::native) || (sizeof(T) == 1)) is possible but if we do so, we will call from_unsigned<T>{}(byteswap(to_unsigned(res))); part even for single-byte types. That's essentially a no-op but it will instantiate byteswap, to_unsigned and from_unsigned to do nothing so I'm not sure it's a good idea.

[ujos]

Maybe better then to specialize the set_primitive / get_primitive functions:

1. One for native (regardless of T size)
2. One for non-native in case it sizeof(T) == 1
3. One for non-native in case it sizeof(T) != 1

[ujos]

Actually two specializations are needed:

#include <type_traits>
#include <cstring>

enum endianness
{
    little,
    big,
};

constexpr auto NativeEndianness = endianness::little;

template<endianness E, typename T>
typename std::enable_if<E == NativeEndianness || sizeof(T) == 1>::type
    set_primitive(void* addr, T v)
{
   memcpy(addr, &v, sizeof(v));
}

template<endianness E, typename T>
typename std::enable_if<E != NativeEndianness && sizeof(T) != 1>::type
    set_primitive(void* addr, T v)
{
   auto const reversed = byteswap(v);
   memcpy(addr, &reversed, sizeof(reversed));
}

char buf[120];

int main()
{
   set_primitive<endianness::little>(buf, '1');
   set_primitive<endianness::little>(buf, 1);
   set_primitive<endianness::big>(buf, '1');
   set_primitive<endianness::big>(buf, 1);
}

[OleksandrKvl]

that will only cause code duplication without any benefits, existing PR fixes the problem with only a couple of lines.

[ujos]

Ok, up to you.

I will try to verify your change as quickly as possible. I need to switch from Conan to Git repo somehow.

[ujos]

that will only cause code duplication

Actually the way as I proposed is better, as it makes code more clear. It removes IF statement out of set_primitive and makes it straight and forward.

[ujos]

Can you please check if the fix from PR works for you? I don't have access to MSVC compiler at the moment.

That works. Thank you very much

[OleksandrKvl]

Good, then I'm merging, thank you for reporting and testing the fix!

[OleksandrKvl]

@ujos do you want this fix in Conan ASAP? I'm thinking if I should wait till the end of the week, maybe you'll discover more MSVC-related problems.

[OleksandrKvl]

Actually, I was wrong about avoidance of from_unsigned<T>{}(byteswap(to_unsigned(res)));. Since it's not a constexpr-if, this part is always compiled even if never used so there's no space for optimization at all. I will consider further improvements in this area.

[ujos]

@ujos do you want this fix in Conan ASAP? I'm thinking if I should wait till the end of the week, maybe you'll discover more MSVC-related problems.

That was the only issue I found so far. That would be just great to have a release in the Conan next week.

[OleksandrKvl]

@ujos you can monitor this PR to know when it's available on CCI.