diff mbox

[ARM] Fix PR target/56846

Message ID 000901cfc04f$ee99a5f0$cbccf1d0$@arm.com
State New
Headers show

Commit Message

Tony Wang Aug. 25, 2014, 10:32 a.m. UTC 
Hi all,

The bug is reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56846, and it’s about the problem that
when exception handler is involved in the function, then _Unwind_Backtrace function will run into deadloop on
arm target. 

Cmd line: arm-none-eabi-g++ -mthumb -mcpu=cortex-m3 -O0 -g -std=c++11 -specs=rdimon.specs main.c -o main.exe
#include <unwind.h>
#include <stdio.h>
_Unwind_Reason_Code trace_func(struct _Unwind_Context * context, void* arg)
{
  void *ip = (void *)_Unwind_GetIP(context);
  printf("Address: %p\n", ip);
  return _URC_NO_REASON;
}
void bar()
{
  puts("This is in bar");
  _Unwind_Backtrace((_Unwind_Trace_Fn)&trace_func, 0);
}
void foo()
{
  try
  {
    bar();
  }
  catch (...)
  {
    puts("Exception");
  }
}

The potential of such a bug is discussed long time ago in mail:
https://gcc.gnu.org/ml/gcc/2007-08/msg00235.html. Basically, as the ARM EHABI does not define how to implement
the Unwind_Backtrace, Andrew give control to the personality routine to unwind the stack, and use the unwind
state combination of “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND” to represent that the caller is asking the
personality routine to only unwind the stack for it. 

However, the pr in the libstdc++-v3 doesn’t handle such a unwind state pattern correctly. When the backtrace
function passes such a pattern to it, it will still return _URC_HANDLER_FOUND to the caller in some cases.
It’s because the pr will think that the _Unwind_Backtrace is raising a none type exception to it, so if the
exception handler in current stack frame can catch anything(like catch(…)), the pr will return
_URC_HANDLER_FOUND to the caller and ask for next step. But definitely, the unwind backtrace function don’t
know what to do when pr return an exception handler to it.

So this patch just evaluate such a unwind state pattern at the beginning of the personality routine in
libstdc++-v3, if we meet with “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND”, then we directly call macro
CONTINUE_UNWINDING to unwind the stack and return.

Is this a reasonable fix?

gcc/libstdc++-v3/ChangeLog:
2014-8-25   Tony Wang <tony.wang@arm.com>

                 PR target/56846
                 * libsupc++/eh_personality.cc: Return with CONTINUE_UNWINDING
                 when meet with the unwind state pattern: _US_VIRTUAL_UNWIND_FRAME |
                 _US_FORCE_UNWIND

Comments

Andrew Haley Sept. 5, 2014, 7:42 a.m. UTC | #1 
On 25/08/14 11:32, Tony Wang wrote:
> Hi all,
> 
> The bug is reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56846, and it’s about the problem that
> when exception handler is involved in the function, then _Unwind_Backtrace function will run into deadloop on
> arm target. 
> 
> Cmd line: arm-none-eabi-g++ -mthumb -mcpu=cortex-m3 -O0 -g -std=c++11 -specs=rdimon.specs main.c -o main.exe
> #include <unwind.h>
> #include <stdio.h>
> _Unwind_Reason_Code trace_func(struct _Unwind_Context * context, void* arg)
> {
>   void *ip = (void *)_Unwind_GetIP(context);
>   printf("Address: %p\n", ip);
>   return _URC_NO_REASON;
> }
> void bar()
> {
>   puts("This is in bar");
>   _Unwind_Backtrace((_Unwind_Trace_Fn)&trace_func, 0);
> }
> void foo()
> {
>   try
>   {
>     bar();
>   }
>   catch (...)
>   {
>     puts("Exception");
>   }
> }
> 
> The potential of such a bug is discussed long time ago in mail:
> https://gcc.gnu.org/ml/gcc/2007-08/msg00235.html. Basically, as the ARM EHABI does not define how to implement
> the Unwind_Backtrace, Andrew give control to the personality routine to unwind the stack, and use the unwind
> state combination of “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND” to represent that the caller is asking the
> personality routine to only unwind the stack for it. 
> 
> However, the pr in the libstdc++-v3 doesn’t handle such a unwind state pattern correctly. When the backtrace
> function passes such a pattern to it, it will still return _URC_HANDLER_FOUND to the caller in some cases.
> It’s because the pr will think that the _Unwind_Backtrace is raising a none type exception to it, so if the
> exception handler in current stack frame can catch anything(like catch(…)), the pr will return
> _URC_HANDLER_FOUND to the caller and ask for next step. But definitely, the unwind backtrace function don’t
> know what to do when pr return an exception handler to it.
> 
> So this patch just evaluate such a unwind state pattern at the beginning of the personality routine in
> libstdc++-v3, if we meet with “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND”, then we directly call macro
> CONTINUE_UNWINDING to unwind the stack and return.
> 
> Is this a reasonable fix?
> 
> gcc/libstdc++-v3/ChangeLog:
> 2014-8-25   Tony Wang <tony.wang@arm.com>
> 
>                  PR target/56846
>                  * libsupc++/eh_personality.cc: Return with CONTINUE_UNWINDING
>                  when meet with the unwind state pattern: _US_VIRTUAL_UNWIND_FRAME |
>                  _US_FORCE_UNWIND

