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

  I've attached patch v2 to this email with the fixes suggested by Adhemerval.

One last issue is regard to the copyright on the tests. I put a 
copyright there but, the test is based on a test the user reported in 
bugzilla. I've adapted to libsupport and changed some of the code. What 
should I do in that case? I have to get the permission from the author 
first since the test is a derivative work? Some of the tests doesn't 
have copyright, and some of them mention the person who reports the test 
like stdio-common/tstscanf.c but I can't find a standard.

Em 16-05-2018 21:42, libc-alpha-digest-help@sourceware.org escreveu:
> 
> On 16/05/2018 16:36, Rogerio Alves wrote:
>> Attached to this email I am sending the first version of the patch but I have a problem with this patch. I can't make the test work. By some reason it can't find the .so needed by the test:
>>
>> FAIL:
>> ./setjmp-bug21895.so: cannot open shared object file: No such file or directory
>>
>> Notice that if I manually execute the command on make check stdout inside the folder it works fine. I'd appreciate any help on that.
>>
>> Regards
>>
>> Em 15-05-2018 17:48, Tulio Magno Quites Machado Filho escreveu:
>>> Florian Weimer<fw@deneb.enyo.de>  writes:
>>>
>>>> * Rogerio Alves:
>>>>
>>>>> One simple solution would be always restore the TOC pointer by uncomment
>>>>> the line bellow:
>>>>>
>>>>> /*     std r2,FRAME_TOC_SAVE(r1)       Restore the TOC save area.  */
>>>>>
>>>>> Or maybe we can check if we have a valid TOC pointer before restore it,
>>>>> instead #if defined SHARED.
>>>> Is the register reserved for the TOC pointer in static builds, too?
>>>> Then I suggest to unconditionally save nad restore it; not doing so
>>>> looks like a pointless micro-optimization.
>>>>
>>>> Another problem with sharing jump buffers across static dlopen is that
>>>> you might not have identical pointer guard values.
>>>>
>>>>> I would like to request for comments on this matter: Should we fix/work
>>>>> this? Is feasible to change longjmp to always restore TOC pointer?
>>>> Does setjmp already save it unonditionally?
>>>>
>>>> Removal of static dlopen is still some time away; it's likely not
>>>> going to happen in this cycle, and the fix looks simple enough.
>>> If static dlopen is still going to be supported for some cycles, I also agree
>>> it should be saved and restored unconditionally.
>>>
>> 0001-PATCH-v1-powerpc-Always-restore-TOC-on-longjmp.patch
>>
>>
>>  From eb9895f1548c8f6e1826095aee3221eaf9ce84c9 Mon Sep 17 00:00:00 2001
>> From: Rogerio Alves<rcardoso@linux.vnet.ibm.com>
>> Date: Wed, 16 May 2018 14:20:53 -0500
>> Subject: [PATCH] [PATCH v1] powerpc: Always restore TOC on longjmp.
>>
>> 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
> Please indicate where you actually tested this patch (since the make check
> is broken).
> 

I've fixed the test thanks to your suggestion.

>> 2018-05-16 Rogerio A. Cardoso<rcardoso@linux.vnet.ibm.com>
>>
>> 	*sysdeps/powerpc/powerpc64/__longjmp-common.S: Remove condition code for
>> 	restore r2 on longjmp.
>> 	*setjmp/Makefile: Include test build directives.
> You need to add each rules you might add, the ones you are changing, and
> removing (check other ChangeLog for example).
> 

Done.
>> 	*setjmp/setjmp-bug21895.c: new test file.
>> 	*setjmp/tst-setjmp-bug21895.c: new test file.
> Capitalize the new.
> 

Done.

