Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc

From: Pádraig Brady <[email-masked]>
Date: Mon, 07 Jan 2013 02:09:12 +0000

On 01/04/2013 02:47 PM, Pádraig Brady wrote:
> So what I'm going to do is pull all the _LP64 amendments to longlong.h
> and instead avoid it completely with this in factor.c:
>
> diff --git a/src/factor.c b/src/factor.c
> index 473eee7..95451a5 100644
> --- a/src/factor.c
> +++ b/src/factor.c
> @@ -118,7 +118,14 @@
>   #endif
>
>   #ifndef USE_LONGLONG_H
> -# define USE_LONGLONG_H 1
> +/* With the way we use longlong.h, it's only safe to use
> +   when UWtype = UHWtype, as there were various cases
> +   (as can be seen in the history for longlong.h) where
> +   for example, _LP64 was required to enable W_TYPE_SIZE==64 code,
> +   to avoid compile time or run time issues.  */
> +# if LONG_MAX == INTMAX_MAX
> +#  define USE_LONGLONG_H 1
> +# endif
>   #endif
>
>   #if USE_LONGLONG_H
>
> That works for i686 x86_64 and sparcv7 and sparcv9 at least.

No comments, so I'm pushing the attached.

thanks,
Pádraig.
>From 8292a881c3d55b5cd9deb24f001353244d811e84 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Fri, 4 Jan 2013 18:07:01 +0000
Subject: [PATCH] factor: apply a more general fix to enable correct assembly

In addition to the previous 64 bit guards we've placed in longlong.h
there are additional _LP64 guards required for mips with -mcpu >= 3,
to avoid a build failure (http://bugs.gnu.org/13353) and on sparc
with -mcpu >= v9 in 32 bit mode where for example,
`factor 2123123123123123123123` would go into an infinite loop.

Since factor.c currently operates on uintmax_t, we restrict the use
of the assembly in longlong.h to when 'long' has the same width, to
provide a more general guard for this code.

* src/factor.c: Restrict the use of longlong.h assembly code,
to when the width of intmax_t == long.
* src/longlong.h: Remove the previous _LP64 guards to avoid
divergence from GMP's longlong.h
* NEWS: Adjust the info on build and runtime fixes.
---
 NEWS           |    6 +++---
 src/factor.c   |    9 ++++++++-
 src/longlong.h |   16 ++++------------
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/NEWS b/NEWS
index 5eb8ef3..754b2cf 100644
--- a/NEWS
+++ b/NEWS
@@ -29,7 +29,7 @@ GNU coreutils NEWS                                    -*- outline -*-
   another range.  Before, "echo 123|cut --output-delim=: -b2-,3" would print
   "2:3".  Now it prints "23".  [bug introduced in 5.3.0]
 
-  factor no longer loops infinitely on 32 bit powerpc systems.
+  factor no longer loops infinitely on 32 bit powerpc or sparc systems.
   [bug introduced in coreutils-8.20]
 
   install -m M SOURCE DEST no longer has a race condition where DEST's
@@ -85,8 +85,8 @@ GNU coreutils NEWS                                    -*- outline -*-
   build failure when building from modified sources, as is common practice
   for a patched distribution package.
 
-  factor now builds on x86_64 with x32 ABI, by avoiding incompatible asm.
-  [bug introduced in coreutils-8.20]
+  factor now builds on x86_64 with x32 ABI, 32 bit MIPS, and all HPPA systems,
+  by avoiding incompatible asm.  [bug introduced in coreutils-8.20]
 
   A root-only test predicate would always fail.  Its job was to determine
   whether our dummy user, $NON_ROOT_USERNAME, was able to run binaries from
diff --git a/src/factor.c b/src/factor.c
index 473eee7..95451a5 100644
--- a/src/factor.c
+++ b/src/factor.c
@@ -118,7 +118,14 @@
 #endif
 
 #ifndef USE_LONGLONG_H
-# define USE_LONGLONG_H 1
+/* With the way we use longlong.h, it's only safe to use
+   when UWtype = UHWtype, as there were various cases
+   (as can be seen in the history for longlong.h) where
+   for example, _LP64 was required to enable W_TYPE_SIZE==64 code,
+   to avoid compile time or run time issues.  */
+# if LONG_MAX == INTMAX_MAX
+#  define USE_LONGLONG_H 1
+# endif
 #endif
 
 #if USE_LONGLONG_H
diff --git a/src/longlong.h b/src/longlong.h
index 00aeb73..4681642 100644
--- a/src/longlong.h
+++ b/src/longlong.h
@@ -678,13 +678,7 @@ extern UWtype __MPN(udiv_qrnnd) (UWtype *, UWtype, UWtype, UWtype);
 /* These macros are for ABI=2.0w.  In ABI=2.0n they can't be used, since GCC
    (3.2) puts longlong into two adjacent 32-bit registers.  Presumably this
    is just a case of no direct support for 2.0n but treating it like 1.0. */
-#if defined (__hppa) && W_TYPE_SIZE == 64 && ! defined (_LONG_LONG_LIMB) \
-  && defined (_PA_RISC2_0) && defined (_LP64)
-/* Note the _PA_RISC2_0 above is to exclude this code from GCC with
-   default -march options which doesn't support these instructions.
-   Also the width check for 'long' is to avoid ILP32 runtimes where
-   GNU/Linux and narrow HP-UX kernels are known to have issues with
-   clobbering of context between the add and add,dc instructions.  */
+#if defined (__hppa) && W_TYPE_SIZE == 64 && ! defined (_LONG_LONG_LIMB)
 #define add_ssaaaa(sh, sl, ah, al, bh, bl) \
   __asm__ ("add%I5 %5,%r4,%1\n\tadd,dc %r2,%r3,%0"			\
 	   : "=r" (sh), "=&r" (sl)					\
@@ -1005,9 +999,7 @@ extern UWtype __MPN(udiv_qrnnd) (UWtype *, UWtype, UWtype, UWtype);
 #endif
 #endif /* 80x86 */
 
-#if defined (__amd64__) && W_TYPE_SIZE == 64 && defined (_LP64)
-/* Note the width check for 'long' is to avoid ILP32 runtimes (x32)
-   where the ABI is known to be incompatible with the following.  */
+#if defined (__amd64__) && W_TYPE_SIZE == 64
 #define add_ssaaaa(sh, sl, ah, al, bh, bl) \
   __asm__ ("addq %5,%q1\n\tadcq %3,%q0"					\
 	   : "=r" (sh), "=&r" (sl)					\
@@ -1399,7 +1391,7 @@ extern UWtype __MPN(udiv_qrnnd) (UWtype *, UWtype, UWtype, UWtype);
 
 /* We should test _IBMR2 here when we add assembly support for the system
    vendor compilers.  */
-#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64 && defined (_LP64)
+#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64
 #if !defined (_LONG_LONG_LIMB)
 /* _LONG_LONG_LIMB is ABI=mode32 where adde operates on 32-bit values.  So
    use adde etc only when not _LONG_LONG_LIMB.  */
@@ -1752,7 +1744,7 @@ extern UWtype __MPN(udiv_qrnnd) (UWtype *, UWtype, UWtype, UWtype);
 #endif /* udiv_qrnnd */
 #endif /* __sparc__ */
 
-#if (defined (__sparc_v9) || defined (__sparc_v9__)) && W_TYPE_SIZE == 64
+#if defined (__sparc__) && W_TYPE_SIZE == 64
 #define add_ssaaaa(sh, sl, ah, al, bh, bl) \
   __asm__ (								\
        "addcc	%r4,%5,%1\n"						\
-- 
1.7.6.4