Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can catch support checking for abort? #553

Closed
luxe opened this issue Dec 10, 2015 · 23 comments
Closed

Can catch support checking for abort? #553

luxe opened this issue Dec 10, 2015 · 23 comments

Comments

@luxe
Copy link
Contributor

luxe commented Dec 10, 2015

Our codebase is using assertions to check for preconditions and post conditions.

Example:

void Do_Computation(int n){
    assert(n > 0);
    //impl...
}

When we call Do_Computation(0) in debug mode, the program aborts.

I see that Catch supports CHECK_THROWS, but does it support CHECK_ABORT?
Would implementing that even be possible?
I guess you could write a signal handler, but I'm not sure if you could recover from it.

@nabijaczleweli
Copy link
Contributor

Installing a signal(SIGABRT) handler is an option and looks like it's legal to return from it normally (which continues execution), however:

On entry to the signal handler, the state of the floating-point environment and the values of all objects is unspecified, except for

  • objects of type volatile std::sig_atomic_t
  • objects of std::atomic types
  • side effects made visible through std::atomic_signal_fence

Which means, that there'd need to be a separate (static) environment for this kinda thing, which drops multithreading.

@luxe
Copy link
Contributor Author

luxe commented Dec 10, 2015

It seems like it may not be legal to return from a signal handler.
http://stackoverflow.com/questions/34210772/is-it-possible-to-recover-from-stdabort/34210981

This is probably not do-able.

@refi64
Copy link

refi64 commented Dec 10, 2015

You could always write a custom assert macro:

#ifdef DEBUG
class AssertionFailure : public std::runtime_error {}

inline void assert_impl(bool cond, std::string expr, std::string file, std::string func, int line) {
    if (!c) {
        std::stringstream ss;
        ss << "assertion " << expr << " in " << file << ':' << line << " (" << func << ") failed";
        throw AssertionError(ss.str());
    }
}

#define assert(x) assert_impl((x), #x, __FILE__, __func__, __LINE__)
#else
#define assert(x) (void)0
#endif

@luxe
Copy link
Contributor Author

luxe commented Dec 10, 2015

That's a good idea, but the code I'm testing is in C.
I can't overwrite assert to throw exceptions and still get the C code to compile.
The C code is built with a C compiler, and the C headers have "extern C" guards so that they can be called from C++, and then used inside Catch.

@nabijaczleweli
Copy link
Contributor

If the user defined function returns when handling SIGFPE, SIGILL, SIGSEGV or any other implementation-defined signal specifying a computational exception, the behavior is undefined.

Returning from the handler is legal for <see your local compiler's documentation> signals.

@luxe
Copy link
Contributor Author

luxe commented Dec 11, 2015

From SO, it seems like the approach google test takes, is to fork off a new process, and check if it exits.

@nabijaczleweli
Copy link
Contributor

Sounds expensive as fuck, though.

@luxe
Copy link
Contributor Author

luxe commented Dec 11, 2015

Agreed.

I wouldn't mind the performance cost if it let me specify individual use cases where aborts should occur. It's not a cost anyone would need to pay for unless they chose to check for aborts. It doesn't seem like returning from a signal handler, and still having all of the program state defined, is plausible.

Currently, I'm making a ton of binaries, and having a bash script check their exit status. It's messy/hacky, but works fine. Implementing a similar thing into the framework(through forking) is just as bad, but at least I could specify all my use cases in the same file, and run them from the same binary.

@nabijaczleweli
Copy link
Contributor

If I read the cppreference correctly, it's only UB for some cases, and
SIGABRT is not one of them, so it'd well-defined.

On 11/12/2015 18:31, Trevor Hickey wrote:

@nabijaczleweli https://github.com/nabijaczleweli
Agreed.

I wouldn't mind the performance cost if it let me specify individual
use cases where aborts should occur. It doesn't seem like returning
from a signal handler, and still having all of the program state
defined, is plausible.

Currently, I'm making a ton of binaries, and having a bash script
check their exit status. It's not great. Implementing a similar thing
into the framework through forking is just as bad, but at least I
could specify all my use cases in the same file/same binary.


Reply to this email directly or view it on GitHub
#553 (comment).

@luxe
Copy link
Contributor Author

luxe commented Dec 11, 2015

I can't find any documentation that says handling SIGABRT keeps the program in a defined state. I see what you were saying about the "static environment" though, and how it would drop the potential for multi-threading.

@luxe
Copy link
Contributor Author

luxe commented Dec 11, 2015

I'm going to try and fork a process as part of my unit test case, and then compare the exit code using CHECK(exit code == ...);