>> ---
>>   setjmp/Makefile                              | 18 ++++++--
>>   setjmp/setjmp-bug21895.c                     | 42 ++++++++++++++++++
>>   setjmp/tst-setjmp-bug21895.c                 | 65 ++++++++++++++++++++++++++++
>>   sysdeps/powerpc/powerpc64/__longjmp-common.S |  5 +--
>>   4 files changed, 123 insertions(+), 7 deletions(-)
>>   create mode 100644 setjmp/setjmp-bug21895.c
>>   create mode 100644 setjmp/tst-setjmp-bug21895.c
>>
>> diff --git a/setjmp/Makefile b/setjmp/Makefile
>> index dc2fcc6..e715ee6 100644
>> --- a/setjmp/Makefile
>> +++ b/setjmp/Makefile
>> @@ -22,16 +22,28 @@ subdir	:= setjmp
>>   
>>   include ../Makeconfig
>>   
>> -headers	:= setjmp.h bits/setjmp.h bits/setjmp2.h
>> +headers	:= setjmp.h bits/setjmp.h bits/setjmp2.h bits/dlfcn.h dlfcn/dlfcn.h
> Why does it require adding more headers here? This is meant be installed afaik
> and I am not following the requirement here.
> 
>>   
>>   routines	:= setjmp sigjmp bsd-setjmp bsd-_setjmp \
>>   		   longjmp __longjmp jmp-unwind
>>   
>>   tests		:= tst-setjmp jmpbug bug269-setjmp tst-setjmp-fp \
>> -		   tst-sigsetjmp tst-setjmp-static
>> +		   tst-sigsetjmp tst-setjmp-static tst-setjmp-bug21895
>> +
> Since the idea is test for static build I would recommend to follow the other
> static naming, i.e, tst-setjmp-bug21895-static.
> 

Done. Added static suffix.

>>   tests-static	:= tst-setjmp-static
>>   
>> +modules-names = setjmp-bug21895
>>   
>>   include ../Rules
>>   
>> -$(objpfx)tst-setjmp-fp: $(libm)
> I do not think you meant to remove this rule.
> 

It was not removed I've write the new rules before this rule. Anyway 
I've fixed it on v2.

>> +test-modules = $(addprefix $(objpfx),$(addsuffix .so,$(modules-names)))
>> +
>> +ifeq ($(build-shared),yes)
>> +tests: $(test-modules)
>> +endif
>> +
>> +$(objpfx)setjmp-bug21895.so: $(libdl)
>> +$(objpfx)tst-setjmp-bug21895: $(libdl)
>> +$(objpfx)tst-setjmp-bug21895.out: $(objpfx)setjmp-bug21895.so
>> +
>> +$(objpfx)ts-tsetjmp-fp: $(libm)
> I think the idea is to build the test even if build-shared is set to no (since
> you are testing static linking).  I think what you want is:
> 
> ---
> 
> diff --git a/setjmp/Makefile b/setjmp/Makefile
> index dc2fcc6..231474b 100644
> --- a/setjmp/Makefile
> +++ b/setjmp/Makefile
> @@ -28,10 +28,17 @@ routines    := setjmp sigjmp bsd-setjmp bsd-_setjmp \
>                     longjmp __longjmp jmp-unwind
> 
>   tests          := tst-setjmp jmpbug bug269-setjmp tst-setjmp-fp \
> -                  tst-sigsetjmp tst-setjmp-static
> -tests-static   := tst-setjmp-static
> +                  tst-sigsetjmp tst-setjmp-static tst-setjmp-bug21895-static
> 
> +tests-static   := tst-setjmp-static tst-setjmp-bug21895-static
> +
> +modules-names = setjmp-bug21895
> 
>   include ../Rules
> 
>   $(objpfx)tst-setjmp-fp: $(libm)
> +$(objpfx)tst-setjmp-bug21895-static: $(common-objpfx)dlfcn/libdl.a
> +$(objpfx)tst-setjmp-bug21895-static.out: $(objpfx)setjmp-bug21895.so
> +
> +tst-setjmp-bug21895-static-ENV = \
> +  LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)setjmp:$(common-objpfx)elf
> 
> ---
> 
> At least with qemu-static it seems to work with your patch.
> 
> 

Thanks to your help it's working now. That's exactly what I wanted.

