Message-ID: <CAKwvOdmZuzhyaOzh_ruNQUp96zed8+BAz3DCw1hrZrseVNg9tA@mail.gmail.com>
Date:   Wed, 22 Aug 2018 16:05:36 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     joe@...ches.com
Cc:     asmadeus@...ewreck.org,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Kees Cook <keescook@...omium.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Arnd Bergmann <arnd@...db.de>, dwmw@...zon.co.uk,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will.deacon@....com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        daniel@...earbox.net, hpa@...or.com
Subject: Re: [PATCH] compiler-gcc: get back Clang build

On Wed, Aug 22, 2018 at 1:50 PM Joe Perches <joe@...ches.com> wrote:
>
> On Wed, 2018-08-22 at 11:31 -0700, Nick Desaulniers wrote:
> > On Tue, Aug 21, 2018 at 9:32 PM Dominique Martinet
> > <asmadeus@...ewreck.org> wrote:
> > >
> > > Joe Perches wrote on Tue, Aug 21, 2018:
> > > > On Wed, 2018-08-22 at 06:16 +0200, Dominique Martinet wrote:
> > > > > I think that could work, but at the point making a separate
> > > > > compiler-common.h and not including compiler-gcc.h for clang sounds
> > > > > better to me... More importantly here, either solution sound complex
> > > > > enough to require more than a few days and proper testing for all archs
> > > > > etc when compared to the partial revert we have here.
> > > >
> > > > The immediate need for a partial revert seems unnecessary as
> > > > clang hasn't really worked for a couple releases now.
> > >
> > > Sorry for repeating myself, clang is used by bcc for compiling BPF
> > > programs (e.g. bpf_module_create_c_from_string() or any similar function
> > > will use the clang libs to compile the bpf program with linux headers),
> > > and this has always been working because it's not using our makefiles.
> > >
> > > This broke today in master and I only joined this thread after looking
> > > at why the build started failing today and noticing this patch, it
> > > definitely hasn't been broken for two releases, or I wouldn't be here
> > > with this timing :)
> > >
> > >
> > > > The separate compiler file changes are much more sensible,
> > > > even if it takes a few days.
> > >
> > > A few days are fine, but I think some form of fix in 4.19-rc1 would be
> > > good.
> > >
> > > I'll stop taking your time now, but I think you are/were underestimating
> > > how many people use clang with the linux kernel headers indirectly.
> > > BPF is a well-used tool :)
> >
> > Hi Dominique,
> > I'm currently testing a fix in
> > https://github.com/ClangBuiltLinux/linux/commit/1f89ae7622c26b8131f42f3a362d6ef41b88a595,
> > can you please share with me your steps to test/verify that the patch
> > fixes the issue for eBPF?  I'll go talk to a co-worker who know more
> > about eBPF, but I've not yet done anything with it.
>
> A mild suggestion about the patch would be to break it up into
> 2 patches to improve how people read and review them.
>
> 1 include/linux/compiler-*
> 2 everything else
>
> Yes, some kernel configs might not build properly between 1 and 2
> but that likely doesn't matter as those configs probably don't
> build before 1 either.

If we ordered the patches so that the "everything else" went in first,
it would not be a problem.  The first patch would just be the checks
that GCC_VERSION is defined.

In general, I'm happy to split patches, but in this suggested case, it
only shaves off 26 lines from the main body of work.  I don't think 26
lines is enough to justify splitting for readability.  But let me know
if you feel strongly about that point and I'll be happy to split. (or
possibly by "everything else" you meant something else?)

> Perhaps the test in arch/arm/kernel/asm-offsets.c for incompatible
> gcc compiler versions from 4.8.0 to 4.8.2 should be moved to
> compiler-gcc.h.

This a good suggestion, and one I've thought about, although in the
context of builtins.  *Detour ahead*.  Builtins are not portable by
default, and their use really should be feature detected (impossible
currently on gcc due to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970) or version checks
and protected by macros like the symbols in my current patch.  I think
I might soon hoist them to a shared header that safely detects their
support or provides a fallback when possible.  One issue is that some
builtins are arch specific, so do you want to feature detect arch
specific ones for builds of a different arch?  And that's the problem
I have with hoisting arch specific compiler checks into a shared
header; builds of other archs should pay no build penalty for
unrelated archs.  All that to say that I think it's best to keep arch
specific checks in arch/ specific subdirs, but I welcome more thoughts
on this.

Ok, I've boot tested this patch for x86_64 and arm64 in qemu, with
CC=gcc-7.2, CC=clang-8, and CC=clang-8 HOSTCC=clang-8 and am feeling
quite confident to send it for review.

>> Also, does anyone know who I should talk to about ICC testing?
> No clue

+ Daniel and HPA (the last commits to include/linux/compiler*
mentioning `icc` in 2015 and 2013 respectively)
--
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists