Re: [Libunwind-devel] [patch 1/3] tests/: test-resume-sig false PASS on non-ia64

[David Mosberger-Tang]

Hi Jan,

I see the issue you're trying to fix with this patch-set (1-3) but I'm
a bit worried about its approach.  In particular, I'm worried about
using unw_get_proc_name() and name-matching to check on the correct
execution of the test-case.  Similarly, I don't think particularly
like building platform-dependencies such as #ifdef __ia64 into the
tests.  Is there perhaps a cleaner way to make the test more portable,
such that it succeeds when it should and fails otherwise?

I did check in your other three patches.  They should show up in the
GIT repository shortly.

Oh, would you mind adding appropriate "Signed-off-by:" trailers in
future patches?

Thanks for the patches!

 --david

On 5/10/07, Jan Kratochvil <address@hidden> wrote:
> Hi,
>
> currently the testcases PASSes on non-ia64 with upstream GLIBC snapshot but it
> should FAIL there as the tested functionality is not implemented on non-ia64
> platforms.
>
>
>
> 2006-12-10  Jan Kratochvil  <address@hidden>
>
>         * tests/Gtest-resume-sig.c (handler): Fixed false PASS on non-ia64.
>         Be adaptive to the number of unwinding steps from `SIGUSR1'.
>
> --- ./tests/Gtest-resume-sig.c  31 May 2006 18:22:16 -0000      1.1
> +++ ./tests/Gtest-resume-sig.c  10 Dec 2006 14:31:14 -0000      1.2
> @@ -32,6 +32,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DE
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> +#include <string.h>
>
>  #ifdef HAVE_IA64INTRIN_H
>  # include <ia64intrin.h>
> @@ -68,6 +69,7 @@ handler (int sig)
>    unw_cursor_t c;
>    char foo;
>    int ret;
> +  int stepno;
>
>  #if UNW_TARGET_IA64
>    if (verbose)
> @@ -95,11 +97,32 @@ handler (int sig)
>        if ((ret = unw_init_local (&c, &uc)) < 0)
>         panic ("unw_init_local() failed: ret=%d\n", ret);
>
> -      if ((ret = unw_step (&c)) < 0)           /* step to signal trampoline */
> -       panic ("unw_step(1) failed: ret=%d\n", ret);
> -
> -      if ((ret = unw_step (&c)) < 0)           /* step to signal trampoline */
> -       panic ("unw_step(2) failed: ret=%d\n", ret);
> +      /* After fixing glibc's `__restore_rt' unwinding by CFI in:
> +               
> http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/sysdeps/unix/sysv/linux/x86_64/sigaction.c.diff?cvsroot=glibc&r1=text&tr1=1.10&r2=text&tr2=1.12&f=u
> +        This test started failing as the original code stepped back directly
> +        out of `main' into `__libc_start_main' and so despite `SIGUSR2' was
> +        never hit (at least on GNU/Linux) the test PASSed due to exit code 0.
> +        Patch fixing the unwind is in Fedora Core glibc-2.5.90-11 upwards.
> +        Needing to implement also non-ia64 `resume_restores_sigmask'.  */
> +
> +      for (stepno = 0;; stepno++)
> +        {
> +         char buf[512];
> +
> +         if ((ret = unw_get_proc_name (&c, buf, sizeof (buf), NULL)) < 0)
> +           panic ("unw_get_proc_name(%d) failed: ret=%d\n", stepno, ret);
> +         else
> +           {
> +             if (verbose)
> +               printf ("unw_get_proc_name(%d): %s\n", stepno, buf);
> +             if (!strcmp (buf, "kill") || !strcmp (buf, "main"))
> +               break;
> +           }
> +
> +         /* Step one frame behind `main' to get into `__libc_start_main'.  */
> +         if ((ret = unw_step (&c)) < 0)
> +           panic ("unw_step(%d) failed: ret=%d\n", stepno, ret);
> +       }
>
>        if ((ret = unw_get_reg (&c, UNW_REG_IP, &ip)) < 0)
>         panic ("unw_get_reg(IP) failed: ret=%d\n", ret);
>
>
> _______________________________________________
> Libunwind-devel mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/libunwind-devel


--
Mosberger Consulting LLC, http://www.mosberger-consulting.com/