GNU bug report logs - #13030
factor: infinite loop on Linux/powerpc

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13030 in the body.
You can then email your comments to 13030 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Colin Watson <cjwatson <at> ubuntu.com>
To: bug-coreutils <at> gnu.org
Subject: factor: infinite loop on Linux/powerpc
Date: Thu, 29 Nov 2012 14:28:18 +0000

Both Debian and Ubuntu builds of coreutils 8.20 hang while running the
test suite on powerpc.  This turns out to be reproducible using 'factor
122'.

This turns out to be somewhat related to
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=12754, but not quite the
same.  uintmax_t is 64 bits, but the cntlzw instruction takes 32-bit
operands, and the cntlzd option is only available on 64-bit hardware.  I
believe the correct answer is to add an _LP64 check around the PPC64
code, so that this falls back to the C implementations:

--- coreutils-8.20~/src/longlong.h	2012-11-29 14:25:07.000000000 +0000
+++ coreutils-8.20/src/longlong.h	2012-11-29 14:26:40.000000000 +0000
@@ -1398,7 +1398,7 @@
 
 /* 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
+#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64 && defined (_LP64)
 #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.  */

Thanks,

-- 
Colin Watson                                       [cjwatson <at> ubuntu.com]

Message #10 received at 13030-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Colin Watson <cjwatson <at> ubuntu.com>
Cc: coreutils <at> packages.debian.org, 13030-done <at> debbugs.gnu.org,
	Torbjorn Granlund <tg <at> gmplib.org>
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Thu, 29 Nov 2012 19:34:03 +0000

On 11/29/2012 02:28 PM, Colin Watson wrote:
> Both Debian and Ubuntu builds of coreutils 8.20 hang while running the
> test suite on powerpc.  This turns out to be reproducible using 'factor
> 122'.
>
> This turns out to be somewhat related to
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=12754, but not quite the
> same.  uintmax_t is 64 bits, but the cntlzw instruction takes 32-bit
> operands, and the cntlzd option is only available on 64-bit hardware.  I
> believe the correct answer is to add an _LP64 check around the PPC64
> code, so that this falls back to the C implementations:
>
> --- coreutils-8.20~/src/longlong.h	2012-11-29 14:25:07.000000000 +0000
> +++ coreutils-8.20/src/longlong.h	2012-11-29 14:26:40.000000000 +0000
> @@ -1398,7 +1398,7 @@
>
>   /* 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
> +#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64 && defined (_LP64)
>   #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.  */
>
> Thanks,
>

The fix looks good thanks.
I'll commit that in your name along with this NEWS entry:

** Bug fixes

  factor no longer hangs on 32 bit powerpc systems.
  [bug introduced in coreutil-8.20]

thanks,
Pádraig.

Message #13 received at 13030-done <at> debbugs.gnu.org (full text, mbox):

From: Torbjorn Granlund <tg <at> gmplib.org>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 13030-done <at> debbugs.gnu.org, Colin Watson <cjwatson <at> ubuntu.com>,
	coreutils <at> packages.debian.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Thu, 29 Nov 2012 20:36:38 +0100

Pádraig Brady <P <at> draigBrady.com> writes:

  > -#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64
  > +#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64 && defined (_LP64)
  >   #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.  */
  >
  > Thanks,
  >
  
I suppose it might be much better to make W_TYPE_SIZE not be set to a
size not supported by the present ABI.

That way, we will avoid longlong.h divergence.

-- 
Torbjörn

Message #16 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Torbjorn Granlund <tg <at> gmplib.org>
Cc: 13030 <at> debbugs.gnu.org, Colin Watson <cjwatson <at> ubuntu.com>,
	coreutils <at> packages.debian.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Thu, 29 Nov 2012 20:08:43 +0000

