RPMLint reports serios compiler warning in Font.cpp

[susnux]

When compiling for openSUSE 42.2 and 42.3 (not yes released) I get this error:

Serious compiler warning found:
I: Program is likely to break with new gcc. Try -fno-strict-aliasing.
W: sfml2 strict-aliasing-punning /home/abuild/rpmbuild/BUILD/SFML-2.4.1/src/SFML/Graphics/Font.cpp:328

GCC 4.8 is used for native builds.

[binary1248]

This is a known issue that has been reported for a while by SFML's own CI static analysis pass:
https://ci.sfml-dev.org/builders/static-analysis/builds/312/steps/cppcheck/logs/warnings%20%2816%29

[LaurentGomila]

This is the corresponding code:

    // Build the key by combining the code point, bold flag, and outline thickness
    Uint64 key = (static_cast<Uint64>(*reinterpret_cast<Uint32*>(&outlineThickness)) << 32)
               | (static_cast<Uint64>(bold ? 1 : 0) << 31)
               |  static_cast<Uint64>(codePoint);

The cast is safe here because we just want a unique key, we are not interested in the value itself. However, this means that outlineTichkness values that differ by tiny amount (like 1e-5) will produce distinct keys and thus produce two different glyphs although they will visually be similar in terms of pixels.

I would rather truncate the floating point value to a fixed number of decimals, multiply by the corresponding power of ten to make it integer, and perform a true cast to int32. This would remove the warning, and ensure that outlineThickness values that are close enough will use the same glyph.

[binary1248]

I propose a mapping function such as this:

#include <cstring>
#include <cstdint>

template <class T, class U>
inline T reinterpret(const U& input)
{
    T output;
    std::memcpy(&output, &input, sizeof(U));
    return output;
}

std::uint64_t combine(float outlineThickness, bool bold, std::uint32_t codePoint)
{
    return (reinterpret<std::uint64_t>(outlineThickness) << 32) | (static_cast<std::uint64_t>(bold) << 31) | codePoint;
}

https://godbolt.org/g/5wefPf

This will get rid of the warning and not even incur any performance penalty over the current implementation. The problem (and justification for the warning) is that the current implementation is allowed to invoke undefined behaviour when compiled by optimizing compilers. Even though we are telling the compiler to read the same address "in a different way" it will have no objections to reading from a completely different address because it assumes that pointers to objects of different types cannot reside at the same address.

@LaurentGomila The solution you propose would work in trivial cases, however there are edge cases where we really shouldn't try to be overly smart. If the user for whatever reason wants to scale glyphs using an sf::View, this would unnecessarily force them to make due with the fact that glyphs are only rasterized by SFML in discrete size increments. Considering that we have enough bits in the key to represent the 32+1+21 bits required, and the user has to explicitly request text with very similar sizes (i.e. this can't unintentionally happen), I can't see any reason why we should go through the extra effort and risk looking for a solution to a problem that doesn't exist.

[jcowgill]

@binary1248 that looks broken on big-endian to me

[binary1248]

@jcowgill How so?

std::memcpy is endian-independant unless it is completely broken and arithmetic operations are guaranteed to be endian-independant by the language standards, see here. The fact that we are forcing the binary contents of an int into a float already throws any sense of value preservation out the window. The only thing we are interested in here is getting a 1:1 bijective mapping from any (float, bool, int31) triple to uint64_t. We aren't interested in the actual value of the result.

[jcowgill]

Example:

#include <iostream>
#include <iomanip>
#include <cstring>
#include <stdint.h>

template <class T, class U>
inline T reinterpret(const U& input)
{
    T output {};
    std::memcpy(&output, &input, sizeof(U));
    return output;
}

int main(void)
{
    uint32_t x = 0xDEADBEEF;
    std::cout << std::uppercase << std::hex << std::setw(16) << std::setfill('0');
    std::cout << reinterpret<uint64_t>(x) << std::endl;
}

Little endian prints: 00000000DEADBEEF
Big endian prints: DEADBEEF00000000

Therefore reinterpret<std::uint64_t>(outlineThickness) << 32 in your example is always 0 on big-endian.

Note that you need T output {} otherwise the value of output is undefined.

[LaurentGomila]

True, it should rather be static_cast<Uint64>(reinterpret<Uint32>(outlineThickness)) << 32.

[binary1248]

@jcowgill Thanks for pointing that out. Indeed I overlooked that I was passing 2 differently sized types to reinterpret. @LaurentGomila's version is what it should look like.

#include <cstring>

template <typename T, typename U>
inline T reinterpret(const U& input)
{
    T output;
    std::memcpy(&output, &input, sizeof(U));
    return output;
}

Uint64 combine(float outlineThickness, bool bold, Uint32 codePoint)
{
    return (static_cast<Uint64>(reinterpret<Uint32>(outlineThickness)) << 32) | (static_cast<Uint64>(bold) << 31) | codePoint;
}

[pranavk]

So, whats stopping us to commit this now ?

[eXpl0it3r]

I've been busy with releasing 2.4.2 and exams the past few weeks. If @binary1248's solution is accepted by everyone, I could or binary1248 could create a PR for it.

[aggsol]

Is this already merged?

[binary1248]

See #1396.