diff mbox

[ARM] RFC: Backtracing through C++ exception-handling constructs

Message ID 20120730151840.0bf5eb81@octopus
State New
Headers show

Commit Message

Julian Brown July 30, 2012, 2:18 p.m. UTC
Hi,

I've been investigating a patch which we've been using locally to fix
an issue with backtraces (using, e.g., glibc's backtrace() function)
through C++ exception-handling constructs on ARM. The original author of
the patch was Daniel Jacobowitz (please correct me if my understanding
is wrong!).

There are two issues in play here:

1. Exception-handling is handled in a target-specific way for ARM,
defined in the EHABI document ("Exception handling ABI for the ARM
architecture", IHI 0038A). However, no mention of "forced unwinding" is
made in this document.

2. Backtracing in particular isn't even the "normal" use case for
forced unwinding: e.g.,

http://www.ucw.cz/~hubicka/papers/abi/node25.html#SECTION00923200000000000000

suggests that forced unwinding is "a single-phase process (phase 2 of
the normal exception-handling process)", whereas for producing a
backtrace, something more like a phase 1 lookup is done (no cleanup
handlers are called -- we're merely observing the state of the stack).

So, to be clear, we're definitely dealing with a corner case here. The
problem is that _Unwind_Backtrace in libgcc will fail to make progress
in some cases if it hits a frame with a cleanup associated with it,
leading to unhelpful behaviour like (for the attached program):

bar calling abort
abort handler invoked, depth 25
./test() [0x8968]
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(__default_rt_sa_restorer_v2+0) [0x401cf860]
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(gsignal+0x40) [0x401ce5e0]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]
./test() [0x8a4c]

which is clearly wrong, no matter how you look at it. I'll defer to Dan
for a better description of the problem/fix (on a private branch, circa
February 2010):

"This bug was a failure of backtrace() when presented with a C++ abort
- in particular, one which inherited throw() but called cout, so
needed its own call to __cxa_call_unexpected.  We'd get stuck in a
loop in _Unwind_Backtrace because the code was not prepared for the
handler to return _URC_HANDLER_FOUND.

"The GCC ARM unwinders already have _US_FORCED_UNWIND passed to the
personality routine.  ISTM that forced unwinding when doing
essentially a 'phase 1' lookup has no other useful meaning, and this
is a useful meaning to assign to it: skip handlers, keep unwinding."

The patch still seems to produce a reasonable improvement in
behaviour, giving:

build$ arm-none-linux-gnueabi-g++ test.cc -o test -g

$ ./test
bar calling abort                                                               
abort handler invoked, depth 5                                                  
./test() [0x8968]                                                               
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(__default_rt_sa_restorer_v2+0) [0x401cf860]                                                                
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(gsignal+0x40) [0x401ce5e0] 
./test() [0x8a4c]                                                               
./test() [0x8a4c]

although tbh I'd hope for backtrace_symbols to produce something a
little more useful than that (unrelated to this patch), and I'd also
expect identical results from the test whether the "throw ()" is
present on the declaration of "bar", or not -- which unfortunately
isn't the case. Without throw (), we get:

bar calling abort
abort handler invoked, depth 8
./test() [0x897c]
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(__default_rt_sa_restorer_v2+0) [0x401cf860]
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(gsignal+0x40) [0x401ce5e0]
./test() [0x8a60]
./test() [0x8a60]
./test() [0x8a7c]
./test() [0x8acc]
../install/arm-none-linux-gnueabi/libc/lib/libc.so.6(__libc_start_main+0x114) [0x401b8e44]

I.e., it looks like the backtrace progresses all the way to the
outermost frame -- which IIUC, was the intended resulting behaviour for
the attached patch to start with.

So: does anyone have an opinion about whether the attached is a correct
fix, or if the spinning-during-backtrace problem might have a better
solution? (I'm a little fuzzy on the intricate details of all this
stuff!).

Thanks,

Julian

ChangeLog

    Daniel Jacobowitz  <drow@false.org>

    libstdc++-v3/
    * libsupc++/eh_personality.cc (PERSONALITY_FUNCTION): For
    ARM EABI, skip handlers for _US_VIRTUAL_UNWIND_FRAME
    | _US_FORCE_UNWIND.

Comments

Andrew Haley July 30, 2012, 4:10 p.m. UTC | #1
On 07/30/2012 03:18 PM, Julian Brown wrote:
> There are two issues in play here:
> 
> 1. Exception-handling is handled in a target-specific way for ARM,
> defined in the EHABI document ("Exception handling ABI for the ARM
> architecture", IHI 0038A). However, no mention of "forced unwinding" is
> made in this document.
> 
> 2. Backtracing in particular isn't even the "normal" use case for
> forced unwinding: e.g.,
>
> http://www.ucw.cz/~hubicka/papers/abi/node25.html#SECTION00923200000000000000
> 
> suggests that forced unwinding is "a single-phase process (phase 2 of
> the normal exception-handling process)", whereas for producing a
> backtrace, something more like a phase 1 lookup is done (no cleanup
> handlers are called -- we're merely observing the state of the stack).

That's right.  I wrote that code.  I think I didn't realize that
forced unwinding was a single-phase process.

It looks to me like checking for _US_VIRTUAL_UNWIND_FRAME and
_US_FORCE_UNWIND, as you have done, is right.

Andrew.


P.S.  The ARM unwinder data was never intended for all the things we
do with it, and it's pretty much a matter of luck that it works.  I
don't really know why we didn't adopt DWARF unwinder data for ARM,
given that the ARM unwinder data is really only intended for
exceptions, and we need a lot more.
diff mbox

Patch

commit eafec6ead2e5a5a5a1b6504311a6a7ec6f0420af
Author: Julian Brown <jbrown@build6-lucid-cs.sje.mentorg.com>
Date:   Wed Jul 25 11:43:08 2012 -0700

    Backtrace through throw.

diff --git a/libstdc++-v3/libsupc++/eh_personality.cc b/libstdc++-v3/libsupc++/eh_personality.cc
index 72f596e..3322b6c 100644
--- a/libstdc++-v3/libsupc++/eh_personality.cc
+++ b/libstdc++-v3/libsupc++/eh_personality.cc
@@ -380,6 +380,8 @@  PERSONALITY_FUNCTION (int version,
   switch (state & _US_ACTION_MASK)
     {
     case _US_VIRTUAL_UNWIND_FRAME:
+      if (state & _US_FORCE_UNWIND)
+	CONTINUE_UNWINDING;
       actions = _UA_SEARCH_PHASE;
       break;