On 11/29/2012 07:36 PM, Torbjorn Granlund wrote:
> Pádraig Brady <P <at> draigBrady.com> writes:
>
>    > -#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64
>    > +#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64 && defined (_LP64)
>    >   #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.  */
>    >
>    > Thanks,
>    >
>
> I suppose it might be much better to make W_TYPE_SIZE not be set to a
> size not supported by the present ABI.
>
> That way, we will avoid longlong.h divergence.

So that's a bit in contradiction to:
http://bugs.gnu.org/12754#20

So you're suggesting that this would be safest?

diff --git a/src/factor.c b/src/factor.c
index 6d1d17a..d2a4158 100644
--- a/src/factor.c
+++ b/src/factor.c
@@ -126,11 +126,11 @@
 /* Make definitions for longlong.h to make it do what it can do for us */

 /* bitcount for uintmax_t */
-# if UINTMAX_MAX == UINT32_MAX
+# if ULONG_MAX == UINT32_MAX
 #  define W_TYPE_SIZE 32
-# elif UINTMAX_MAX == UINT64_MAX
+# elif ULONG_MAX == UINT64_MAX
 #  define W_TYPE_SIZE 64
-# elif UINTMAX_MAX == UINT128_MAX
+# elif ULONG_MAX == UINT128_MAX
 #  define W_TYPE_SIZE 128
 # endif

In this particular case that would enable
the 32 bit ppc assembly in longlong.h

thanks,
Pádraig.

Message #19 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: 13030 <at> debbugs.gnu.org, cjwatson <at> ubuntu.com
Cc: P <at> draigBrady.com
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Wed, 05 Dec 2012 09:58:11 +0100

On 11/29/2012 08:34 PM, Pádraig Brady wrote:
> On 11/29/2012 02:28 PM, Colin Watson wrote:

> The fix looks good thanks.
> I'll commit [...]

Now, "make syntax-check" fails:

  Colin Watson
  maint.mk: remove the above names from THANKS.in
  make: *** [sc_THANKS_in_duplicates] Error 1

Before removing that entry:
Are you the same Colin Watson as in THANKS.in?

  Colin Watson    cjw44 <at> riva.ucam.org

Have a nice day,
Berny

Message #22 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Colin Watson <cjwatson <at> ubuntu.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: P <at> draigBrady.com, 13030 <at> debbugs.gnu.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Wed, 5 Dec 2012 10:08:18 +0000

On Wed, Dec 05, 2012 at 09:58:11AM +0100, Bernhard Voelker wrote:
> On 11/29/2012 08:34 PM, Pádraig Brady wrote:
> > The fix looks good thanks.
> > I'll commit [...]
> 
> Now, "make syntax-check" fails:
> 
>   Colin Watson
>   maint.mk: remove the above names from THANKS.in
>   make: *** [sc_THANKS_in_duplicates] Error 1
> 
> Before removing that entry:
> Are you the same Colin Watson as in THANKS.in?
> 
>   Colin Watson    cjw44 <at> riva.ucam.org

Yes, that's me.  The new address is better.

Cheers,

-- 
Colin Watson                                       [cjwatson <at> ubuntu.com]

Message #25 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Colin Watson <cjwatson <at> ubuntu.com>
Cc: P <at> draigBrady.com, 13030 <at> debbugs.gnu.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Wed, 05 Dec 2012 11:51:47 +0100

On 12/05/2012 11:08 AM, Colin Watson wrote:
> On Wed, Dec 05, 2012 at 09:58:11AM +0100, Bernhard Voelker wrote:
>> Now, "make syntax-check" fails:
>>
>>   Colin Watson
>>   maint.mk: remove the above names from THANKS.in
>>   make: *** [sc_THANKS_in_duplicates] Error 1
>>
>> Before removing that entry:
>> Are you the same Colin Watson as in THANKS.in?
>>
>>   Colin Watson    cjw44 <at> riva.ucam.org
> 
> Yes, that's me.  The new address is better.

Thanks, I'll push this soon.

Have a nice day,
Berny

From 7b4c56b4c65b2b58c185908ff1cbdf0b0399140d Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail <at> bernhard-voelker.de>
Date: Wed, 5 Dec 2012 11:47:18 +0100
Subject: [PATCH] maint: remove now auto-added entry from THANKS.in

The syntax-check sc_THANKS_in_duplicates complained about
that excess entry.

* THANKS.in (Colin Watson): Remove entry, now that it will be
automatically included in the generated THANKS file.
---
 THANKS.in |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/THANKS.in b/THANKS.in
index 6a79e04..f0ef9b8 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -128,7 +128,6 @@ Chusslove Illich                    caslav.ilic <at> gmx.net
 Clark Morgan                        cmorgan <at> aracnet.com
 Clement Wang                        clem.wang <at> overture.com
 Colin Plumb                         colin <at> nyx.net
-Colin Watson                        cjw44 <at> riva.ucam.org
 Collin Rogowski                     collin <at> rogowski.de
 Cray-Cyber Project                  http://www.cray-cyber.org
 Cristian Cadar                      cristic <at> stanford.edu
-- 
1.7.7

Message #32 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 13030 <at> debbugs.gnu.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Fri, 04 Jan 2013 14:47:06 +0000

On 11/29/2012 08:08 PM, Pádraig Brady wrote:
> On 11/29/2012 07:36 PM, Torbjorn Granlund wrote:
>> Pádraig Brady <P <at> draigBrady.com> writes:
>>
>>    > -#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64
>>    > +#if HAVE_HOST_CPU_FAMILY_powerpc && W_TYPE_SIZE == 64 && defined (_LP64)
>>    >   #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.  */
>>    >
>>    > Thanks,
>>    >
>>
>> I suppose it might be much better to make W_TYPE_SIZE not be set to a
>> size not supported by the present ABI.
>>
>> That way, we will avoid longlong.h divergence.
>
> So that's a bit in contradiction to:
> http://bugs.gnu.org/12754#20
>
> So you're suggesting that this would be safest?
>
> diff --git a/src/factor.c b/src/factor.c
> index 6d1d17a..d2a4158 100644
> --- a/src/factor.c
> +++ b/src/factor.c
> @@ -126,11 +126,11 @@
>   /* Make definitions for longlong.h to make it do what it can do for us */
>
>   /* bitcount for uintmax_t */
> -# if UINTMAX_MAX == UINT32_MAX
> +# if ULONG_MAX == UINT32_MAX
>   #  define W_TYPE_SIZE 32
> -# elif UINTMAX_MAX == UINT64_MAX
> +# elif ULONG_MAX == UINT64_MAX
>   #  define W_TYPE_SIZE 64
> -# elif UINTMAX_MAX == UINT128_MAX
> +# elif ULONG_MAX == UINT128_MAX
>   #  define W_TYPE_SIZE 128
>   # endif
>
> In this particular case that would enable
> the 32 bit ppc assembly in longlong.h

If you do the above, you hit run time assertions like:

$ factor 2123123123123123123123
factor: factor.c:974: mulredc2: Assertion `(a1 >> (32 - 1)) == 0' failed.
32767000000000000000000:Aborted

which makes sense as we're using uintmax_t throughout factor.c.
But something general is required here rather than fixing
up every architecture. I've just noticed that sparc v9 in 32 bit mode
will also loop for ever with the above command with what's in
place in longlong.h at present.

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.

thanks,
Pádraig.

Message #35 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 13030 <at> debbugs.gnu.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Mon, 07 Jan 2013 02:09:12 +0000

[Message part 1 (text/plain, inline)]

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.

[factor-asm-general.diff (text/x-patch, attachment)]

Message #38 received at 13030 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 13030 <at> debbugs.gnu.org
Subject: Re: bug#13030: factor: infinite loop on Linux/powerpc
Date: Mon, 07 Jan 2013 07:17:39 +0100

Pádraig Brady wrote:
...
> No comments, so I'm pushing the attached.
...
> 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.

Nice work.  Thanks!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.