That looks very sensible, but it's not my call to approve it.

Andrew.
Ramana Radhakrishnan Sept. 9, 2014, 8:33 a.m. UTC | #2 
On Mon, Aug 25, 2014 at 11:32 AM, Tony Wang <tony.wang@arm.com> wrote:
> Hi all,
>
> The bug is reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56846, and it’s about the problem that
> when exception handler is involved in the function, then _Unwind_Backtrace function will run into deadloop on
> arm target.

You mean an infinite loop.

>
> Cmd line: arm-none-eabi-g++ -mthumb -mcpu=cortex-m3 -O0 -g -std=c++11 -specs=rdimon.specs main.c -o main.exe
> #include <unwind.h>
> #include <stdio.h>
> _Unwind_Reason_Code trace_func(struct _Unwind_Context * context, void* arg)
> {
>   void *ip = (void *)_Unwind_GetIP(context);
>   printf("Address: %p\n", ip);
>   return _URC_NO_REASON;
> }
> void bar()
> {
>   puts("This is in bar");
>   _Unwind_Backtrace((_Unwind_Trace_Fn)&trace_func, 0);
> }
> void foo()
> {
>   try
>   {
>     bar();
>   }
>   catch (...)
>   {
>     puts("Exception");
>   }
> }
>
> The potential of such a bug is discussed long time ago in mail:
> https://gcc.gnu.org/ml/gcc/2007-08/msg00235.html. Basically, as the ARM EHABI does not define how to implement
> the Unwind_Backtrace, Andrew give control to the personality routine to unwind the stack, and use the unwind
> state combination of “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND” to represent that the caller is asking the
> personality routine to only unwind the stack for it.
>
> However, the pr in the libstdc++-v3 doesn’t handle such a unwind state pattern correctly. When the backtrace
> function passes such a pattern to it, it will still return _URC_HANDLER_FOUND to the caller in some cases.
> It’s because the pr will think that the _Unwind_Backtrace is raising a none type exception to it, so if the
> exception handler in current stack frame can catch anything(like catch(…)), the pr will return
> _URC_HANDLER_FOUND to the caller and ask for next step. But definitely, the unwind backtrace function don’t
> know what to do when pr return an exception handler to it.
>
> So this patch just evaluate such a unwind state pattern at the beginning of the personality routine in
> libstdc++-v3, if we meet with “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND”, then we directly call macro
> CONTINUE_UNWINDING to unwind the stack and return.
>
> Is this a reasonable fix?

I'd like another review here however it looks sane to me. You need to
CC libstdc++@gcc.gnu.org for libstdc++ patches. Your email doesn't say
how you tested this patch. Can you make sure you've run this through a
bootstrap and regression test on GNU/Linux and a cross regression test
on arm-none-eabi with no regressions ?


regards
Ramana



