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

Bug in setTerminate() #10765

Closed
cdunn2001 opened this issue Mar 2, 2019 · 12 comments

Comments

Projects
None yet
5 participants
@cdunn2001
Copy link
Contributor

commented Mar 2, 2019

Example

https://github.com/bio-nim/pbbam/blob/f5a659e9fbed4b675f4458734fdfc9fe8162b2ac/nim/foo.nim

Current Output

https://forum.nim-lang.org/t/4686

Traceback (most recent call last)
foo.nim(55)              foo
foo.nim(49)              main
system.nim(4341)         :anonymous
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS

Expected Output

We should see the exception message, similar to the default g++ terminate-handler.

Possible Solution

I have added prints into lib/system.nim::setTerminate() in order to find where the crash actually occurs. The problem is that let ex = getCurrentException() is nil, so writing ex.name or ex.msg fails.

It's nil because nothing called setCurrentException.

I might be able to call setTerminate() (which is juststd::set_terminate()) to install my own exception-trapping handler, as shown here:

   void terminate_handler()
   {
       try { throw; }
       catch(const std::exception& e) { log(e.what()); }
       catch(...) {}
   }

But I think this is a real bug anyway. Minimally, the anon proc given to setTerminate() should at least test nil == ex.

Ideally, we want both the C++ exception and the Nim stack-trace. Maybe a good approach is for Nim's default terminate-handler to show the Nim stack-trace first, and then to call __gnu_cxx::__verbose_terminate_handler if the Nim exception is nil and the backend is cpp and GNU.

Even fancier, the C++ stack-trace is harder to obtain, but that could be helpful too. (Would you like a pull-request for that, at least with GNU g++?)

Additional Information

@cdunn2001

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Another idea is for lib/system.nim to record the current terminate_handler, and for the replacement to call the old one after showing the Nim stack-trace. (We still need nil-checks, of course.) That way, we can avoid calling the very specific __gnu_cxx::__verbose_terminate_handler directly; it was probably the old handler anyway, installed by default.

@alaviss

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

Maybe we can use std::current_exception to interface with C++ exceptions directly in getCurrentException()? With tweaks to the codegen we might be able to natively handle C++ exceptions in Nim.

As of stacktrace, on Linux, if you're using glibc, you can compile Nim compiler with -d:nativeStacktrace (which employs execinfo.h).

@cdunn2001

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

On std::current_exception, yes, I think it's fair to require >= C++11.

On -d:nativeStacktrace, I'll try that next week, when I'm near a Linux machine. I'm curious how that might compare with GNU backtrace().

@alaviss

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

If you read the source, then -d:nativeStacktrace uses GNU backtrace() ;)

@cdunn2001

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

-d:nativeStacktrace uses GNU backtrace() ;)

Oh, I see now. That's interesting. But it's not related to C++ exceptions. When an exception is thrown, unless someone actually attaches a backtrace to the exception, the stacktrace is lost. But I can see how that would be helpful for crashes in C/C++ code.

@cdunn2001

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Back to handling untrapped exceptions ...

The idea is to test for getCurrentException() == nil in Nim's terminate-handler, and to call the previous terminate-handler (which we would record at program start-up) in that case.

So we have 3 choices:

  1. __gnu_cxx::__verbose_terminate_handler()
  1. std::get_terminate() to record the previous handler and call it after ours
  1. std::current_exception()

(1) and (2) work fine for me on Linux (haven't tried (3), but I do not know how to check for either C++11 or GNU.

@pb-cdunn

This comment has been minimized.

Copy link

commented Mar 2, 2019

In Nim/lib/system.nim:

4328   proc getTerminate: proc() {.noconv.}
4329     {.importc: "std::get_terminate", header: "<exception>".} # C++11
4330   var oldTerminate = getTerminate()
4331   proc setTerminate(handler: proc() {.noconv.})
4332     {.importc: "std::set_terminate", header: "<exception>".}
4333   setTerminate proc() {.noconv.} =
4334     # Remove ourself as a handler, reinstalling the default handler.
4335     setTerminate(nil)
4336
4337     let ex = getCurrentException()
4338     if nil == ex:
4339       stderr.writeLine "getCurrentException() -> nil";  # need template for `genode`?
4342       #verboseTerminate()
4343       if nil != oldTerminate:
4344         stderr.writeLine "Calling previous terminate_handler..."
4345         oldTerminate()
4346     let trace = ex.getStackTrace()
...
getCurrentException() -> nil
Calling previous terminate_handler...
terminate called after throwing an instance of 'std::runtime_error'
  what():  BamFile: could not open: foo
Traceback (most recent call last)
foo.nim(55)              foo
foo.nim(49)              main
system.nim(4345)         :anonymous
SIGABRT: Abnormal termination.

That's what I want! Both the C++ exception message and the Nim stacktrace. (I don't actually know where the Nim stackrace is printed, but it is.)

@cooldome

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

I will not have computer for next 4 days to write full answer. getCurrentExeception works only for Nim exception. Use the following

try:
... 
except Exception as ex:
... 
except StdCppImportedException as ex:
...

Importcpp the std::exception for the last line

@pb-cdunn

This comment has been minimized.

Copy link

commented Mar 6, 2019

This is a bug in the Nim std library, right? I see several ways to fix it. Could you show your suggestion explicitly.

@pb-cdunn

This comment has been minimized.

Copy link

commented Mar 8, 2019

d70dd6f

This is a Nim standard library bug. I've submitted a sample PR, but if someone would tell me roughly what solution would be acceptable, I'd be happy to amend it myself.

@pb-cdunn

This comment has been minimized.

Copy link

commented Mar 8, 2019

@cooldome , I understand what you're saying: I can trap exceptions in Nim. That's pretty cool:

type
  StdCppImportedException* {.importc: "std::exception", header: "<exception>".} = object
proc what*(this: StdCppImportedException): cstring {.importcpp: "#.what()".}

try:
  main()
except Exception as ex:
  echo "trapped Nim exception"
  echo repr(ex) # THIS CRASHES when I raise a simple OSError in Nim -- a separate weird bug
except StdCppImportedException as ex:
  echo "caught C++ exception:", ex.what()
except:
  echo "trapped unknown exception"
  echo "msg:", getCurrentExceptionMsg()

(Note: I found a different Nim crash when printing repr(ex).)

But I'm saying there is a seg-fault in the Nim standard library when we do not trap all exceptions. See pr #10812 for possible fixes.

cdunn2001 pushed a commit to bio-nim/pbbam that referenced this issue Mar 8, 2019

cdunn2001 pushed a commit to bio-nim/pbbam that referenced this issue Apr 8, 2019

@Araq Araq closed this in #10993 Apr 11, 2019

Araq added a commit that referenced this issue Apr 11, 2019

narimiran added a commit that referenced this issue Apr 11, 2019

fixes #10765 (#10993) [backport]
(cherry picked from commit de02fd0)
@cdunn2001

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Verified. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.