Re: [PATCH] ftrace/x86: Fix function graph tracer reset path

[Namhyung Kim]

On Mon, May 16, 2016 at 05:56:14PM +0900, Masami Hiramatsu wrote:
> On Sun, 15 May 2016 22:06:52 +0900
> Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> 
> > On Sun, May 15, 2016 at 09:41:41AM +0900, Masami Hiramatsu wrote:
> > 
> > Hi Masami,
> > 
> > > Hi Namhyung,
> > > 
> > > I'm interested in this problem, and it seems compiling environment
> > > related or kconfig related problem.
> > > If you can reproduce this kernel, would you share what the "AS"
> > > commandline shows? That can be done as below;
> > > 
> > >  $ make V=1 arch/x86/kernel/mcount_64.o | grep mcount_64
> > 
> >   gcc -Wp,-MD,arch/x86/kernel/.mcount_64.o.d  -nostdinc -isystem /usr/lib/gcc/x86_64-unknown-linux-gnu/5.3.0/include -I/home/namhyung/project/linux/arch/x86/include -Iarch/x86/include/generated/uapi -Iarch/x86/include/generated  -I/home/namhyung/project/linux/include -Iinclude -I/home/namhyung/project/linux/arch/x86/include/uapi -Iarch/x86/include/generated/uapi -I/home/namhyung/project/linux/include/uapi -Iinclude/generated/uapi -include /home/namhyung/project/linux/include/linux/kconfig.h -D__KERNEL__ -D__ASSEMBLY__ -m64 -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -DCONFIG_AS_FXSAVEQ=1 -DCONFIG_AS_SSSE3=1 -DCONFIG_AS_CRC32=1 -DCONFIG_AS_AVX=1 -DCONFIG_AS_AVX2=1 -DCONFIG_AS_SHA1_NI=1 -DCONFIG_AS_SHA256_NI=1 -mfentry -DCC_USING_FENTRY -DCC_HAVE_ASM_GOTO   -c -o arch/x86/kernel/mcount_64.o /home/namhyung/project/linux/arch/x86/kernel/mcount_64.S
> 
> Thanks! I could reproduced on Ubuntu 16.04 which has gcc-5.3.1/as-2.26.
> On the other hand, on Fedora21(gcc 4.9.2/as-2.24) with same kconfig,
> I didn't see such short jump.
> 
> OK, I did bisect binutils and found below commit caused it ;D

Thanks for doing this!

> 
> 8dcea93252a9ea7dff57e85220a719e2a5e8ab41 is the first bad commit
> commit 8dcea93252a9ea7dff57e85220a719e2a5e8ab41
> Author: H.J. Lu <hjl.tools@xxxxxxxxx>
> Date:   Fri May 15 03:17:31 2015 -0700
> 
>     Add -mshared option to x86 ELF assembler
>     
>     This patch adds -mshared option to x86 ELF assembler.  By default,
>     assembler will optimize out non-PLT relocations against defined non-weak
>     global branch targets with default visibility.  The -mshared option tells
>     the assembler to generate code which may go into a shared library
>     where all non-weak global branch targets with default visibility can
>     be preempted.  The resulting code is slightly bigger.  This option
>     only affects the handling of branch instructions.
>     
>     This Linux kernel patch is needed to create a working x86 Linux kernel if
>     it hasn't been applied:
>     
>     diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>     index ae6588b..b91a00c 100644
>     --- a/arch/x86/kernel/head_64.S
>     +++ b/arch/x86/kernel/head_64.S
>     @@ -339,8 +339,8 @@ early_idt_handlers:
>      	i = i + 1
>      	.endr
>     
>     -/* This is global to keep gas from relaxing the jumps */
>     -ENTRY(early_idt_handler)
>     +/* This is weak to keep gas from relaxing the jumps */
>     +WEAK(early_idt_handler)
>      	cld
>     
>      	cmpl $2,(%rsp)		# X86_TRAP_NMI
>     --
>     
>     gas/
>     
>     	* config/tc-i386.c (shared): New.
>     	(OPTION_MSHARED): Likewise.
>     	(elf_symbol_resolved_in_segment_p): Add relocation argument.
>     	Check PLT relocations and shared.
>     	(md_estimate_size_before_relax): Pass fragP->fr_var to
>     	elf_symbol_resolved_in_segment_p.
>     	(md_longopts): Add -mshared.
>     	(md_show_usage): Likewise.
>     	(md_parse_option): Handle OPTION_MSHARED.
>     	* doc/c-i386.texi: Document -mshared.
>     
>     gas/testsuite/
>     
>     	* gas/i386/i386.exp: Don't run pcrel for ELF targets.  Run
>     	pcrel-elf, relax-4 and x86-64-relax-3 for ELF targets.
>     	* gas/i386/pcrel-elf.d: New file.
>     	* gas/i386/relax-4.d: Likewise.
>     	* gas/i386/x86-64-relax-3.d: Likewise.
>     	* gas/i386/relax-3.d: Pass -mshared to assembler.  Updated.
>     	* gas/i386/x86-64-relax-2.d: Likewise.
>     	* gas/i386/relax-3.s: Add test for PLT relocation.
> 
> :040000 040000 e6c503c04807ae3ca8e74c78f6531050272923ba 8b4f3af73c6a3bac007faa5fe40ac22d45b8fa9f M	gas
> 
> 
> And I found that as above commit said, if we passed the -mshared option to
> gas ("-Wa,-mshared") as I did below, gas generated 5 byte jump again.

So IIUC, this -mshared option disables the optimization on branch
instructions and generates slightly bigger code.  Not sure how much
affected by this though.

Steve, do you think it's better to use this option?

Thanks,
Namhyung


> 
> 
> gcc -Wp,-MD,arch/x86/kernel/.mcount_64.o.d  -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/4.9.2/include -I/home/mhiramat/ksrc/linux/arch/x86/include -Iarch/x86/include/generated/uapi -Iarch/x86/include/generated  -I/home/mhiramat/ksrc/linux/include -Iinclude -I/home/mhiramat/ksrc/linux/arch/x86/include/uapi -Iarch/x86/include/generated/uapi -I/home/mhiramat/ksrc/linux/include/uapi -Iinclude/generated/uapi -include /home/mhiramat/ksrc/linux/include/linux/kconfig.h -D__KERNEL__ -D__ASSEMBLY__ -m64 -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -DCONFIG_AS_FXSAVEQ=1 -DCONFIG_AS_SSSE3=1 -DCONFIG_AS_CRC32=1 -DCONFIG_AS_AVX=1 -DCONFIG_AS_AVX2=1 -DCONFIG_AS_SHA1_NI=1 -DCONFIG_AS_SHA256_NI=1 -mfentry -DCC_USING_FENTRY -DCC_HAVE_ASM_GOTO -Wa,-mshared  -c -o arch/x86/kernel/mcount_64.o /home/mhiramat/ksrc/linux/arch/x86/kernel/mcount_64.S
> 
> 
> 
> 00000000000000b6 <ftrace_epilogue>:
>   b6:   e9 00 00 00 00          jmpq   bb <ftrace_stub>
> 
> 00000000000000bb <ftrace_stub>:
>   bb:   c3                      retq   
>   bc:   0f 1f 40 00             nopl   0x0(%rax)
> 
> 
> Thank you,
> 
> -- 
> Masami Hiramatsu <mhiramat@xxxxxxxxxx>