Re: [Libunwind-devel] [PATCH 20/27] ARM: fix non-signal-frame local unw_resume() due to compiler optimization cleverness

[Ladislav Michl]

On Tue, Nov 13, 2012 at 11:41:18AM +0200, Tommi Rantala wrote:
> 2012/11/13 Ladislav Michl <address@hidden>:
> > 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.

Thanks for noticing, copy&paste is evil.

> > +  [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.

The whole purpose is to signal compiler nothing past this point is
ever executed to silence warning. Anyway, the loop could be simplified.

> 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

I do not like hiding stuff too much. __builtin prefix should mean exactly
that it is built in. Just checked kernel source, they are doing it the same
way, so it must be right ;-)
---
 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..7d549aa 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()]])],
+  [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..966a3e3 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 { } while (1)
+#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;
 }