Patchwork Implement stap probe on ARM's unwinder

login
register
mail settings
Submitter Sergio Durigan Junior
Date Nov. 22, 2011, 7:21 p.m.
Message ID <m3k46sotdr.fsf@redhat.com>
Download mbox | patch
Permalink /patch/127148/
State New
Headers show

Comments

Sergio Durigan Junior - Nov. 22, 2011, 7:21 p.m.
Hello,

This is the implementation of
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01016.html> for the ARM
unwinder.  Since ARM has a different unwinder, I basically replicated
the existing code (on unwind-dw2.c) into it, with a few modifications in
order to gather the necessary information for the probe.  This feature
is pretty useful for GDB, which uses the probe inserted here to
implement the "next over throw" feature.

I built and regtested it on a Beagle board, and found no regressions.
Is it OK to apply?

Thanks,

Sergio.
Sergio Durigan Junior - Nov. 30, 2011, 4:26 p.m.
Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Hello,
>
> This is the implementation of
> <http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01016.html> for the ARM
> unwinder.  Since ARM has a different unwinder, I basically replicated
> the existing code (on unwind-dw2.c) into it, with a few modifications in
> order to gather the necessary information for the probe.  This feature
> is pretty useful for GDB, which uses the probe inserted here to
> implement the "next over throw" feature.

Ping.
Ramana Radhakrishnan - Dec. 1, 2011, 12:01 p.m.
Sergio: Other than a few minor tweaks to the Changelog it largely
looks obvious to me.

Bernd, could you take another look at this since this is now shared
with the c6x backend ?

> Thanks,
>
> Sergio.
>
> diff --git a/libgcc/ChangeLog b/libgcc/ChangeLog
> index 305e8ad..f6e9dec 100644
> --- a/libgcc/ChangeLog
> +++ b/libgcc/ChangeLog
> @@ -1,3 +1,15 @@
> +2011-11-22  Sergio Durigan Junior  <sergiodj@redhat.com>
> +
> +       Implement ARM Unwinder SystemTap probe.

This line is not required.

> +       * unwind-arm-common.inc: Include `tconfig.h', `tsystem.h' and
> +       `sys/sdt.h'.
> +       (_Unwind_DebugHook): New function.
> +       (uw_restore_core_regs): New define.
> +       (unwind_phase2): Use `uw_restore_core_regs' instead of
> +       `restore_core_regs'.

You don't need the `' quoting of the function names in the ChangeLog.

> +       (unwind_phase2_forced): Likewise.
> +       (__gnu_Unwind_Resume): Likewise.
> +
>  2011-11-22  Iain Sandoe  <iains@gcc.gnu.org>
>
>        * config/darwin-crt-tm.c: New file.
> diff --git a/libgcc/unwind-arm-common.inc b/libgcc/unwind-arm-common.inc
> index 0713056..bf16902 100644
> --- a/libgcc/unwind-arm-common.inc
> +++ b/libgcc/unwind-arm-common.inc
> @@ -21,8 +21,15 @@
>    see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>    <http://www.gnu.org/licenses/>.  */
>
> +#include "tconfig.h"
> +#include "tsystem.h"
>  #include "unwind.h"
>
> +/* Used for SystemTap unwinder probe.  */
> +#ifdef HAVE_SYS_SDT_H
> +#include <sys/sdt.h>
> +#endif
> +
>  /* We add a prototype for abort here to avoid creating a dependency on
>    target headers.  */
>  extern void abort (void);
> @@ -105,6 +112,44 @@ static inline _uw selfrel_offset31 (const _uw *p);
>
>  static _uw __gnu_unwind_get_pr_addr (int idx);
>
> +static void _Unwind_DebugHook (void *, void *)
> +  __attribute__ ((__noinline__, __used__, __noclone__));
> +
> +/* This function is called during unwinding.  It is intended as a hook
> +   for a debugger to intercept exceptions.  CFA is the CFA of the
> +   target frame.  HANDLER is the PC to which control will be
> +   transferred.  */
> +
> +static void
> +_Unwind_DebugHook (void *cfa __attribute__ ((__unused__)),
> +                  void *handler __attribute__ ((__unused__)))
> +{
> +  /* We only want to use stap probes starting with v3.  Earlier
> +     versions added too much startup cost.  */
> +#if defined (HAVE_SYS_SDT_H) && defined (STAP_PROBE2) && _SDT_NOTE_TYPE >= 3
> +  STAP_PROBE2 (libgcc, unwind, cfa, handler);
> +#else
> +  asm ("");
> +#endif
> +}
> +
> +/* This is a wrapper to be called when we need to restore core registers.
> +   It will call `_Unwind_DebugHook' before restoring the registers, thus
> +   making it possible to intercept and debug exceptions.
> +
> +   When calling `_Unwind_DebugHook', the first argument (the CFA) is zero
> +   because we are not interested in it.  However, it must be there (even
> +   being zero) because GDB expects to find it when using the probe.  */
> +
> +#define uw_restore_core_regs(TARGET, CORE)                                   \
> +  do                                                                         \
> +    {                                                                        \
> +      void *handler = __builtin_frob_return_addr ((void *) VRS_PC (TARGET));  \
> +      _Unwind_DebugHook (0, handler);                                        \
> +      restore_core_regs (CORE);                                                      \
> +    }                                                                        \
> +  while (0)
> +
>  /* Perform a binary search for RETURN_ADDRESS in TABLE.  The table contains
>    NREC entries.  */
>
> @@ -253,8 +298,8 @@ unwind_phase2 (_Unwind_Control_Block * ucbp, phase2_vrs * vrs)
>
>   if (pr_result != _URC_INSTALL_CONTEXT)
>     abort();
> -
> -  restore_core_regs (&vrs->core);
> +
> +  uw_restore_core_regs (vrs, &vrs->core);
>  }
>
>  /* Perform phase2 forced unwinding.  */
> @@ -339,7 +384,7 @@ unwind_phase2_forced (_Unwind_Control_Block *ucbp, phase2_vrs *entry_vrs,
>       return _URC_FAILURE;
>     }
>
> -  restore_core_regs (&saved_vrs.core);
> +  uw_restore_core_regs (&saved_vrs, &saved_vrs.core);
>  }
>
>  /* This is a very limited implementation of _Unwind_GetCFA.  It returns
> @@ -450,7 +495,7 @@ __gnu_Unwind_Resume (_Unwind_Control_Block * ucbp, phase2_vrs * entry_vrs)
>     {
>     case _URC_INSTALL_CONTEXT:
>       /* Upload the registers to enter the landing pad.  */
> -      restore_core_regs (&entry_vrs->core);
> +      uw_restore_core_regs (entry_vrs, &entry_vrs->core);
>
>     case _URC_CONTINUE_UNWIND:
>       /* Continue unwinding the next frame.  */

Otherwise looks ok to me .

Ramana
Bernd Schmidt - Dec. 2, 2011, 12:25 p.m.
On 12/01/11 13:01, Ramana Radhakrishnan wrote:
> Sergio: Other than a few minor tweaks to the Changelog it largely
> looks obvious to me.
> 
> Bernd, could you take another look at this since this is now shared
> with the c6x backend ?

Doesn't look like it would cause problems. I have no idea what
builtin_frob_return_addr does but it appears to exist everywhere.


Bernd
Sergio Durigan Junior - Dec. 2, 2011, 6:26 p.m.
Bernd Schmidt <bernds@codesourcery.com> writes:

> On 12/01/11 13:01, Ramana Radhakrishnan wrote:
>> Sergio: Other than a few minor tweaks to the Changelog it largely
>> looks obvious to me.
>> 
>> Bernd, could you take another look at this since this is now shared
>> with the c6x backend ?
>
> Doesn't look like it would cause problems. I have no idea what
> builtin_frob_return_addr does but it appears to exist everywhere.

Thanks for the reviews.  I guess I'll leave the call to
builtin_frob_return_addr there.  So, after addressing Ramana's
suggestions to ChangeLog, is this patch OK to go in?

Thanks.
Richard Henderson - Dec. 3, 2011, midnight
On 12/02/2011 04:25 AM, Bernd Schmidt wrote:
> Doesn't look like it would cause problems. I have no idea what
> builtin_frob_return_addr does but it appears to exist everywhere.

It's for adjusting the return address in magic ways.  E.g. Sparc pc+8 for 
structure returns, ARM low bit for thumb returns.  I.e. whatever the target
needs for EH handling.


r~
Sergio Durigan Junior - Dec. 8, 2011, 6:32 p.m.
Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Bernd Schmidt <bernds@codesourcery.com> writes:
>
>> On 12/01/11 13:01, Ramana Radhakrishnan wrote:
>>> Sergio: Other than a few minor tweaks to the Changelog it largely
>>> looks obvious to me.
>>> 
>>> Bernd, could you take another look at this since this is now shared
>>> with the c6x backend ?
>>
>> Doesn't look like it would cause problems. I have no idea what
>> builtin_frob_return_addr does but it appears to exist everywhere.
>
> Thanks for the reviews.  I guess I'll leave the call to
> builtin_frob_return_addr there.  So, after addressing Ramana's
> suggestions to ChangeLog, is this patch OK to go in?

Ping.
Sergio Durigan Junior - Dec. 14, 2011, 12:18 p.m.
Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> Bernd Schmidt <bernds@codesourcery.com> writes:
>>
>>> On 12/01/11 13:01, Ramana Radhakrishnan wrote:
>>>> Sergio: Other than a few minor tweaks to the Changelog it largely
>>>> looks obvious to me.
>>>> 
>>>> Bernd, could you take another look at this since this is now shared
>>>> with the c6x backend ?
>>>
>>> Doesn't look like it would cause problems. I have no idea what
>>> builtin_frob_return_addr does but it appears to exist everywhere.
>>
>> Thanks for the reviews.  I guess I'll leave the call to
>> builtin_frob_return_addr there.  So, after addressing Ramana's
>> suggestions to ChangeLog, is this patch OK to go in?
>
> Ping.

