Alter FailFast behaviour of memcpy

[mjp41]

This commit changes the codegen for error messages for failed memcpys.
This no longer generates a stack frame and correctly tail calls the
error messages generator.

It also turns the error messages on in Release builds. This will lead
to better adoption experience.

[mjp41]

@davidchisnall I am not sure if I should make this part of the change. It is required to get it to print stack traces, but systems could use core dumps or debuggers to get those.

[davidchisnall]

On non-glibc systems, the backtrace thing is in a separate library, so it's probably not a good idea to include it. In a non-debug build, you won't have the unwind tables and so it may be able to fall back to trying to follow chains of frame pointers and return addresses, but outside of AArch64 those aren't mandated to exist by the ABI, so there's no guarantee that the backtrace will actually contain anything useful.

[mjp41]

Reverted

[davidchisnall]

I think we probably need to iterate on this a bit more. The original code was a set of tools that could be used to add bounds checks to multiple routines, with an example in memcpy. The new version is specific to memcpy and is not useable elsewhere, yet maintains the separation into generic and memcpy-specific files.

[davidchisnall]

On non-glibc systems, the backtrace thing is in a separate library, so it's probably not a good idea to include it. In a non-debug build, you won't have the unwind tables and so it may be able to fall back to trying to follow chains of frame pointers and return addresses, but outside of AArch64 those aren't mandated to exist by the ABI, so there's no guarantee that the backtrace will actually contain anything useful.

[davidchisnall]

Suggested change

	* off. It defaults to false for both debug and release builds and
	* off. It defaults to false and

[mjp41]

Made a slightly different rewrite, but thanks for pointing out the overcomplicated nature of what I had put.

[davidchisnall]

Can you file an LLVM bug about this? It's the exact opposite of what I'd have expected: you should always be able to tail call a noreturn function because nothing cares about the state of the stack above it and so you can completely corrupt any prior argument frame without affecting the program. This may only be possible with noexcept functions, because I think noreturn functions can still throw.

[mjp41]

It is a known feature from what I understand:
https://stackoverflow.com/questions/55656938/why-noreturn-builtin-unreachable-prevents-tail-call-optimization

[davidchisnall]

I don't like that this has gone away because I was using it in other functions. The goal for this header was to provide a set of tools that could be used for wrappers around other libc functions.

[mjp41]

I have moved the if constexpr back inside, so you can now do

check_bounds<CheckReads>(...)

Is this acceptable? Could you elaborate on where you feel the CheckDirection approach would be clearer?
It seems you have to pass two parameters

check_bounds<CheckDirection::Read, CheckReads>(...)

I have updated the comments and error message changes to not be memcpy specific, and have added a FakeReturn, so that different things could be tail called in case the function needs a different return type.

[davidchisnall]

The check needs two pieces of information:

• Is this a read or a write?
• Are we checking reads or writes?

I'm not sure how you encode that in a single template parameter.

I am using ifunc resolvers to instantiate the outer functions multiple times so that I get two versions:

• No checks
• Checks on writes
• Checks on reads and writes

If it's a wrapper, then I use the underlying function for the no-checks version. I can then switch between them based on an environment variable at process-launch time. As long as I can still do that concisely, I'm happy.

Making check_bounds take two arguments meant that I didn't need any if constexpr in the outer function, which simplifies my wrappers considerably because they just unconditionally forward their arguments. My ifunc resolver is, itself, generated from a templated function that takes a template template parameter and instantiates it with the wrapper version to generate (read or write checks).

Most of the libc function wrappers can be a simple template that calls check_bounds<> on any read/write pointer arguments and then forwards to the underlying implementation.

[mjp41]

So with a single template parameter I can do the following, which has no if constexpr in the caller.

    // Check the bounds of the arguments.
    if (SNMALLOC_UNLIKELY(!check_bounds<Checked && ReadsChecked>(src, len)))
      return report_fatal_bounds_error(
        src, len, "memcpy with source out of bounds of heap allocation");
    if (SNMALLOC_UNLIKELY(!check_bounds<Checked>(dst, len)))
      return report_fatal_bounds_error(
        dst, len, "memcpy with destination out of bounds of heap allocation");

I can push the SNMALLOC_UNLIKELY inside the call, but it doesn't seem to bias the branches correctly.

It has to be two calls to get the good codegen, when we have error messages as we need to conditionally tail call the error message.

[davidchisnall]

Fair enough. I can always wrap the two calls in a macro.

[mjp41]

So a macro would work, but you will probably need two versions one for a void returning function, and another that takes a type parameter for a not void function.

[davidchisnall]

One of my favourite small parts of C++ is that you can return an expression that evaluates to a void type from a function with a void return, so this should be possible to make generic.

[davidchisnall]

When we're just doing this for memcpy, the increase in (source) code size in the checked function is fine. My intent with this code was that we'd be able to gradually instrument any libc (or external) function that takes pointer arguments with bounds checks. There's going to be a lot more code duplication for this in the new version than the old.