Compiler Attributes for v4.20-rc1

Hi Linus,

Here it is the Compiler Attributes series/tree, which tries to disentangle the include/linux/compiler*.h headers and bring them up to date.

The patches have been in linux-next for a while, *except* the last two from Nick which came a bit later. Since AFAIU there will be no linux-next this week, I included them here; but let me know if you prefer to take them out.

You may see merge conflicts from a few other trees, from what we have seen in linux-next.

I am not sure if you followed this, so please let me know if there is anything you don't agree with.

And welcome back!

Cheers,
Miguel

The following changes since commit 17b57b1883c1285f3d0dc2266e8f79286a7bef38:

Linux 4.19-rc6 (2018-09-30 07:15:35 -0700)

are available in the Git repository at:

https://github.com/ojeda/linux.git tags/compiler-attributes-for-linus-4.20-rc1

for you to fetch changes up to 1ff2fea5e30ca15752777441ecb64a169fe22e9e:

compiler-gcc: remove comment about gcc 4.5 from unreachable() (2018-10-19 08:47:30 +0200)

----------------------------------------------------------------
The Compiler Attributes series

This is an effort to disentangle the include/linux/compiler*.h headers and bring them up to date.

The main idea behind the series is to use feature checking macros (i.e. __has_attribute) instead of compiler version checks (e.g. GCC_VERSION), which are compiler-agnostic (so they can be shared, reducing the size of compiler-specific headers) and version-agnostic.

Other related improvements have been performed in the headers as well, which on top of the use of __has_attribute it has amounted to a significant simplification of these headers (e.g. GCC_VERSION is now only guarding a few non-attribute macros).

This series should also help the efforts to support compiling the kernel with clang and icc. A fair amount of documentation and comments have also been added, clarified or removed; and the headers are now more readable, which should help kernel developers in general.

The series was triggered due to the move to gcc >= 4.6. In turn, this series has also triggered Sparse to gain the ability to recognize __has_attribute on its own.

Finally, the __nonstring variable attribute series has been also applied on top; plus two related patches from Nick Desaulniers for unreachable() that came a bit afterwards.

----------------------------------------------------------------
Miguel Ojeda (15):
Compiler Attributes: remove unused attributes
Compiler Attributes: always use the extra-underscores syntax Compiler Attributes: remove unneeded tests
Compiler Attributes: homogenize __must_be_array Compiler Attributes: remove unneeded sparse (__CHECKER__) tests Compiler Attributes: add missing SPDX ID in compiler_types.h Compiler Attributes: use feature checks instead of version checks Compiler Attributes: KENTRY used twice the "used" attribute Compiler Attributes: remove uses of __attribute__ from compiler.h Compiler Attributes: add Doc/process/programming-language.rst Compiler Attributes: add MAINTAINERS entry
Compiler Attributes: add support for __nonstring (gcc >= 8) Compiler Attributes: enable -Wstringop-truncation on W=1 (gcc >= 8) Compiler Attributes: auxdisplay: panel: use __nonstring Compiler Attributes: ext4: remove local __nonstring definition

[email protected] (2):
compiler.h: update definition of unreachable()
compiler-gcc: remove comment about gcc 4.5 from unreachable()

 Documentation/process/index.rst                |   1 +
 Documentation/process/programming-language.rst |  45 +++++
 MAINTAINERS                                    |   5 +
 drivers/auxdisplay/panel.c                     |   7 +-
 fs/ext4/ext4.h                                 |   9 -
 include/linux/compiler-clang.h                 |   5 -
 include/linux/compiler-gcc.h                   |  74 +------
 include/linux/compiler-intel.h                 |   9 -
 include/linux/compiler.h                       |  24 +--
 include/linux/compiler_attributes.h            | 258 +++++++++++++++++++++++++
 include/linux/compiler_types.h                 | 101 ++--------
 scripts/Makefile.extrawarn                     |   1 +
 12 files changed, 345 insertions(+), 194 deletions(-)
 create mode 100644 Documentation/process/programming-language.rst
 create mode 100644 include/linux/compiler_attributes.h

I've finally emptied the "normal" pull queues, and am looking at the odd ones that I felt I needed to review more in-depth.

This looked fine to me, and I already pulled, but when looking at the conflict for __no_sanitize_address_or_inline, I also noticed that you actually changed the gcc version logic.

The logic for using __no_sanitize_address *used* to be

    #if GCC_VERSION >= 40902

but you have changed it to

  # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)

so now it's gcc-4.8+ rather than gcc-4.9.2+.

I'm not sure how much that matters (maybe the original check for 4.9.2 was just a random pick by Andrey? Added to cc), but together with the movement to <linux/compiler_attributes.h> that looks like it also wouldn't want the CONFIG_KASAN tests, I wonder what the right merge resolution would be.

Yes, I see the resolution in linux-next, and I think that one is odd and dubious, and now it *mixes* that old test of gcc-4.9.2 with the different test in compiler-attributes.

I'm _inclined_ to just do

#ifdef CONFIG_KASAN
#define __no_sanitize_address_or_inline \
__no_sanitize_address __maybe_unused notrace
#else
#define __no_sanitize_address_or_inline inline
#endif

without any compiler versions at all, because I don't think it matters (maybe we won't get address sanitation, and we will not get inlining either, but do we care?).

But I'm also unsure whether you meant for the "__has_attribute()" test to be usable outside the linux/compiler_attributes.h file, in which case I could just do

  #if defined(CONFIG_KASAN) && __has_attribute(__no_sanitize_address__)

instead.

End result: it's not the merge resolution itself that raises questions, it's just semantics in general.

So I've dropped this for now, in the hope that you/Andrey can answer these questions.

Linus

Good catch. I don't recall any special logic when I did that change, so most likely I simply did like for the rest of the attributes and took a look at when it was first supported (and documentation in gcc's docs) in order to implement __has_attribute by hand.

But indeed, it *may* be that there is an (undocumented) problem between 4.8 <= gcc < 4.9.2 with it. If so, we should document it down and fix it. Andrey?

I missed that conflict completely, my bad (I did not miss all of them, at least; one required fixing).

Hm.... at a quick look, why is it only on compiler-gcc.h? It should either have a corresponding #define elsewhere or just be put directly in another common header, no? (Adding Vasily & Martin to CC.)

I think that (using __has_attribute() outside) may be a good idea: I wanted to keep compiler_attributes.h as simple as possible by avoiding #ifdefs inside that header (except for __has_attribute itself), as an attempt to avoid going back to the mess of #ifdefs we had previously. Basically, keeping the attributes in compiler_attributes.h that do not depend on complex logic. So using __has_attribute *outside* the header actually goes well with that principle, because it helps keeping stuff out of it if they depend on other config options; without having to rely on GCC_VERSION either.

[By the way, in case it clarifies: note that "optional" in that file actually is a bit of a misnomer. I meant to say "optional" as in "not supported by all compilers, so conditionally defined" ("optionally" defined); rather than "optional" in the sense of "code still works without the attribute". It caught Rasmus in one of his patches sent a few days ago on top of this tree, so I want to change it or explain it to avoid confusion.]

Cheers,
Miguel

Ok, looking around, I think this has less to do with the attribute being recognized, and simply just being because KASAN itself wants gcc-4.9.2.

I'm actually not seeing that KASAN dependency in the Kconfig scripts (and it probably _should_ be now that we can just add compiler version dependencies there), but that explains why the gcc version check is different from "gcc supports the attribute".

Anyway, I decided to do the merge by just getting rid of the GCC_VERSION check around __no_sanitize_address_or_inline entirely. If you enable KASAN, then a function with that marking just won't be marked inline.

End result: pulled. I'm as confused as you are as to why __no_sanitize_address_or_inline is in the gcc header, but I guess it ends up being the same issue: KASAN depends on gcc even if that dependency doesn't seem to be spelled out in lib/Kconfig.kasan.

So I _think_ the KASAN config should have a

    depends on CC_IS_GCC && GCC_VERSION >= 40902

on it, but maybe there is something I'm missing.

But from a pull standpoint, I don't want to mess with those (unrelated) issues, so I just kept the merge resolution as simple and straightforward as possible.

Miguel, please do double-check the merge (it's not pushed out yet, I'm doing the usual build tests etc first).

Linus

I was a bit confused when reading the gcc bug reports, i.e. why gcc did *not* complain in 4.9 but it did in 5.1 (when it was supposed to complain also in 4.9). It turns out that gcc 5.1 takes into account who is the actual caller due to this change:

+ cgraph_node *caller = e->caller->global.inlined_to
+ ? e->caller->global.inlined_to : e->caller;
...
- else if (!sanitize_attrs_match_for_inline_p (e->caller->decl, callee->decl))
+ else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl))

change; e.g. this:

#define really_inline inline __attribute__((always_inline)) #define no_sanitize __attribute__((no_sanitize_address))

really_inline void f() {}
really_inline void g() { f(); }
no_sanitize void h() { g(); }

Complains in gcc 4.9 -O0, 5.1 -O0 and 5.1 -O2; but *not* in 4.9 -O2.
https://godbolt.org/z/kNApqk

Anyway, this is orthogonal but in case it clarifies that for someone else...

I was sleeping, didn't manage to see it (in your GitHub, I guess?).

I did the merge myself, and arrived at the same thing as you. I quickly inspected the rest and seems fine. By the way, I spotted an extra space at:

+ * we do one or the other.

Cheers,
Miguel

I'd rather use cc-option instead of version check, since we also support clang.

It should be possible to express compiler requirements for subfeatures like stack/inline instrumentation in Kconfig. But that would be not that trivial, see the scripts/Makefile.kasan

That would be even better, but I thought the requirement for 4.9.2 came not from the option existing, but because of some bugs getting fixed?

But if we can do it without version checks, that would be lovely.

Linus

It looks unusual but -fsanitize=kernel-address really showed up only in 4.9.2. It was actually backported from 5.0 branch to 4.9 for whatever reason.

As you said in other email - "this has less to do with the attribute being recognized, and simply just being because KASAN itself wants gcc-4.9.2."

gcc < 4.9.2 simply doesn't have -fsanitize=kernel-address.
But no_sanitize attribute originally comes with user space ASAN (-fsanitize=address) which exist in earlier GCCs like 4.8.
Checking against 4.8 gcc should be fine. If we compile the kernel with gcc 4.8 it will be compiled without instrumentation, so the attribute won't have any effect.

You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline shouldn't have been added in the first place, because we already have almost the same __no_kasan_or_inline:

#ifdef CONFIG_KASAN
/*
* We can't declare function 'inline' because __no_sanitize_address confilcts
* with inlining. Attempt to inline it may cause a build failure.
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
* '__maybe_unused' allows us to avoid defined-but-not-used warnings. */

# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
#else
# define __no_kasan_or_inline __always_inline
#endif

There are small differences like absence of notrace, but notrace could be added to the function together with __no_kasan_or_inline.
And inline is *not* redefined to __always_inline only on x86 when CONFIG_OPTIMIZE_INLINING=y

So there is absolutely no reason to have both __no_kasan_or_inline and __no_sanitize_address_or_inline.

Ahh, very good.

Vasily, Martin - since __no_sanitize_address_or_inline was added just for s390, would you mind replacing it with __no_kasan_or_inline instead, and testing that in whatever failed before?

Then we can just remove that unnecessary symbol #define entirely..

Thanks,

Linus

Ok, will do.

blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Follow-up question: the __no_sanitize_address_or_inline define has the 'notrace' option that is missing for __no_kasan_or_inline. We need that option for __load_psw_mask, if we do the replacement all users of __no_kasan_or_inline would get the option as well. This affects __read_once_size_nocheck and read_word_at_a_time. Do these function have to be traceable ?

This patch would work for me:
--
From 4aaa09fe4b54e930edabac86606dee979b12647c Mon Sep 17 00:00:00 2001
From: Martin Schwidefsky <[email protected]>
Date: Mon, 5 Nov 2018 07:36:28 +0100
Subject: [PATCH] compiler: remove __no_sanitize_address_or_inline again

The __no_sanitize_address_or_inline and __no_kasan_or_inline defines are almost identical. The only difference is that __no_kasan_or_inline does not have the 'notrace' attribute.

To be able to replace __no_sanitize_address_or_inline with the older definition, add 'notrace' to __no_kasan_or_inline and change to two users of __no_sanitize_address_or_inline in the s390 code.

The 'notrace' option is necessary for e.g. the __load_psw_mask function in arch/s390/include/asm/processor.h. Without the option it is possible to trace __load_psw_mask which leads to kernel stack overflow.

---
 arch/s390/include/asm/processor.h |  4 ++--
 include/linux/compiler-gcc.h      | 12 ------------
 include/linux/compiler.h          |  2 +-
 3 files changed, 3 insertions(+), 15 deletions(-)

index 302795c47c06..81038ab357ce 100644
@@ -236,7 +236,7 @@ static inline unsigned long current_stack_pointer(void)
 	return sp;
 }
 
-static __no_sanitize_address_or_inline unsigned short stap(void)
+static __no_kasan_or_inline unsigned short stap(void)
 {
 	unsigned short cpu_address;
 

@@ -330,7 +330,7 @@ static inline void __load_psw(psw_t psw)
  * Set PSW mask to specified value, while leaving the
  * PSW addr pointing to the next instruction.
  */
-static __no_sanitize_address_or_inline void __load_psw_mask(unsigned long mask)
+static __no_kasan_or_inline void __load_psw_mask(unsigned long mask)
 {
 	unsigned long addr;
 	psw_t psw;

index c0f5db3a9621..2010493e1040 100644
@@ -143,18 +143,6 @@
 #define KASAN_ABI_VERSION 3
 #endif
 
-/*
- * Because __no_sanitize_address conflicts with inlining:
- *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * we do one or the other. 
- */
-#ifdef CONFIG_KASAN
-#define __no_sanitize_address_or_inline					\
-	__no_sanitize_address __maybe_unused notrace
-#else
-#define __no_sanitize_address_or_inline inline
-#endif
-
 #if GCC_VERSION >= 50100
 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
 #endif

index 18c80cfa4fc4..06396c1cf127 100644
@@ -189,7 +189,7 @@ void __read_once_size(const volatile void *p, void *res, int size)
  * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
  * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
  */
-# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
 #else
 # define __no_kasan_or_inline __always_inline
 #endif

2.16.4
-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Some history on why I added notrace to inline. It was to make things more consistent. Since gcc wont add a fentry/mcount call to inlined functions, having those functions show up in the trace depending on whether or not gcc honored the "inline" tag made things confusing. And it even once caused a crash when local_irq_save() stopped being inlined.

I added notrace to inline so that if you mark something as inline, it would not show up in the trace regardless of gcc deciding to inline the function or not.

-- Steve

Thanks, applied,

Linus