Replace mem family function with typesafe functions (fixes #5228)

[Chairn]

This adds more type safety into the code. Generated code should be similar with no performance impacts. Fixes https://github.com/ddnet/ddnet/issues/5228.

I kept mem_zero and mem_copy on default language type.

Reference: https://en.cppreference.com/w/cpp/algorithm/copy (allow overlap if dest starts left of source) https://en.cppreference.com/w/cpp/algorithm/copy_n (no overlap allowed) https://en.cppreference.com/w/cpp/algorithm/copy_backward (allow overlap if dest ends right of source)

Checklist

• [x] Tested the change ingame
• [ ] Provided screenshots if it is a visual change
• [ ] Tested in combination with possibly related configuration options
• [ ] Written a unit test (especially base/) or added coverage to integration test
• [x] Considered possible null pointers and out of bounds array indexing
• [x] Changed no physics that affect existing maps
• [ ] Tested the change with ASan+UBSan or valgrind's memcheck (optional)

[Chairn]

Windows segfault is triggered in chkstk function. Looks like stack space is limited on windows: https://stackoverflow.com/questions/8400118/what-is-the-purpose-of-the-chkstk-function

And i have no idea how to fix clang-tidy...

[heinrich5991]

Generated code should be similar with no performance impacts.

Have you verified this?

[Chairn]

I have for mem_zero functions (https://godbolt.org/z/6d17Teq6W). Not for mem_copy yet.

[heinrich5991]

I have for mem_zero functions (https://godbolt.org/z/6d17Teq6W). Not for mem_copy yet.

It seems that for arrays, the code generation is worse than memset.

[Chairn]

Personally, I think this decreases readability in some cases. mem_zero zeroes out the structure, but if I see a constructor, I have to chase down some more code. What do others think?

But mem_zero is not type safe, we had the case when @C0D3D3V added std::set. Default constructor is guarenteed to zero initialize all members or call their default constructor as well. In my debugging, i've found some classes that were zeroed although a member had a default constructor setting some pointer (CStaticRingBuffer for instance).

For mem_copy, the same applies but in our codebase, i think it doesn't change anything until someone add a non copyable member to a class (like std::vector for instance).

If we ever move to c++20, we could also replace the mem_comp function with default equality operator.

Edit: for the readability, we can move all the ugly code inside the mem_zero/mem_copy function with the templated version, that could ease things and also reduce the diff size.

[heinrich5991]

But mem_zero is not type safe

You're probably saying that it's not safe to apply mem_zero to non-POD types? Instead of removing every reference to mem_zero, would it be possible to only replace mem_zero for non-POD types and add a static check for the PODness of parameters passed to mem_zero? That would probably be a less intrusive change?

[Chairn]

Yup that's the edit part in my comment, not sure if you commented after or before i edited it :D

Edit: for the readability, we can move all the ugly code inside the mem_zero/mem_copy function with the templated version, that could ease things and also reduce the diff size.

[Chairn]

Note: the clang-tidy fails are not introduced by this PR, they are just revealed because mem_functions are now declared and defined when compiling other files. You can test first commit without the static_assert to see that. Doesn't change the fact i still need to find a way around (i.e. fix them)

[Jupeyy]

template<class T>
void mem_zero(T *block, unsigned size)
{
	static_assert(!std::is_polymorphic<T>::value, "Used polymorphic class inside mem_zero");
	memset(block, 0, size);
}

It found one error:

static CClient *CreateClient()
{
	CClient *pClient = static_cast<CClient *>(malloc(sizeof(*pClient)));
	mem_zero(pClient, sizeof(CClient));
	return new(pClient) CClient;
}

Which might be ok in this case, but its still weird, that its coded like this xDD completly random, probs bcs mem_alloc had some checks back in the days

[Chairn]

This one is also triggered by my asserts. Actually, there were more in the code, this one that is safe because the constructor is called after. The other one that is not safe and fixed by this PR: https://github.com/ddnet/ddnet/blob/35280a7a2fad69e15486aa9cfd4240d757a6c495/src/engine/shared/network_server.cpp#L49 This one isn't safe either: https://github.com/ddnet/ddnet/blob/35280a7a2fad69e15486aa9cfd4240d757a6c495/src/game/client/gameclient.cpp#L1124-L1127

[heinrich5991]

The other one that is not safe and fixed by this PR:

These classes are not POD? That surprises me.

[Chairn]

The other one that is not safe and fixed by this PR:

These classes are not POD? That surprises me.

No, they are not trivial, i.e. they have a default constructor (or a member variable that has a default constructor) that is overridden by the call to mem_zero. CStaticRingBuffer is the culprit in this case (here https://github.com/ddnet/ddnet/blob/35280a7a2fad69e15486aa9cfd4240d757a6c495/src/engine/shared/ringbuffer.h#L49-L55).

[Learath2]

Hm, does !is_polymorphic really imply is safe to zero out? Maybe is_trivially_copyable is also needed to ensure it's gapless?

[Chairn]

Personally, I think this decreases readability in some cases. mem_zero zeroes out the structure, but if I see a constructor, I have to chase down some more code. What do others think?

Should i create another function (mem_init?) to encapsulate constructor as it does not guarentee memory to be zero, just that the class is correctly initialized or reset (which in most cases is equal to zero initialize) ?

[Jupeyy]

before the diff gets too big, lets decide what we want

i personally don't care going with std::copy, tho you can probs replace most mem_zeros with standard classes list std::array<whatever, size> p = {};

and dont need any of it. Similar for other functions, so this change might be overkill and instead we should focus on doing above in future more and refactor parts whenever its needed/someone wants to do it

[Chairn]

I still need to work on that one and im willing to carry on working on it (when i'll have time though...)

[heinrich5991]

Status: Waiting for author, @Chairn has recently said that they're still willing to work on it.

[Chairn]

But mem_zero is not type safe

You're probably saying that it's not safe to apply mem_zero to non-POD types? Instead of removing every reference to mem_zero, would it be possible to only replace mem_zero for non-POD types and add a static check for the PODness of parameters passed to mem_zero? That would probably be a less intrusive change?

This is what i came up with, what do you think?

template<typename T>
inline void mem_zero(T *block, unsigned size)
{
	static_assert(std::is_same<T, void>::value || std::is_trivial<T>::value || std::is_standard_layout<T>::value);

	memset(block, 0, size);
}
template<typename T>
inline void mem_zero(T& block, size_t size)
{
	new(&block) T{};
}
template <typename T, size_t size> 
inline void mem_zero(T (&block)[size])
{
	new(block) T[size]{};
}

[heinrich5991]

Superseded by #6256.