[CRIU] [PATCH] ppc64: handle gcc memcpy optimisation

[Laurent Dufour]

On 07/10/2015 13:49, Christopher Covington wrote:
> Hi Laurent,
> 
> On 10/07/2015 05:09 AM, Laurent Dufour wrote:
>> The commit 871da9a11147 ("pie: Give VDSO symbol table local scope")
>> move the definition of the vdso_symbols from global to local variable
>> in vdso_fill_symtable(). This makes sense since this variable is only
>> used in this function. However this raises a build issue on powerPC,
>> where a memcpy undefined symbol is detected when doing the first
>> relocation phase of the parasite code:
>>
>> parasite_blob: Error (pie/piegen/elf.c:258): Unexpected undefined
>> symbol:memcpy
>>
>> This memcpy symbol is pulled by the C compiler generated code which
>> tries to optimize the stack initialization when entering
>> vdso_fill_symtable(). The optimization is done by copying the
>> initialized data to the stack using memcpy. But when building the
>> parasite code, the C library is not linked and there is no memcpy
>> symbol. However there is builtin_memcpy() which is doing the same.
>>
>> Ideally, the builtin_memcpy should be named memcpy() to replace the C
>> library one, and it should only be built for the parasite/restorer
>> code. But the way CRIU is built, the same vdso-util.o file is used
>> twice for criu which is linked with the C library and by the
>> parasite/restorer code. Thus naming builtin_memcpy memcpy leads to
>> belongs on builtin_memcpy even when the C library is in the picture,
>> which is not the best option (assuming C library mem operation are
>> more efficient).
>>
>> Among the memcpy symbol issue, this shows that same objects are used
>> both in CRIU and the parasite/restorer code. This should not be the
>> case since parasite/restorer are built in pie form and criu's object
>> not. The shared code should be built twice, once on pie form for the
>> parasite/restorer code, once *normally* for the criu binary.
>>
>> Addressing the build issue implies more work than expected.
>> For the moment, this patch is defining a memcpy service when building
>> the parasite code to fix the build issue on ppc64.
>> Once the build issue is addressed, builtin_memcpy should be renamed
>> memcpy and only be used for parasite/restorer code, and this
>> definition removed.
>>
>> Signed-off-by: Laurent Dufour <ldufour at linux.vnet.ibm.com>
> 
> Reviewed-by: Christopher Covington <cov at codeaurora.org>
> 
>> diff --git a/arch/ppc64/vdso-pie.c b/arch/ppc64/vdso-pie.c
>> index 30437d5cc686..cab61cca3e74 100644
>> --- a/arch/ppc64/vdso-pie.c
>> +++ b/arch/ppc64/vdso-pie.c
>> @@ -16,6 +16,20 @@
>>  /* This symbols are defined in vdso-trampoline.S */
>>  extern char *vdso_trampoline, *vdso_trampoline_end;
>>  
>> +/*
>> + * When building the parasite code, the compiler may rely on the C library
>> + * service memcpy to initialise big local variable in the stack.
>> + * Since we are not linked with the C library, we have to remap this service
>> + * to the builtin one we are using.
>> + * Note this cannot be done through macro since the compiler is creating the
>> + * memcpy call in our back. So we have to define a *real* symbol.
>> + */
>> +void *memcpy(void *to, const void *from, unsigned long n)
>> +{
>> +    return builtin_memcpy(to, from, n);
>> +}
>> +
>> +
> 
> Looks good to me. (I'm a little curious if the --defsym=memcpy=builtin_memcpy
> linker option could do the same thing, but even if it did I have no reason to
> prefer one approach over the other.)

Hi Christopher,

Thanks for bringing to me the --defsym ld option.

I did test with it and it works fine, and I'd rather prefer this
approach which remove an useless intermediate call.

Anyway, as I mentioned in the commit log, the best way is to build
object for parasite/restorer code separately with PIE/PIC options and
with memcpy plugged to the per architecture builtin function. For ppc64,
I'd then simply add a label in the assembly code for memcpy.

Cheers,
Laurent.