>> diff --git a/setjmp/setjmp-bug21895.c b/setjmp/setjmp-bug21895.c
>> new file mode 100644
>> index 0000000..d6f5516
>> --- /dev/null
>> +++ b/setjmp/setjmp-bug21895.c
>> @@ -0,0 +1,42 @@
>> +/* Copyright (C) 2013-2018 Free Software Foundation, Inc.
> Wrong Copyright date span and missing one line description.
> 
Fixed.
>> +   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/>.  */
>> +
>> +/* Test that setjmp/longjmp interoperability with static dlopen.
>> +   Bugzila #21895.  */
>> +#include <alloca.h>
>> +#include <string.h>
>> +#include <setjmp.h>
>> +
>> +jmp_buf jb;
>> +void (*bar)(jmp_buf);
>> +
>> +void
>> +lbar (int i, ...)
>> +{
>> +  bar(jb);
>> +  for(;;);
>> +}
>> +
>> +void
>> +foo (void)
>> +{
>> +  int i = setjmp(jb);
>> +  char *c = alloca(256);
>> +  memset(c, 0, 256);
>> +  lbar(i);
>> +  for(;;);
>> +}
>> diff --git a/setjmp/tst-setjmp-bug21895.c b/setjmp/tst-setjmp-bug21895.c
>> new file mode 100644
>> index 0000000..5333494
>> --- /dev/null
>> +++ b/setjmp/tst-setjmp-bug21895.c
>> @@ -0,0 +1,65 @@
>> +/* Copyright (C) 2013-2018 Free Software Foundation, Inc.
> Wrong Copyright date span and missing one line description.
> 
Fixed.
>> +   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/>.  */
>> +
>> +/* Test that setjmp/longjmp interoperability with static dlopen.
>> +   Bugzila #21895.  */
>> +
>> +#include <setjmp.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <dlfcn.h>
>> +
>> +static void
>> +bar (jmp_buf jb)
>> +{
>> +  static int i;
>> +  if (i++==1) exit(0);
>> +  longjmp(jb, i);
>> +}
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  void *h = dlopen("./setjmp-bug21895.so", RTLD_NOW);
> Better to just use name, since on test environment variable in Makefile
> change I proposed the path to build setjmp folder is already on the
> LD_LIBRARY_PATH list.
> 
>> +  if (!h) {
>> +    puts ("FAIL: ");
>> +    puts (dlerror());
>> +    return 1;
>> +  }
> Wrong indentation and please use libsupport macros.
> 

Fixed. Using libsupport now.
>> +
>> +  void (*pfoo)(void) = dlsym(h, "foo");
>> +  if (!pfoo) {
>> +    puts ("FAIL: ");
>> +    puts (dlerror());
>> +    return 1;
>> +  }
>> +
>> +  void (**ppbar)(jmp_buf) = dlsym(h, "bar");
>> +  if (!ppbar) {
>> +    puts ("FAIL: ");
>> +    puts (dlerror());
>> +    return 1;
>> +  }
>> +
>> +  *ppbar = bar;
>> +  pfoo();
>> +
>> +  for(;;);
>> +}
>> +
>> +#define TEST_FUNCTION do_test ()
>> +#include "../test-skeleton.c"
> We do not use test-skeleton.c inclusion any longer, use libsupport
> (support/test-driver.c).
> 

Done.

>> diff --git a/sysdeps/powerpc/powerpc64/__longjmp-common.S b/sysdeps/powerpc/powerpc64/__longjmp-common.S
>> index 0e10b8d..a5973c9 100644
>> --- a/sysdeps/powerpc/powerpc64/__longjmp-common.S
>> +++ b/sysdeps/powerpc/powerpc64/__longjmp-common.S
>> @@ -130,9 +130,6 @@ L(no_vmx):
>>   	ld r0,(JB_LR*8)(r3)
>>   	ld r14,((JB_GPRS+0)*8)(r3)
>>   	lfd fp14,((JB_FPRS+0)*8)(r3)
>> -#if defined SHARED && !IS_IN (rtld)
>> -	std r2,FRAME_TOC_SAVE(r1)	/* Restore the callers TOC save area.  */
>> -#endif
>>   	ld r15,((JB_GPRS+1)*8)(r3)
>>   	lfd fp15,((JB_FPRS+1)*8)(r3)
>>   	ld r16,((JB_GPRS+2)*8)(r3)
>> @@ -152,7 +149,7 @@ L(no_vmx):
>>   	second argument (-4@4), and target address (8@0), respectively.  */
>>   	LIBC_PROBE (longjmp, 3, 8@3, -4@4, 8@0)
>>   	mtlr r0
>> -/* 	std r2,FRAME_TOC_SAVE(r1)	Restore the TOC save area.  */
>> + 	std r2,FRAME_TOC_SAVE(r1)	/* Restore the TOC save area.  */
> Space before tab in indent.
>

Done.

>>   	ld r21,((JB_GPRS+7)*8)(r3)
>>   	lfd fp21,((JB_FPRS+7)*8)(r3)
>>   	ld r22,((JB_GPRS+8)*8)(r3)
>> -- 2.7.4
>>

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

Commit Message

Comments

Patch