That won't require any changes to the Catch framework.
Having CHECK_ABORT(...) would be a nice convenience, but its not necessary to accomplish what I want.

@nabijaczleweli
Copy link
Contributor

Signal handler
The following limitations are imposed on the user-defined function that is installed as a signal handler.

  • If the user defined function returns when handling SIGFPE, SIGILL, SIGSEGV or any other implementation-defined signal specifying a computational exception, the behavior is undefined.

From cppreference on std::signal()

@luxe
Copy link
Contributor Author

luxe commented Dec 11, 2015

the user defined function

our signal handler

is undefined behavior when handling implementation-defined signals

As far as I can tell, SIGABRT is implementation-defined.

@nabijaczleweli
Copy link
Contributor

any other implementation-defined signal specifying a computational exception

And SIGABRT is standard.

@luxe
Copy link
Contributor Author

luxe commented Dec 11, 2015

Ok cool. I was looking at the value of SIGABRT. that value is implementation-defined.
http://en.cppreference.com/w/c/program/SIG_types

I guess that doesn't mean the same thing as not being standard though.

@luxe luxe changed the title Does catch support checking for abort? Can catch support checking for abort? Dec 11, 2015
@philsquared
Copy link
Collaborator

A few observations:

  1. Catch already has a signal handler. It doesn't attempt to fully recover from it (as that is not possible in the general case) but it tries to reconstruct its immediate context just enough to print out the final results before terminating. Not quite what you want - but worth bearing in mind. I haven't looked at the references cited that suggest that SIGABRT is fully recoverable - but it's possible the existing handler could be extended to allow that (do a search for "fatal" in the codebase to see all the places it would touch).
  2. Spawning a child process for testing exit status, terminal behaviour, or just to be able to run test in full isolation (if you're prepared to pay that cost) is the approach I've had in mind for this. I've been putting that off as it's a non-trivial chunk of work - but it is on my list.
  3. In the meantime if you want to be able to do this yourself (either writing the spawning code into your tests, or controlling from a script) then just remember that this is an ideal application for tags. Tag the tests you want to run in isolation - and probably "hide" them too (tagged, [.] or with the . prefix). You might also want to combine that with --list-test-names-only, which writes test names out line by line. A script can write out all the tests tagged for running in isolation, then read the file in and execute Catch for each line.

HTH

@luxe
Copy link
Contributor Author

luxe commented Dec 14, 2015

This is my current integration:

template<class F, class... Args>
void CHECK_ABORT(F&& f, Args&&... args)
{
    //spawn a new process
    auto child_pid = fork();

    //if the fork succeed
    if (child_pid >= 0){

      //if we are in the child process
      if (child_pid == 0){

         //call the function that we expect to abort
         std::invoke(std::forward<F>(f),std::forward<Args>(args)...);

         //if the function didn't abort, we'll exit cleanly
         std::exit(EXIT_SUCCESS);
      }
    }

    //determine if the child process aborted
    int exit_status;
    wait(&exit_status);

    //we check the exit status instead of a signal interrupt, because
    //Catch is going to catch the signal and exit with an error
    bool aborted = WEXITSTATUS(exit_status);

    return aborted;
}

It's used like so:

void Test_Function(int n){
  assert(n != 7);
  return;
}

TEST_CASE("test"){
  CHECK(CHECK_ABORT(Test_Function,6)); //fails
  CHECK(CHECK_ABORT(Test_Function,7)); //passes (but prints misleading information to the terminal)
}

This is sufficient for my use cases.

  • CHECK_ABORT is not a macro
  • not cross-compatible
  • std::invoke comes from C++17...
  • etc, etc
  • I would have liked to use CHECK_ABORT by itself in a TEST_CASE

One of the new problems this gives me, is that assert prints to the terminal, and the signal handler by Catch prints that the unit test failed. This results in me having to use WEXITSTATUS instead of WIFSIGNALED. If I look at the last result of running the program, it will tell me the correct statistical information. If I could suppress printing to the terminal in CHECK_ABORT, it would be much more use-able.

Here is a terminal dump to better illustrate the problem:

$ ./main
main: main.cpp:85: void Test_Function(int): Assertion `n != 7' failed.

main is a Catch v1.3.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
test
-------------------------------------------------------------------------------
main.cpp:89
...............................................................................

main.cpp:89: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases: 1 | 1 failed
assertions: 1 | 1 failed

===============================================================================
All tests passed (1 assertion in 1 test case)
$

@luxe
Copy link
Contributor Author

luxe commented Dec 14, 2015

I've disabled output on the forked process, and I'm pretty satisfied with how this is working.

full program example:

#include <iostream>
#include <string>
#include <cstdlib>
#include <unistd.h>
#include <sys/types.h>
#include <errno.h>
#include <stdio.h>
#include <sys/wait.h>
#include <stdlib.h>
#include <cassert>
#include <functional>
#define CATCH_CONFIG_MAIN
#include "catch.hpp"

//copied from cppreference as possible implementation
namespace detail {
template <class F, class... Args>
inline auto INVOKE(F&& f, Args&&... args) ->
    decltype(std::forward<F>(f)(std::forward<Args>(args)...)) {
      return std::forward<F>(f)(std::forward<Args>(args)...);
}

template <class Base, class T, class Derived>
inline auto INVOKE(T Base::*pmd, Derived&& ref) ->
    decltype(std::forward<Derived>(ref).*pmd) {
      return std::forward<Derived>(ref).*pmd;
}

template <class PMD, class Pointer>
inline auto INVOKE(PMD pmd, Pointer&& ptr) ->
    decltype((*std::forward<Pointer>(ptr)).*pmd) {
      return (*std::forward<Pointer>(ptr)).*pmd;
}

template <class Base, class T, class Derived, class... Args>
inline auto INVOKE(T Base::*pmf, Derived&& ref, Args&&... args) ->
    decltype((std::forward<Derived>(ref).*pmf)(std::forward<Args>(args)...)) {
      return (std::forward<Derived>(ref).*pmf)(std::forward<Args>(args)...);
}

template <class PMF, class Pointer, class... Args>
inline auto INVOKE(PMF pmf, Pointer&& ptr, Args&&... args) ->
    decltype(((*std::forward<Pointer>(ptr)).*pmf)(std::forward<Args>(args)...)) {
      return ((*std::forward<Pointer>(ptr)).*pmf)(std::forward<Args>(args)...);
}
}

namespace custom{
  template< class F, class... ArgTypes>
  decltype(auto) invoke(F&& f, ArgTypes&&... args) {
      return detail::INVOKE(std::forward<F>(f), std::forward<ArgTypes>(args)...);
  }
}

// - ignore the output from Catch's signal handler
// - ignore the message from assert
void Disable_Console_Output(){

   //close C file descriptors
   //this will prevent cout and cin as well
   fclose(stdout);
   fclose(stderr);
}

template<class F, class... Args>
bool Function_Aborts(F&& f, Args&&... args)
{
    //spawn a new process
    auto child_pid = fork();

    //if the fork succeeded
    if (child_pid >= 0){

      //if we are in the child process
      if (child_pid == 0){

         //call the function that we expect to abort
         Disable_Console_Output();
         custom::invoke(std::forward<F>(f),std::forward<Args>(args)...);

         //if the function didn't abort, we'll exit cleanly
         std::exit(EXIT_SUCCESS);
      }
    }

    //determine if the function aborted
    //**this does not work because of Catch's signal handler**
    int exit_status;
    wait(&exit_status);
    bool aborted = WEXITSTATUS(exit_status);
    return aborted;
}

void Test_Function(int n){
  assert(n != 7);
  return;
}

TEST_CASE("test"){
  CHECK(Function_Aborts(Test_Function,6)); //fails (no extraneous output)
  CHECK(Function_Aborts(Test_Function,7)); //passes
}

//data appears correctly in console

console output:

$ ./main                       

main is a Catch v1.3.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
test
-------------------------------------------------------------------------------
main.cpp:102
...............................................................................

main.cpp:103: FAILED:
  CHECK( Function_Aborts(Test_Function,6) )
with expansion:
  false

===============================================================================
test cases: 1 | 1 failed
assertions: 2 | 1 passed | 1 failed

I don't expect any of this to be integrated, but I wanted to show the linux work-around that has been working well for me. Hopefully it will help anyone else who wants to check for aborts on linux.

I'm not sure about any global state that may get changed from the forked process, or how it would affect other test cases. I have not run into any problems myself testing our C code this way.

@nabijaczleweli
Copy link
Contributor

IIRC fclose()ing stdout and stderr isn't guaranteed to work on cout and cerr.

@luxe
Copy link
Contributor Author

luxe commented Dec 14, 2015

In that case, I'll add:

std::cout.setstate(std::ios_base::failbit);
std::cin.setstate(std::ios_base::failbit);

@nabijaczleweli
Copy link
Contributor

Or redirect it to a stringstream via rdbuf(), or make a null buffer and use that. (Un)Setting failbit is a common error handling technique

@luxe
Copy link
Contributor Author

luxe commented Dec 14, 2015

good point. rdbuf() seems better.

@horenmar
Copy link
Member

Since this will not be added to Catch Classic, but might be added to Catch 2 at some point, I opened #853 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants