libunwind-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Libunwind-devel] [PATCH 20/27] ARM: fix non-signal-frame local unw_


From: Tommi Rantala
Subject: Re: [Libunwind-devel] [PATCH 20/27] ARM: fix non-signal-frame local unw_resume() due to compiler optimization cleverness
Date: Tue, 13 Nov 2012 11:41:18 +0200

2012/11/13 Ladislav Michl <address@hidden>:
> On Wed, Aug 22, 2012 at 02:28:46PM +0300, Tommi Rantala wrote:
>> When cross-compiling libunwind with optimizations (-O1 or higher),
>> gcc-4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5) optimizes away the memory
>> writes prior to the inline asm() statement in arm_local_resume() in the
>> non-signal-frame path, causing the `regs' array to be only allocated on
>> the stack, but not populated. This means that we are restoring garbage
>> to the registers.
>>
>> As suggested in the GCC docs, add a fixed size input memory constraint
>> for the array content. This is enough to get the desired code to be
>> generated.
>>
>> Adding __builtin_unreachable() to the point that we should never reach
>> was also in itself enough to inhibit the optimization. It also reduces
>> the function size by a few instructions.
>> ---
>>  src/arm/Gresume.c |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/arm/Gresume.c b/src/arm/Gresume.c
>> index da45dec..4100d87 100644
>> --- a/src/arm/Gresume.c
>> +++ b/src/arm/Gresume.c
>> @@ -1,6 +1,7 @@
>>  /* libunwind - a platform-independent unwind library
>>     Copyright (C) 2008 CodeSourcery
>>     Copyright 2011 Linaro Limited
>> +   Copyright (C) 2012 Tommi Rantala <address@hidden>
>>
>>  This file is part of libunwind.
>>
>> @@ -51,11 +52,16 @@ arm_local_resume (unw_addr_space_t as, unw_cursor_t 
>> *cursor, void *arg)
>>        regs[8] = uc->regs[13]; /* SP */
>>        regs[9] = uc->regs[14]; /* LR */
>>
>> +      struct regs_overlay {
>> +           char x[sizeof(regs)];
>> +      };
>> +
>>        asm __volatile__ (
>>       "ldmia %0, {r4-r12, lr}\n"
>>       "mov sp, r12\n"
>>       "bx lr\n"
>> -     : : "r" (regs)
>> +     : : "r" (regs),
>> +         "m" (*(struct regs_overlay *)regs)
>>        );
>>      }
>>    else
>> @@ -90,6 +96,7 @@ arm_local_resume (unw_addr_space_t as, unw_cursor_t 
>> *cursor, void *arg)
>>       : : "r" (c->sigcontext_sp), "r" (c->sigcontext_pc)
>>        );
>>     }
>> +  __builtin_unreachable();
>>  #else
>>    printf ("%s: implement me\n", __FUNCTION__);
>>  #endif
>
> This patch breaks building with older compilers (and the very same problem is
> with SH arch). __builtin_unreachable comes with GCC 4.5 and later, so what
> about this?

Hi, __builtin_unreachable is indeed GCC specific, and since someone
might want to use some other compiler, providing this check is IMO
reasonable.

> ---
>  configure.ac          |   11 +++++++++++
>  include/libunwind_i.h |    6 ++++++
>  src/arm/Gresume.c     |    2 +-
>  src/sh/Gresume.c      |    2 +-
>  4 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index cffe19b..e3569ad 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -285,6 +285,17 @@ if test x$have__builtin___clear_cache = xyes; then
>  fi
>  AC_MSG_RESULT([$have__builtin___clear_cache])
>
> +AC_MSG_CHECKING([for __builtin_unreachable])
> +AC_LINK_IFELSE(
> +  [AC_LANG_PROGRAM([[]], [[__builtin_unreachable(0, 0)]])],

__builtin_unreachable() does not take arguments, so please remove them
from here.

> +  [have__builtin_unreachable=yes],
> +  [have__builtin_unreachable=no])
> +if test x$have__builtin_unreachable = xyes; then
> +  AC_DEFINE([HAVE__BUILTIN_UNREACHABLE], [1],
> +            [Defined if __builtin_unreachable() is available])
> +fi
> +AC_MSG_RESULT([$have__builtin_unreachable])
> +
>  AC_MSG_CHECKING([for __sync atomics])
>  AC_LINK_IFELSE(
>    [AC_LANG_PROGRAM([[]], [[
> diff --git a/include/libunwind_i.h b/include/libunwind_i.h
> index 23f615e..b9ff649 100644
> --- a/include/libunwind_i.h
> +++ b/include/libunwind_i.h
> @@ -72,6 +72,12 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE 
> SOFTWARE.  */
>  # endif
>  #endif
>
> +#if defined(HAVE__BUILTIN_UNREACHABLE)
> +# define unreachable() __builtin_unreachable()
> +#else
> +# define unreachable() do { for (;;) ; } while (0)
> +#endif
> +

I'm a bit surprised by the loop, I would expect something like a call
to abort(), so that if someone actually hits this they'll get a signal
and a core dump.

How about defining this only when we do _not_ have the GCC builtin
function available, something like:

#ifndef HAVE__BUILTIN_UNREACHABLE
# define __builtin_unreachable() ...
#endif

>  #ifdef DEBUG
>  # define UNW_DEBUG     1
>  #else
> diff --git a/src/arm/Gresume.c b/src/arm/Gresume.c
> index 4100d87..2e5e9fd 100644
> --- a/src/arm/Gresume.c
> +++ b/src/arm/Gresume.c
> @@ -96,7 +96,7 @@ arm_local_resume (unw_addr_space_t as, unw_cursor_t 
> *cursor, void *arg)
>         : : "r" (c->sigcontext_sp), "r" (c->sigcontext_pc)
>        );
>     }
> -  __builtin_unreachable();
> +  unreachable();
>  #else
>    printf ("%s: implement me\n", __FUNCTION__);
>  #endif
> diff --git a/src/sh/Gresume.c b/src/sh/Gresume.c
> index 6a76fe1..536670f 100644
> --- a/src/sh/Gresume.c
> +++ b/src/sh/Gresume.c
> @@ -109,7 +109,7 @@ sh_local_resume (unw_addr_space_t as, unw_cursor_t 
> *cursor, void *arg)
>           "r" (c->sigcontext_pc)
>        );
>     }
> -  __builtin_unreachable();
> +  unreachable();
>  #endif
>    return -UNW_EINVAL;
>  }



reply via email to

[Prev in Thread] Current Thread [Next in Thread]