Message-ID: <0025D05B-9171-46ED-8DD8-83E487BC88B7@zytor.com>
Date:   Tue, 23 Oct 2018 16:01:58 -0700
From:   hpa@...or.com
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Nathan Chancellor <natechancellor@...il.com>
CC:     namit@...are.com, Thomas Gleixner <tglx@...utronix.de>,
        mingo@...hat.com, bp@...en8.de, x86@...nel.org,
        LKML <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: Re: [PATCH RFC] x86: Don't include '-Wa,-' when building with Clang

On October 23, 2018 3:53:10 PM PDT, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
>On Tue, Oct 23, 2018 at 3:44 PM Nathan Chancellor
><natechancellor@...il.com> wrote:
>>
>> On Tue, Oct 23, 2018 at 03:08:53PM -0700, Nick Desaulniers wrote:
>> > On Tue, Oct 23, 2018 at 2:58 PM Nathan Chancellor
>> > <natechancellor@...il.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
><namit@...are.com> wrote:
>> > > > >>
>> > > > >> at 5:37 PM, Nathan Chancellor <natechancellor@...il.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 <natechancellor@...il.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 for these measurements.  With these in mind I agree with HPA
>that `-pipe -Wa,-` doesn't buy us anything, and would be simpler to
>remove it for compatibility with Clang.
>
>>
>> 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

So -pipe actually hurts sightly. Let's kill it.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Powered by blists - more mailing lists