>
> gcc/libstdc++-v3/ChangeLog:
> 2014-8-25   Tony Wang <tony.wang@arm.com>
>
>                  PR target/56846
>                  * libsupc++/eh_personality.cc: Return with CONTINUE_UNWINDING
>                  when meet with the unwind state pattern: _US_VIRTUAL_UNWIND_FRAME |
>                  _US_FORCE_UNWIND
>
> diff --git a/libstdc++-v3/libsupc++/eh_personality.cc b/libstdc++-v3/libsupc++/eh_personality.cc
> index f315a83..c2b30e9 100644
> --- a/libstdc++-v3/libsupc++/eh_personality.cc
> +++ b/libstdc++-v3/libsupc++/eh_personality.cc
> @@ -378,6 +378,11 @@ PERSONALITY_FUNCTION (int version,
>    switch (state & _US_ACTION_MASK)
>      {
>      case _US_VIRTUAL_UNWIND_FRAME:
> +      // If the unwind state pattern is _US_VIRTUAL_UNWIND_FRAME |
> +      // _US_FORCE_UNWIND, we don't need to search for any handler
> +      // as it is not a real exception. Just unwind the stack.
> +      if (state & _US_FORCE_UNWIND)
> +        CONTINUE_UNWINDING;
>        actions = _UA_SEARCH_PHASE;
>        break;
>
>
Tony Wang Sept. 9, 2014, 8:44 a.m. UTC | #3 
> -----Original Message-----
> From: Ramana Radhakrishnan [mailto:ramana.gcc@googlemail.com]
> Sent: Tuesday, September 09, 2014 4:33 PM
> To: Tony Wang
> Cc: gcc-patches; dan@debian.org; aph-gcc@littlepinkcloud.com; Richard Earnshaw; Ramana Radhakrishnan;
> libstdc++@gcc.gnu.org
> Subject: Re: [Patch ARM] Fix PR target/56846
> 
> On Mon, Aug 25, 2014 at 11:32 AM, Tony Wang <tony.wang@arm.com> wrote:
> > Hi all,
> >
> > The bug is reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56846, and it’s about the problem that
> > when exception handler is involved in the function, then _Unwind_Backtrace function will run into deadloop
> on
> > arm target.
> 
> You mean an infinite loop.
> 
> >
> > Cmd line: arm-none-eabi-g++ -mthumb -mcpu=cortex-m3 -O0 -g -std=c++11 -specs=rdimon.specs main.c -o
> main.exe
> > #include <unwind.h>
> > #include <stdio.h>
> > _Unwind_Reason_Code trace_func(struct _Unwind_Context * context, void* arg)
> > {
> >   void *ip = (void *)_Unwind_GetIP(context);
> >   printf("Address: %p\n", ip);
> >   return _URC_NO_REASON;
> > }
> > void bar()
> > {
> >   puts("This is in bar");
> >   _Unwind_Backtrace((_Unwind_Trace_Fn)&trace_func, 0);
> > }
> > void foo()
> > {
> >   try
> >   {
> >     bar();
> >   }
> >   catch (...)
> >   {
> >     puts("Exception");
> >   }
> > }
> >
> > The potential of such a bug is discussed long time ago in mail:
> > https://gcc.gnu.org/ml/gcc/2007-08/msg00235.html. Basically, as the ARM EHABI does not define how to
> implement
> > the Unwind_Backtrace, Andrew give control to the personality routine to unwind the stack, and use the
> unwind
> > state combination of “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND” to represent that the caller is
> asking the
> > personality routine to only unwind the stack for it.
> >
> > However, the pr in the libstdc++-v3 doesn’t handle such a unwind state pattern correctly. When the backtrace
> > function passes such a pattern to it, it will still return _URC_HANDLER_FOUND to the caller in some cases.
> > It’s because the pr will think that the _Unwind_Backtrace is raising a none type exception to it, so if the
> > exception handler in current stack frame can catch anything(like catch(…)), the pr will return
> > _URC_HANDLER_FOUND to the caller and ask for next step. But definitely, the unwind backtrace function
> don’t
> > know what to do when pr return an exception handler to it.
> >
> > So this patch just evaluate such a unwind state pattern at the beginning of the personality routine in
> > libstdc++-v3, if we meet with “_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND”, then we directly call
> macro
> > CONTINUE_UNWINDING to unwind the stack and return.
> >
> > Is this a reasonable fix?
> 
> I'd like another review here however it looks sane to me. You need to
> CC libstdc++@gcc.gnu.org for libstdc++ patches. Your email doesn't say
> how you tested this patch. Can you make sure you've run this through a
> bootstrap and regression test on GNU/Linux and a cross regression test
> on arm-none-eabi with no regressions ?

Hi Ramana,

Thanks for you review. After this patch, the infinite loop will be fixed for 
the above test case, and I can make sure that no regression is happen 
during bootstrap and regression test on Linux and a cross regression test 
on arm-none-eabi.

Regards,
Tony

> 
> 
> regards
> Ramana
> 
> 
> 
> >
> > gcc/libstdc++-v3/ChangeLog:
> > 2014-8-25   Tony Wang <tony.wang@arm.com>
> >
> >                  PR target/56846
> >                  * libsupc++/eh_personality.cc: Return with CONTINUE_UNWINDING
> >                  when meet with the unwind state pattern: _US_VIRTUAL_UNWIND_FRAME |
> >                  _US_FORCE_UNWIND
> >
> > diff --git a/libstdc++-v3/libsupc++/eh_personality.cc b/libstdc++-v3/libsupc++/eh_personality.cc
> > index f315a83..c2b30e9 100644
> > --- a/libstdc++-v3/libsupc++/eh_personality.cc
> > +++ b/libstdc++-v3/libsupc++/eh_personality.cc
> > @@ -378,6 +378,11 @@ PERSONALITY_FUNCTION (int version,
> >    switch (state & _US_ACTION_MASK)
> >      {
> >      case _US_VIRTUAL_UNWIND_FRAME:
> > +      // If the unwind state pattern is _US_VIRTUAL_UNWIND_FRAME |
> > +      // _US_FORCE_UNWIND, we don't need to search for any handler
> > +      // as it is not a real exception. Just unwind the stack.
> > +      if (state & _US_FORCE_UNWIND)
> > +        CONTINUE_UNWINDING;
> >        actions = _UA_SEARCH_PHASE;
> >        break;
> >
> >
Jonathan Wakely Sept. 9, 2014, 9:16 a.m. UTC | #4 
On 09/09/14 09:33 +0100, Ramana Radhakrishnan wrote:
>I'd like another review here however it looks sane to me. You need to
>CC libstdc++@gcc.gnu.org for libstdc++ patches. Your email doesn't say
>how you tested this patch. Can you make sure you've run this through a
>bootstrap and regression test on GNU/Linux and a cross regression test
>on arm-none-eabi with no regressions ?

Thanks for forwarding this, Ramana.

I don't know the EABI unwinder code so if Ramana is OK with it and no
other ARM maintainers have any comments then the patch is OK with me
too, with a couple of small tweaks ...

>>
>> gcc/libstdc++-v3/ChangeLog:
>> 2014-8-25   Tony Wang <tony.wang@arm.com>
>>
>>                  PR target/56846
>>                  * libsupc++/eh_personality.cc: Return with CONTINUE_UNWINDING
>>                  when meet with the unwind state pattern: _US_VIRTUAL_UNWIND_FRAME |
>>                  _US_FORCE_UNWIND

The changelog should say which function is being changed:

	* libsupc++/eh_personality.cc (__gxx_personality_v0): ...

Or maybe:

	* libsupc++/eh_personality.cc (PERSONALITY_FUNCTION): ...

Instead of "when meet with the unwind state pattern" please say "when the state
pattern contains"

>> diff --git a/libstdc++-v3/libsupc++/eh_personality.cc b/libstdc++-v3/libsupc++/eh_personality.cc
>> index f315a83..c2b30e9 100644
>> --- a/libstdc++-v3/libsupc++/eh_personality.cc
>> +++ b/libstdc++-v3/libsupc++/eh_personality.cc
>> @@ -378,6 +378,11 @@ PERSONALITY_FUNCTION (int version,
>>    switch (state & _US_ACTION_MASK)
>>      {
>>      case _US_VIRTUAL_UNWIND_FRAME:
>> +      // If the unwind state pattern is _US_VIRTUAL_UNWIND_FRAME |
>> +      // _US_FORCE_UNWIND, we don't need to search for any handler
>> +      // as it is not a real exception. Just unwind the stack.

I think this comment would be easier to read if the expression with the two
constants was all on one line:

      // If the unwind state pattern is
      // _US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND
      // then we don't need to search for any handler as it is not a real
      // exception. Just unwind the stack.

>> +      if (state & _US_FORCE_UNWIND)
>> +        CONTINUE_UNWINDING;
>>        actions = _UA_SEARCH_PHASE;
>>        break;
>>
>>
Tony Wang Sept. 9, 2014, 9:23 a.m. UTC | #5 
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of Jonathan Wakely
> Sent: Tuesday, September 09, 2014 5:16 PM
> To: Ramana Radhakrishnan
> Cc: Tony Wang; gcc-patches; dan@debian.org; aph-gcc@littlepinkcloud.com; Richard Earnshaw; Ramana
> Radhakrishnan; libstdc++@gcc.gnu.org
> Subject: Re: [Patch ARM] Fix PR target/56846
> 
> On 09/09/14 09:33 +0100, Ramana Radhakrishnan wrote:
> >I'd like another review here however it looks sane to me. You need to
> >CC libstdc++@gcc.gnu.org for libstdc++ patches. Your email doesn't say
> >how you tested this patch. Can you make sure you've run this through a
> >bootstrap and regression test on GNU/Linux and a cross regression test
> >on arm-none-eabi with no regressions ?
> 
> Thanks for forwarding this, Ramana.
> 
> I don't know the EABI unwinder code so if Ramana is OK with it and no
> other ARM maintainers have any comments then the patch is OK with me
> too, with a couple of small tweaks ...

Thanks for your comment, Jonathan.

I will send a new patch to cover your comment.

BR,
Tony

> 
> >>
> >> gcc/libstdc++-v3/ChangeLog:
> >> 2014-8-25   Tony Wang <tony.wang@arm.com>
> >>
> >>                  PR target/56846
> >>                  * libsupc++/eh_personality.cc: Return with CONTINUE_UNWINDING
> >>                  when meet with the unwind state pattern: _US_VIRTUAL_UNWIND_FRAME |
> >>                  _US_FORCE_UNWIND
> 
> The changelog should say which function is being changed:
> 
> 	* libsupc++/eh_personality.cc (__gxx_personality_v0): ...
> 
> Or maybe:
> 
> 	* libsupc++/eh_personality.cc (PERSONALITY_FUNCTION): ...
> 
> Instead of "when meet with the unwind state pattern" please say "when the state
> pattern contains"
> 
> >> diff --git a/libstdc++-v3/libsupc++/eh_personality.cc b/libstdc++-v3/libsupc++/eh_personality.cc
> >> index f315a83..c2b30e9 100644
> >> --- a/libstdc++-v3/libsupc++/eh_personality.cc
> >> +++ b/libstdc++-v3/libsupc++/eh_personality.cc
> >> @@ -378,6 +378,11 @@ PERSONALITY_FUNCTION (int version,
> >>    switch (state & _US_ACTION_MASK)
> >>      {
> >>      case _US_VIRTUAL_UNWIND_FRAME:
> >> +      // If the unwind state pattern is _US_VIRTUAL_UNWIND_FRAME |
> >> +      // _US_FORCE_UNWIND, we don't need to search for any handler
> >> +      // as it is not a real exception. Just unwind the stack.
> 
> I think this comment would be easier to read if the expression with the two
> constants was all on one line:
> 
>       // If the unwind state pattern is
>       // _US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND
>       // then we don't need to search for any handler as it is not a real
>       // exception. Just unwind the stack.
> 
> >> +      if (state & _US_FORCE_UNWIND)
> >> +        CONTINUE_UNWINDING;
> >>        actions = _UA_SEARCH_PHASE;
> >>        break;
> >>
> >>
Thomas Preud'homme Nov. 19, 2014, 5:59 p.m. UTC | #6 
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Tony Wang
> 

> 
> Hi all,
> 
> The bug is reported at
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56846, and it’s about the
> problem that
> when exception handler is involved in the function, then
> _Unwind_Backtrace function will run into deadloop on
> arm target.

The patch (in r215101) can be backported without any change on 4.8 and
4.9 branches. I checked in QEMU with and without the patch on both
branches and it indeed solves the problem.

Testsuite run without regression when compiled with arm-none-eabi
cross compiler and executed on QEMU emulating Cortex-M3.

I also bootstrapped gcc on x86_64-linux-gnu and run the testsuite without
regressions.

Is it ok for backport?

Best regards,

Thomas
diff mbox

Patch

diff --git a/libstdc++-v3/libsupc++/eh_personality.cc b/libstdc++-v3/libsupc++/eh_personality.cc
index f315a83..c2b30e9 100644
--- a/libstdc++-v3/libsupc++/eh_personality.cc
+++ b/libstdc++-v3/libsupc++/eh_personality.cc
@@ -378,6 +378,11 @@  PERSONALITY_FUNCTION (int version,
   switch (state & _US_ACTION_MASK)
     {
     case _US_VIRTUAL_UNWIND_FRAME:
+      // If the unwind state pattern is _US_VIRTUAL_UNWIND_FRAME |
+      // _US_FORCE_UNWIND, we don't need to search for any handler
+      // as it is not a real exception. Just unwind the stack.
+      if (state & _US_FORCE_UNWIND)
+        CONTINUE_UNWINDING;
       actions = _UA_SEARCH_PHASE;
       break;