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

[Christopher Covington]

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.)

Regards,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project