Re: [RFC] powerpc: restore TOC when static longjmp to shared object

[Tulio Magno Quites Machado Filho <tuliom at linux dot ibm dot com>]

Rogerio Alves <rcardoso@linux.vnet.ibm.com> writes:

> I've make the changes you asked for. Thank you for your review. Patch v4 
> is attached.

I understand this patch solved all the requested changes.
I found some minor issues, which I fixed myself and pushed it.

> Subject: [PATCH v4] powerpc: Always restore TOC on longjmp.

We usually mention the bug report in the commit title, e.g.:

[PATCH v4] powerpc: Always restore TOC on longjmp [BZ #21895]

> This patch change longjmp to always restore the TOC pointer (r2 register)
> to the caller frame on powerpc. This is related to bug 21895[1] that reports
> a situation where you have a static longjmp to a shared object file.
>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=21895
>
> 2018-05-16 Rogerio A. Cardoso <rcardoso@linux.vnet.ibm.com>

Added 2 spaces around your name.

We also mention the bug report in the ChangeLog entry, e.g.:

	[BZ #21895]
>
> 	*sysdeps/powerpc/powerpc64/__longjmp-common.S: Remove condition code for
         ^
         Missing space here.

> diff --git a/sysdeps/powerpc/powerpc64/setjmp-bug21895.c b/sysdeps/powerpc/powerpc64/setjmp-bug21895.c
> new file mode 100644
> index 0000000..9e65f23
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/setjmp-bug21895.c
> @@ -0,0 +1,52 @@
> +/* Shared object part of test for setjmp interoperability with static
> +   dlopen BZ #21895.
> +   Copyright (C) 2017-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <string.h>
> +#include <setjmp.h>
> +
> +/* Copy r1 adress to a local variable.  */
> +#define GET_STACK_POINTER(sp) \
> + ({                           \
> + asm volatile ("mr %0, 1\n\t" \
> +	: "=r" (sp)               \
> +  );                          \

Replaced 8 spaces with tabs.

> diff --git a/sysdeps/powerpc/powerpc64/tst-setjmp-bug21895-static.c b/sysdeps/powerpc/powerpc64/tst-setjmp-bug21895-static.c
> new file mode 100644
> index 0000000..235ba50
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/tst-setjmp-bug21895-static.c
> @@ -0,0 +1,81 @@
> +/* Test setjmp interoperability with static dlopen BZ #21895.
> +   Copyright (C) 2017-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <setjmp.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <dlfcn.h>
> +
> +/* Set TOC area pointed by sp to zero.  */
> +#define SET_TOC_TO_ZERO(sp)   \
> + ({                           \
> +    unsigned int zero = 0;    \
> +    asm volatile(             \

Likewise.

> +/* Make sure the test will not stuck if jmp fails and fall into one of
> +   for(;;).  */
> +#define TIMEOUT 100

I don't think this test needs 100s.  I confirmed the default timeout (20s) is
enough for this test and removed these 3 lines.

Thanks!

-- 
Tulio Magno