Fixed misaligned memory access flagged by UBSan

[seanm]

Use memcpy to copy correctly even for unaligned memory. This was already done for some functions here, but not all.

Also took the oppurtunity to remove a bunch of seemingly obsolete/commented code.

[seanm]

@WardF @DennisHeimbigner here's another small PR hoisted from #2692

[WardF]

Hah, this looks very much like what I'm working on in #2799. Thanks!

[dopplershift]

Good sign that running UBSan on a regular basis in CI might have some value.

[seanm]

Hah, this looks very much like what I'm working on in #2799. Thanks!

That's a shame that you restarted / rediscovered what I'd already done 6 months ago in #2692.

Good sign that running UBSan on a regular basis in CI might have some value.

I've used it in many projects and its a godsend.

[WardF]

@dopplershift No doubt.

[WardF]

@seanm Indeed, although sadly it can be deceptively tricky. In this case, running this change/the change submitted in #2799 fixes the warnings under Linux and MacOS, but results in a crash on Windows/Visual Studio. So, it'll take a little bit of doing to figure out what is going on. But it's absolutely for the best we eliminate undefined behavior, so thank you very much!

[WardF]

That said, I'll be picking up with this in the AM; thanks again for the PR's, we'll get these sorted.

[DennisHeimbigner]

I would have thought that using 2 byte copies would be faster than
using memcpy:
something like this:
unsigned char* op = (unsigned char*)dst;
unsigned char* ip = (unsigned char*)src;
for(i=0;i<nnsizeof(uint16_t);i+=sizeof(uint16_t)) {
op[i] = ip[i+1]; / instead of swap2 */
op[i+1] = ip[i];
}

[seanm]

I would have thought that using 2 byte copies would be faster than using memcpy:

memcpy() is highly optimized by compilers. Let me see if I can create a demo on godbolt...

[seanm]

@DennisHeimbigner so here's the current code vs my proposal: https://godbolt.org/z/nrY8fnhaW

It's less CPU instructions, so hopefully faster.

Your proposal, if I'm not mistaken, won't work when dst == src, which the existing comments suggests needs to be supported.

[seanm]

@WardF so all was ok on Windows afterall?

[WardF]

There was a Visual Studio update, and the issue went away. A little more testing convinced me that the issue wasn't being introduced by the PR, it was environmental or something in VS, and either way, I wasn't introducing something we'd have to chase down later. So yeah, it looks like it was ok. Thanks!