[PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Nick Desaulniers]

Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
recently exposed a brittle part of the build for supporting non-gcc
compilers.

Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
__GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
added compiler specific checks for __clang__ or __INTEL_COMPILER.

This is brittle, as they happened to get compatibility by posing as a
certain version of GCC.  This broke when upgrading the minimal version
of GCC required to build the kernel, to a version above what ICC and
Clang claim to be.

Rather than always including compiler-gcc.h then undefining or
redefining macros in compiler-intel.h or compiler-clang.h, let's
separate out the compiler specific macro definitions into mutually
exclusive headers, do more proper compiler detection, and keep shared
definitions in compiler_types.h.

Reported-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Suggested-by: Eli Friedman <efriedma@codeaurora.org>
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/arm/kernel/asm-offsets.c     |  13 +-
 drivers/gpu/drm/i915/i915_utils.h |   2 +-
 drivers/watchdog/kempld_wdt.c     |   5 -
 include/linux/compiler-clang.h    |  20 ++-
 include/linux/compiler-gcc.h      |  88 -----------
 include/linux/compiler-intel.h    |  13 +-
 include/linux/compiler_types.h    | 238 ++++++++++++++----------------
 mm/ksm.c                          |   4 +-
 mm/migrate.c                      |   3 +-
 9 files changed, 133 insertions(+), 253 deletions(-)

diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 974d8d7d1bcd..3968d6c22455 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -38,25 +38,14 @@
 #error Sorry, your compiler targets APCS-26 but this kernel requires APCS-32
 #endif
 /*
- * GCC 3.0, 3.1: general bad code generation.
- * GCC 3.2.0: incorrect function argument offset calculation.
- * GCC 3.2.x: miscompiles NEW_AUX_ENT in fs/binfmt_elf.c
- *            (http://gcc.gnu.org/PR8896) and incorrect structure
- *	      initialisation in fs/jffs2/erase.c
  * GCC 4.8.0-4.8.2: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58854
  *	      miscompiles find_get_entry(), and can result in EXT3 and EXT4
  *	      filesystem corruption (possibly other FS too).
  */
-#ifdef __GNUC__
-#if (__GNUC__ == 3 && __GNUC_MINOR__ < 3)
-#error Your compiler is too buggy; it is known to miscompile kernels.
-#error    Known good compilers: 3.3, 4.x
-#endif
-#if GCC_VERSION >= 40800 && GCC_VERSION < 40803
+#if defined(GCC_VERSION) && GCC_VERSION >= 40800 && GCC_VERSION < 40803
 #error Your compiler is too buggy; it is known to miscompile kernels
 #error and result in filesystem corruption and oopses.
 #endif
-#endif
 
 int main(void)
 {
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 00165ad55fb3..395dd2511568 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -43,7 +43,7 @@
 #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
 			     __stringify(x), (long)(x))
 
-#if GCC_VERSION >= 70000
+#if defined(GCC_VERSION) && GCC_VERSION >= 70000
 #define add_overflows(A, B) \
 	__builtin_add_overflow_p((A), (B), (typeof((A) + (B)))0)
 #else
