Re: [PATCH v2 10/36] x86/vdso: Define BUILD_VDSO while building and emit .eh_frame in asm

[Ingo Molnar <mingo@kernel.org>]

* Andy Lutomirski <luto@kernel.org> wrote:

> For the vDSO, user code wants runtime unwind info.  Make sure that,
> if we use .cfi directives, we generate it.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/vdso/Makefile  |  4 ++--
>  arch/x86/include/asm/dwarf2.h | 13 ++++++++++---
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index 3bfb39e7b8b2..265c0ed68118 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -67,7 +67,7 @@ $(obj)/vdso-image-%.c: $(obj)/vdso%.so.dbg $(obj)/vdso%.so $(obj)/vdso2c FORCE
>  CFL := $(PROFILING) -mcmodel=small -fPIC -O2 -fasynchronous-unwind-tables -m64 \
>         $(filter -g%,$(KBUILD_CFLAGS)) $(call cc-option, -fno-stack-protector) \
>         -fno-omit-frame-pointer -foptimize-sibling-calls \
> -       -DDISABLE_BRANCH_PROFILING
> +       -DDISABLE_BRANCH_PROFILING -DBUILD_VDSO
>  
>  $(vobjs): KBUILD_CFLAGS += $(CFL)
>  
> @@ -131,7 +131,7 @@ targets += vdso32/vdso32.lds
>  targets += vdso32/note.o vdso32/vclock_gettime.o vdso32/system_call.o
>  targets += vdso32/vclock_gettime.o
>  
> -KBUILD_AFLAGS_32 := $(filter-out -m64,$(KBUILD_AFLAGS))
> +KBUILD_AFLAGS_32 := $(filter-out -m64,$(KBUILD_AFLAGS)) -DBUILD_VDSO
>  $(obj)/vdso32.so.dbg: KBUILD_AFLAGS = $(KBUILD_AFLAGS_32)
>  $(obj)/vdso32.so.dbg: asflags-$(CONFIG_X86_64) += -m32
>  
> diff --git a/arch/x86/include/asm/dwarf2.h b/arch/x86/include/asm/dwarf2.h
> index de1cdaf4d743..09133ba032b3 100644
> --- a/arch/x86/include/asm/dwarf2.h
> +++ b/arch/x86/include/asm/dwarf2.h
> @@ -36,15 +36,22 @@
>  #endif
>  
>  #if defined(CONFIG_AS_CFI_SECTIONS) && defined(__ASSEMBLY__)
> +#ifndef BUILD_VDSO
>  	/*
>  	 * Emit CFI data in .debug_frame sections, not .eh_frame sections.
>  	 * The latter we currently just discard since we don't do DWARF
>  	 * unwinding at runtime.  So only the offline DWARF information is
> -	 * useful to anyone.  Note we should not use this directive if this
> -	 * file is used in the vDSO assembly, or if vmlinux.lds.S gets
> -	 * changed so it doesn't discard .eh_frame.
> +	 * useful to anyone.  Note we should not use this directive if
> +	 * vmlinux.lds.S gets changed so it doesn't discard .eh_frame.
>  	 */
>  	.cfi_sections .debug_frame
> +#else
> +	 /*
> +	  * For the vDSO, emit both runtime unwind information and debug
> +	  * symbols for the .dbg file.
> +	  */
> +	.cfi_sections .eh_frame, .debug_frame
> +#endif
>  #endif

So it's exactly such hacks why I dislike the old CFI code:

1)

Emitting assembler directives from .h files is just disgusting. What's wrong with 
adding it to the .c file, so that it becomes obvious to everyone what's going on? 
Or if it's possible, do it in the build command line - the vDSO is built in a 
different way anyway.

2)

Could we require that to build the vDSO you need CFI capable tooling? All other 
tooling is probably on very ancient systems which can live with only having the 
vsyscall and the regular int80 entry methods, or something like that.

So could you please clean this all up instead of reintroducing old hacks? I'll 
apply the patches and see how well they work to move things forward, but this part 
is disgusting and needs to be resolved.

Thanks,

	Ingo