Re: [PATCH RFC] x86: Don't include '-Wa,-' when building with Clang

[Nathan Chancellor]

On Tue, Oct 23, 2018 at 03:08:53PM -0700, Nick Desaulniers wrote:
> On Tue, Oct 23, 2018 at 2:58 PM Nathan Chancellor
> <natechancel...@gmail.com> wrote:
> >
> > On Tue, Oct 23, 2018 at 01:01:22PM -0700, H. Peter Anvin wrote:
> > > On 10/23/18 11:40, Nick Desaulniers wrote:
> > > > On Mon, Oct 22, 2018 at 10:11 PM Nadav Amit <na...@vmware.com> wrote:
> > > >>
> > > >> at 5:37 PM, Nathan Chancellor <natechancel...@gmail.com> wrote:
> > > >>
> > > >> Commit 77b0bf55bc67 ("kbuild/Makefile: Prepare for using macros in
> > > >> inline assembly code to work around asm() related GCC inlining bugs")
> > > >> added this flag to KBUILD_CFLAGS, where it works perfectly fine with
> > > >> GCC. However, when building with Clang, all of the object files compile
> > > >> fine but the build hangs indefinitely at init/main.o, right before the
> > > >> linking stage. Don't include this flag when building with Clang.
> > > >>
> > > >> The kernel builds and boots to a shell in QEMU with both GCC and Clang
> > > >> with this patch applied.
> > > >>
> > > >> Link: 
> > > >> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FClangBuiltLinux%2Flinux%2Fissues%2F213&amp;data=02%7C01%7Cnamit%40vmware.com%7C871daebc2ca44947d28d08d638811fb5%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636758524579997650&amp;sdata=shuxW81QRrO3TSqbgf462wgZYdLeAKeQEdGRxmnUX30%3D&amp;reserved=0
> > > >> Signed-off-by: Nathan Chancellor <natechancel...@gmail.com>
> > > >> ---
> > > >>
> > > >> The reason this patch is labeled RFC is while I can verify that this
> > > >> fixes the issue, I'm not entirely sure why the '-Wa,-' works for GCC
> > > >> and not Clang. I looked into what the flag means and I couldn't really
> > > >> find anything so I just assume it's taking input from stdin? The issue
> > > >> could stem from how GCC forks gas versus how Clang does it. If this
> > > >> isn't of concern and the maintainers are happy with this patch as is,
> > > >> feel free to take it.
> > > >>
> > >
> > > Perhaps someone could actually, you know, time the build and see how
> > > much -pipe actually matters, if at all?
> > >
> > >       -hpa
> > >
> >
> > Thank you for the suggestion! With the attached diff for removing
> > '-pipe' and 'make -j1' with defconfig (just to make sure any variance
> > would stand out), here are my results:
> >
> > -pipe (GCC):
> >
> > real    15m55.202s
> > user    14m17.748s
> > sys     1m47.496s
> >
> > No -pipe (GCC):
> >
> > real    16m4.430s
> > user    14m16.277s
> > sys     1m46.604s
> >
> > -pipe (Clang):
> >
> > real    21m26.016s
> > user    19m21.722s
> > sys     2m2.606s
> >
> > No -pipe (Clang):
> >
> > real    21m27.822s
> > user    19m22.092s
> > sys     2m4.151s
> 
> Looks like Clang eats `-pipe`:
> https://github.com/llvm-mirror/clang/blob/391667a023f79287f9c40868f34f08c161555556/lib/Driver/Driver.cpp#L962
> commit r110007 has the log:
>     Driver: Start ripping out support for -pipe, which is worthless
> and complicates
>     too many other things.
> 

In that case, we can either keep this change (I'll resend with the
explanation that Clang doesn't respect -pipe) or we can just rip out
-pipe for GCC too. Here are three separate results for GCC with my
normal jobs flag:

-pipe (GCC):

real    3m40.813s
real    3m44.449s
real    3m39.648s

No -pipe (GCC):

real    3m38.492s
real    3m38.335s
real    3m38.975s

Practically no variance.

Thanks,
Nathan

> >
> > Certainly seems like -pipe doesn't make a ton of difference. If this is
> > a better fix, I am happy to draft up a proper commit message and send
> > it out for review.
> >
> > ==================================================
> >
> > diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> > index 73f4831283ac..672c689c1faa 100644
> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -213,8 +213,6 @@ ifdef CONFIG_X86_64
> >  KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
> >  endif
> >
> > -# Speed up the build
> > -KBUILD_CFLAGS += -pipe
> >  # Workaround for a gcc prelease that unfortunately was shipped in a suse 
> > release
> >  KBUILD_CFLAGS += -Wno-sign-compare
> >  #
> > @@ -239,7 +237,7 @@ archheaders:
> >  archmacros:
> >         $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
> >
> > -ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s -Wa,-
> > +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
> >  export ASM_MACRO_FLAGS
> >  KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers