Fix GAPBIND14_TRY memory leak

[Joseph-Edwards]

This PR fixes a memory leak in the GAPBIND14_TRY macro that was a result of the awkward interaction between C- and C++-style error handling. A summary of how we address this is included above the macro definition, and repeated below for clarity:

Care needs to be taken when calling the function ErrorQuit, since it handles errors in a C-style way with longjmp. This can cause issues with the destructors of non-trivially-destructable objects. In particular, if ErrorQuit is called within a catch block, the exception object's destructor is sometimes not called. This can leak memory.

To combat this issue, we avoid calling ErrorQuit inside a catch block, thus allowing the exception object to be destroyed as normal. Instead, we store the information of the error message in a static std::string, and pass that to ErrorQuit outside of the catch block.

This fix initially introduced a warning "control reaches end of non-void function", so we introduced the macro GAPBIND14_TRY_WITH_RETURN to fix this.

[Joseph-Edwards]

This is a draft either until we have a valgrind CI job that can check if this actually fixes the memory leak, or until I can verify this locally on my PC. I have tested it on small, quick examples, but will feel more confident once valgrind has run the full standard test suite.

[james-d-mitchell]

This looks good, I think we should do two things:

1. Rebase this on #1104 so that we can see if this fixes the issues with valgrind or not;
2. check that __builtin_unreachable exists during configuration, and error out if does not.

[Joseph-Edwards]

1. Rebase this on Add a Valgrind GitHub Actions workflow #1104 so that we can see if this fixes the issues with valgrind or not;

I don't think this will resolve the issue we are currently facing in #1104. From what I can tell, there is a function that calls KnuthBendix that seems to take a very very long time to run on valgrind. I'm going to modify that PR so that we don't run the slow valgrind tests, and then I will rebase in the hope that we get no leaks.

2. check that __builtin_unreachable exists during configuration, and error out if does not.

In 6d664a4, I've done something similar to this. We don't error out if __builtin_unreachable is unavailable, and instead make use of the [[noreturn]] attribute to get the same behaviour. If you think it would be cleaner to just error out in the absence of __builtin_unreachable then I am happy to do that instead @james-d-mitchell.

[james-d-mitchell]

I'm happy with your solution looks great!

[james-d-mitchell]

@Joseph-Edwards can we merge this now?

[Joseph-Edwards]

I'm currently running valgrind locally on the test files to make sure that this actually resolves those possibly lost issues. If everything looks okay, I will be happy to merge.

[Joseph-Edwards]

==265787== LEAK SUMMARY:
==265787==    definitely lost: 0 bytes in 0 blocks
==265787==    indirectly lost: 0 bytes in 0 blocks
==265787==      possibly lost: 808 bytes in 8 blocks
==265787==    still reachable: 67,211,879 bytes in 163,635 blocks
==265787==                       of which reachable via heuristic:
==265787==                         stdstring          : 232 bytes in 4 blocks
==265787==         suppressed: 32 bytes in 1 blocks

I'll investigate this further

[james-d-mitchell]

eek noting that that's the same number of bytes as before, no?

[Joseph-Edwards]

eek noting that that's the same number of bytes as before, no?

Yeah I think so. I'm hoping it's related to the non-trivially-destructible stuff, and will be an easy fix

[Joseph-Edwards]

Even better: it's the same number because seemingly I forgot to remake 🙃 I'm getting no leaks now. Happy to merge @james-d-mitchell?

[james-d-mitchell]

Merge?