All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] lib: Add shared copy of __lshrti3 from libgcc
@ 2019-03-15 20:54 Matthias Kaehlcke
  2019-03-15 22:06 ` Nick Desaulniers
  2019-03-18 21:31 ` Matthias Kaehlcke
  0 siblings, 2 replies; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-15 20:54 UTC (permalink / raw)
  To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin
  Cc: x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Matthias Kaehlcke

The compiler may emit calls to __lshrti3 from the compiler runtime
library, which results in undefined references:

arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
  include/linux/math64.h:186: undefined reference to `__lshrti3'

Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).

Include the function for x86 builds with clang, which is the
environment where the above error was observed.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/x86/Kconfig       |  1 +
 include/linux/libgcc.h | 16 ++++++++++++++++
 lib/Kconfig            |  3 +++
 lib/Makefile           |  1 +
 lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
 5 files changed, 52 insertions(+)
 create mode 100644 lib/lshrti3.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1f9b3cf437c..a5e0d923845d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -105,6 +105,7 @@ config X86
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_RESERVATION_MODE
 	select GENERIC_IRQ_SHOW
+	select GENERIC_LIB_LSHRTI3		if CC_IS_CLANG
 	select GENERIC_PENDING_IRQ		if SMP
 	select GENERIC_SMP_IDLE_THREAD
 	select GENERIC_STRNCPY_FROM_USER
diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
index 32e1e0f4b2d0..a71036471838 100644
--- a/include/linux/libgcc.h
+++ b/include/linux/libgcc.h
@@ -22,15 +22,26 @@
 #include <asm/byteorder.h>

 typedef int word_type __attribute__ ((mode (__word__)));
+typedef int TItype __attribute__ ((mode (TI)));

 #ifdef __BIG_ENDIAN
 struct DWstruct {
 	int high, low;
 };
+
+struct DWstruct128 {
+	long long high, low;
+};
+
 #elif defined(__LITTLE_ENDIAN)
 struct DWstruct {
 	int low, high;
 };
+
+struct DWstruct128 {
+	long long low, high;
+};
+
 #else
 #error I feel sick.
 #endif
@@ -40,4 +51,9 @@ typedef union {
 	long long ll;
 } DWunion;

+typedef union {
+	struct DWstruct128 s;
+	TItype ll;
+} DWunion128;
+
 #endif /* __ASM_LIBGCC_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index a9e56539bd11..369e10259ea6 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
 config GENERIC_LIB_LSHRDI3
 	bool

+config GENERIC_LIB_LSHRTI3
+	bool
+
 config GENERIC_LIB_MULDI3
 	bool

diff --git a/lib/Makefile b/lib/Makefile
index 4e066120a0d6..42648411f451 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
 obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
 obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
 obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
+obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
 obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
 obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
 obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
diff --git a/lib/lshrti3.c b/lib/lshrti3.c
new file mode 100644
index 000000000000..2d2123bb3030
--- /dev/null
+++ b/lib/lshrti3.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/export.h>
+#include <linux/libgcc.h>
+
+long long __lshrti3(long long u, word_type b)
+{
+	DWunion128 uu, w;
+	word_type bm;
+
+	if (b == 0)
+		return u;
+
+	uu.ll = u;
+	bm = 64 - b;
+
+	if (bm <= 0) {
+		w.s.high = 0;
+		w.s.low = (unsigned long long) uu.s.high >> -bm;
+	} else {
+		const unsigned long long carries =
+			(unsigned long long) uu.s.high << bm;
+		w.s.high = (unsigned long long) uu.s.high >> b;
+		w.s.low = ((unsigned long long) uu.s.low >> b) | carries;
+	}
+
+	return w.ll;
+}
+#ifndef BUILD_VDSO
+EXPORT_SYMBOL(__lshrti3);
+#endif
-- 
2.21.0.360.g471c308f928-goog

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 20:54 [PATCH] lib: Add shared copy of __lshrti3 from libgcc Matthias Kaehlcke
@ 2019-03-15 22:06 ` Nick Desaulniers
  2019-03-15 22:08   ` hpa
                     ` (3 more replies)
  2019-03-18 21:31 ` Matthias Kaehlcke
  1 sibling, 4 replies; 21+ messages in thread
From: Nick Desaulniers @ 2019-03-15 22:06 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> The compiler may emit calls to __lshrti3 from the compiler runtime
> library, which results in undefined references:
>
> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>   include/linux/math64.h:186: undefined reference to `__lshrti3'

Looks like Clang will emit this at -Oz (but not -O2):
https://godbolt.org/z/w1_2YC

>
> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).

Has it changed since? If so why not a newer version of libgcc_s?

>
> Include the function for x86 builds with clang, which is the
> environment where the above error was observed.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  arch/x86/Kconfig       |  1 +
>  include/linux/libgcc.h | 16 ++++++++++++++++
>  lib/Kconfig            |  3 +++
>  lib/Makefile           |  1 +
>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
>  5 files changed, 52 insertions(+)
>  create mode 100644 lib/lshrti3.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c1f9b3cf437c..a5e0d923845d 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -105,6 +105,7 @@ config X86
>         select GENERIC_IRQ_PROBE
>         select GENERIC_IRQ_RESERVATION_MODE
>         select GENERIC_IRQ_SHOW
> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
>         select GENERIC_PENDING_IRQ              if SMP
>         select GENERIC_SMP_IDLE_THREAD
>         select GENERIC_STRNCPY_FROM_USER
> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> index 32e1e0f4b2d0..a71036471838 100644
> --- a/include/linux/libgcc.h
> +++ b/include/linux/libgcc.h
> @@ -22,15 +22,26 @@
>  #include <asm/byteorder.h>
>
>  typedef int word_type __attribute__ ((mode (__word__)));
> +typedef int TItype __attribute__ ((mode (TI)));

Well that's an interesting new compiler attribute.
https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
typedef int TItype __mode(TI);

>
>  #ifdef __BIG_ENDIAN
>  struct DWstruct {
>         int high, low;
>  };
> +
> +struct DWstruct128 {
> +       long long high, low;
> +};
> +
>  #elif defined(__LITTLE_ENDIAN)
>  struct DWstruct {
>         int low, high;
>  };
> +
> +struct DWstruct128 {
> +       long long low, high;
> +};
> +
>  #else
>  #error I feel sick.
>  #endif
> @@ -40,4 +51,9 @@ typedef union {
>         long long ll;
>  } DWunion;
>
> +typedef union {
> +       struct DWstruct128 s;
> +       TItype ll;
> +} DWunion128;
> +
>  #endif /* __ASM_LIBGCC_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index a9e56539bd11..369e10259ea6 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
>  config GENERIC_LIB_LSHRDI3
>         bool
>
> +config GENERIC_LIB_LSHRTI3
> +       bool
> +
>  config GENERIC_LIB_MULDI3
>         bool
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 4e066120a0d6..42648411f451 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> new file mode 100644
> index 000000000000..2d2123bb3030
> --- /dev/null
> +++ b/lib/lshrti3.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/export.h>
> +#include <linux/libgcc.h>
> +
> +long long __lshrti3(long long u, word_type b)
> +{
> +       DWunion128 uu, w;
> +       word_type bm;
> +
> +       if (b == 0)
> +               return u;
> +
> +       uu.ll = u;
> +       bm = 64 - b;
> +
> +       if (bm <= 0) {
> +               w.s.high = 0;
> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
> +       } else {
> +               const unsigned long long carries =
> +                       (unsigned long long) uu.s.high << bm;
> +               w.s.high = (unsigned long long) uu.s.high >> b;
> +               w.s.low = ((unsigned long long) uu.s.low >> b) | carries;
> +       }
> +
> +       return w.ll;
> +}
> +#ifndef BUILD_VDSO
> +EXPORT_SYMBOL(__lshrti3);
> +#endif

I don't think you want this.  Maybe that was carried over from
https://lkml.org/lkml/2019/3/15/669
by accident?  The above linkage error mentions arch/x86/kvm/x86.o
which I wouldn't expect to be linked into the VDSO image?
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 22:06 ` Nick Desaulniers
@ 2019-03-15 22:08   ` hpa
  2019-03-15 23:34     ` Matthias Kaehlcke
  2019-03-15 22:12   ` hpa
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 21+ messages in thread
From: hpa @ 2019-03-15 22:08 UTC (permalink / raw)
  To: Nick Desaulniers, Matthias Kaehlcke
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, LKML, Manoj Gupta, Tiancong Wang, Stephen Hines,
	clang-built-linux, Miguel Ojeda

