paravirt depends on a custom calling convention (callee saved), but
expects this from a static inline function that it then forces to be
outlined. This is problematic because different compilers or flags can
then add a stack guard that violates the calling conventions.
Uses extern inline with the out-of-line definition in assembly to
prevent compilers from adding stack guards to the outlined version.
Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely
problematic* for extern inline, as the sematics are completely the
opposite depending on what C standard is used.
http://blahg.josefsipek.net/?p=529
Changes since v3:
Take Joe's suggestion to hoist __inline__ and __inline out of
conditional block.
Changes since v2:
Take hpa's _ASM_ARG patch into the set in order to simplify cross
32b/64b x86 assembly and rebase my final patch to use it. Apply
Sedat's typo fix to commit message and sign off on it. Take Joe's
suggestion to simplify __inline__ and __inline. Add Arnd to list of
folks who made helpful suggestions.
Changes since v1:
Prefer gnu_inline function attribute instead of explicitly setting C
standard compiler flag in problematic Makefiles. We should instead
carefully evaluate if those Makefiles should be overwriting
KBUILD_CFLAGS at all. Dropped the previous first two patches and added
a new first patch.
H. Peter Anvin (1):
x86/asm: add _ASM_ARG* constants for argument registers to <asm/asm.h>
Nick Desaulniers (2):
compiler-gcc.h: add gnu_inline to all inline declarations
x86: paravirt: make native_save_fl extern inline
arch/x86/include/asm/asm.h | 59 +++++++++++++++++++++++++++++++++
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/irqflags.S | 26 +++++++++++++++
include/linux/compiler-gcc.h | 17 ++++++----
5 files changed, 97 insertions(+), 8 deletions(-)
create mode 100644 arch/x86/kernel/irqflags.S
--
2.17.1.1185.g55be947832-goog
Functions marked extern inline do not emit an externally visible
function when the gnu89 C standard is used. Some KBUILD Makefiles
overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without
an explicit C standard specified, the default is gnu11. Since c99, the
semantics of extern inline have changed such that an externally visible
function is always emitted. This can lead to multiple definition errors
of extern inline functions at link time of compilation units whose build
files have removed an explicit C standard compiler flag for users of GCC
5.1+ or Clang.
Signed-off-by: Nick Desaulniers <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Suggested-by: Joe Perches <[email protected]>
---
include/linux/compiler-gcc.h | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index b4bf73f5e38f..0c79c588e572 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -72,19 +72,22 @@
* -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) || (__GNUC__ < 4)
-#define inline inline __attribute__((always_inline,unused)) notrace
-#define __inline__ __inline__ __attribute__((always_inline,unused)) notrace
-#define __inline __inline __attribute__((always_inline,unused)) notrace
+#define inline \
+ inline __attribute__((always_inline, unused, gnu_inline)) notrace
#else
-/* A lot of inline functions can cause havoc with function tracing */
-#define inline inline __attribute__((unused)) notrace
-#define __inline__ __inline__ __attribute__((unused)) notrace
-#define __inline __inline __attribute__((unused)) notrace
+#define inline inline __attribute__((unused, gnu_inline)) notrace
#endif
+#define __inline__ inline
+#define __inline inline
#define __always_inline inline __attribute__((always_inline))
#define noinline __attribute__((noinline))
--
2.17.1.1185.g55be947832-goog
From: "H. Peter Anvin" <[email protected]>
i386 and x86-64 uses different registers for arguments; make them
available so we don't have to #ifdef in the actual code.
Native size and specified size (q, l, w, b) versions are provided.
Suggested-by: Sedat Dilek <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
arch/x86/include/asm/asm.h | 59 ++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 219faaec51df..990770f9e76b 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -46,6 +46,65 @@
#define _ASM_SI __ASM_REG(si)
#define _ASM_DI __ASM_REG(di)
+#ifndef __x86_64__
+/* 32 bit */
+
+#define _ASM_ARG1 _ASM_AX
+#define _ASM_ARG2 _ASM_DX
+#define _ASM_ARG3 _ASM_CX
+
+#define _ASM_ARG1L eax
+#define _ASM_ARG2L edx
+#define _ASM_ARG3L ecx
+
+#define _ASM_ARG1W ax
+#define _ASM_ARG2W dx
+#define _ASM_ARG3W cx
+
+#define _ASM_ARG1B al
+#define _ASM_ARG2B dl
+#define _ASM_ARG3B cl
+
+#else
+/* 64 bit */
+
+#define _ASM_ARG1 _ASM_DI
+#define _ASM_ARG2 _ASM_SI
+#define _ASM_ARG3 _ASM_DX
+#define _ASM_ARG4 _ASM_CX
+#define _ASM_ARG5 r8
+#define _ASM_ARG6 r9
+
+#define _ASM_ARG1Q rdi
+#define _ASM_ARG2Q rsi
+#define _ASM_ARG3Q rdx
+#define _ASM_ARG4Q rcx
+#define _ASM_ARG5Q r8
+#define _ASM_ARG6Q r9
+
+#define _ASM_ARG1L edi
+#define _ASM_ARG2L esi
+#define _ASM_ARG3L edx
+#define _ASM_ARG4L ecx
+#define _ASM_ARG5L r8d
+#define _ASM_ARG6L r9d
+
+#define _ASM_ARG1W di
+#define _ASM_ARG2W si
+#define _ASM_ARG3W dx
+#define _ASM_ARG4W cx
+#define _ASM_ARG5W r8w
+#define _ASM_ARG6W r9w
+
+#define _ASM_ARG1B dil
+#define _ASM_ARG2B sil
+#define _ASM_ARG3B dl
+#define _ASM_ARG4B cl
+#define _ASM_ARG5B r8b
+#define _ASM_ARG6B r9b
+
+#endif
+
/*
* Macros to generate condition code outputs from inline assembly,
* The output operand must be type "bool".
--
2.17.1.1185.g55be947832-goog
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Debugged-by: Alistair Strachan <[email protected]>
Debugged-by: Matthias Kaehlcke <[email protected]>
Reported-by: Sedat Dilek <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Suggested-by: Tom Stellar <[email protected]>
Tested-by: Sedat Dilek <[email protected]>
---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/irqflags.S | 26 ++++++++++++++++++++++++++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index 000000000000..ddeeaac8adda
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/asm.h>
+#include <asm/export.h>
+#include <linux/linkage.h>
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %eax/%rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_ARG1
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.17.1.1185.g55be947832-goog
On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
<[email protected]> wrote:
> Functions marked extern inline do not emit an externally visible
> function when the gnu89 C standard is used. Some KBUILD Makefiles
> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without
> an explicit C standard specified, the default is gnu11. Since c99, the
> semantics of extern inline have changed such that an externally visible
> function is always emitted. This can lead to multiple definition errors
> of extern inline functions at link time of compilation units whose build
> files have removed an explicit C standard compiler flag for users of GCC
> 5.1+ or Clang.
>
> Signed-off-by: Nick Desaulniers <[email protected]>
> Suggested-by: H. Peter Anvin <[email protected]>
> Suggested-by: Joe Perches <[email protected]>
I suspect this will break Geert's gcc-4.1.2, which I think doesn't have that
attribute yet (4.1.3 or higher have it according to the documentation.
It wouldn't be hard to work around that if we want to keep that version
working, or we could decide that it's time to officially stop supporting
that version, but we should probably decide on one or the other.
Arnd
On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann <[email protected]> wrote:
> On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
> <[email protected]> wrote:
>> Functions marked extern inline do not emit an externally visible
>> function when the gnu89 C standard is used. Some KBUILD Makefiles
>> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without
>> an explicit C standard specified, the default is gnu11. Since c99, the
>> semantics of extern inline have changed such that an externally visible
>> function is always emitted. This can lead to multiple definition errors
>> of extern inline functions at link time of compilation units whose build
>> files have removed an explicit C standard compiler flag for users of GCC
>> 5.1+ or Clang.
>>
>> Signed-off-by: Nick Desaulniers <[email protected]>
>> Suggested-by: H. Peter Anvin <[email protected]>
>> Suggested-by: Joe Perches <[email protected]>
>
> I suspect this will break Geert's gcc-4.1.2, which I think doesn't have that
> attribute yet (4.1.3 or higher have it according to the documentation.
>
> It wouldn't be hard to work around that if we want to keep that version
> working, or we could decide that it's time to officially stop supporting
> that version, but we should probably decide on one or the other.
>
Good point.
What is the minimum requirement of GCC version currently?
AFAICS x86/asm-goto support requires GCC >= 4.5?
Just FYI...
...saw the last days in upstream commits that kbuild/kconfig for
4.18-rc1 offers possibilities to check for cc-version dependencies.
- sed@ -
On Fri, Jun 08, 2018 at 12:04:36PM +0200, Sedat Dilek wrote:
> On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann <[email protected]> wrote:
> > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
> > <[email protected]> wrote:
> >> Functions marked extern inline do not emit an externally visible
> >> function when the gnu89 C standard is used. Some KBUILD Makefiles
> >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without
> >> an explicit C standard specified, the default is gnu11. Since c99, the
> >> semantics of extern inline have changed such that an externally visible
> >> function is always emitted. This can lead to multiple definition errors
> >> of extern inline functions at link time of compilation units whose build
> >> files have removed an explicit C standard compiler flag for users of GCC
> >> 5.1+ or Clang.
> >>
> >> Signed-off-by: Nick Desaulniers <[email protected]>
> >> Suggested-by: H. Peter Anvin <[email protected]>
> >> Suggested-by: Joe Perches <[email protected]>
> >
> > I suspect this will break Geert's gcc-4.1.2, which I think doesn't have that
> > attribute yet (4.1.3 or higher have it according to the documentation.
> >
> > It wouldn't be hard to work around that if we want to keep that version
> > working, or we could decide that it's time to officially stop supporting
> > that version, but we should probably decide on one or the other.
> >
>
> Good point.
> What is the minimum requirement of GCC version currently?
Good question ;-) (I recently had the impression that
Documentation/process/changes.rst was making fun of me ;-)
> AFAICS x86/asm-goto support requires GCC >= 4.5?
>
> Just FYI...
> ...saw the last days in upstream commits that kbuild/kconfig for
> 4.18-rc1 offers possibilities to check for cc-version dependencies.
Good to know! Mind retrieving/sharing the commit id(s)
or links to the corresponding discussion on LKML?
Thanks,
Andrea
>
> - sed@ -
On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
<[email protected]> wrote:
> paravirt depends on a custom calling convention (callee saved), but
> expects this from a static inline function that it then forces to be
> outlined. This is problematic because different compilers or flags can
> then add a stack guard that violates the calling conventions.
>
> Uses extern inline with the out-of-line definition in assembly to
> prevent compilers from adding stack guards to the outlined version.
>
> Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely
> problematic* for extern inline, as the sematics are completely the
> opposite depending on what C standard is used.
> http://blahg.josefsipek.net/?p=529
>
> Changes since v3:
> Take Joe's suggestion to hoist __inline__ and __inline out of
> conditional block.
>
> Changes since v2:
> Take hpa's _ASM_ARG patch into the set in order to simplify cross
> 32b/64b x86 assembly and rebase my final patch to use it. Apply
> Sedat's typo fix to commit message and sign off on it. Take Joe's
> suggestion to simplify __inline__ and __inline. Add Arnd to list of
> folks who made helpful suggestions.
>
> Changes since v1:
> Prefer gnu_inline function attribute instead of explicitly setting C
> standard compiler flag in problematic Makefiles. We should instead
> carefully evaluate if those Makefiles should be overwriting
> KBUILD_CFLAGS at all. Dropped the previous first two patches and added
> a new first patch.
>
> H. Peter Anvin (1):
> x86/asm: add _ASM_ARG* constants for argument registers to <asm/asm.h>
>
> Nick Desaulniers (2):
> compiler-gcc.h: add gnu_inline to all inline declarations
> x86: paravirt: make native_save_fl extern inline
>
> arch/x86/include/asm/asm.h | 59 +++++++++++++++++++++++++++++++++
> arch/x86/include/asm/irqflags.h | 2 +-
> arch/x86/kernel/Makefile | 1 +
> arch/x86/kernel/irqflags.S | 26 +++++++++++++++
> include/linux/compiler-gcc.h | 17 ++++++----
> 5 files changed, 97 insertions(+), 8 deletions(-)
> create mode 100644 arch/x86/kernel/irqflags.S
>
> --
> 2.17.1.1185.g55be947832-goog
>
Tested v4 against Linux v4.14.48 and clang 7-svn333836 and booted on
bare metal/hardware.
- Sedat -
On Fri, Jun 8, 2018 at 1:28 PM, Andrea Parri
<[email protected]> wrote:
> On Fri, Jun 08, 2018 at 12:04:36PM +0200, Sedat Dilek wrote:
>> On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann <[email protected]> wrote:
>> > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
>> > <[email protected]> wrote:
>> >> Functions marked extern inline do not emit an externally visible
>> >> function when the gnu89 C standard is used. Some KBUILD Makefiles
>> >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without
>> >> an explicit C standard specified, the default is gnu11. Since c99, the
>> >> semantics of extern inline have changed such that an externally visible
>> >> function is always emitted. This can lead to multiple definition errors
>> >> of extern inline functions at link time of compilation units whose build
>> >> files have removed an explicit C standard compiler flag for users of GCC
>> >> 5.1+ or Clang.
>> >>
>> >> Signed-off-by: Nick Desaulniers <[email protected]>
>> >> Suggested-by: H. Peter Anvin <[email protected]>
>> >> Suggested-by: Joe Perches <[email protected]>
>> >
>> > I suspect this will break Geert's gcc-4.1.2, which I think doesn't have that
>> > attribute yet (4.1.3 or higher have it according to the documentation.
>> >
>> > It wouldn't be hard to work around that if we want to keep that version
>> > working, or we could decide that it's time to officially stop supporting
>> > that version, but we should probably decide on one or the other.
>> >
>>
>> Good point.
>> What is the minimum requirement of GCC version currently?
>
> Good question ;-) (I recently had the impression that
> Documentation/process/changes.rst was making fun of me ;-)
>
>
>> AFAICS x86/asm-goto support requires GCC >= 4.5?
>>
>> Just FYI...
>> ...saw the last days in upstream commits that kbuild/kconfig for
>> 4.18-rc1 offers possibilities to check for cc-version dependencies.
>
> Good to know! Mind retrieving/sharing the commit id(s)
> or links to the corresponding discussion on LKML?
>
Sorry, it is not yet in Linus upstream. see [1].
Documentation: kconfig: add recommended way to describe compiler support
kconfig: add CC_IS_CLANG and CLANG_VERSION
kconfig: add CC_IS_GCC and GCC_VERSION
- sed@ -
[1] https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/log/?h=kbuild
On Fri, Jun 8, 2018 at 3:04 AM Sedat Dilek <[email protected]> wrote:
>
> On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann <[email protected]> wrote:
> > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
> > <[email protected]> wrote:
> >> Functions marked extern inline do not emit an externally visible
> >> function when the gnu89 C standard is used. Some KBUILD Makefiles
> >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without
> >> an explicit C standard specified, the default is gnu11. Since c99, the
> >> semantics of extern inline have changed such that an externally visible
> >> function is always emitted. This can lead to multiple definition errors
> >> of extern inline functions at link time of compilation units whose build
> >> files have removed an explicit C standard compiler flag for users of GCC
> >> 5.1+ or Clang.
> >>
> >> Signed-off-by: Nick Desaulniers <[email protected]>
> >> Suggested-by: H. Peter Anvin <[email protected]>
> >> Suggested-by: Joe Perches <[email protected]>
> >
> > I suspect this will break Geert's gcc-4.1.2, which I think doesn't have that
> > attribute yet (4.1.3 or higher have it according to the documentation.
> >
> > It wouldn't be hard to work around that if we want to keep that version
> > working, or we could decide that it's time to officially stop supporting
> > that version, but we should probably decide on one or the other.
Heh, so earlier we decided against compiler flags (-std=gnu89 or
-fgnu89-inline) in preference to function attributes. The function
attribute is preferable as some of the Makefiles [accidentally?]
overwrite KBUILD_CFLAGS, which is problematic for gcc 5.1 users as the
implicit c standard used was changed to gnu11 from gnu89. What's nice
is that to support gcc 4.1 users, we simply don't need to add any
attribute, as their implicit c standard is gnu89 which has the
semantics for extern inline that we want. I have a simple change to
this patch that can support users of various gcc versions, see below:
> Good point.
> What is the minimum requirement of GCC version currently?
> AFAICS x86/asm-goto support requires GCC >= 4.5?
Yes, but that's only for x86, IIUC. It seems the kernel may have
different minimum required versions of GCC based on arch then? That
may be ok, but I'm not sure that's easy to keep track of without
having it explicitly stated somewhere like the docs perhaps?
> Just FYI...
> ...saw the last days in upstream commits that kbuild/kconfig for
> 4.18-rc1 offers possibilities to check for cc-version dependencies.
Those will be helpful. If we want to pursue compiler flags, which get
set some Makefiles, then yes. But I think a simpler change to my
patch would be as below.
It seems gcc did not get __has_attribute [0] until 5.1, but will
define __GNUC_GNU_INLINE__ if supported. [1] Unfortunately, Clang
does not define __GNUC_GNU_INLINE__ [2]. So a proper feature test
might look like:
```
#ifndef __has_attribute
#define __has_attribute(x) 0
#endif
#if defined(__GNUC_GNU_INLINE__) || __has_attribute(gnu_inline)
#define __gnu_inline __attribute__(gnu_inline)
#endif
#define inline inline __attribute__((always_inline, unused)) notrace
__gnu_inline
```
Thoughts on this approach? I can send a v5 tomorrow if there's no
major issues. Feedback appreciated, as always.
[0] https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
[2] https://bugs.llvm.org/show_bug.cgi?id=37784
--
Thanks,
~Nick Desaulniers
<[email protected]>,Greg KH <[email protected]>,[email protected],[email protected],Josh Poimboeuf <[email protected]>,Matthias Kaehlcke <[email protected]>,[email protected],Thiebaud Weksteen <[email protected]>,[email protected],[email protected]
From: [email protected]
Message-ID: <[email protected]>
On June 12, 2018 11:33:14 AM PDT, Nick Desaulniers <[email protected]> wrote:
>On Fri, Jun 8, 2018 at 3:04 AM Sedat Dilek <[email protected]>
>wrote:
>>
>> On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann <[email protected]> wrote:
>> > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
>> > <[email protected]> wrote:
>> >> Functions marked extern inline do not emit an externally visible
>> >> function when the gnu89 C standard is used. Some KBUILD Makefiles
>> >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as
>without
>> >> an explicit C standard specified, the default is gnu11. Since c99,
>the
>> >> semantics of extern inline have changed such that an externally
>visible
>> >> function is always emitted. This can lead to multiple definition
>errors
>> >> of extern inline functions at link time of compilation units whose
>build
>> >> files have removed an explicit C standard compiler flag for users
>of GCC
>> >> 5.1+ or Clang.
>> >>
>> >> Signed-off-by: Nick Desaulniers <[email protected]>
>> >> Suggested-by: H. Peter Anvin <[email protected]>
>> >> Suggested-by: Joe Perches <[email protected]>
>> >
>> > I suspect this will break Geert's gcc-4.1.2, which I think doesn't
>have that
>> > attribute yet (4.1.3 or higher have it according to the
>documentation.
>> >
>> > It wouldn't be hard to work around that if we want to keep that
>version
>> > working, or we could decide that it's time to officially stop
>supporting
>> > that version, but we should probably decide on one or the other.
>
>Heh, so earlier we decided against compiler flags (-std=gnu89 or
>-fgnu89-inline) in preference to function attributes. The function
>attribute is preferable as some of the Makefiles [accidentally?]
>overwrite KBUILD_CFLAGS, which is problematic for gcc 5.1 users as the
>implicit c standard used was changed to gnu11 from gnu89. What's nice
>is that to support gcc 4.1 users, we simply don't need to add any
>attribute, as their implicit c standard is gnu89 which has the
>semantics for extern inline that we want. I have a simple change to
>this patch that can support users of various gcc versions, see below:
>
>> Good point.
>> What is the minimum requirement of GCC version currently?
>> AFAICS x86/asm-goto support requires GCC >= 4.5?
>
>Yes, but that's only for x86, IIUC. It seems the kernel may have
>different minimum required versions of GCC based on arch then? That
>may be ok, but I'm not sure that's easy to keep track of without
>having it explicitly stated somewhere like the docs perhaps?
>
>> Just FYI...
>> ...saw the last days in upstream commits that kbuild/kconfig for
>> 4.18-rc1 offers possibilities to check for cc-version dependencies.
>
>Those will be helpful. If we want to pursue compiler flags, which get
>set some Makefiles, then yes. But I think a simpler change to my
>patch would be as below.
>
>It seems gcc did not get __has_attribute [0] until 5.1, but will
>define __GNUC_GNU_INLINE__ if supported. [1] Unfortunately, Clang
>does not define __GNUC_GNU_INLINE__ [2]. So a proper feature test
>might look like:
>
>```
>#ifndef __has_attribute
>#define __has_attribute(x) 0
>#endif
>
>#if defined(__GNUC_GNU_INLINE__) || __has_attribute(gnu_inline)
>#define __gnu_inline __attribute__(gnu_inline)
>#endif
>
>#define inline inline __attribute__((always_inline, unused)) notrace
>__gnu_inline
>```
>
>Thoughts on this approach? I can send a v5 tomorrow if there's no
>major issues. Feedback appreciated, as always.
>
>[0] https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
>[1]
>https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>[2] https://bugs.llvm.org/show_bug.cgi?id=37784
Please fix clang. It isn't all that hard to fix.
However, __GCC_GNU_INLINE__ means you are in GNU mode by default, on gcc's new enough to have multiple misses.
The right thing to look for is __GCC_STDC_INLINE__ in which case you need the attribute.
By the way, you should check clang against gcc's predefined macros by doing:
gcc [options] -x c -Wp,-dM -E /dev/null | sort
Options can change the predefined macros substantially, especially the, -std=, arch and -O options. -x c can be replaced with e.g. -x c++, objective-c, assembler-with-cpp etc.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Tue, Jun 12, 2018 at 11:51 AM H. Peter Anvin <[email protected]> wrote:
>
> <[email protected]>,Greg KH <[email protected]>,[email protected],[email protected],Josh Poimboeuf <[email protected]>,Matthias Kaehlcke <[email protected]>,[email protected],Thiebaud Weksteen <[email protected]>,[email protected],[email protected]
> From: [email protected]
> Message-ID: <[email protected]>
>
> On June 12, 2018 11:33:14 AM PDT, Nick Desaulniers <[email protected]> wrote:
> >On Fri, Jun 8, 2018 at 3:04 AM Sedat Dilek <[email protected]>
> >wrote:
> >>
> >> On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann <[email protected]> wrote:
> >> > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers
> >> > <[email protected]> wrote:
> >> >> Functions marked extern inline do not emit an externally visible
> >> >> function when the gnu89 C standard is used. Some KBUILD Makefiles
> >> >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as
> >without
> >> >> an explicit C standard specified, the default is gnu11. Since c99,
> >the
> >> >> semantics of extern inline have changed such that an externally
> >visible
> >> >> function is always emitted. This can lead to multiple definition
> >errors
> >> >> of extern inline functions at link time of compilation units whose
> >build
> >> >> files have removed an explicit C standard compiler flag for users
> >of GCC
> >> >> 5.1+ or Clang.
> >> >>
> >> >> Signed-off-by: Nick Desaulniers <[email protected]>
> >> >> Suggested-by: H. Peter Anvin <[email protected]>
> >> >> Suggested-by: Joe Perches <[email protected]>
> >> >
> >> > I suspect this will break Geert's gcc-4.1.2, which I think doesn't
> >have that
> >> > attribute yet (4.1.3 or higher have it according to the
> >documentation.
> >> >
> >> > It wouldn't be hard to work around that if we want to keep that
> >version
> >> > working, or we could decide that it's time to officially stop
> >supporting
> >> > that version, but we should probably decide on one or the other.
> >
> >Heh, so earlier we decided against compiler flags (-std=gnu89 or
> >-fgnu89-inline) in preference to function attributes. The function
> >attribute is preferable as some of the Makefiles [accidentally?]
> >overwrite KBUILD_CFLAGS, which is problematic for gcc 5.1 users as the
> >implicit c standard used was changed to gnu11 from gnu89. What's nice
> >is that to support gcc 4.1 users, we simply don't need to add any
> >attribute, as their implicit c standard is gnu89 which has the
> >semantics for extern inline that we want. I have a simple change to
> >this patch that can support users of various gcc versions, see below:
> >
> >> Good point.
> >> What is the minimum requirement of GCC version currently?
> >> AFAICS x86/asm-goto support requires GCC >= 4.5?
> >
> >Yes, but that's only for x86, IIUC. It seems the kernel may have
> >different minimum required versions of GCC based on arch then? That
> >may be ok, but I'm not sure that's easy to keep track of without
> >having it explicitly stated somewhere like the docs perhaps?
> >
> >> Just FYI...
> >> ...saw the last days in upstream commits that kbuild/kconfig for
> >> 4.18-rc1 offers possibilities to check for cc-version dependencies.
> >
> >Those will be helpful. If we want to pursue compiler flags, which get
> >set some Makefiles, then yes. But I think a simpler change to my
> >patch would be as below.
> >
> >It seems gcc did not get __has_attribute [0] until 5.1, but will
> >define __GNUC_GNU_INLINE__ if supported. [1] Unfortunately, Clang
> >does not define __GNUC_GNU_INLINE__ [2]. So a proper feature test
> >might look like:
> >
> >```
> >#ifndef __has_attribute
> >#define __has_attribute(x) 0
> >#endif
> >
> >#if defined(__GNUC_GNU_INLINE__) || __has_attribute(gnu_inline)
> >#define __gnu_inline __attribute__(gnu_inline)
> >#endif
> >
> >#define inline inline __attribute__((always_inline, unused)) notrace
> >__gnu_inline
> >```
> >
> >Thoughts on this approach? I can send a v5 tomorrow if there's no
> >major issues. Feedback appreciated, as always.
> >
> >[0] https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
> >[1]
> >https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> >[2] https://bugs.llvm.org/show_bug.cgi?id=37784
>
> Please fix clang. It isn't all that hard to fix.
>
> However, __GCC_GNU_INLINE__ means you are in GNU mode by default, on gcc's new enough to have multiple misses.
>
> The right thing to look for is __GCC_STDC_INLINE__ in which case you need the attribute.
Oh, by [0] __GCC_STDC_INLINE__ is indeed actually the correct symbol
to check for. Clang does support that, so nothing to fix there.
> By the way, you should check clang against gcc's predefined macros by doing:
>
> gcc [options] -x c -Wp,-dM -E /dev/null | sort
>
> Options can change the predefined macros substantially, especially the, -std=, arch and -O options. -x c can be replaced with e.g. -x c++, objective-c, assembler-with-cpp etc.
Neat, I'll have to bookmark that incantation. I can s/gcc/clang/ to
get a similar list (which is how I know it supports
__GCC_STDC_INLINE__).
Patch now becomes something like:
#ifdef __GNUC_GNU_INLINE__
#define __gnu_inline __attribute__((gnu_inline))
#else
#define __gnu_inline
#endif
#define inline inline __attribute__((always_inline, unused)) notrace
__gnu_inline
...
Issues with that approach?
[0] https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
--
Thanks,
~Nick Desaulniers
On 06/12/18 13:19, Nick Desaulniers wrote:
>
> Oh, by [0] __GCC_STDC_INLINE__ is indeed actually the correct symbol
> to check for. Clang does support that, so nothing to fix there.
>
>> By the way, you should check clang against gcc's predefined macros by doing:
>>
>> gcc [options] -x c -Wp,-dM -E /dev/null | sort
>>
>> Options can change the predefined macros substantially, especially the, -std=, arch and -O options. -x c can be replaced with e.g. -x c++, objective-c, assembler-with-cpp etc.
>
> Neat, I'll have to bookmark that incantation. I can s/gcc/clang/ to
> get a similar list (which is how I know it supports
> __GCC_STDC_INLINE__).>
I bet that if you add -fgnu89-inlines then it *does* define
__GNUC_GNU_INLINE__.
>
> Patch now becomes something like:
>
> #ifdef __GNUC_GNU_INLINE__
> #define __gnu_inline __attribute__((gnu_inline))
> #else
> #define __gnu_inline
> #endif
>
> #define inline inline __attribute__((always_inline, unused)) notrace
> __gnu_inline
> ...
>
> Issues with that approach?
>
s/__GNUC_GNU_INLINE__/__GNUC_STDC_INLINE__/
-hpa
On Tue, Jun 12, 2018 at 2:31 PM H. Peter Anvin <[email protected]> wrote:
>
> On 06/12/18 13:19, Nick Desaulniers wrote:
> >
> > Oh, by [0] __GCC_STDC_INLINE__ is indeed actually the correct symbol
> > to check for. Clang does support that, so nothing to fix there.
> >
> >> By the way, you should check clang against gcc's predefined macros by doing:
> >>
> >> gcc [options] -x c -Wp,-dM -E /dev/null | sort
> >>
> >> Options can change the predefined macros substantially, especially the, -std=, arch and -O options. -x c can be replaced with e.g. -x c++, objective-c, assembler-with-cpp etc.
> >
> > Neat, I'll have to bookmark that incantation. I can s/gcc/clang/ to
> > get a similar list (which is how I know it supports
> > __GCC_STDC_INLINE__).>
>
> I bet that if you add -fgnu89-inlines then it *does* define
> __GNUC_GNU_INLINE__.
Indeed! I see what you mean about [options] changing the predefined symbol list.
> > Patch now becomes something like:
> >
> > #ifdef __GNUC_GNU_INLINE__
> > #define __gnu_inline __attribute__((gnu_inline))
> > #else
> > #define __gnu_inline
> > #endif
> >
> > #define inline inline __attribute__((always_inline, unused)) notrace
> > __gnu_inline
> > ...
> >
> > Issues with that approach?
> >
>
> s/__GNUC_GNU_INLINE__/__GNUC_STDC_INLINE__/
Oops, sorry, yes good catch.
--
Thanks,
~Nick Desaulniers