diff --git a/drivers/watchdog/kempld_wdt.c b/drivers/watchdog/kempld_wdt.c
index 2f3b049ea301..e268add43010 100644
--- a/drivers/watchdog/kempld_wdt.c
+++ b/drivers/watchdog/kempld_wdt.c
@@ -146,12 +146,7 @@ static int kempld_wdt_set_stage_timeout(struct kempld_wdt_data *wdt_data,
 	u32 remainder;
 	u8 stage_cfg;
 
-#if GCC_VERSION < 40400
-	/* work around a bug compiling do_div() */
-	prescaler = READ_ONCE(kempld_prescaler[PRESCALER_21]);
-#else
 	prescaler = kempld_prescaler[PRESCALER_21];
-#endif
 
 	if (!stage)
 		return -EINVAL;
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 7087446c24c8..5f0754692ce6 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -6,11 +6,7 @@
 /* Some compiler specific definitions are overwritten here
  * for Clang compiler
  */
-
-#ifdef uninitialized_var
-#undef uninitialized_var
 #define uninitialized_var(x) x = *(&(x))
-#endif
 
 /* same as gcc, this was present in clang-2.6 so we can assume it works
  * with any version that can compile the kernel
@@ -25,14 +21,8 @@
 #define __SANITIZE_ADDRESS__
 #endif
 
-#undef __no_sanitize_address
 #define __no_sanitize_address __attribute__((no_sanitize("address")))
 
-/* Clang doesn't have a way to turn it off per-function, yet. */
-#ifdef __noretpoline
-#undef __noretpoline
-#endif
-
 /*
  * Not all versions of clang implement the the type-generic versions
  * of the builtin overflow checkers. Fortunately, clang implements
@@ -40,9 +30,17 @@
  * checks. Unfortunately, we don't know which version of gcc clang
  * pretends to be, so the macro may or may not be defined.
  */
-#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
 #if __has_builtin(__builtin_mul_overflow) && \
     __has_builtin(__builtin_add_overflow) && \
     __has_builtin(__builtin_sub_overflow)
 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
 #endif
+
+/* The following are for compatibility with GCC, from compiler-gcc.h,
+ * and may be redefined here because they should not be shared with other
+ * compilers, like ICC.
+ */
+#define barrier() (__asm__ __volatile__("" : : : "memory"))
+#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
+#define __assume_aligned(a, ...)	\
+	__attribute__((__assume_aligned__(a, ## __VA_ARGS__)))
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 250b9b7cfd60..763bbad1e258 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -75,48 +75,6 @@
 #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
 #endif
 
-/*
- * Feature detection for gnu_inline (gnu89 extern inline semantics). Either
- * __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics,
- * and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not
- * defined so the gnu89 semantics are the default.
- */
-#ifdef __GNUC_STDC_INLINE__
-# define __gnu_inline	__attribute__((gnu_inline))
-#else
-# define __gnu_inline
-#endif
-
-/*
- * Force always-inline if the user requests it so via the .config,
- * or if gcc is too old.
- * GCC does not warn about unused static inline functions for
- * -Wunused-function.  This turns out to avoid the need for complex #ifdef
- * directives.  Suppress the warning in clang as well by using "unused"
- * function attribute, which is redundant but not harmful for gcc.
- * Prefer gnu_inline, so that extern inline functions do not emit an
- * externally visible function. This makes extern inline behave as per gnu89
- * semantics rather than c99. This prevents multiple symbol definition errors
- * of extern inline functions at link time.
- * A lot of inline functions can cause havoc with function tracing.
- */
-#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
-    !defined(CONFIG_OPTIMIZE_INLINING)
-#define inline \
-	inline __attribute__((always_inline, unused)) notrace __gnu_inline
-#else
-#define inline inline		__attribute__((unused)) notrace __gnu_inline
-#endif
-
-#define __inline__ inline
-#define __inline inline
-#define __always_inline	inline __attribute__((always_inline))
-#define  noinline	__attribute__((noinline))
-
-#define __packed	__attribute__((packed))
-#define __weak		__attribute__((weak))
-#define __alias(symbol)	__attribute__((alias(#symbol)))
-
 #ifdef RETPOLINE
 #define __noretpoline __attribute__((indirect_branch("keep")))
 #endif
@@ -135,55 +93,9 @@
  */
 #define __naked		__attribute__((naked)) noinline __noclone notrace
 
-#define __noreturn	__attribute__((noreturn))
-
-/*
- * From the GCC manual:
- *
- * Many functions have no effects except the return value and their
- * return value depends only on the parameters and/or global
- * variables.  Such a function can be subject to common subexpression
- * elimination and loop optimization just as an arithmetic operator
- * would be.
- * [...]
- */
-#define __pure			__attribute__((pure))
-#define __aligned(x)		__attribute__((aligned(x)))
-#define __aligned_largest	__attribute__((aligned))
-#define __printf(a, b)		__attribute__((format(printf, a, b)))
-#define __scanf(a, b)		__attribute__((format(scanf, a, b)))
-#define __attribute_const__	__attribute__((__const__))
-#define __maybe_unused		__attribute__((unused))
-#define __always_unused		__attribute__((unused))
-#define __mode(x)               __attribute__((mode(x)))
-
-#define __must_check		__attribute__((warn_unused_result))
-#define __malloc		__attribute__((__malloc__))
-
-#define __used			__attribute__((__used__))
-#define __compiler_offsetof(a, b)					\
-	__builtin_offsetof(a, b)
-
-/* Mark functions as cold. gcc will assume any path leading to a call
- * to them will be unlikely.  This means a lot of manual unlikely()s
- * are unnecessary now for any paths leading to the usual suspects
- * like BUG(), printk(), panic() etc. [but let's keep them for now for
- * older compilers]
- *
- * Early snapshots of gcc 4.3 don't support this and we can't detect this
- * in the preprocessor, but we can live with this because they're unreleased.
- * Maketime probing would be overkill here.
- *
- * gcc also has a __attribute__((__hot__)) to move hot functions into
- * a special section, but I don't see any sense in this right now in
- * the kernel context
- */
-#define __cold			__attribute__((__cold__))
-
 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
 
 #define __optimize(level)	__attribute__((__optimize__(level)))
-#define __nostackprotector	__optimize("no-stack-protector")
 
 #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 
diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
index 547cdc920a3c..4c7f9befa9f6 100644
--- a/include/linux/compiler-intel.h
+++ b/include/linux/compiler-intel.h
@@ -14,10 +14,6 @@
 /* Intel ECC compiler doesn't support gcc specific asm stmts.
  * It uses intrinsics to do the equivalent things.
  */
-#undef barrier
-#undef barrier_data
-#undef RELOC_HIDE
-#undef OPTIMIZER_HIDE_VAR
 
 #define barrier() __memory_barrier()
 #define barrier_data(ptr) barrier()
@@ -38,13 +34,12 @@
 
 #endif
 
-#ifndef __HAVE_BUILTIN_BSWAP16__
 /* icc has this, but it's called _bswap16 */
 #define __HAVE_BUILTIN_BSWAP16__
 #define __builtin_bswap16 _bswap16
-#endif
 
-/*
- * icc defines __GNUC__, but does not implement the builtin overflow checkers.
+/* The following are for compatibility with GCC, from compiler-gcc.h,
+ * and may be redefined here because they should not be shared with other
+ * compilers, like clang.
  */
-#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
+#define __visible	__attribute__((externally_visible))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index fbf337933fd8..90479a0f3986 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -54,32 +54,20 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #ifdef __KERNEL__
 
-#ifdef __GNUC__
-#include <linux/compiler-gcc.h>
-#endif
-
-#if defined(CC_USING_HOTPATCH) && !defined(__CHECKER__)
-#define notrace __attribute__((hotpatch(0,0)))
-#else
-#define notrace __attribute__((no_instrument_function))
-#endif
-
-/* Intel compiler defines __GNUC__. So we will overwrite implementations
- * coming from above header files here
- */
-#ifdef __INTEL_COMPILER
-# include <linux/compiler-intel.h>
-#endif
-
-/* Clang compiler defines __GNUC__. So we will overwrite implementations
- * coming from above header files here
- */
+/* Compiler specific macros. */
 #ifdef __clang__
 #include <linux/compiler-clang.h>
+#elif defined(__INTEL_COMPILER)
+#include <linux/compiler-intel.h>
+#elif defined(__GNUC__)
+/* The above compilers also define __GNUC__, so order is important here. */
+#include <linux/compiler-gcc.h>
+#else
+#error "Unknown compiler"
 #endif
 
 /*
- * Generic compiler-dependent macros required for kernel
+ * Generic compiler-independent macros required for kernel
  * build go below this comment. Actual compiler/compiler version
  * specific implementations come from the above header files
  */
@@ -106,93 +94,19 @@ struct ftrace_likely_data {
 	unsigned long			constant;
 };
 
-#endif /* __KERNEL__ */
-
-#endif /* __ASSEMBLY__ */
-
-#ifdef __KERNEL__
-
 /* Don't. Just don't. */
 #define __deprecated
 #define __deprecated_for_modules
 
-#ifndef __must_check
-#define __must_check
-#endif
-
-#ifndef CONFIG_ENABLE_MUST_CHECK
-#undef __must_check
-#define __must_check
-#endif
-
-#ifndef __malloc
-#define __malloc
-#endif
-
-/*
- * Allow us to avoid 'defined but not used' warnings on functions and data,
- * as well as force them to be emitted to the assembly file.
- *
- * As of gcc 3.4, static functions that are not marked with attribute((used))
- * may be elided from the assembly file.  As of gcc 3.4, static data not so
- * marked will not be elided, but this may change in a future gcc version.
- *
- * NOTE: Because distributions shipped with a backported unit-at-a-time
- * compiler in gcc 3.3, we must define __used to be __attribute__((used))
- * for gcc >=3.3 instead of 3.4.
- *
- * In prior versions of gcc, such functions and data would be emitted, but
- * would be warned about except with attribute((unused)).
- *
- * Mark functions that are referenced only in inline assembly as __used so
- * the code is emitted even though it appears to be unreferenced.
- */
-#ifndef __used
-# define __used			/* unimplemented */
-#endif
-
-#ifndef __maybe_unused
-# define __maybe_unused		/* unimplemented */
-#endif
-
-#ifndef __always_unused
-# define __always_unused	/* unimplemented */
-#endif
-
-#ifndef noinline
-#define noinline
-#endif
-
-/*
- * Rather then using noinline to prevent stack consumption, use
- * noinline_for_stack instead.  For documentation reasons.
- */
-#define noinline_for_stack noinline
-
-#ifndef __always_inline
-#define __always_inline inline
-#endif
-
 #endif /* __KERNEL__ */
 
+#endif /* __ASSEMBLY__ */
+
 /*
- * From the GCC manual:
- *
- * Many functions do not examine any values except their arguments,
- * and have no effects except the return value.  Basically this is
- * just slightly more strict class than the `pure' attribute above,
- * since function is not allowed to read global memory.
- *
- * Note that a function that has pointer arguments and examines the
- * data pointed to must _not_ be declared `const'.  Likewise, a
- * function that calls a non-`const' function usually must not be
- * `const'.  It does not make sense for a `const' function to return
- * `void'.
+ * The below symbols may be defined for one or more, but not ALL, of the above
+ * compilers. We don't consider that to be an error, so set them to nothing.
+ * For example, some of them are for compiler specific plugins.
  */
-#ifndef __attribute_const__
-# define __attribute_const__	/* unimplemented */
-#endif
-
 #ifndef __designated_init
 # define __designated_init
 #endif
@@ -214,28 +128,10 @@ struct ftrace_likely_data {
 # define randomized_struct_fields_end
 #endif
 
-/*
- * Tell gcc if a function is cold. The compiler will assume any path
- * directly leading to the call is unlikely.
- */
-
-#ifndef __cold
-#define __cold
-#endif
-
-/* Simple shorthand for a section definition */
-#ifndef __section
-# define __section(S) __attribute__ ((__section__(#S)))
-#endif
-
 #ifndef __visible
 #define __visible
 #endif
 
-#ifndef __nostackprotector
-# define __nostackprotector
-#endif
-
 /*
  * Assume alignment of return value.
  */
@@ -243,17 +139,23 @@ struct ftrace_likely_data {
 #define __assume_aligned(a, ...)
 #endif
 
-
 /* Are two types/vars the same type (ignoring qualifiers)? */
-#ifndef __same_type
-# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
-#endif
+#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
 /* Is this type a native word size -- useful for atomic operations */
-#ifndef __native_word
-# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
+#define __native_word(t) \
+	(sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
+	 sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
+
+#ifndef __attribute_const__
+#define __attribute_const__	__attribute__((__const__))
 #endif
 
+#ifndef __noclone
+#define __noclone
+#endif
+
+/* Helpers for emitting diagnostics in pragmas. */
 #ifndef __diag
 #define __diag(string)
 #endif
@@ -272,4 +174,92 @@ struct ftrace_likely_data {
 #define __diag_error(compiler, version, option, comment) \
 	__diag_ ## compiler(version, error, option)
 
+/*
+ * From the GCC manual:
+ *
+ * Many functions have no effects except the return value and their
+ * return value depends only on the parameters and/or global
+ * variables.  Such a function can be subject to common subexpression
+ * elimination and loop optimization just as an arithmetic operator
+ * would be.
+ * [...]
+ */
+#define __pure			__attribute__((pure))
+#define __aligned(x)		__attribute__((aligned(x)))
+#define __aligned_largest	__attribute__((aligned))
+#define __printf(a, b)		__attribute__((format(printf, a, b)))
+#define __scanf(a, b)		__attribute__((format(scanf, a, b)))
+#define __maybe_unused		__attribute__((unused))
+#define __always_unused		__attribute__((unused))
+#define __mode(x)		__attribute__((mode(x)))
+#define __malloc		__attribute__((__malloc__))
+#define __used			__attribute__((__used__))
+#define __noreturn		__attribute__((noreturn))
+#define __packed		__attribute__((packed))
+#define __weak			__attribute__((weak))
+#define __alias(symbol)		__attribute__((alias(#symbol)))
+#define __cold			__attribute__((cold))
+#define __section(S)		__attribute__((__section__(#S)))
+
+
+#ifdef CONFIG_ENABLE_MUST_CHECK
+#define __must_check		__attribute__((warn_unused_result))
+#else
+#define __must_check
+#endif
+
+#if defined(CC_USING_HOTPATCH) && !defined(__CHECKER__)
+#define notrace			__attribute__((hotpatch(0, 0)))
+#else
+#define notrace			__attribute__((no_instrument_function))
+#endif
+
+#define __compiler_offsetof(a, b)	__builtin_offsetof(a, b)
+
+/*
+ * Feature detection for gnu_inline (gnu89 extern inline semantics). Either
+ * __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics,
+ * and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not
+ * defined so the gnu89 semantics are the default.
+ */
+#ifdef __GNUC_STDC_INLINE__
+# define __gnu_inline	__attribute__((gnu_inline))
+#else
+# define __gnu_inline
+#endif
+
+/*
+ * Force always-inline if the user requests it so via the .config.
+ * GCC does not warn about unused static inline functions for
+ * -Wunused-function.  This turns out to avoid the need for complex #ifdef
+ * directives.  Suppress the warning in clang as well by using "unused"
+ * function attribute, which is redundant but not harmful for gcc.
+ * Prefer gnu_inline, so that extern inline functions do not emit an
+ * externally visible function. This makes extern inline behave as per gnu89
+ * semantics rather than c99. This prevents multiple symbol definition errors
+ * of extern inline functions at link time.
+ * A lot of inline functions can cause havoc with function tracing.
+ */
+#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
+	!defined(CONFIG_OPTIMIZE_INLINING)
+#define inline \
+	inline __attribute__((always_inline, unused)) notrace __gnu_inline
+#else
+#define inline inline	__attribute__((unused)) notrace __gnu_inline
+#endif
+
+#define __inline__ inline
+#define __inline inline
+#define noinline	__attribute__((noinline))
+
+#ifndef __always_inline
+#define __always_inline inline __attribute__((always_inline))
+#endif
+
+/*
+ * Rather then using noinline to prevent stack consumption, use
+ * noinline_for_stack instead.  For documentation reasons.
+ */
+#define noinline_for_stack noinline
+
 #endif /* __LINUX_COMPILER_TYPES_H */
diff --git a/mm/ksm.c b/mm/ksm.c
index 1bd514c9e5d0..5b0894b45ee5 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -652,9 +652,9 @@ static void remove_node_from_stable_tree(struct stable_node *stable_node)
 	 * list_head to stay clear from the rb_parent_color union
 	 * (aligned and different than any node) and also different
 	 * from &migrate_nodes. This will verify that future list.h changes
-	 * don't break STABLE_NODE_DUP_HEAD.
+	 * don't break STABLE_NODE_DUP_HEAD. Only recent gcc can handle it.
 	 */
-#if GCC_VERSION >= 40903 /* only recent gcc can handle it */
+#if defined(GCC_VERSION) && GCC_VERSION >= 40903
 	BUILD_BUG_ON(STABLE_NODE_DUP_HEAD <= &migrate_nodes);
 	BUILD_BUG_ON(STABLE_NODE_DUP_HEAD >= &migrate_nodes + 1);
 #endif
diff --git a/mm/migrate.c b/mm/migrate.c
index 4a83268e23c2..c27e97b5b69d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1131,7 +1131,8 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
  * gcc 4.7 and 4.8 on arm get an ICEs when inlining unmap_and_move().  Work
  * around it.
  */
-#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
+#if defined(CONFIG_ARM) && \
+	defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700
 #define ICE_noinline noinline
 #else
 #define ICE_noinline
-- 
2.18.0.1017.ga543ac7ca45-goog

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Joe Perches]

On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.

style trivia:

> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
[]
> @@ -54,32 +54,20 @@ extern void __chk_io_ptr(const volatile void __iomem *);
[]
> +/* Compiler specific macros. */
>  #ifdef __clang__
>  #include <linux/compiler-clang.h>

probably better as

#if defined(__clang)

to match the style of the #elif defined()s below it

> +#elif defined(__INTEL_COMPILER)
> +#include <linux/compiler-intel.h>
> +#elif defined(__GNUC__)
[]
> @@ -272,4 +174,92 @@ struct ftrace_likely_data {
[]
> +#ifdef __GNUC_STDC_INLINE__
> +# define __gnu_inline	__attribute__((gnu_inline))
> +#else
> +# define __gnu_inline
> +#endif

Perhaps __gnu_inline should be in compiler-gcc and this
should use

#ifndef __gnu_inline
#define __gnu_inline
#endif
 
> +
> +/*
> + * Force always-inline if the user requests it so via the .config.
> + * GCC does not warn about unused static inline functions for
> + * -Wunused-function.  This turns out to avoid the need for complex #ifdef
> + * directives.  Suppress the warning in clang as well by using "unused"
> + * function attribute, which is redundant but not harmful for gcc.
> + * Prefer gnu_inline, so that extern inline functions do not emit an
> + * externally visible function. This makes extern inline behave as per gnu89
> + * semantics rather than c99. This prevents multiple symbol definition errors
> + * of extern inline functions at link time.
> + * A lot of inline functions can cause havoc with function tracing.
> + */
> +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> +	!defined(CONFIG_OPTIMIZE_INLINING)
> +#define inline \
> +	inline __attribute__((always_inline, unused)) notrace __gnu_inline
> +#else
> +#define inline inline	__attribute__((unused)) notrace __gnu_inline
> +#endif

This bit might be better adding another __<foo> attribute like:

#if defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) &&
    defined(CONFIG_OPTIMIZE_INLINING)
#define __optimized_inline __unused
#else
#define __optimized_inline __unused __attribute__((always_inline))
#endif

#define inline inline __optimized_inline notrace __gnu_inline

> +
> +#define __inline__ inline
> +#define __inline inline
> +#define noinline	__attribute__((noinline))
> +
> +#ifndef __always_inline
> +#define __always_inline inline __attribute__((always_inline))
> +#endif

[]

> diff --git a/mm/migrate.c b/mm/migrate.c
[]
> @@ -1131,7 +1131,8 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>   * gcc 4.7 and 4.8 on arm get an ICEs when inlining unmap_and_move().  Work
>   * around it.
>   */
> -#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
> +#if defined(CONFIG_ARM) && \
> +	defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700

I find the reversed version tests a bit odd to read

>  #define ICE_noinline noinline
>  #else
>  #define ICE_noinline

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Linus Torvalds]

On Wed, Aug 22, 2018 at 4:39 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.

Ack, especially with the other cleanups leading to

>  9 files changed, 133 insertions(+), 253 deletions(-)

and I just took it directly, since I had taken the original
cafa0010cd51 directly too.

I'm sure this can still be tweaked, but let's get the baseline in and working.

               Linus

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Dominique Martinet]

Nick Desaulniers wrote on Wed, Aug 22, 2018:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
> 
> Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
> __GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
> added compiler specific checks for __clang__ or __INTEL_COMPILER.
> 
> This is brittle, as they happened to get compatibility by posing as a
> certain version of GCC.  This broke when upgrading the minimal version
> of GCC required to build the kernel, to a version above what ICC and
> Clang claim to be.
> 
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.
> 
> Reported-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Suggested-by: Eli Friedman <efriedma@codeaurora.org>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Overall looks good to me, just pointing at the same error I wrote in my
other mail here -- I saw that by the time I was done writing this this
patch got taken but that alone will probably warrant a follow-up :/


I've also added a few style nitpicks/questions but feel free to ignore
these.

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)

> ---
>  arch/arm/kernel/asm-offsets.c     |  13 +-
>  drivers/gpu/drm/i915/i915_utils.h |   2 +-
>  drivers/watchdog/kempld_wdt.c     |   5 -
>  include/linux/compiler-clang.h    |  20 ++-
>  include/linux/compiler-gcc.h      |  88 -----------
>  include/linux/compiler-intel.h    |  13 +-
>  include/linux/compiler_types.h    | 238 ++++++++++++++----------------
>  mm/ksm.c                          |   4 +-
>  mm/migrate.c                      |   3 +-
>  9 files changed, 133 insertions(+), 253 deletions(-)
> 
> [...]
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 7087446c24c8..5f0754692ce6 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> [...]
> @@ -40,9 +30,17 @@
>   * checks. Unfortunately, we don't know which version of gcc clang
>   * pretends to be, so the macro may or may not be defined.
>   */
> -#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
>  #if __has_builtin(__builtin_mul_overflow) && \
>      __has_builtin(__builtin_add_overflow) && \
>      __has_builtin(__builtin_sub_overflow)
>  #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
>  #endif
> +
> +/* The following are for compatibility with GCC, from compiler-gcc.h,
> + * and may be redefined here because they should not be shared with other
> + * compilers, like ICC.
> + */
> +#define barrier() (__asm__ __volatile__("" : : : "memory"))

barrier here has gained external () compared to the definition and
compiler-gcc.h:
#define barrier() __asm__ __volatile__("": : :"memory")

And this fails with bpf:
In file included from <built-in>:3:
In file included from /virtual/include/bcc/helpers.h:23:
In file included from /lib/modules/4.18.0+/build/include/linux/log2.h:16:
In file included from /lib/modules/4.18.0+/build/include/linux/bitops.h:18:
/lib/modules/4.18.0+/build/arch/x86/include/asm/bitops.h:170:2: error: expected expression
        barrier();
        ^
/lib/modules/4.18.0+/build/include/linux/compiler-clang.h:43:20: note: expanded from macro 'barrier'
#define barrier() (__asm__ __volatile__("" : : : "memory"))
                   ^


> +#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> +#define __assume_aligned(a, ...)	\
> +	__attribute__((__assume_aligned__(a, ## __VA_ARGS__)))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 250b9b7cfd60..763bbad1e258 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> [..]
> -#define __cold			__attribute__((__cold__))
> -
>  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
>  
>  #define __optimize(level)	__attribute__((__optimize__(level)))
> -#define __nostackprotector	__optimize("no-stack-protector")

I do not see this being added back, it's probably fine though as it
doesn't look used?
(I didn't check all removed lines, maybe about half)
  
>  #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index fbf337933fd8..90479a0f3986 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> [...]
>  /* Is this type a native word size -- useful for atomic operations */
> -#ifndef __native_word
> -# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
> +#define __native_word(t) \
> +	(sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
> +	 sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
> +
> +#ifndef __attribute_const__

I'm not sure I understand the logic of where you removed ifndef and
where you kept them (but, well, this should work)

> +#define __attribute_const__	__attribute__((__const__))
>  #endif
>  
> +#ifndef __noclone
> +#define __noclone
> +#endif
> +
> +/* Helpers for emitting diagnostics in pragmas. */
>  #ifndef __diag
>  #define __diag(string)
>  #endif
> @@ -272,4 +174,92 @@ struct ftrace_likely_data {
>  #define __diag_error(compiler, version, option, comment) \
>  	__diag_ ## compiler(version, error, option)
>  
> +/*
> + * From the GCC manual:
> + *
> + * Many functions have no effects except the return value and their
> + * return value depends only on the parameters and/or global
> + * variables.  Such a function can be subject to common subexpression
> + * elimination and loop optimization just as an arithmetic operator
> + * would be.
> + * [...]
> + */
> +#define __pure			__attribute__((pure))
> +#define __aligned(x)		__attribute__((aligned(x)))
> +#define __aligned_largest	__attribute__((aligned))
> +#define __printf(a, b)		__attribute__((format(printf, a, b)))
> +#define __scanf(a, b)		__attribute__((format(scanf, a, b)))
> +#define __maybe_unused		__attribute__((unused))
> +#define __always_unused		__attribute__((unused))
> +#define __mode(x)		__attribute__((mode(x)))
> +#define __malloc		__attribute__((__malloc__))
> +#define __used			__attribute__((__used__))
> +#define __noreturn		__attribute__((noreturn))
> +#define __packed		__attribute__((packed))
> +#define __weak			__attribute__((weak))
> +#define __alias(symbol)		__attribute__((alias(#symbol)))
> +#define __cold			__attribute__((cold))
> +#define __section(S)		__attribute__((__section__(#S)))

If you tried to align these, __always_unused and __alias(symbol) look
off


Thanks!
-- 
Dominique

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Linus Torvalds]

On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Overall looks good to me, just pointing at the same error I wrote in my
> other mail here -- I saw that by the time I was done writing this this
> patch got taken but that alone will probably warrant a follow-up :/

I've fixed that manually, but when I tried to test it I just hit the

  arch/x86/Makefile:179: *** Compiler lacks asm-goto support..  Stop.

error.

Do you have some experimental clang build with asm goto support? What
version? Or is it just that you're building ARM, not x86?

             Linus

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Dominique Martinet]

Linus Torvalds wrote on Wed, Aug 22, 2018:
> I've fixed that manually, but when I tried to test it I just hit the
> 
>   arch/x86/Makefile:179: *** Compiler lacks asm-goto support..  Stop.
> 
> error.
> 
> Do you have some experimental clang build with asm goto support? What
> version? Or is it just that you're building ARM, not x86?

I'm not building linux directly, but BPF programs that indirectly uses
clang with bcc

On fedora you can install bcc-tools and run
e.g. /usr/share/bcc/tools/execsnoop that will compile something at
runtime.
If the package isn't available for your distro you can just install bcc
and run the script from their repo[1]

[1] https://raw.githubusercontent.com/iovisor/bcc/master/tools/execsnoop.py


Thanks,
-- 
Dominique

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Linus Torvalds]

On Wed, Aug 22, 2018 at 6:10 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> I'm not building linux directly, but BPF programs that indirectly uses
> clang with bcc

Oh, ok.

I _can_ test the basic build (just not get a working link and a
kernel) by just forcing it, so I guess I'll call that testing enough.

I'll push it out. It's just Nick's patch, but with the extra barrier
parenthesis removed.

          Linus

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Masahiro Yamada]

Hi Nick

2018-08-23 8:37 GMT+09:00 Nick Desaulniers <ndesaulniers@google.com>:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
>
> Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
> __GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
> added compiler specific checks for __clang__ or __INTEL_COMPILER.
>
> This is brittle, as they happened to get compatibility by posing as a
> certain version of GCC.  This broke when upgrading the minimal version
> of GCC required to build the kernel, to a version above what ICC and
> Clang claim to be.
>
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.
>
> Reported-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Suggested-by: Eli Friedman <efriedma@codeaurora.org>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  arch/arm/kernel/asm-offsets.c     |  13 +-
>  drivers/gpu/drm/i915/i915_utils.h |   2 +-
>  drivers/watchdog/kempld_wdt.c     |   5 -
>  include/linux/compiler-clang.h    |  20 ++-
>  include/linux/compiler-gcc.h      |  88 -----------
>  include/linux/compiler-intel.h    |  13 +-
>  include/linux/compiler_types.h    | 238 ++++++++++++++----------------
>  mm/ksm.c                          |   4 +-
>  mm/migrate.c                      |   3 +-
>  9 files changed, 133 insertions(+), 253 deletions(-)



If I build ARM linux with Clang,
I see the following error.



arch/arm/firmware/trusted_foundations.c:34:13: error: variable has
incomplete type 'void'
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
            ^
arch/arm/firmware/trusted_foundations.c:34:20: error: expected ';'
after top level declarator
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
                   ^
                   ;
arch/arm/firmware/trusted_foundations.c:77:25: error: use of
undeclared identifier 'trusted_foundations_ops'
        register_firmware_ops(&trusted_foundations_ops);
                               ^
3 errors generated.
scripts/Makefile.build:307: recipe for target
'arch/arm/firmware/trusted_foundations.o' failed
make[2]: *** [arch/arm/firmware/trusted_foundations.o] Error 1
Makefile:1057: recipe for target 'arch/arm/firmware' failed
make[1]: *** [arch/arm/firmware] Error 2
make[1]: *** Waiting for unfinished jobs....





__naked is defined in <linux/compiler-gcc.h>


It was previous included by all compilers,
but now it is only by _true_ GCC.


Even if I move the __naked definition
to <linux/compiler_types.h>,
I see a different error.



arch/arm/firmware/trusted_foundations.c:47:10: error: parameter
references not allowed in naked functions
                : "r" (type), "r" (arg1), "r" (arg2)
                       ^
arch/arm/firmware/trusted_foundations.c:34:13: note: attribute is here
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
            ^
./include/linux/compiler_types.h:277:33: note: expanded from macro '__naked'
#define __naked         __attribute__((naked))
                                       ^



I do not know what a correct fix is.







-- 
Best Regards
Masahiro Yamada

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Kees Cook]

On Wed, Aug 22, 2018 at 6:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet
> <asmadeus@codewreck.org> wrote:
>>
>> Overall looks good to me, just pointing at the same error I wrote in my
>> other mail here -- I saw that by the time I was done writing this this
>> patch got taken but that alone will probably warrant a follow-up :/
>
> I've fixed that manually, but when I tried to test it I just hit the
>
>   arch/x86/Makefile:179: *** Compiler lacks asm-goto support..  Stop.
>
> error.
>
> Do you have some experimental clang build with asm goto support? What
> version? Or is it just that you're building ARM, not x86?

FWIW, when I do Clang test builds lately[1], I've been using:

make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CC=clang

(Note this requires a cross-compiled binutils installed in the PATH
with as aarch64-linux-gnu-*, which is where Debian and Ubuntu put
them.)

-Kees

[1] for today's defconfig build to finish, I had to:

./scripts/config -d CONFIG_ARM64_LSE_ATOMICS
(I think my binutils are old)

and comment out the BUILD_BUG_ON() in net/core/filter.c from 2dbb9b9e6df67
(which looks like it needs some attention since gcc has no problem with this)

-- 
Kees Cook
Pixel Security

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Nick Desaulniers]

One reply for a bunch of the various threads, to keep the number of emails down:

On Wed, Aug 22, 2018 at 5:20 PM Joe Perches <joe@perches.com> wrote:
> On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > +/* Compiler specific macros. */
> >  #ifdef __clang__
> >  #include <linux/compiler-clang.h>
>
> probably better as
>
> #if defined(__clang)
>
> to match the style of the #elif defined()s below it

Hi Joe,
Thanks for the feedback. I always appreciate it.  If you have some
cleanups, want to send them to me, and I'll bundle them up for a PR?
I'm ok with that change.

> > +#ifdef __GNUC_STDC_INLINE__
> > +# define __gnu_inline        __attribute__((gnu_inline))
> > +#else
> > +# define __gnu_inline
> > +#endif
>
> Perhaps __gnu_inline should be in compiler-gcc and this
> should use
>
> #ifndef __gnu_inline
> #define __gnu_inline
> #endif

Not this case; it's how we get gnu89 semantics for `extern inline` is
not compiler specific (therefor should not go in a compiler specific
header).

> > +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> > +     !defined(CONFIG_OPTIMIZE_INLINING)
> > +#define inline \
> > +     inline __attribute__((always_inline, unused)) notrace __gnu_inline
> > +#else
> > +#define inline inline        __attribute__((unused)) notrace __gnu_inline
> > +#endif
>
> This bit might be better adding another __<foo> attribute like:
>
> #if defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) &&
>     defined(CONFIG_OPTIMIZE_INLINING)
> #define __optimized_inline __unused
> #else
> #define __optimized_inline __unused __attribute__((always_inline))
> #endif
>
> #define inline inline __optimized_inline notrace __gnu_inline

Sure, as above I'm happy to take clean ups, but that might result in
treewide changes (maybe less benefit but could separate `inline` from
`__optimized_inline`).  Maybe bikeshed territory, but `force_inline`
or some notion that we're trying to overrule the compiler here might
make intent clearer.

> > -#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
> > +#if defined(CONFIG_ARM) && \
> > +     defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700
>
> I find the reversed version tests a bit odd to read

Sorry, I probably didn't need to reorder that. My thought process was
in terms of short circuiting, what order was most likely for us to
bail out of the condition first?  If it hurts readability, I'm happy
to take clean ups.

On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
> Overall looks good to me, just pointing at the same error I wrote in my
> other mail here -- I saw that by the time I was done writing this this
> patch got taken but that alone will probably warrant a follow-up :/
> > +#define barrier() (__asm__ __volatile__("" : : : "memory"))
>
> barrier here has gained external () compared to the definition and
> compiler-gcc.h:
> #define barrier() __asm__ __volatile__("": : :"memory")

Dominique,
Yes, sorry about that (looks like Linus noticed that, too).  Was a
stupid last minute mistake on my part, trying to appease
checkpatch.pl.  One of these days, I'll get frustrated enough to
rewrite checkpatch.pl as a set of clang tidy checks (so that it
actually parses the code), but I'll have to learn how to read perl
first to start translating.

> I've also added a few style nitpicks/questions but feel free to ignore
> these.

No, please, I really appreciate you taking the time to actually test
this and provide feedback.  That kind of support is critical in open
source.

> 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?

> > -#define __nostackprotector   __optimize("no-stack-protector")
>
> I do not see this being added back, it's probably fine though as it
> doesn't look used?
> (I didn't check all removed lines, maybe about half)

For each case, I triple checked that the macro was actually being used
in the code.  __nostackprotector was not, so I dropped it.

Sounds like I may have messed up `__naked` though, see below.

> I'm not sure I understand the logic of where you removed ifndef and
> where you kept them (but, well, this should work)

There were some cases were some symbols were defined in glibc's
headers, so `make CC=clang` (implying that HOSTCC=gcc) would produce
redefinition warnings.  I should've added a comment to clarify.

> 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.

On Wed, Aug 22, 2018 at 6:02 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> I've fixed that manually, but when I tried to test it I just hit the
>
>   arch/x86/Makefile:179: *** Compiler lacks asm-goto support..  Stop.
>
> error.
>
> Do you have some experimental clang build with asm goto support? What
> version? Or is it just that you're building ARM, not x86?

Linus,
Not yet.  I'll probably start implementing it myself if I don't see
patches soon.

Arm64 builds with defconfig minus CONFIG_ARM64_LSE_ATOMICS, or x86_64
with some #errors and a few other things commented out produces a
working build (but I realize that's living on borrowed time, hence
trying to document each bug and fix it properly).

Looks like I need to create some Arm32 bit qemu images for testing, see below.

On Wed, Aug 22, 2018 at 7:43 PM Masahiro Yamada
<yamada.masahiro@socionext.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.

Kees,
Thanks for the additional comments in the bug tracker.  Seems like
some differences in builtin_constant_p between compilers, let's keep
tracking this (maybe there's more that can be done in the Clang
patch).

Thanks everyone for the feedback.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Joe Perches]

On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> One of these days, I'll get frustrated enough to
> rewrite checkpatch.pl as a set of clang tidy checks (so that it
> actually parses the code), but I'll have to learn how to read perl
> first to start translating.

Good luck with that, remember that checkpatch works
on diff chunks and not files.

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Joe Perches]

On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> One reply for a bunch of the various threads, to keep the number of emails down:
> 
> On Wed, Aug 22, 2018 at 5:20 PM Joe Perches <joe@perches.com> wrote:
> > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > +/* Compiler specific macros. */
> > >  #ifdef __clang__
> > >  #include <linux/compiler-clang.h>
> > 
> > probably better as
> > 
> > #if defined(__clang)
> > 
> > to match the style of the #elif defined()s below it
> 
> Hi Joe,
> Thanks for the feedback. I always appreciate it.  If you have some
> cleanups, want to send them to me, and I'll bundle them up for a PR?
> I'm ok with that change.
> 
> > > +#ifdef __GNUC_STDC_INLINE__
> > > +# define __gnu_inline        __attribute__((gnu_inline))
> > > +#else
> > > +# define __gnu_inline
> > > +#endif
> > 
> > Perhaps __gnu_inline should be in compiler-gcc and this
> > should use
> > 
> > #ifndef __gnu_inline
> > #define __gnu_inline
> > #endif
> 
> Not this case; it's how we get gnu89 semantics for `extern inline` is
> not compiler specific (therefor should not go in a compiler specific
> header).

It's not possible to know that compilers support what
__attribute__((<foo>)) and at what version that support
exists unless it is specified somewhere.

As far as I can tell,  gnu_inline is not recognized by clang.

https://clang.llvm.org/docs/AttributeReference.html

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Dominique Martinet]

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@socionext.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

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Nick Desaulniers]

On Thu, Aug 23, 2018 at 2:19 PM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> > One reply for a bunch of the various threads, to keep the number of emails down:
> >
> > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches <joe@perches.com> wrote:
> > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > > +/* Compiler specific macros. */
> > > >  #ifdef __clang__
> > > >  #include <linux/compiler-clang.h>
> > >
> > > probably better as
> > >
> > > #if defined(__clang)
> > >
> > > to match the style of the #elif defined()s below it
> >
> > Hi Joe,
> > Thanks for the feedback. I always appreciate it.  If you have some
> > cleanups, want to send them to me, and I'll bundle them up for a PR?
> > I'm ok with that change.
> >
> > > > +#ifdef __GNUC_STDC_INLINE__
> > > > +# define __gnu_inline        __attribute__((gnu_inline))
> > > > +#else
> > > > +# define __gnu_inline
> > > > +#endif
> > >
> > > Perhaps __gnu_inline should be in compiler-gcc and this
> > > should use
> > >
> > > #ifndef __gnu_inline
> > > #define __gnu_inline
> > > #endif
> >
> > Not this case; it's how we get gnu89 semantics for `extern inline` is
> > not compiler specific (therefor should not go in a compiler specific
> > header).
>
> It's not possible to know that compilers support what
> __attribute__((<foo>)) and at what version that support
> exists unless it is specified somewhere.

__has_attribute:
https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute

The release notes of GCC-5 mention __has_attribute.
https://gcc.gnu.org/gcc-5/changes.html

The point of feature detection is that it _doesn't matter_ what
version that support exists.  Either it does and you can use it, or it
doesn't and you can decide whether to stop compiling or there's a
valid work around.

Feature detection should be preferred to explicit version checks
except in 2 specific cases:
1. It's not possible to properly perform feature detection.  Language
features should not be added unless it's possible to safely check for
them.
2. A very specific version of a very specific compiler is broken and
needs to be explicitly guarded against.

> As far as I can tell,  gnu_inline is not recognized by clang.
>
> https://clang.llvm.org/docs/AttributeReference.html

If that was the case, I would not have added it in commit d03db2bc26f0
("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
declarations").
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d03db2bc26f0e4a6849ad649a09c9c73fccdc656

Docs can sometimes fall behind, the lone source of truth is the source code.
https://github.com/llvm-mirror/clang/search?q=gnu_inline&unscoped_q=gnu_inline

Godbolt is also incredibly helpful for testing various compiler versions:
https://godbolt.org/z/uMJ-mo


https://reviews.llvm.org/D51190

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Joe Perches]

On Thu, 2018-08-23 at 16:12 -0700, Nick Desaulniers wrote:
> On Thu, Aug 23, 2018 at 2:19 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> > > One reply for a bunch of the various threads, to keep the number of emails down:
> > > 
> > > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches <joe@perches.com> wrote:
> > > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > > > +/* Compiler specific macros. */
> > > > >  #ifdef __clang__
> > > > >  #include <linux/compiler-clang.h>
> > > > 
> > > > probably better as
> > > > 
> > > > #if defined(__clang)
> > > > 
> > > > to match the style of the #elif defined()s below it
> > > 
> > > Hi Joe,
> > > Thanks for the feedback. I always appreciate it.  If you have some
> > > cleanups, want to send them to me, and I'll bundle them up for a PR?
> > > I'm ok with that change.
> > > 
> > > > > +#ifdef __GNUC_STDC_INLINE__
> > > > > +# define __gnu_inline        __attribute__((gnu_inline))
> > > > > +#else
> > > > > +# define __gnu_inline
> > > > > +#endif
> > > > 
> > > > Perhaps __gnu_inline should be in compiler-gcc and this
> > > > should use
> > > > 
> > > > #ifndef __gnu_inline
> > > > #define __gnu_inline
> > > > #endif
> > > 
> > > Not this case; it's how we get gnu89 semantics for `extern inline` is
> > > not compiler specific (therefor should not go in a compiler specific
> > > header).
> > 
> > It's not possible to know that compilers support what
> > __attribute__((<foo>)) and at what version that support
> > exists unless it is specified somewhere.
> 
> __has_attribute:
> https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
> 
> The release notes of GCC-5 mention __has_attribute.
> https://gcc.gnu.org/gcc-5/changes.html

So not available in the now minimum supported gcc 4.6?

> The point of feature detection is that it _doesn't matter_ what
> version that support exists.  Either it does and you can use it, or it
> doesn't and you can decide whether to stop compiling or there's a
> valid work around.
> 
> Feature detection should be preferred to explicit version checks
> except in 2 specific cases:
> 1. It's not possible to properly perform feature detection.  Language
> features should not be added unless it's possible to safely check for
> them.
> 2. A very specific version of a very specific compiler is broken and
> needs to be explicitly guarded against.
> 
> > As far as I can tell,  gnu_inline is not recognized by clang.
> > 
> > https://clang.llvm.org/docs/AttributeReference.html
> 
> If that was the case, I would not have added it in commit d03db2bc26f0
> ("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> declarations").
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d03db2bc26f0e4a6849ad649a09c9c73fccdc656

Hard to know.
That commit message does not mention clang.

> Docs can sometimes fall behind, the lone source of truth is the source code.
> https://github.com/llvm-mirror/clang/search?q=gnu_inline&unscoped_q=gnu_inline

Which no compiler user should have to read.

> Godbolt is also incredibly helpful for testing various compiler versions:
> https://godbolt.org/z/uMJ-mo

Thanks for that.

> https://reviews.llvm.org/D51190

That too.

cheers, Joe

Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive

[Miguel Ojeda]

Hi all,

On Fri, Aug 24, 2018 at 1:31 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2018-08-23 at 16:12 -0700, Nick Desaulniers wrote:
>> On Thu, Aug 23, 2018 at 2:19 PM Joe Perches <joe@perches.com> wrote:
>> >
>> > On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
>> > > Not this case; it's how we get gnu89 semantics for `extern inline` is
>> > > not compiler specific (therefor should not go in a compiler specific
>> > > header).
>> >
>> > It's not possible to know that compilers support what
>> > __attribute__((<foo>)) and at what version that support
>> > exists unless it is specified somewhere.
>>
>> __has_attribute:
>> https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
>>
>> The release notes of GCC-5 mention __has_attribute.
>> https://gcc.gnu.org/gcc-5/changes.html
>
> So not available in the now minimum supported gcc 4.6?
>

We can workaround that until we have 5 as the minimum. I have tried
and the result is quite nice.

Sending the patch in a few moments -- compile-testing for a while
allmodconfig on both 4.6 and 8.2...

Cheers,
Miguel