On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers <ndesaulniers@google.com> wrote:
>On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
>wrote:
>>
>> The compiler may emit calls to __lshrti3 from the compiler runtime
>> library, which results in undefined references:
>>
>> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>>   include/linux/math64.h:186: undefined reference to `__lshrti3'
>
>Looks like Clang will emit this at -Oz (but not -O2):
>https://godbolt.org/z/w1_2YC
>
>>
>> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>
>Has it changed since? If so why not a newer version of libgcc_s?
>
>>
>> Include the function for x86 builds with clang, which is the
>> environment where the above error was observed.
>>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  arch/x86/Kconfig       |  1 +
>>  include/linux/libgcc.h | 16 ++++++++++++++++
>>  lib/Kconfig            |  3 +++
>>  lib/Makefile           |  1 +
>>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
>>  5 files changed, 52 insertions(+)
>>  create mode 100644 lib/lshrti3.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index c1f9b3cf437c..a5e0d923845d 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -105,6 +105,7 @@ config X86
>>         select GENERIC_IRQ_PROBE
>>         select GENERIC_IRQ_RESERVATION_MODE
>>         select GENERIC_IRQ_SHOW
>> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
>>         select GENERIC_PENDING_IRQ              if SMP
>>         select GENERIC_SMP_IDLE_THREAD
>>         select GENERIC_STRNCPY_FROM_USER
>> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> index 32e1e0f4b2d0..a71036471838 100644
>> --- a/include/linux/libgcc.h
>> +++ b/include/linux/libgcc.h
>> @@ -22,15 +22,26 @@
>>  #include <asm/byteorder.h>
>>
>>  typedef int word_type __attribute__ ((mode (__word__)));
>> +typedef int TItype __attribute__ ((mode (TI)));
>
>Well that's an interesting new compiler attribute.
>https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
>Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
>typedef int TItype __mode(TI);
>
>>
>>  #ifdef __BIG_ENDIAN
>>  struct DWstruct {
>>         int high, low;
>>  };
>> +
>> +struct DWstruct128 {
>> +       long long high, low;
>> +};
>> +
>>  #elif defined(__LITTLE_ENDIAN)
>>  struct DWstruct {
>>         int low, high;
>>  };
>> +
>> +struct DWstruct128 {
>> +       long long low, high;
>> +};
>> +
>>  #else
>>  #error I feel sick.
>>  #endif
>> @@ -40,4 +51,9 @@ typedef union {
>>         long long ll;
>>  } DWunion;
>>
>> +typedef union {
>> +       struct DWstruct128 s;
>> +       TItype ll;
>> +} DWunion128;
>> +
>>  #endif /* __ASM_LIBGCC_H */
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index a9e56539bd11..369e10259ea6 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
>>  config GENERIC_LIB_LSHRDI3
>>         bool
>>
>> +config GENERIC_LIB_LSHRTI3
>> +       bool
>> +
>>  config GENERIC_LIB_MULDI3
>>         bool
>>
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 4e066120a0d6..42648411f451 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
>>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
>>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
>>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
>> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
>>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
>>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
>>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
>> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> new file mode 100644
>> index 000000000000..2d2123bb3030
>> --- /dev/null
>> +++ b/lib/lshrti3.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/export.h>
>> +#include <linux/libgcc.h>
>> +
>> +long long __lshrti3(long long u, word_type b)
>> +{
>> +       DWunion128 uu, w;
>> +       word_type bm;
>> +
>> +       if (b == 0)
>> +               return u;
>> +
>> +       uu.ll = u;
>> +       bm = 64 - b;
>> +
>> +       if (bm <= 0) {
>> +               w.s.high = 0;
>> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
>> +       } else {
>> +               const unsigned long long carries =
>> +                       (unsigned long long) uu.s.high << bm;
>> +               w.s.high = (unsigned long long) uu.s.high >> b;
>> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
>carries;
>> +       }
>> +
>> +       return w.ll;
>> +}
>> +#ifndef BUILD_VDSO
>> +EXPORT_SYMBOL(__lshrti3);
>> +#endif
>
>I don't think you want this.  Maybe that was carried over from
>https://lkml.org/lkml/2019/3/15/669
>by accident?  The above linkage error mentions arch/x86/kvm/x86.o
>which I wouldn't expect to be linked into the VDSO image?

If we compile with the default ABI we can simply link with libgcc; with -mregparm=3 all versions of gcc are broken.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 22:06 ` Nick Desaulniers
  2019-03-15 22:08   ` hpa
@ 2019-03-15 22:12   ` hpa
  2019-03-15 23:47     ` Matthias Kaehlcke
  2019-03-15 23:29   ` Matthias Kaehlcke
  2019-03-18  9:14   ` Peter Zijlstra
  3 siblings, 1 reply; 21+ messages in thread
From: hpa @ 2019-03-15 22:12 UTC (permalink / raw)
  To: Nick Desaulniers, Matthias Kaehlcke
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, LKML, Manoj Gupta, Tiancong Wang, Stephen Hines,
	clang-built-linux, Miguel Ojeda

On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers <ndesaulniers@google.com> wrote:
>On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
>wrote:
>>
>> The compiler may emit calls to __lshrti3 from the compiler runtime
>> library, which results in undefined references:
>>
>> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>>   include/linux/math64.h:186: undefined reference to `__lshrti3'
>
>Looks like Clang will emit this at -Oz (but not -O2):
>https://godbolt.org/z/w1_2YC
>
>>
>> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>
>Has it changed since? If so why not a newer version of libgcc_s?
>
>>
>> Include the function for x86 builds with clang, which is the
>> environment where the above error was observed.
>>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  arch/x86/Kconfig       |  1 +
>>  include/linux/libgcc.h | 16 ++++++++++++++++
>>  lib/Kconfig            |  3 +++
>>  lib/Makefile           |  1 +
>>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
>>  5 files changed, 52 insertions(+)
>>  create mode 100644 lib/lshrti3.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index c1f9b3cf437c..a5e0d923845d 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -105,6 +105,7 @@ config X86
>>         select GENERIC_IRQ_PROBE
>>         select GENERIC_IRQ_RESERVATION_MODE
>>         select GENERIC_IRQ_SHOW
>> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
>>         select GENERIC_PENDING_IRQ              if SMP
>>         select GENERIC_SMP_IDLE_THREAD
>>         select GENERIC_STRNCPY_FROM_USER
>> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> index 32e1e0f4b2d0..a71036471838 100644
>> --- a/include/linux/libgcc.h
>> +++ b/include/linux/libgcc.h
>> @@ -22,15 +22,26 @@
>>  #include <asm/byteorder.h>
>>
>>  typedef int word_type __attribute__ ((mode (__word__)));
>> +typedef int TItype __attribute__ ((mode (TI)));
>
>Well that's an interesting new compiler attribute.
>https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
>Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
>typedef int TItype __mode(TI);
>
>>
>>  #ifdef __BIG_ENDIAN
>>  struct DWstruct {
>>         int high, low;
>>  };
>> +
>> +struct DWstruct128 {
>> +       long long high, low;
>> +};
>> +
>>  #elif defined(__LITTLE_ENDIAN)
>>  struct DWstruct {
>>         int low, high;
>>  };
>> +
>> +struct DWstruct128 {
>> +       long long low, high;
>> +};
>> +
>>  #else
>>  #error I feel sick.
>>  #endif
>> @@ -40,4 +51,9 @@ typedef union {
>>         long long ll;
>>  } DWunion;
>>
>> +typedef union {
>> +       struct DWstruct128 s;
>> +       TItype ll;
>> +} DWunion128;
>> +
>>  #endif /* __ASM_LIBGCC_H */
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index a9e56539bd11..369e10259ea6 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
>>  config GENERIC_LIB_LSHRDI3
>>         bool
>>
>> +config GENERIC_LIB_LSHRTI3
>> +       bool
>> +
>>  config GENERIC_LIB_MULDI3
>>         bool
>>
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 4e066120a0d6..42648411f451 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
>>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
>>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
>>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
>> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
>>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
>>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
>>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
>> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> new file mode 100644
>> index 000000000000..2d2123bb3030
>> --- /dev/null
>> +++ b/lib/lshrti3.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/export.h>
>> +#include <linux/libgcc.h>
>> +
>> +long long __lshrti3(long long u, word_type b)
>> +{
>> +       DWunion128 uu, w;
>> +       word_type bm;
>> +
>> +       if (b == 0)
>> +               return u;
>> +
>> +       uu.ll = u;
>> +       bm = 64 - b;
>> +
>> +       if (bm <= 0) {
>> +               w.s.high = 0;
>> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
>> +       } else {
>> +               const unsigned long long carries =
>> +                       (unsigned long long) uu.s.high << bm;
>> +               w.s.high = (unsigned long long) uu.s.high >> b;
>> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
>carries;
>> +       }
>> +
>> +       return w.ll;
>> +}
>> +#ifndef BUILD_VDSO
>> +EXPORT_SYMBOL(__lshrti3);
>> +#endif
>
>I don't think you want this.  Maybe that was carried over from
>https://lkml.org/lkml/2019/3/15/669
>by accident?  The above linkage error mentions arch/x86/kvm/x86.o
>which I wouldn't expect to be linked into the VDSO image?

Or just "u64"...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 22:06 ` Nick Desaulniers
  2019-03-15 22:08   ` hpa
  2019-03-15 22:12   ` hpa
@ 2019-03-15 23:29   ` Matthias Kaehlcke
  2019-03-18  9:14   ` Peter Zijlstra
  3 siblings, 0 replies; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-15 23:29 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

On Fri, Mar 15, 2019 at 03:06:37PM -0700, 'Nick Desaulniers' via Clang Built Linux wrote:
> On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > The compiler may emit calls to __lshrti3 from the compiler runtime
> > library, which results in undefined references:
> >
> > arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >   include/linux/math64.h:186: undefined reference to `__lshrti3'
> 
> Looks like Clang will emit this at -Oz (but not -O2):
> https://godbolt.org/z/w1_2YC
> 
> >
> > Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> 
> Has it changed since? If so why not a newer version of libgcc_s?

Our compiler folks who maintain this gcc version dug this up for
me. In the gcc sources there is no direct implementation of __lshrti3,
I was told it is somehow derived from __lshrdi3.

> > Include the function for x86 builds with clang, which is the
> > environment where the above error was observed.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  arch/x86/Kconfig       |  1 +
> >  include/linux/libgcc.h | 16 ++++++++++++++++
> >  lib/Kconfig            |  3 +++
> >  lib/Makefile           |  1 +
> >  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
> >  5 files changed, 52 insertions(+)
> >  create mode 100644 lib/lshrti3.c
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index c1f9b3cf437c..a5e0d923845d 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -105,6 +105,7 @@ config X86
> >         select GENERIC_IRQ_PROBE
> >         select GENERIC_IRQ_RESERVATION_MODE
> >         select GENERIC_IRQ_SHOW
> > +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
> >         select GENERIC_PENDING_IRQ              if SMP
> >         select GENERIC_SMP_IDLE_THREAD
> >         select GENERIC_STRNCPY_FROM_USER
> > diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> > index 32e1e0f4b2d0..a71036471838 100644
> > --- a/include/linux/libgcc.h
> > +++ b/include/linux/libgcc.h
> > @@ -22,15 +22,26 @@
> >  #include <asm/byteorder.h>
> >
> >  typedef int word_type __attribute__ ((mode (__word__)));
> > +typedef int TItype __attribute__ ((mode (TI)));
> 
> Well that's an interesting new compiler attribute.
> https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
> Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
> typedef int TItype __mode(TI);

ok

> >  #ifdef __BIG_ENDIAN
> >  struct DWstruct {
> >         int high, low;
> >  };
> > +
> > +struct DWstruct128 {
> > +       long long high, low;
> > +};
> > +
> >  #elif defined(__LITTLE_ENDIAN)
> >  struct DWstruct {
> >         int low, high;
> >  };
> > +
> > +struct DWstruct128 {
> > +       long long low, high;
> > +};
> > +
> >  #else
> >  #error I feel sick.
> >  #endif
> > @@ -40,4 +51,9 @@ typedef union {
> >         long long ll;
> >  } DWunion;
> >
> > +typedef union {
> > +       struct DWstruct128 s;
> > +       TItype ll;
> > +} DWunion128;
> > +
> >  #endif /* __ASM_LIBGCC_H */
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index a9e56539bd11..369e10259ea6 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
> >  config GENERIC_LIB_LSHRDI3
> >         bool
> >
> > +config GENERIC_LIB_LSHRTI3
> > +       bool
> > +
> >  config GENERIC_LIB_MULDI3
> >         bool
> >
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 4e066120a0d6..42648411f451 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
> >  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
> >  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
> >  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
> > +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
> >  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> >  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> >  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> > diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> > new file mode 100644
> > index 000000000000..2d2123bb3030
> > --- /dev/null
> > +++ b/lib/lshrti3.c
> > @@ -0,0 +1,31 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/export.h>
> > +#include <linux/libgcc.h>
> > +
> > +long long __lshrti3(long long u, word_type b)

