linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nick Desaulniers <ndesaulniers@google.com>
To: joe@perches.com
Cc: asmadeus@codewreck.org,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Kees Cook <keescook@chromium.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Jonathan Corbet <corbet@lwn.net>, Arnd Bergmann <arnd@arndb.de>,
	dwmw@amazon.co.uk, LKML <linux-kernel@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Will Deacon <will.deacon@arm.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	daniel@iogearbox.net, hpa@zytor.com
Subject: Re: [PATCH] compiler-gcc: get back Clang build
Date: Wed, 22 Aug 2018 16:05:36 -0700	[thread overview]
Message-ID: <CAKwvOdmZuzhyaOzh_ruNQUp96zed8+BAz3DCw1hrZrseVNg9tA@mail.gmail.com> (raw)
In-Reply-To: <39b6242771fd80fe19104c8a057bcc6afc0e41e5.camel@perches.com>

On Wed, Aug 22, 2018 at 1:50 PM Joe Perches <joe@perches.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@codewreck.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

  reply	other threads:[~2018-08-22 23:05 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-21  6:48 [PATCH] compiler-gcc: get back Clang build Masahiro Yamada
2018-08-21  8:09 ` Nick Desaulniers
2018-08-21 10:39 ` Joe Perches
2018-08-21 16:35   ` Nick Desaulniers
2018-08-21 17:13   ` Masahiro Yamada
2018-08-21 17:20     ` Joe Perches
2018-08-21 12:38 ` Dominique Martinet
2018-08-21 16:32   ` Nick Desaulniers
2018-08-21 16:45     ` Joe Perches
2018-08-21 17:00       ` Nick Desaulniers
2018-08-22  4:16         ` Dominique Martinet
2018-08-22  4:22           ` Joe Perches
2018-08-22  4:32             ` Dominique Martinet
2018-08-22 18:31               ` Nick Desaulniers
2018-08-22 19:01                 ` Nick Desaulniers
2018-08-22 20:50                 ` Joe Perches
2018-08-22 23:05                   ` Nick Desaulniers [this message]
2018-08-22 23:32                     ` Joe Perches
2018-08-22 23:57                 ` Dominique Martinet
2018-08-21 16:33 ` Joe Perches
2018-08-21 16:57   ` Nick Desaulniers
2018-08-21 17:22     ` Joe Perches
2018-08-21 17:07   ` Masahiro Yamada

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKwvOdmZuzhyaOzh_ruNQUp96zed8+BAz3DCw1hrZrseVNg9tA@mail.gmail.com \
    --to=ndesaulniers@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=asmadeus@codewreck.org \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=dwmw@amazon.co.uk \
    --cc=geert@linux-m68k.org \
    --cc=hpa@zytor.com \
    --cc=joe@perches.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=will.deacon@arm.com \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).