Message-ID: <20180823220834.GA25096@nautica>
Date:   Fri, 24 Aug 2018 00:08:34 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        joe@...ches.com, Masahiro Yamada <yamada.masahiro@...ionext.com>,
        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] include/linux/compiler*.h: make compiler-*.h mutually
 exclusive

Nick Desaulniers wrote on Thu, Aug 23, 2018:
> > On a side note, I noticed tools/include/linux/compiler.h includes
> > linux/compiler-gcc.h but maybe it should include linux/compiler_types.h?
> > (I'm not sure at who uses that header, so it really is an open question
> > here)
> 
> Without looking into the code, this sounds like compiler.h is wrong.
> Looking at the source, there's references to ancient Android NDK's
> (what?! Let me show this to the NDK maintainers).  This whole thing
> needs to be cleaned up, too, IMO.  Oh, there's two compiler-gcc.h in
> the tree:
> 
> - tools/include/linux/compiler-gcc.h (that's what's being included in
> the case you point out)
> - include/linux/compiler-gcc.h
> 
> Maybe they can be combined?

Ah, I didn't notice there is another compiler-gcc...
Looking closer there seem to be other "reused" headers (almost all of
the headers in tools/include/linux have a counterpart in include/linux),
and some e.g. rbtree.h went from being a copy of the main include's to a
single '#include "../../../../include/linux/rbtree.h"' line to a
slightly simplified copy again to avoid bringing in rcu as dependency...

I think compiler.h could be done with a similar trick with a bit of
massaging, but as things stand linux/compiler.h includes
uapi/linux/types.h, and this complains about using kernel headers from
user space... So this isn't as trivial as just making tools use the
kernel's include at least.



> > If you tried to align these, __always_unused and __alias(symbol) look
> > off
> 
> I have `set tabstop=8` in vim, and it looks correct there, but both
> `git diff` and github's code viewer show it as off.  Maybe I have my
> settings wrong in vim and need to go back to first grade, but
> (personal opinion ahead) you don't have this kind of nonsense
> (ambiguity around how many spaces a tab should be displayed as in
> various code viewers) if you just always use spaces everywhere, for
> everything.  Other large codebases use automatic code formatters (like
> clang-format) and that prevents holy wars and other distractions.
> There's even a cool utility called `git-clang-format` that can check
> just the code you changed, which is useful for not reformatting the
> entire codebase messing up git blame.

Right, this is my mistake - the diff view adds a space so these "inner
tabs" alignments can be messed up.
FWIW I think checkpatch only complains about leading space-based
indentation, the inner filling can be whatever you want, but this is
fine and I was overzealous.


> On Wed, Aug 22, 2018 at 7:43 PM Masahiro Yamada
> <yamada.masahiro@...ionext.com> wrote:
> > It was previous included by all compilers,
> > but now it is only by _true_ GCC.
> 
> Good catch, and thanks for the report and testing.  I missed that
> because I was testing x86_64 and arm64 (which I guess don't hit that
> in the configs I tested) and not arm32.  Should be a simple fix to
> move it to shared the definition.  Send me a patch, or I'll get to it.
> 
> > Even if I move the __naked definition
> > to <linux/compiler_types.h>,
> > I see a different error.
> 
> Did that regress due to my change?  If so, sorry and I'll look into it
> more soon.

I've looked at that one quickly and that warning looks legitimate to me,
I'm not sure why it would appear only now but clang does document[1] not
allowing the parameters to be used even in extended asm, and gcc
documentation[2] says "While using extended asm or a mixture of basic
asm and C code may appear to work, they cannot be depended upon to work
reliably and are not supported."

[1] http://infocenter.arm.com/help/topic/com.arm.doc.dui0774g/jhg1476893564298.html
[2] https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html

In this case it looks like the arguments are only used for sanity with
__asmeq but in the first place the original commit for trusted
foundations talks about it not respecting the API so naked makes sense
but they're not making the API function naked (and the one which takes
an argument does use its argument) so this all feels a bit weird to me.

It might be worth asking the original authors on this one...

-- 
Dominique Martinet

Powered by blists - more mailing lists