This signature matches with the gcc documentation
(https://gcc.gnu.org/onlinedocs/gccint/Integer-library-routines.html),
however the gcc implementation of the function has 128-bit values as
input/output, so I guess to meet gcc's expectations the 'long long's
need to be changed to TItype.

> > +{
> > +       DWunion128 uu, w;
> > +       word_type bm;
> > +
> > +       if (b == 0)
> > +               return u;
> > +
> > +       uu.ll = u;
> > +       bm = 64 - b;
> > +
> > +       if (bm <= 0) {
> > +               w.s.high = 0;
> > +               w.s.low = (unsigned long long) uu.s.high >> -bm;
> > +       } else {
> > +               const unsigned long long carries =
> > +                       (unsigned long long) uu.s.high << bm;
> > +               w.s.high = (unsigned long long) uu.s.high >> b;
> > +               w.s.low = ((unsigned long long) uu.s.low >> b) | carries;
> > +       }
> > +
> > +       return w.ll;
> > +}
> > +#ifndef BUILD_VDSO
> > +EXPORT_SYMBOL(__lshrti3);
> > +#endif
> 
> I don't think you want this.  Maybe that was carried over from
> https://lkml.org/lkml/2019/3/15/669
> by accident?  The above linkage error mentions arch/x86/kvm/x86.o
> which I wouldn't expect to be linked into the VDSO image?

I put it preemptively, since I know from
https://lkml.org/lkml/2019/3/15/669 that the vDSO build is unhappy
about EXPORT_SYMBOL. Obviously we can also wait until a vDSO needs to
include this file.

Thanks

Matthias

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 22:08   ` hpa
@ 2019-03-15 23:34     ` Matthias Kaehlcke
  2019-03-15 23:51       ` hpa
  0 siblings, 1 reply; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-15 23:34 UTC (permalink / raw)
  To: hpa
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

Hi Peter,

On Fri, Mar 15, 2019 at 03:08:57PM -0700, hpa@zytor.com wrote:
> On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
> >wrote:
> >>
> >> The compiler may emit calls to __lshrti3 from the compiler runtime
> >> library, which results in undefined references:
> >>
> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
> >
> >Looks like Clang will emit this at -Oz (but not -O2):
> >https://godbolt.org/z/w1_2YC
> >
> >>
> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> >
> >Has it changed since? If so why not a newer version of libgcc_s?
> >
> >>
> >> Include the function for x86 builds with clang, which is the
> >> environment where the above error was observed.
> >>
> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >>  arch/x86/Kconfig       |  1 +
> >>  include/linux/libgcc.h | 16 ++++++++++++++++
> >>  lib/Kconfig            |  3 +++
> >>  lib/Makefile           |  1 +
> >>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
> >>  5 files changed, 52 insertions(+)
> >>  create mode 100644 lib/lshrti3.c
> >>
> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> index c1f9b3cf437c..a5e0d923845d 100644
> >> --- a/arch/x86/Kconfig
> >> +++ b/arch/x86/Kconfig
> >> @@ -105,6 +105,7 @@ config X86
> >>         select GENERIC_IRQ_PROBE
> >>         select GENERIC_IRQ_RESERVATION_MODE
> >>         select GENERIC_IRQ_SHOW
> >> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
> >>         select GENERIC_PENDING_IRQ              if SMP
> >>         select GENERIC_SMP_IDLE_THREAD
> >>         select GENERIC_STRNCPY_FROM_USER
> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> >> index 32e1e0f4b2d0..a71036471838 100644
> >> --- a/include/linux/libgcc.h
> >> +++ b/include/linux/libgcc.h
> >> @@ -22,15 +22,26 @@
> >>  #include <asm/byteorder.h>
> >>
> >>  typedef int word_type __attribute__ ((mode (__word__)));
> >> +typedef int TItype __attribute__ ((mode (TI)));
> >
> >Well that's an interesting new compiler attribute.
> >https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
> >Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
> >typedef int TItype __mode(TI);
> >
> >>
> >>  #ifdef __BIG_ENDIAN
> >>  struct DWstruct {
> >>         int high, low;
> >>  };
> >> +
> >> +struct DWstruct128 {
> >> +       long long high, low;
> >> +};
> >> +
> >>  #elif defined(__LITTLE_ENDIAN)
> >>  struct DWstruct {
> >>         int low, high;
> >>  };
> >> +
> >> +struct DWstruct128 {
> >> +       long long low, high;
> >> +};
> >> +
> >>  #else
> >>  #error I feel sick.
> >>  #endif
> >> @@ -40,4 +51,9 @@ typedef union {
> >>         long long ll;
> >>  } DWunion;
> >>
> >> +typedef union {
> >> +       struct DWstruct128 s;
> >> +       TItype ll;
> >> +} DWunion128;
> >> +
> >>  #endif /* __ASM_LIBGCC_H */
> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> index a9e56539bd11..369e10259ea6 100644
> >> --- a/lib/Kconfig
> >> +++ b/lib/Kconfig
> >> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
> >>  config GENERIC_LIB_LSHRDI3
> >>         bool
> >>
> >> +config GENERIC_LIB_LSHRTI3
> >> +       bool
> >> +
> >>  config GENERIC_LIB_MULDI3
> >>         bool
> >>
> >> diff --git a/lib/Makefile b/lib/Makefile
> >> index 4e066120a0d6..42648411f451 100644
> >> --- a/lib/Makefile
> >> +++ b/lib/Makefile
> >> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
> >>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
> >>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
> >>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
> >> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
> >>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> >>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> >>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> >> new file mode 100644
> >> index 000000000000..2d2123bb3030
> >> --- /dev/null
> >> +++ b/lib/lshrti3.c
> >> @@ -0,0 +1,31 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/export.h>
> >> +#include <linux/libgcc.h>
> >> +
> >> +long long __lshrti3(long long u, word_type b)
> >> +{
> >> +       DWunion128 uu, w;
> >> +       word_type bm;
> >> +
> >> +       if (b == 0)
> >> +               return u;
> >> +
> >> +       uu.ll = u;
> >> +       bm = 64 - b;
> >> +
> >> +       if (bm <= 0) {
> >> +               w.s.high = 0;
> >> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
> >> +       } else {
> >> +               const unsigned long long carries =
> >> +                       (unsigned long long) uu.s.high << bm;
> >> +               w.s.high = (unsigned long long) uu.s.high >> b;
> >> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
> >carries;
> >> +       }
> >> +
> >> +       return w.ll;
> >> +}
> >> +#ifndef BUILD_VDSO
> >> +EXPORT_SYMBOL(__lshrti3);
> >> +#endif
> >
> >I don't think you want this.  Maybe that was carried over from
> >https://lkml.org/lkml/2019/3/15/669
> >by accident?  The above linkage error mentions arch/x86/kvm/x86.o
> >which I wouldn't expect to be linked into the VDSO image?
> 
> If we compile with the default ABI we can simply link with libgcc;
> with -mregparm=3 all versions of gcc are broken.

I suppose you refer to linking the VDSO against libgcc? Would this be
acceptable (as opposed to linking the kernel against it)?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 22:12   ` hpa
@ 2019-03-15 23:47     ` Matthias Kaehlcke
  2019-03-15 23:53       ` hpa
  0 siblings, 1 reply; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-15 23:47 UTC (permalink / raw)
  To: hpa
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

On Fri, Mar 15, 2019 at 03:12:08PM -0700, hpa@zytor.com wrote:
> On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
> >wrote:
> >>
> >> The compiler may emit calls to __lshrti3 from the compiler runtime
> >> library, which results in undefined references:
> >>
> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
> >
> >Looks like Clang will emit this at -Oz (but not -O2):
> >https://godbolt.org/z/w1_2YC
> >
> >>
> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> >
> >Has it changed since? If so why not a newer version of libgcc_s?
> >
> >>
> >> Include the function for x86 builds with clang, which is the
> >> environment where the above error was observed.
> >>
> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >>  arch/x86/Kconfig       |  1 +
> >>  include/linux/libgcc.h | 16 ++++++++++++++++
> >>  lib/Kconfig            |  3 +++
> >>  lib/Makefile           |  1 +
> >>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
> >>  5 files changed, 52 insertions(+)
> >>  create mode 100644 lib/lshrti3.c
> >>
> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> index c1f9b3cf437c..a5e0d923845d 100644
> >> --- a/arch/x86/Kconfig
> >> +++ b/arch/x86/Kconfig
> >> @@ -105,6 +105,7 @@ config X86
> >>         select GENERIC_IRQ_PROBE
> >>         select GENERIC_IRQ_RESERVATION_MODE
> >>         select GENERIC_IRQ_SHOW
> >> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
> >>         select GENERIC_PENDING_IRQ              if SMP
> >>         select GENERIC_SMP_IDLE_THREAD
> >>         select GENERIC_STRNCPY_FROM_USER
> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> >> index 32e1e0f4b2d0..a71036471838 100644
> >> --- a/include/linux/libgcc.h
> >> +++ b/include/linux/libgcc.h
> >> @@ -22,15 +22,26 @@
> >>  #include <asm/byteorder.h>
> >>
> >>  typedef int word_type __attribute__ ((mode (__word__)));
> >> +typedef int TItype __attribute__ ((mode (TI)));
> >
> >Well that's an interesting new compiler attribute.
> >https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
> >Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
> >typedef int TItype __mode(TI);
> >
> >>
> >>  #ifdef __BIG_ENDIAN
> >>  struct DWstruct {
> >>         int high, low;
> >>  };
> >> +
> >> +struct DWstruct128 {
> >> +       long long high, low;
> >> +};
> >> +
> >>  #elif defined(__LITTLE_ENDIAN)
> >>  struct DWstruct {
> >>         int low, high;
> >>  };
> >> +
> >> +struct DWstruct128 {
> >> +       long long low, high;
> >> +};
> >> +
> >>  #else
> >>  #error I feel sick.
> >>  #endif
> >> @@ -40,4 +51,9 @@ typedef union {
> >>         long long ll;
> >>  } DWunion;
> >>
> >> +typedef union {
> >> +       struct DWstruct128 s;
> >> +       TItype ll;
> >> +} DWunion128;
> >> +
> >>  #endif /* __ASM_LIBGCC_H */
> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> index a9e56539bd11..369e10259ea6 100644
> >> --- a/lib/Kconfig
> >> +++ b/lib/Kconfig
> >> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
> >>  config GENERIC_LIB_LSHRDI3
> >>         bool
> >>
> >> +config GENERIC_LIB_LSHRTI3
> >> +       bool
> >> +
> >>  config GENERIC_LIB_MULDI3
> >>         bool
> >>
> >> diff --git a/lib/Makefile b/lib/Makefile
> >> index 4e066120a0d6..42648411f451 100644
> >> --- a/lib/Makefile
> >> +++ b/lib/Makefile
> >> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
> >>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
> >>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
> >>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
> >> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
> >>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> >>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> >>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> >> new file mode 100644
> >> index 000000000000..2d2123bb3030
> >> --- /dev/null
> >> +++ b/lib/lshrti3.c
> >> @@ -0,0 +1,31 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/export.h>
> >> +#include <linux/libgcc.h>
> >> +
> >> +long long __lshrti3(long long u, word_type b)
> >> +{
> >> +       DWunion128 uu, w;
> >> +       word_type bm;
> >> +
> >> +       if (b == 0)
> >> +               return u;
> >> +
> >> +       uu.ll = u;
> >> +       bm = 64 - b;
> >> +
> >> +       if (bm <= 0) {
> >> +               w.s.high = 0;
> >> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
> >> +       } else {
> >> +               const unsigned long long carries =
> >> +                       (unsigned long long) uu.s.high << bm;
> >> +               w.s.high = (unsigned long long) uu.s.high >> b;
> >> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
> >carries;
> >> +       }
> >> +
> >> +       return w.ll;
> >> +}
> >> +#ifndef BUILD_VDSO
> >> +EXPORT_SYMBOL(__lshrti3);
> >> +#endif
> >
> >I don't think you want this.  Maybe that was carried over from
> >https://lkml.org/lkml/2019/3/15/669
> >by accident?  The above linkage error mentions arch/x86/kvm/x86.o
> >which I wouldn't expect to be linked into the VDSO image?
> 
> Or just "u64"...

Are you referring to TItype? It currenty is a 128-bit value.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 23:34     ` Matthias Kaehlcke
@ 2019-03-15 23:51       ` hpa
  0 siblings, 0 replies; 21+ messages in thread
From: hpa @ 2019-03-15 23:51 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

On March 15, 2019 4:34:10 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>Hi Peter,
>
>On Fri, Mar 15, 2019 at 03:08:57PM -0700, hpa@zytor.com wrote:
>> On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers
><ndesaulniers@google.com> wrote:
>> >On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
>> >wrote:
>> >>
>> >> The compiler may emit calls to __lshrti3 from the compiler runtime
>> >> library, which results in undefined references:
>> >>
>> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
>> >
>> >Looks like Clang will emit this at -Oz (but not -O2):
>> >https://godbolt.org/z/w1_2YC
>> >
>> >>
>> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>> >
>> >Has it changed since? If so why not a newer version of libgcc_s?
>> >
>> >>
>> >> Include the function for x86 builds with clang, which is the
>> >> environment where the above error was observed.
>> >>
>> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> >> ---
>> >>  arch/x86/Kconfig       |  1 +
>> >>  include/linux/libgcc.h | 16 ++++++++++++++++
>> >>  lib/Kconfig            |  3 +++
>> >>  lib/Makefile           |  1 +
>> >>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
>> >>  5 files changed, 52 insertions(+)
>> >>  create mode 100644 lib/lshrti3.c
>> >>
>> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> >> index c1f9b3cf437c..a5e0d923845d 100644
>> >> --- a/arch/x86/Kconfig
>> >> +++ b/arch/x86/Kconfig
>> >> @@ -105,6 +105,7 @@ config X86
>> >>         select GENERIC_IRQ_PROBE
>> >>         select GENERIC_IRQ_RESERVATION_MODE
>> >>         select GENERIC_IRQ_SHOW
>> >> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
>> >>         select GENERIC_PENDING_IRQ              if SMP
>> >>         select GENERIC_SMP_IDLE_THREAD
>> >>         select GENERIC_STRNCPY_FROM_USER
>> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> >> index 32e1e0f4b2d0..a71036471838 100644
>> >> --- a/include/linux/libgcc.h
>> >> +++ b/include/linux/libgcc.h
>> >> @@ -22,15 +22,26 @@
>> >>  #include <asm/byteorder.h>
>> >>
>> >>  typedef int word_type __attribute__ ((mode (__word__)));
>> >> +typedef int TItype __attribute__ ((mode (TI)));
>> >
>> >Well that's an interesting new compiler attribute.
>>
>>https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
>> >Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
>> >typedef int TItype __mode(TI);
>> >
>> >>
>> >>  #ifdef __BIG_ENDIAN
>> >>  struct DWstruct {
>> >>         int high, low;
>> >>  };
>> >> +
>> >> +struct DWstruct128 {
>> >> +       long long high, low;
>> >> +};
>> >> +
>> >>  #elif defined(__LITTLE_ENDIAN)
>> >>  struct DWstruct {
>> >>         int low, high;
>> >>  };
>> >> +
>> >> +struct DWstruct128 {
>> >> +       long long low, high;
>> >> +};
>> >> +
>> >>  #else
>> >>  #error I feel sick.
>> >>  #endif
>> >> @@ -40,4 +51,9 @@ typedef union {
>> >>         long long ll;
>> >>  } DWunion;
>> >>
>> >> +typedef union {
>> >> +       struct DWstruct128 s;
>> >> +       TItype ll;
>> >> +} DWunion128;
>> >> +
>> >>  #endif /* __ASM_LIBGCC_H */
>> >> diff --git a/lib/Kconfig b/lib/Kconfig
>> >> index a9e56539bd11..369e10259ea6 100644
>> >> --- a/lib/Kconfig
>> >> +++ b/lib/Kconfig
>> >> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
>> >>  config GENERIC_LIB_LSHRDI3
>> >>         bool
>> >>
>> >> +config GENERIC_LIB_LSHRTI3
>> >> +       bool
>> >> +
>> >>  config GENERIC_LIB_MULDI3
>> >>         bool
>> >>
>> >> diff --git a/lib/Makefile b/lib/Makefile
>> >> index 4e066120a0d6..42648411f451 100644
>> >> --- a/lib/Makefile
>> >> +++ b/lib/Makefile
>> >> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
>> >>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
>> >> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
>> >>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
>> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> >> new file mode 100644
>> >> index 000000000000..2d2123bb3030
>> >> --- /dev/null
>> >> +++ b/lib/lshrti3.c
>> >> @@ -0,0 +1,31 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +#include <linux/export.h>
>> >> +#include <linux/libgcc.h>
>> >> +
>> >> +long long __lshrti3(long long u, word_type b)
>> >> +{
>> >> +       DWunion128 uu, w;
>> >> +       word_type bm;
>> >> +
>> >> +       if (b == 0)
>> >> +               return u;
>> >> +
>> >> +       uu.ll = u;
>> >> +       bm = 64 - b;
>> >> +
>> >> +       if (bm <= 0) {
>> >> +               w.s.high = 0;
>> >> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
>> >> +       } else {
>> >> +               const unsigned long long carries =
>> >> +                       (unsigned long long) uu.s.high << bm;
>> >> +               w.s.high = (unsigned long long) uu.s.high >> b;
>> >> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
>> >carries;
>> >> +       }
>> >> +
>> >> +       return w.ll;
>> >> +}
>> >> +#ifndef BUILD_VDSO
>> >> +EXPORT_SYMBOL(__lshrti3);
>> >> +#endif
>> >
>> >I don't think you want this.  Maybe that was carried over from
>> >https://lkml.org/lkml/2019/3/15/669
>> >by accident?  The above linkage error mentions arch/x86/kvm/x86.o
>> >which I wouldn't expect to be linked into the VDSO image?
>> 
>> If we compile with the default ABI we can simply link with libgcc;
>> with -mregparm=3 all versions of gcc are broken.
>
>I suppose you refer to linking the VDSO against libgcc? Would this be
>acceptable (as opposed to linking the kernel against it)?

Yes.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 23:47     ` Matthias Kaehlcke
@ 2019-03-15 23:53       ` hpa
  2019-03-18 16:38         ` Matthias Kaehlcke
  0 siblings, 1 reply; 21+ messages in thread
From: hpa @ 2019-03-15 23:53 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

On March 15, 2019 4:47:01 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>On Fri, Mar 15, 2019 at 03:12:08PM -0700, hpa@zytor.com wrote:
>> On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers
><ndesaulniers@google.com> wrote:
>> >On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
>> >wrote:
>> >>
>> >> The compiler may emit calls to __lshrti3 from the compiler runtime
>> >> library, which results in undefined references:
>> >>
>> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
>> >
>> >Looks like Clang will emit this at -Oz (but not -O2):
>> >https://godbolt.org/z/w1_2YC
>> >
>> >>
>> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>> >
>> >Has it changed since? If so why not a newer version of libgcc_s?
>> >
>> >>
>> >> Include the function for x86 builds with clang, which is the
>> >> environment where the above error was observed.
>> >>
>> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> >> ---
>> >>  arch/x86/Kconfig       |  1 +
>> >>  include/linux/libgcc.h | 16 ++++++++++++++++
>> >>  lib/Kconfig            |  3 +++
>> >>  lib/Makefile           |  1 +
>> >>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
>> >>  5 files changed, 52 insertions(+)
>> >>  create mode 100644 lib/lshrti3.c
>> >>
>> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> >> index c1f9b3cf437c..a5e0d923845d 100644
>> >> --- a/arch/x86/Kconfig
>> >> +++ b/arch/x86/Kconfig
>> >> @@ -105,6 +105,7 @@ config X86
>> >>         select GENERIC_IRQ_PROBE
>> >>         select GENERIC_IRQ_RESERVATION_MODE
>> >>         select GENERIC_IRQ_SHOW
>> >> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
>> >>         select GENERIC_PENDING_IRQ              if SMP
>> >>         select GENERIC_SMP_IDLE_THREAD
>> >>         select GENERIC_STRNCPY_FROM_USER
>> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> >> index 32e1e0f4b2d0..a71036471838 100644
>> >> --- a/include/linux/libgcc.h
>> >> +++ b/include/linux/libgcc.h
>> >> @@ -22,15 +22,26 @@
>> >>  #include <asm/byteorder.h>
>> >>
>> >>  typedef int word_type __attribute__ ((mode (__word__)));
>> >> +typedef int TItype __attribute__ ((mode (TI)));
>> >
>> >Well that's an interesting new compiler attribute.
>>
>>https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
>> >Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
>> >typedef int TItype __mode(TI);
>> >
>> >>
>> >>  #ifdef __BIG_ENDIAN
>> >>  struct DWstruct {
>> >>         int high, low;
>> >>  };
>> >> +
>> >> +struct DWstruct128 {
>> >> +       long long high, low;
>> >> +};
>> >> +
>> >>  #elif defined(__LITTLE_ENDIAN)
>> >>  struct DWstruct {
>> >>         int low, high;
>> >>  };
>> >> +
>> >> +struct DWstruct128 {
>> >> +       long long low, high;
>> >> +};
>> >> +
>> >>  #else
>> >>  #error I feel sick.
>> >>  #endif
>> >> @@ -40,4 +51,9 @@ typedef union {
>> >>         long long ll;
>> >>  } DWunion;
>> >>
>> >> +typedef union {
>> >> +       struct DWstruct128 s;
>> >> +       TItype ll;
>> >> +} DWunion128;
>> >> +
>> >>  #endif /* __ASM_LIBGCC_H */
>> >> diff --git a/lib/Kconfig b/lib/Kconfig
>> >> index a9e56539bd11..369e10259ea6 100644
>> >> --- a/lib/Kconfig
>> >> +++ b/lib/Kconfig
>> >> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
>> >>  config GENERIC_LIB_LSHRDI3
>> >>         bool
>> >>
>> >> +config GENERIC_LIB_LSHRTI3
>> >> +       bool
>> >> +
>> >>  config GENERIC_LIB_MULDI3
>> >>         bool
>> >>
>> >> diff --git a/lib/Makefile b/lib/Makefile
>> >> index 4e066120a0d6..42648411f451 100644
>> >> --- a/lib/Makefile
>> >> +++ b/lib/Makefile
>> >> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
>> >>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
>> >> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
>> >>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
>> >>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
>> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> >> new file mode 100644
>> >> index 000000000000..2d2123bb3030
>> >> --- /dev/null
>> >> +++ b/lib/lshrti3.c
>> >> @@ -0,0 +1,31 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +#include <linux/export.h>
>> >> +#include <linux/libgcc.h>
>> >> +
>> >> +long long __lshrti3(long long u, word_type b)
>> >> +{
>> >> +       DWunion128 uu, w;
>> >> +       word_type bm;
>> >> +
>> >> +       if (b == 0)
>> >> +               return u;
>> >> +
>> >> +       uu.ll = u;
>> >> +       bm = 64 - b;
>> >> +
>> >> +       if (bm <= 0) {
>> >> +               w.s.high = 0;
>> >> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
>> >> +       } else {
>> >> +               const unsigned long long carries =
>> >> +                       (unsigned long long) uu.s.high << bm;
>> >> +               w.s.high = (unsigned long long) uu.s.high >> b;
>> >> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
>> >carries;
>> >> +       }
>> >> +
>> >> +       return w.ll;
>> >> +}
>> >> +#ifndef BUILD_VDSO
>> >> +EXPORT_SYMBOL(__lshrti3);
>> >> +#endif
>> >
>> >I don't think you want this.  Maybe that was carried over from
>> >https://lkml.org/lkml/2019/3/15/669
>> >by accident?  The above linkage error mentions arch/x86/kvm/x86.o
>> >which I wouldn't expect to be linked into the VDSO image?
>> 
>> Or just "u64"...
>
>Are you referring to TItype? It currenty is a 128-bit value.

You're right, so "unsigned __int128"...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 22:06 ` Nick Desaulniers
                     ` (2 preceding siblings ...)
  2019-03-15 23:29   ` Matthias Kaehlcke
@ 2019-03-18  9:14   ` Peter Zijlstra
  2019-03-18 14:43     ` David Laight
  3 siblings, 1 reply; 21+ messages in thread
From: Peter Zijlstra @ 2019-03-18  9:14 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Matthias Kaehlcke, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H . Peter Anvin, x86, LKML, Manoj Gupta,
	Tiancong Wang, Stephen Hines, clang-built-linux, Miguel Ojeda

On Fri, Mar 15, 2019 at 03:06:37PM -0700, Nick Desaulniers wrote:
> On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > The compiler may emit calls to __lshrti3 from the compiler runtime
> > library, which results in undefined references:
> >
> > arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >   include/linux/math64.h:186: undefined reference to `__lshrti3'
> 
> Looks like Clang will emit this at -Oz (but not -O2):
> https://godbolt.org/z/w1_2YC

*OMG*, what is that compiler smoking and why do we want that?

It doesn't even do that for "-Os".

So where "-Os" is "Optimize for Sadness" this "-Oz" thing is like a
downright depression. Please just take it out back. Don't enable crap
like this.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18  9:14   ` Peter Zijlstra
@ 2019-03-18 14:43     ` David Laight
  2019-03-18 14:54       ` Peter Zijlstra
  0 siblings, 1 reply; 21+ messages in thread
From: David Laight @ 2019-03-18 14:43 UTC (permalink / raw)
  To: 'Peter Zijlstra', Nick Desaulniers
  Cc: Matthias Kaehlcke, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H . Peter Anvin, x86, LKML, Manoj Gupta,
	Tiancong Wang, Stephen Hines, clang-built-linux, Miguel Ojeda

From: Peter Zijlstra
> Sent: 18 March 2019 09:14
> On Fri, Mar 15, 2019 at 03:06:37PM -0700, Nick Desaulniers wrote:
> > On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > The compiler may emit calls to __lshrti3 from the compiler runtime
> > > library, which results in undefined references:
> > >
> > > arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> > >   include/linux/math64.h:186: undefined reference to `__lshrti3'
> >
> > Looks like Clang will emit this at -Oz (but not -O2):
> > https://godbolt.org/z/w1_2YC
> 
> *OMG*, what is that compiler smoking and why do we want that?
> 
> It doesn't even do that for "-Os".

I like the way it moves %edx to %ecx, then %cl to %ecx and finally %ecx back to %edx.
I'm guessing this is all made worse by the prototype containing 'char' not 'int'.

I'm sure the register tracking gets worse in every version of gcc.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 14:43     ` David Laight
@ 2019-03-18 14:54       ` Peter Zijlstra
  0 siblings, 0 replies; 21+ messages in thread
From: Peter Zijlstra @ 2019-03-18 14:54 UTC (permalink / raw)
  To: David Laight
  Cc: Nick Desaulniers, Matthias Kaehlcke, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin,
	x86, LKML, Manoj Gupta, Tiancong Wang, Stephen Hines,
	clang-built-linux, Miguel Ojeda

On Mon, Mar 18, 2019 at 02:43:41PM +0000, David Laight wrote:
> From: Peter Zijlstra
> > Sent: 18 March 2019 09:14
> > On Fri, Mar 15, 2019 at 03:06:37PM -0700, Nick Desaulniers wrote:
> > > On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >
> > > > The compiler may emit calls to __lshrti3 from the compiler runtime
> > > > library, which results in undefined references:
> > > >
> > > > arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> > > >   include/linux/math64.h:186: undefined reference to `__lshrti3'
> > >
> > > Looks like Clang will emit this at -Oz (but not -O2):
> > > https://godbolt.org/z/w1_2YC
> > 
> > *OMG*, what is that compiler smoking and why do we want that?
> > 
> > It doesn't even do that for "-Os".
> 
> I like the way it moves %edx to %ecx, then %cl to %ecx and finally %ecx back to %edx.
> I'm guessing this is all made worse by the prototype containing 'char' not 'int'.
> 
> I'm sure the register tracking gets worse in every version of gcc.

This is clang, no GCC on that list comes even close to generating
anything as 'brilliant' as that.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 23:53       ` hpa
@ 2019-03-18 16:38         ` Matthias Kaehlcke
  0 siblings, 0 replies; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-18 16:38 UTC (permalink / raw)
  To: hpa
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, LKML, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux, Miguel Ojeda

On Fri, Mar 15, 2019 at 04:53:40PM -0700, hpa@zytor.com wrote:
> On March 15, 2019 4:47:01 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
> >On Fri, Mar 15, 2019 at 03:12:08PM -0700, hpa@zytor.com wrote:
> >> On March 15, 2019 3:06:37 PM PDT, Nick Desaulniers
> ><ndesaulniers@google.com> wrote:
> >> >On Fri, Mar 15, 2019 at 1:54 PM Matthias Kaehlcke <mka@chromium.org>
> >> >wrote:
> >> >>
> >> >> The compiler may emit calls to __lshrti3 from the compiler runtime
> >> >> library, which results in undefined references:
> >> >>
> >> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
> >> >
> >> >Looks like Clang will emit this at -Oz (but not -O2):
> >> >https://godbolt.org/z/w1_2YC
> >> >
> >> >>
> >> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> >> >
> >> >Has it changed since? If so why not a newer version of libgcc_s?
> >> >
> >> >>
> >> >> Include the function for x86 builds with clang, which is the
> >> >> environment where the above error was observed.
> >> >>
> >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> >> ---
> >> >>  arch/x86/Kconfig       |  1 +
> >> >>  include/linux/libgcc.h | 16 ++++++++++++++++
> >> >>  lib/Kconfig            |  3 +++
> >> >>  lib/Makefile           |  1 +
> >> >>  lib/lshrti3.c          | 31 +++++++++++++++++++++++++++++++
> >> >>  5 files changed, 52 insertions(+)
> >> >>  create mode 100644 lib/lshrti3.c
> >> >>
> >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> >> index c1f9b3cf437c..a5e0d923845d 100644
> >> >> --- a/arch/x86/Kconfig
> >> >> +++ b/arch/x86/Kconfig
> >> >> @@ -105,6 +105,7 @@ config X86
> >> >>         select GENERIC_IRQ_PROBE
> >> >>         select GENERIC_IRQ_RESERVATION_MODE
> >> >>         select GENERIC_IRQ_SHOW
> >> >> +       select GENERIC_LIB_LSHRTI3              if CC_IS_CLANG
> >> >>         select GENERIC_PENDING_IRQ              if SMP
> >> >>         select GENERIC_SMP_IDLE_THREAD
> >> >>         select GENERIC_STRNCPY_FROM_USER
> >> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> >> >> index 32e1e0f4b2d0..a71036471838 100644
> >> >> --- a/include/linux/libgcc.h
> >> >> +++ b/include/linux/libgcc.h
> >> >> @@ -22,15 +22,26 @@
> >> >>  #include <asm/byteorder.h>
> >> >>
> >> >>  typedef int word_type __attribute__ ((mode (__word__)));
> >> >> +typedef int TItype __attribute__ ((mode (TI)));
> >> >
> >> >Well that's an interesting new compiler attribute.
> >>
> >>https://stackoverflow.com/questions/4559025/what-does-gcc-attribute-modexx-actually-do
> >> >Please use `__mode(TI)` from include/linux/compiler_attributes.h ex.
> >> >typedef int TItype __mode(TI);
> >> >
> >> >>
> >> >>  #ifdef __BIG_ENDIAN
> >> >>  struct DWstruct {
> >> >>         int high, low;
> >> >>  };
> >> >> +
> >> >> +struct DWstruct128 {
> >> >> +       long long high, low;
> >> >> +};
> >> >> +
> >> >>  #elif defined(__LITTLE_ENDIAN)
> >> >>  struct DWstruct {
> >> >>         int low, high;
> >> >>  };
> >> >> +
> >> >> +struct DWstruct128 {
> >> >> +       long long low, high;
> >> >> +};
> >> >> +
> >> >>  #else
> >> >>  #error I feel sick.
> >> >>  #endif
> >> >> @@ -40,4 +51,9 @@ typedef union {
> >> >>         long long ll;
> >> >>  } DWunion;
> >> >>
> >> >> +typedef union {
> >> >> +       struct DWstruct128 s;
> >> >> +       TItype ll;
> >> >> +} DWunion128;
> >> >> +
> >> >>  #endif /* __ASM_LIBGCC_H */
> >> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> >> index a9e56539bd11..369e10259ea6 100644
> >> >> --- a/lib/Kconfig
> >> >> +++ b/lib/Kconfig
> >> >> @@ -624,6 +624,9 @@ config GENERIC_LIB_ASHRDI3
> >> >>  config GENERIC_LIB_LSHRDI3
> >> >>         bool
> >> >>
> >> >> +config GENERIC_LIB_LSHRTI3
> >> >> +       bool
> >> >> +
> >> >>  config GENERIC_LIB_MULDI3
> >> >>         bool
> >> >>
> >> >> diff --git a/lib/Makefile b/lib/Makefile
> >> >> index 4e066120a0d6..42648411f451 100644
> >> >> --- a/lib/Makefile
> >> >> +++ b/lib/Makefile
> >> >> @@ -277,6 +277,7 @@ obj-$(CONFIG_PARMAN) += parman.o
> >> >>  obj-$(CONFIG_GENERIC_LIB_ASHLDI3) += ashldi3.o
> >> >>  obj-$(CONFIG_GENERIC_LIB_ASHRDI3) += ashrdi3.o
> >> >>  obj-$(CONFIG_GENERIC_LIB_LSHRDI3) += lshrdi3.o
> >> >> +obj-$(CONFIG_GENERIC_LIB_LSHRTI3) += lshrti3.o
> >> >>  obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> >> >>  obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> >> >>  obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> >> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> >> >> new file mode 100644
> >> >> index 000000000000..2d2123bb3030
> >> >> --- /dev/null
> >> >> +++ b/lib/lshrti3.c
> >> >> @@ -0,0 +1,31 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +
> >> >> +#include <linux/export.h>
> >> >> +#include <linux/libgcc.h>
> >> >> +
> >> >> +long long __lshrti3(long long u, word_type b)
> >> >> +{
> >> >> +       DWunion128 uu, w;
> >> >> +       word_type bm;
> >> >> +
> >> >> +       if (b == 0)
> >> >> +               return u;
> >> >> +
> >> >> +       uu.ll = u;
> >> >> +       bm = 64 - b;
> >> >> +
> >> >> +       if (bm <= 0) {
> >> >> +               w.s.high = 0;
> >> >> +               w.s.low = (unsigned long long) uu.s.high >> -bm;
> >> >> +       } else {
> >> >> +               const unsigned long long carries =
> >> >> +                       (unsigned long long) uu.s.high << bm;
> >> >> +               w.s.high = (unsigned long long) uu.s.high >> b;
> >> >> +               w.s.low = ((unsigned long long) uu.s.low >> b) |
> >> >carries;
> >> >> +       }
> >> >> +
> >> >> +       return w.ll;
> >> >> +}
> >> >> +#ifndef BUILD_VDSO
> >> >> +EXPORT_SYMBOL(__lshrti3);
> >> >> +#endif
> >> >
> >> >I don't think you want this.  Maybe that was carried over from
> >> >https://lkml.org/lkml/2019/3/15/669
> >> >by accident?  The above linkage error mentions arch/x86/kvm/x86.o
> >> >which I wouldn't expect to be linked into the VDSO image?
> >> 
> >> Or just "u64"...
> >
> >Are you referring to TItype? It currenty is a 128-bit value.
> 
> You're right, so "unsigned __int128"...

I liked the idea, however __int128 isn't supported by all platforms:

include/linux/libgcc.h:57:11: error: __int128 is not supported on this target
        unsigned __int128 ll;

That's from building the 32-bit VDSO for x86.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-15 20:54 [PATCH] lib: Add shared copy of __lshrti3 from libgcc Matthias Kaehlcke
  2019-03-15 22:06 ` Nick Desaulniers
@ 2019-03-18 21:31 ` Matthias Kaehlcke
  2019-03-18 21:50   ` hpa
  1 sibling, 1 reply; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-18 21:31 UTC (permalink / raw)
  To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin
  Cc: x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke wrote:
> The compiler may emit calls to __lshrti3 from the compiler runtime
> library, which results in undefined references:
> 
> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>   include/linux/math64.h:186: undefined reference to `__lshrti3'
> 
> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> 
> Include the function for x86 builds with clang, which is the
> environment where the above error was observed.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

With "Revert "kbuild: use -Oz instead of -Os when using clang"
(https://lore.kernel.org/patchwork/patch/1051932/) the above
error is fixed, a few comments inline for if the patch is
resurrected in the future because __lshrti3 is emitted in a
different context.

> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> index 32e1e0f4b2d0..a71036471838 100644
> --- a/include/linux/libgcc.h
> +++ b/include/linux/libgcc.h
> @@ -22,15 +22,26 @@
>  #include <asm/byteorder.h>
> 
>  typedef int word_type __attribute__ ((mode (__word__)));
> +typedef int TItype __attribute__ ((mode (TI)));

Consider using __int128 instead. Definition and use need a
'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)), since
these 128 bit types aren't supported on all platforms.

>  #ifdef __BIG_ENDIAN
>  struct DWstruct {
>  	int high, low;
>  };
> +
> +struct DWstruct128 {
> +	long long high, low;
> +};

This struct isn't needed, struct DWstruct can be used.

> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> new file mode 100644
> index 000000000000..2d2123bb3030
> --- /dev/null
> +++ b/lib/lshrti3.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/export.h>
> +#include <linux/libgcc.h>
> +
> +long long __lshrti3(long long u, word_type b)

use TItype for input/output, which is what gcc does, though the above
matches the interface in the documentation.

> +{
> +	DWunion128 uu, w;
> +	word_type bm;
> +
> +	if (b == 0)
> +		return u;
> +
> +	uu.ll = u;
> +	bm = 64 - b;
> +
> +	if (bm <= 0) {
> +		w.s.high = 0;
> +		w.s.low = (unsigned long long) uu.s.high >> -bm;

include <linux/types.h> and use u64 instead of unsigned long long.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 21:31 ` Matthias Kaehlcke
@ 2019-03-18 21:50   ` hpa
  2019-03-18 22:16     ` Matthias Kaehlcke
  0 siblings, 1 reply; 21+ messages in thread
From: hpa @ 2019-03-18 21:50 UTC (permalink / raw)
  To: Matthias Kaehlcke, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov
  Cc: x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke wrote:
>> The compiler may emit calls to __lshrti3 from the compiler runtime
>> library, which results in undefined references:
>> 
>> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>>   include/linux/math64.h:186: undefined reference to `__lshrti3'
>> 
>> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>> 
>> Include the function for x86 builds with clang, which is the
>> environment where the above error was observed.
>> 
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>
>With "Revert "kbuild: use -Oz instead of -Os when using clang"
>(https://lore.kernel.org/patchwork/patch/1051932/) the above
>error is fixed, a few comments inline for if the patch is
>resurrected in the future because __lshrti3 is emitted in a
>different context.
>
>> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> index 32e1e0f4b2d0..a71036471838 100644
>> --- a/include/linux/libgcc.h
>> +++ b/include/linux/libgcc.h
>> @@ -22,15 +22,26 @@
>>  #include <asm/byteorder.h>
>> 
>>  typedef int word_type __attribute__ ((mode (__word__)));
>> +typedef int TItype __attribute__ ((mode (TI)));
>
>Consider using __int128 instead. Definition and use need a
>'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)), since
>these 128 bit types aren't supported on all platforms.
>
>>  #ifdef __BIG_ENDIAN
>>  struct DWstruct {
>>  	int high, low;
>>  };
>> +
>> +struct DWstruct128 {
>> +	long long high, low;
>> +};
>
>This struct isn't needed, struct DWstruct can be used.
>
>> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> new file mode 100644
>> index 000000000000..2d2123bb3030
>> --- /dev/null
>> +++ b/lib/lshrti3.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/export.h>
>> +#include <linux/libgcc.h>
>> +
>> +long long __lshrti3(long long u, word_type b)
>
>use TItype for input/output, which is what gcc does, though the above
>matches the interface in the documentation.
>
>> +{
>> +	DWunion128 uu, w;
>> +	word_type bm;
>> +
>> +	if (b == 0)
>> +		return u;
>> +
>> +	uu.ll = u;
>> +	bm = 64 - b;
>> +
>> +	if (bm <= 0) {
>> +		w.s.high = 0;
>> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
>
>include <linux/types.h> and use u64 instead of unsigned long long.

Ok, now I'm really puzzled.

How could we need a 128-bit shift when the prototype only has 64 bits of input?!

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 21:50   ` hpa
@ 2019-03-18 22:16     ` Matthias Kaehlcke
  2019-03-18 23:44       ` hpa
  0 siblings, 1 reply; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-18 22:16 UTC (permalink / raw)
  To: hpa
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On Mon, Mar 18, 2019 at 02:50:44PM -0700, hpa@zytor.com wrote:
> On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
> >On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke wrote:
> >> The compiler may emit calls to __lshrti3 from the compiler runtime
> >> library, which results in undefined references:
> >> 
> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
> >> 
> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> >> 
> >> Include the function for x86 builds with clang, which is the
> >> environment where the above error was observed.
> >> 
> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >
> >With "Revert "kbuild: use -Oz instead of -Os when using clang"
> >(https://lore.kernel.org/patchwork/patch/1051932/) the above
> >error is fixed, a few comments inline for if the patch is
> >resurrected in the future because __lshrti3 is emitted in a
> >different context.
> >
> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> >> index 32e1e0f4b2d0..a71036471838 100644
> >> --- a/include/linux/libgcc.h
> >> +++ b/include/linux/libgcc.h
> >> @@ -22,15 +22,26 @@
> >>  #include <asm/byteorder.h>
> >> 
> >>  typedef int word_type __attribute__ ((mode (__word__)));
> >> +typedef int TItype __attribute__ ((mode (TI)));
> >
> >Consider using __int128 instead. Definition and use need a
> >'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)), since
> >these 128 bit types aren't supported on all platforms.
> >
> >>  #ifdef __BIG_ENDIAN
> >>  struct DWstruct {
> >>  	int high, low;
> >>  };
> >> +
> >> +struct DWstruct128 {
> >> +	long long high, low;
> >> +};
> >
> >This struct isn't needed, struct DWstruct can be used.
> >
> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> >> new file mode 100644
> >> index 000000000000..2d2123bb3030
> >> --- /dev/null
> >> +++ b/lib/lshrti3.c
> >> @@ -0,0 +1,31 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/export.h>
> >> +#include <linux/libgcc.h>
> >> +
> >> +long long __lshrti3(long long u, word_type b)
> >
> >use TItype for input/output, which is what gcc does, though the above
> >matches the interface in the documentation.
> >
> >> +{
> >> +	DWunion128 uu, w;
> >> +	word_type bm;
> >> +
> >> +	if (b == 0)
> >> +		return u;
> >> +
> >> +	uu.ll = u;
> >> +	bm = 64 - b;
> >> +
> >> +	if (bm <= 0) {
> >> +		w.s.high = 0;
> >> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
> >
> >include <linux/types.h> and use u64 instead of unsigned long long.
> 
> Ok, now I'm really puzzled.
> 
> How could we need a 128-bit shift when the prototype only has 64 bits of input?!

Good question, this is the code from libgcc:

TItype
__lshrti3 (TItype u, shift_count_type b)
{
  if (b == 0)
    return u;

  const DWunion uu = {.ll = u};
  const shift_count_type bm = (8 * (8)) - b;
  DWunion w;

  if (bm <= 0)
    {
      w.s.high = 0;
      w.s.low = (UDItype) uu.s.high >> -bm;
    }
  else
    {
      const UDItype carries = (UDItype) uu.s.high << bm;

      w.s.high = (UDItype) uu.s.high >> b;
      w.s.low = ((UDItype) uu.s.low >> b) | carries;
    }

  return w.ll;
}


My compiler knowledge is limited, my guess is that the function is a
generic implementation, and while a long long is 64-bit wide under
Linux it could be 128-bit on other platforms.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 22:16     ` Matthias Kaehlcke
@ 2019-03-18 23:44       ` hpa
  2019-03-18 23:52         ` Matthias Kaehlcke
  0 siblings, 1 reply; 21+ messages in thread
From: hpa @ 2019-03-18 23:44 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On March 18, 2019 3:16:39 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>On Mon, Mar 18, 2019 at 02:50:44PM -0700, hpa@zytor.com wrote:
>> On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke
><mka@chromium.org> wrote:
>> >On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke wrote:
>> >> The compiler may emit calls to __lshrti3 from the compiler runtime
>> >> library, which results in undefined references:
>> >> 
>> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
>> >> 
>> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>> >> 
>> >> Include the function for x86 builds with clang, which is the
>> >> environment where the above error was observed.
>> >> 
>> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> >
>> >With "Revert "kbuild: use -Oz instead of -Os when using clang"
>> >(https://lore.kernel.org/patchwork/patch/1051932/) the above
>> >error is fixed, a few comments inline for if the patch is
>> >resurrected in the future because __lshrti3 is emitted in a
>> >different context.
>> >
>> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> >> index 32e1e0f4b2d0..a71036471838 100644
>> >> --- a/include/linux/libgcc.h
>> >> +++ b/include/linux/libgcc.h
>> >> @@ -22,15 +22,26 @@
>> >>  #include <asm/byteorder.h>
>> >> 
>> >>  typedef int word_type __attribute__ ((mode (__word__)));
>> >> +typedef int TItype __attribute__ ((mode (TI)));
>> >
>> >Consider using __int128 instead. Definition and use need a
>> >'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)), since
>> >these 128 bit types aren't supported on all platforms.
>> >
>> >>  #ifdef __BIG_ENDIAN
>> >>  struct DWstruct {
>> >>  	int high, low;
>> >>  };
>> >> +
>> >> +struct DWstruct128 {
>> >> +	long long high, low;
>> >> +};
>> >
>> >This struct isn't needed, struct DWstruct can be used.
>> >
>> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> >> new file mode 100644
>> >> index 000000000000..2d2123bb3030
>> >> --- /dev/null
>> >> +++ b/lib/lshrti3.c
>> >> @@ -0,0 +1,31 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +#include <linux/export.h>
>> >> +#include <linux/libgcc.h>
>> >> +
>> >> +long long __lshrti3(long long u, word_type b)
>> >
>> >use TItype for input/output, which is what gcc does, though the
>above
>> >matches the interface in the documentation.
>> >
>> >> +{
>> >> +	DWunion128 uu, w;
>> >> +	word_type bm;
>> >> +
>> >> +	if (b == 0)
>> >> +		return u;
>> >> +
>> >> +	uu.ll = u;
>> >> +	bm = 64 - b;
>> >> +
>> >> +	if (bm <= 0) {
>> >> +		w.s.high = 0;
>> >> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
>> >
>> >include <linux/types.h> and use u64 instead of unsigned long long.
>> 
>> Ok, now I'm really puzzled.
>> 
>> How could we need a 128-bit shift when the prototype only has 64 bits
>of input?!
>
>Good question, this is the code from libgcc:
>
>TItype
>__lshrti3 (TItype u, shift_count_type b)
>{
>  if (b == 0)
>    return u;
>
>  const DWunion uu = {.ll = u};
>  const shift_count_type bm = (8 * (8)) - b;
>  DWunion w;
>
>  if (bm <= 0)
>    {
>      w.s.high = 0;
>      w.s.low = (UDItype) uu.s.high >> -bm;
>    }
>  else
>    {
>      const UDItype carries = (UDItype) uu.s.high << bm;
>
>      w.s.high = (UDItype) uu.s.high >> b;
>      w.s.low = ((UDItype) uu.s.low >> b) | carries;
>    }
>
>  return w.ll;
>}
>
>
>My compiler knowledge is limited, my guess is that the function is a
>generic implementation, and while a long long is 64-bit wide under
>Linux it could be 128-bit on other platforms.

Yes, long long is just plain wrong.

How could we end up calling this function on 32 bits?!
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 23:44       ` hpa
@ 2019-03-18 23:52         ` Matthias Kaehlcke
  2019-03-18 23:54           ` hpa
  2019-03-18 23:55           ` hpa
  0 siblings, 2 replies; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-18 23:52 UTC (permalink / raw)
  To: hpa
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On Mon, Mar 18, 2019 at 04:44:03PM -0700, hpa@zytor.com wrote:
> On March 18, 2019 3:16:39 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
> >On Mon, Mar 18, 2019 at 02:50:44PM -0700, hpa@zytor.com wrote:
> >> On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke
> ><mka@chromium.org> wrote:
> >> >On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke wrote:
> >> >> The compiler may emit calls to __lshrti3 from the compiler runtime
> >> >> library, which results in undefined references:
> >> >> 
> >> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >> >>   include/linux/math64.h:186: undefined reference to `__lshrti3'
> >> >> 
> >> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> >> >> 
> >> >> Include the function for x86 builds with clang, which is the
> >> >> environment where the above error was observed.
> >> >> 
> >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> >
> >> >With "Revert "kbuild: use -Oz instead of -Os when using clang"
> >> >(https://lore.kernel.org/patchwork/patch/1051932/) the above
> >> >error is fixed, a few comments inline for if the patch is
> >> >resurrected in the future because __lshrti3 is emitted in a
> >> >different context.
> >> >
> >> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> >> >> index 32e1e0f4b2d0..a71036471838 100644
> >> >> --- a/include/linux/libgcc.h
> >> >> +++ b/include/linux/libgcc.h
> >> >> @@ -22,15 +22,26 @@
> >> >>  #include <asm/byteorder.h>
> >> >> 
> >> >>  typedef int word_type __attribute__ ((mode (__word__)));
> >> >> +typedef int TItype __attribute__ ((mode (TI)));
> >> >
> >> >Consider using __int128 instead. Definition and use need a
> >> >'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)), since
> >> >these 128 bit types aren't supported on all platforms.
> >> >
> >> >>  #ifdef __BIG_ENDIAN
> >> >>  struct DWstruct {
> >> >>  	int high, low;
> >> >>  };
> >> >> +
> >> >> +struct DWstruct128 {
> >> >> +	long long high, low;
> >> >> +};
> >> >
> >> >This struct isn't needed, struct DWstruct can be used.
> >> >
> >> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> >> >> new file mode 100644
> >> >> index 000000000000..2d2123bb3030
> >> >> --- /dev/null
> >> >> +++ b/lib/lshrti3.c
> >> >> @@ -0,0 +1,31 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +
> >> >> +#include <linux/export.h>
> >> >> +#include <linux/libgcc.h>
> >> >> +
> >> >> +long long __lshrti3(long long u, word_type b)
> >> >
> >> >use TItype for input/output, which is what gcc does, though the
> >above
> >> >matches the interface in the documentation.
> >> >
> >> >> +{
> >> >> +	DWunion128 uu, w;
> >> >> +	word_type bm;
> >> >> +
> >> >> +	if (b == 0)
> >> >> +		return u;
> >> >> +
> >> >> +	uu.ll = u;
> >> >> +	bm = 64 - b;
> >> >> +
> >> >> +	if (bm <= 0) {
> >> >> +		w.s.high = 0;
> >> >> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
> >> >
> >> >include <linux/types.h> and use u64 instead of unsigned long long.
> >> 
> >> Ok, now I'm really puzzled.
> >> 
> >> How could we need a 128-bit shift when the prototype only has 64 bits
> >of input?!
> >
> >Good question, this is the code from libgcc:
> >
> >TItype
> >__lshrti3 (TItype u, shift_count_type b)
> >{
> >  if (b == 0)
> >    return u;
> >
> >  const DWunion uu = {.ll = u};
> >  const shift_count_type bm = (8 * (8)) - b;
> >  DWunion w;
> >
> >  if (bm <= 0)
> >    {
> >      w.s.high = 0;
> >      w.s.low = (UDItype) uu.s.high >> -bm;
> >    }
> >  else
> >    {
> >      const UDItype carries = (UDItype) uu.s.high << bm;
> >
> >      w.s.high = (UDItype) uu.s.high >> b;
> >      w.s.low = ((UDItype) uu.s.low >> b) | carries;
> >    }
> >
> >  return w.ll;
> >}
> >
> >
> >My compiler knowledge is limited, my guess is that the function is a
> >generic implementation, and while a long long is 64-bit wide under
> >Linux it could be 128-bit on other platforms.
> 
> Yes, long long is just plain wrong.
> 
> How could we end up calling this function on 32 bits?!

We didn't, in this case the function is called in 64-bit code
(arch/x86/kvm/x86.o: In function `mul_u64_u64_shr'), for the 32-bit
vDSO it was __lshrdi3.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 23:52         ` Matthias Kaehlcke
@ 2019-03-18 23:54           ` hpa
  2019-03-18 23:55           ` hpa
  1 sibling, 0 replies; 21+ messages in thread
From: hpa @ 2019-03-18 23:54 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On March 18, 2019 4:52:19 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>On Mon, Mar 18, 2019 at 04:44:03PM -0700, hpa@zytor.com wrote:
>> On March 18, 2019 3:16:39 PM PDT, Matthias Kaehlcke
><mka@chromium.org> wrote:
>> >On Mon, Mar 18, 2019 at 02:50:44PM -0700, hpa@zytor.com wrote:
>> >> On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke
>> ><mka@chromium.org> wrote:
>> >> >On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke
>wrote:
>> >> >> The compiler may emit calls to __lshrti3 from the compiler
>runtime
>> >> >> library, which results in undefined references:
>> >> >> 
>> >> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>> >> >>   include/linux/math64.h:186: undefined reference to
>`__lshrti3'
>> >> >> 
>> >> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>> >> >> 
>> >> >> Include the function for x86 builds with clang, which is the
>> >> >> environment where the above error was observed.
>> >> >> 
>> >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> >> >
>> >> >With "Revert "kbuild: use -Oz instead of -Os when using clang"
>> >> >(https://lore.kernel.org/patchwork/patch/1051932/) the above
>> >> >error is fixed, a few comments inline for if the patch is
>> >> >resurrected in the future because __lshrti3 is emitted in a
>> >> >different context.
>> >> >
>> >> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> >> >> index 32e1e0f4b2d0..a71036471838 100644
>> >> >> --- a/include/linux/libgcc.h
>> >> >> +++ b/include/linux/libgcc.h
>> >> >> @@ -22,15 +22,26 @@
>> >> >>  #include <asm/byteorder.h>
>> >> >> 
>> >> >>  typedef int word_type __attribute__ ((mode (__word__)));
>> >> >> +typedef int TItype __attribute__ ((mode (TI)));
>> >> >
>> >> >Consider using __int128 instead. Definition and use need a
>> >> >'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)),
>since
>> >> >these 128 bit types aren't supported on all platforms.
>> >> >
>> >> >>  #ifdef __BIG_ENDIAN
>> >> >>  struct DWstruct {
>> >> >>  	int high, low;
>> >> >>  };
>> >> >> +
>> >> >> +struct DWstruct128 {
>> >> >> +	long long high, low;
>> >> >> +};
>> >> >
>> >> >This struct isn't needed, struct DWstruct can be used.
>> >> >
>> >> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> >> >> new file mode 100644
>> >> >> index 000000000000..2d2123bb3030
>> >> >> --- /dev/null
>> >> >> +++ b/lib/lshrti3.c
>> >> >> @@ -0,0 +1,31 @@
>> >> >> +// SPDX-License-Identifier: GPL-2.0
>> >> >> +
>> >> >> +#include <linux/export.h>
>> >> >> +#include <linux/libgcc.h>
>> >> >> +
>> >> >> +long long __lshrti3(long long u, word_type b)
>> >> >
>> >> >use TItype for input/output, which is what gcc does, though the
>> >above
>> >> >matches the interface in the documentation.
>> >> >
>> >> >> +{
>> >> >> +	DWunion128 uu, w;
>> >> >> +	word_type bm;
>> >> >> +
>> >> >> +	if (b == 0)
>> >> >> +		return u;
>> >> >> +
>> >> >> +	uu.ll = u;
>> >> >> +	bm = 64 - b;
>> >> >> +
>> >> >> +	if (bm <= 0) {
>> >> >> +		w.s.high = 0;
>> >> >> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
>> >> >
>> >> >include <linux/types.h> and use u64 instead of unsigned long
>long.
>> >> 
>> >> Ok, now I'm really puzzled.
>> >> 
>> >> How could we need a 128-bit shift when the prototype only has 64
>bits
>> >of input?!
>> >
>> >Good question, this is the code from libgcc:
>> >
>> >TItype
>> >__lshrti3 (TItype u, shift_count_type b)
>> >{
>> >  if (b == 0)
>> >    return u;
>> >
>> >  const DWunion uu = {.ll = u};
>> >  const shift_count_type bm = (8 * (8)) - b;
>> >  DWunion w;
>> >
>> >  if (bm <= 0)
>> >    {
>> >      w.s.high = 0;
>> >      w.s.low = (UDItype) uu.s.high >> -bm;
>> >    }
>> >  else
>> >    {
>> >      const UDItype carries = (UDItype) uu.s.high << bm;
>> >
>> >      w.s.high = (UDItype) uu.s.high >> b;
>> >      w.s.low = ((UDItype) uu.s.low >> b) | carries;
>> >    }
>> >
>> >  return w.ll;
>> >}
>> >
>> >
>> >My compiler knowledge is limited, my guess is that the function is a
>> >generic implementation, and while a long long is 64-bit wide under
>> >Linux it could be 128-bit on other platforms.
>> 
>> Yes, long long is just plain wrong.
>> 
>> How could we end up calling this function on 32 bits?!
>
>We didn't, in this case the function is called in 64-bit code
>(arch/x86/kvm/x86.o: In function `mul_u64_u64_shr'), for the 32-bit
>vDSO it was __lshrdi3.

That makes more sense.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 23:52         ` Matthias Kaehlcke
  2019-03-18 23:54           ` hpa
@ 2019-03-18 23:55           ` hpa
  2019-03-19 17:10             ` Matthias Kaehlcke
  1 sibling, 1 reply; 21+ messages in thread
From: hpa @ 2019-03-18 23:55 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On March 18, 2019 4:52:19 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>On Mon, Mar 18, 2019 at 04:44:03PM -0700, hpa@zytor.com wrote:
>> On March 18, 2019 3:16:39 PM PDT, Matthias Kaehlcke
><mka@chromium.org> wrote:
>> >On Mon, Mar 18, 2019 at 02:50:44PM -0700, hpa@zytor.com wrote:
>> >> On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke
>> ><mka@chromium.org> wrote:
>> >> >On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke
>wrote:
>> >> >> The compiler may emit calls to __lshrti3 from the compiler
>runtime
>> >> >> library, which results in undefined references:
>> >> >> 
>> >> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
>> >> >>   include/linux/math64.h:186: undefined reference to
>`__lshrti3'
>> >> >> 
>> >> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
>> >> >> 
>> >> >> Include the function for x86 builds with clang, which is the
>> >> >> environment where the above error was observed.
>> >> >> 
>> >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> >> >
>> >> >With "Revert "kbuild: use -Oz instead of -Os when using clang"
>> >> >(https://lore.kernel.org/patchwork/patch/1051932/) the above
>> >> >error is fixed, a few comments inline for if the patch is
>> >> >resurrected in the future because __lshrti3 is emitted in a
>> >> >different context.
>> >> >
>> >> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
>> >> >> index 32e1e0f4b2d0..a71036471838 100644
>> >> >> --- a/include/linux/libgcc.h
>> >> >> +++ b/include/linux/libgcc.h
>> >> >> @@ -22,15 +22,26 @@
>> >> >>  #include <asm/byteorder.h>
>> >> >> 
>> >> >>  typedef int word_type __attribute__ ((mode (__word__)));
>> >> >> +typedef int TItype __attribute__ ((mode (TI)));
>> >> >
>> >> >Consider using __int128 instead. Definition and use need a
>> >> >'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)),
>since
>> >> >these 128 bit types aren't supported on all platforms.
>> >> >
>> >> >>  #ifdef __BIG_ENDIAN
>> >> >>  struct DWstruct {
>> >> >>  	int high, low;
>> >> >>  };
>> >> >> +
>> >> >> +struct DWstruct128 {
>> >> >> +	long long high, low;
>> >> >> +};
>> >> >
>> >> >This struct isn't needed, struct DWstruct can be used.
>> >> >
>> >> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
>> >> >> new file mode 100644
>> >> >> index 000000000000..2d2123bb3030
>> >> >> --- /dev/null
>> >> >> +++ b/lib/lshrti3.c
>> >> >> @@ -0,0 +1,31 @@
>> >> >> +// SPDX-License-Identifier: GPL-2.0
>> >> >> +
>> >> >> +#include <linux/export.h>
>> >> >> +#include <linux/libgcc.h>
>> >> >> +
>> >> >> +long long __lshrti3(long long u, word_type b)
>> >> >
>> >> >use TItype for input/output, which is what gcc does, though the
>> >above
>> >> >matches the interface in the documentation.
>> >> >
>> >> >> +{
>> >> >> +	DWunion128 uu, w;
>> >> >> +	word_type bm;
>> >> >> +
>> >> >> +	if (b == 0)
>> >> >> +		return u;
>> >> >> +
>> >> >> +	uu.ll = u;
>> >> >> +	bm = 64 - b;
>> >> >> +
>> >> >> +	if (bm <= 0) {
>> >> >> +		w.s.high = 0;
>> >> >> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
>> >> >
>> >> >include <linux/types.h> and use u64 instead of unsigned long
>long.
>> >> 
>> >> Ok, now I'm really puzzled.
>> >> 
>> >> How could we need a 128-bit shift when the prototype only has 64
>bits
>> >of input?!
>> >
>> >Good question, this is the code from libgcc:
>> >
>> >TItype
>> >__lshrti3 (TItype u, shift_count_type b)
>> >{
>> >  if (b == 0)
>> >    return u;
>> >
>> >  const DWunion uu = {.ll = u};
>> >  const shift_count_type bm = (8 * (8)) - b;
>> >  DWunion w;
>> >
>> >  if (bm <= 0)
>> >    {
>> >      w.s.high = 0;
>> >      w.s.low = (UDItype) uu.s.high >> -bm;
>> >    }
>> >  else
>> >    {
>> >      const UDItype carries = (UDItype) uu.s.high << bm;
>> >
>> >      w.s.high = (UDItype) uu.s.high >> b;
>> >      w.s.low = ((UDItype) uu.s.low >> b) | carries;
>> >    }
>> >
>> >  return w.ll;
>> >}
>> >
>> >
>> >My compiler knowledge is limited, my guess is that the function is a
>> >generic implementation, and while a long long is 64-bit wide under
>> >Linux it could be 128-bit on other platforms.
>> 
>> Yes, long long is just plain wrong.
>> 
>> How could we end up calling this function on 32 bits?!
>
>We didn't, in this case the function is called in 64-bit code
>(arch/x86/kvm/x86.o: In function `mul_u64_u64_shr'), for the 32-bit
>vDSO it was __lshrdi3.

Again, for 64 bits we can include libgcc in the link (with --no-whole-archive).
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] lib: Add shared copy of __lshrti3 from libgcc
  2019-03-18 23:55           ` hpa
@ 2019-03-19 17:10             ` Matthias Kaehlcke
  0 siblings, 0 replies; 21+ messages in thread
From: Matthias Kaehlcke @ 2019-03-19 17:10 UTC (permalink / raw)
  To: hpa
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-kernel, Nick Desaulniers, Manoj Gupta, Tiancong Wang,
	Stephen Hines, clang-built-linux

On Mon, Mar 18, 2019 at 04:55:38PM -0700, hpa@zytor.com wrote:
> On March 18, 2019 4:52:19 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
> >On Mon, Mar 18, 2019 at 04:44:03PM -0700, hpa@zytor.com wrote:
> >> On March 18, 2019 3:16:39 PM PDT, Matthias Kaehlcke
> ><mka@chromium.org> wrote:
> >> >On Mon, Mar 18, 2019 at 02:50:44PM -0700, hpa@zytor.com wrote:
> >> >> On March 18, 2019 2:31:13 PM PDT, Matthias Kaehlcke
> >> ><mka@chromium.org> wrote:
> >> >> >On Fri, Mar 15, 2019 at 01:54:50PM -0700, Matthias Kaehlcke
> >wrote:
> >> >> >> The compiler may emit calls to __lshrti3 from the compiler
> >runtime
> >> >> >> library, which results in undefined references:
> >> >> >> 
> >> >> >> arch/x86/kvm/x86.o: In function `mul_u64_u64_shr':
> >> >> >>   include/linux/math64.h:186: undefined reference to
> >`__lshrti3'
> >> >> >> 
> >> >> >> Add a copy of the __lshrti3 libgcc routine (from gcc v4.9.2).
> >> >> >> 
> >> >> >> Include the function for x86 builds with clang, which is the
> >> >> >> environment where the above error was observed.
> >> >> >> 
> >> >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> >> >
> >> >> >With "Revert "kbuild: use -Oz instead of -Os when using clang"
> >> >> >(https://lore.kernel.org/patchwork/patch/1051932/) the above
> >> >> >error is fixed, a few comments inline for if the patch is
> >> >> >resurrected in the future because __lshrti3 is emitted in a
> >> >> >different context.
> >> >> >
> >> >> >> diff --git a/include/linux/libgcc.h b/include/linux/libgcc.h
> >> >> >> index 32e1e0f4b2d0..a71036471838 100644
> >> >> >> --- a/include/linux/libgcc.h
> >> >> >> +++ b/include/linux/libgcc.h
> >> >> >> @@ -22,15 +22,26 @@
> >> >> >>  #include <asm/byteorder.h>
> >> >> >> 
> >> >> >>  typedef int word_type __attribute__ ((mode (__word__)));
> >> >> >> +typedef int TItype __attribute__ ((mode (TI)));
> >> >> >
> >> >> >Consider using __int128 instead. Definition and use need a
> >> >> >'defined(__SIZEOF_INT128__)' guard  (similar for mode (TI)),
> >since
> >> >> >these 128 bit types aren't supported on all platforms.
> >> >> >
> >> >> >>  #ifdef __BIG_ENDIAN
> >> >> >>  struct DWstruct {
> >> >> >>  	int high, low;
> >> >> >>  };
> >> >> >> +
> >> >> >> +struct DWstruct128 {
> >> >> >> +	long long high, low;
> >> >> >> +};
> >> >> >
> >> >> >This struct isn't needed, struct DWstruct can be used.
> >> >> >
> >> >> >> diff --git a/lib/lshrti3.c b/lib/lshrti3.c
> >> >> >> new file mode 100644
> >> >> >> index 000000000000..2d2123bb3030
> >> >> >> --- /dev/null
> >> >> >> +++ b/lib/lshrti3.c
> >> >> >> @@ -0,0 +1,31 @@
> >> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> >> +
> >> >> >> +#include <linux/export.h>
> >> >> >> +#include <linux/libgcc.h>
> >> >> >> +
> >> >> >> +long long __lshrti3(long long u, word_type b)
> >> >> >
> >> >> >use TItype for input/output, which is what gcc does, though the
> >> >above
> >> >> >matches the interface in the documentation.
> >> >> >
> >> >> >> +{
> >> >> >> +	DWunion128 uu, w;
> >> >> >> +	word_type bm;
> >> >> >> +
> >> >> >> +	if (b == 0)
> >> >> >> +		return u;
> >> >> >> +
> >> >> >> +	uu.ll = u;
> >> >> >> +	bm = 64 - b;
> >> >> >> +
> >> >> >> +	if (bm <= 0) {
> >> >> >> +		w.s.high = 0;
> >> >> >> +		w.s.low = (unsigned long long) uu.s.high >> -bm;
> >> >> >
> >> >> >include <linux/types.h> and use u64 instead of unsigned long
> >long.
> >> >> 
> >> >> Ok, now I'm really puzzled.
> >> >> 
> >> >> How could we need a 128-bit shift when the prototype only has 64
> >bits
> >> >of input?!
> >> >
> >> >Good question, this is the code from libgcc:
> >> >
> >> >TItype
> >> >__lshrti3 (TItype u, shift_count_type b)
> >> >{
> >> >  if (b == 0)
> >> >    return u;
> >> >
> >> >  const DWunion uu = {.ll = u};
> >> >  const shift_count_type bm = (8 * (8)) - b;
> >> >  DWunion w;
> >> >
> >> >  if (bm <= 0)
> >> >    {
> >> >      w.s.high = 0;
> >> >      w.s.low = (UDItype) uu.s.high >> -bm;
> >> >    }
> >> >  else
> >> >    {
> >> >      const UDItype carries = (UDItype) uu.s.high << bm;
> >> >
> >> >      w.s.high = (UDItype) uu.s.high >> b;
> >> >      w.s.low = ((UDItype) uu.s.low >> b) | carries;
> >> >    }
> >> >
> >> >  return w.ll;
> >> >}
> >> >
> >> >
> >> >My compiler knowledge is limited, my guess is that the function is a
> >> >generic implementation, and while a long long is 64-bit wide under
> >> >Linux it could be 128-bit on other platforms.
> >> 
> >> Yes, long long is just plain wrong.
> >> 
> >> How could we end up calling this function on 32 bits?!
> >
> >We didn't, in this case the function is called in 64-bit code
> >(arch/x86/kvm/x86.o: In function `mul_u64_u64_shr'), for the 32-bit
> >vDSO it was __lshrdi3.
> 
> Again, for 64 bits we can include libgcc in the link (with --no-whole-archive).

I gave this a quick try but ended up with:

VDSO    arch/x86/entry/vdso/vdso64.so.dbg
/usr/x86_64-pc-linux-gnu/x86_64-cros-linux-gnu/binutils-bin/2.27.0/ld.bfd.real:
cannot find -lgcc

I guess the solution is to specify the correct directory in LDPATH,
but not sure where that would be coming from.

TBH I'd prefer to leave this to someone more fluent with binutils, I'm
not particularily efficient working in this area.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2019-03-19 17:10 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-15 20:54 [PATCH] lib: Add shared copy of __lshrti3 from libgcc Matthias Kaehlcke
2019-03-15 22:06 ` Nick Desaulniers
2019-03-15 22:08   ` hpa
2019-03-15 23:34     ` Matthias Kaehlcke
2019-03-15 23:51       ` hpa
2019-03-15 22:12   ` hpa
2019-03-15 23:47     ` Matthias Kaehlcke
2019-03-15 23:53       ` hpa
2019-03-18 16:38         ` Matthias Kaehlcke
2019-03-15 23:29   ` Matthias Kaehlcke
2019-03-18  9:14   ` Peter Zijlstra
2019-03-18 14:43     ` David Laight
2019-03-18 14:54       ` Peter Zijlstra
2019-03-18 21:31 ` Matthias Kaehlcke
2019-03-18 21:50   ` hpa
2019-03-18 22:16     ` Matthias Kaehlcke
2019-03-18 23:44       ` hpa
2019-03-18 23:52         ` Matthias Kaehlcke
2019-03-18 23:54           ` hpa
2019-03-18 23:55           ` hpa
2019-03-19 17:10             ` Matthias Kaehlcke

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.