RE: [Patch ARM] Fix PR target/56846

2014-11-19 Thread Thomas Preud'homme 
 From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
 ow...@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

Re: [Patch ARM] Fix PR target/56846

2014-09-09 Thread Ramana Radhakrishnan 
On Mon, Aug 25, 2014 at 11:32 AM, Tony Wang tony.w...@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 libstd...@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.w...@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;

RE: [Patch ARM] Fix PR target/56846

2014-09-09 Thread Tony Wang 
 -Original Message-
 From: Ramana Radhakrishnan [mailto:ramana@googlemail.com]
 Sent: Tuesday, September 09, 2014 4:33 PM
 To: Tony Wang
 Cc: gcc-patches; d...@debian.org; aph-...@littlepinkcloud.com; Richard 
 Earnshaw; Ramana Radhakrishnan;
 libstd...@gcc.gnu.org
 Subject: Re: [Patch ARM] Fix PR target/56846
 
 On Mon, Aug 25, 2014 at 11:32 AM, Tony Wang tony.w...@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 libstd...@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.w...@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;

Re: [Patch ARM] Fix PR target/56846

2014-09-09 Thread Jonathan Wakely 

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 libstd...@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.w...@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;

RE: [Patch ARM] Fix PR target/56846

2014-09-09 Thread Tony Wang 
 -Original Message-
 From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-ow...@gcc.gnu.org] On 
 Behalf Of Jonathan Wakely
 Sent: Tuesday, September 09, 2014 5:16 PM
 To: Ramana Radhakrishnan
 Cc: Tony Wang; gcc-patches; d...@debian.org; aph-...@littlepinkcloud.com; 
 Richard Earnshaw; Ramana
 Radhakrishnan; libstd...@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 libstd...@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.w...@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;

Re: [Patch ARM] Fix PR target/56846

2014-09-05 Thread Andrew Haley 
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.w...@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.

RE: [Patch ARM] Fix PR target/56846

2014-09-03 Thread Tony Wang 
Ping?

 -Original Message-
 From: Tony Wang [mailto:tony.w...@arm.com]
 Sent: Monday, August 25, 2014 6:33 PM
 To: 'gcc-patches@gcc.gnu.org'; 'd...@debian.org'; 
 'aph-...@littlepinkcloud.com'; Richard Earnshaw; Ramana
 Radhakrishnan
 Subject: [Patch ARM] Fix PR target/56846
 
 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.w...@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;