Ping ^2.
Tom Tromey - Dec. 20, 2011, 8:53 p.m.
>>>>> "Ramana" == Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> writes:

Ramana> Otherwise looks ok to me .

Sergio was asked me to ping this for him, but instead I re-read the
entire thread and I think that this was an approval conditional on him
making the request changes (just to ChangeLog), which he did.

So, I am checking it in.

Tom

Patch

diff --git a/libgcc/ChangeLog b/libgcc/ChangeLog
index 305e8ad..f6e9dec 100644
--- a/libgcc/ChangeLog
+++ b/libgcc/ChangeLog
@@ -1,3 +1,15 @@ 
+2011-11-22  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	Implement ARM Unwinder SystemTap probe.
+	* unwind-arm-common.inc: Include `tconfig.h', `tsystem.h' and
+	`sys/sdt.h'.
+	(_Unwind_DebugHook): New function.
+	(uw_restore_core_regs): New define.
+	(unwind_phase2): Use `uw_restore_core_regs' instead of
+	`restore_core_regs'.
+	(unwind_phase2_forced): Likewise.
+	(__gnu_Unwind_Resume): Likewise.
+
 2011-11-22  Iain Sandoe  <iains@gcc.gnu.org>
 
 	* config/darwin-crt-tm.c: New file.
diff --git a/libgcc/unwind-arm-common.inc b/libgcc/unwind-arm-common.inc
index 0713056..bf16902 100644
--- a/libgcc/unwind-arm-common.inc
+++ b/libgcc/unwind-arm-common.inc
@@ -21,8 +21,15 @@ 
    see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include "tconfig.h"
+#include "tsystem.h"
 #include "unwind.h"
 
+/* Used for SystemTap unwinder probe.  */
+#ifdef HAVE_SYS_SDT_H
+#include <sys/sdt.h>
+#endif
+
 /* We add a prototype for abort here to avoid creating a dependency on
    target headers.  */
 extern void abort (void);
@@ -105,6 +112,44 @@  static inline _uw selfrel_offset31 (const _uw *p);
 
 static _uw __gnu_unwind_get_pr_addr (int idx);
 
+static void _Unwind_DebugHook (void *, void *)
+  __attribute__ ((__noinline__, __used__, __noclone__));
+
+/* This function is called during unwinding.  It is intended as a hook
+   for a debugger to intercept exceptions.  CFA is the CFA of the
+   target frame.  HANDLER is the PC to which control will be
+   transferred.  */
+
+static void
+_Unwind_DebugHook (void *cfa __attribute__ ((__unused__)),
+		   void *handler __attribute__ ((__unused__)))
+{
+  /* We only want to use stap probes starting with v3.  Earlier
+     versions added too much startup cost.  */
+#if defined (HAVE_SYS_SDT_H) && defined (STAP_PROBE2) && _SDT_NOTE_TYPE >= 3
+  STAP_PROBE2 (libgcc, unwind, cfa, handler);
+#else
+  asm ("");
+#endif
+}
+
+/* This is a wrapper to be called when we need to restore core registers.
+   It will call `_Unwind_DebugHook' before restoring the registers, thus
+   making it possible to intercept and debug exceptions.
+
+   When calling `_Unwind_DebugHook', the first argument (the CFA) is zero
+   because we are not interested in it.  However, it must be there (even
+   being zero) because GDB expects to find it when using the probe.  */
+
+#define uw_restore_core_regs(TARGET, CORE)				      \
+  do									      \
+    {									      \
+      void *handler = __builtin_frob_return_addr ((void *) VRS_PC (TARGET));  \
+      _Unwind_DebugHook (0, handler);					      \
+      restore_core_regs (CORE);						      \
+    }									      \
+  while (0)
+
 /* Perform a binary search for RETURN_ADDRESS in TABLE.  The table contains
    NREC entries.  */
 
@@ -253,8 +298,8 @@  unwind_phase2 (_Unwind_Control_Block * ucbp, phase2_vrs * vrs)
   
   if (pr_result != _URC_INSTALL_CONTEXT)
     abort();
-  
-  restore_core_regs (&vrs->core);
+
+  uw_restore_core_regs (vrs, &vrs->core);
 }
 
 /* Perform phase2 forced unwinding.  */
@@ -339,7 +384,7 @@  unwind_phase2_forced (_Unwind_Control_Block *ucbp, phase2_vrs *entry_vrs,
       return _URC_FAILURE;
     }
 
-  restore_core_regs (&saved_vrs.core);
+  uw_restore_core_regs (&saved_vrs, &saved_vrs.core);
 }
 
 /* This is a very limited implementation of _Unwind_GetCFA.  It returns
@@ -450,7 +495,7 @@  __gnu_Unwind_Resume (_Unwind_Control_Block * ucbp, phase2_vrs * entry_vrs)
     {
     case _URC_INSTALL_CONTEXT:
       /* Upload the registers to enter the landing pad.  */
-      restore_core_regs (&entry_vrs->core);
+      uw_restore_core_regs (entry_vrs, &entry_vrs->core);
 
     case _URC_CONTINUE_UNWIND:
       /* Continue unwinding the next frame.  */