develooper Front page | perl.perl5.porters | Postings from September 2013

[perl #119753] HAS_QUAD and 32 bit IVs on platforms with "quad"s

Thread Next
From:
Nicholas Clark
Date:
September 12, 2013 10:36
Subject:
[perl #119753] HAS_QUAD and 32 bit IVs on platforms with "quad"s
Message ID:
rt-3.6.HEAD-1873-1378982157-1142.119753-75-0@perl.org
# New Ticket Created by  Nicholas Clark 
# Please include the string:  [perl #119753]
# in the subject line of all future correspondence about this issue. 
# <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=119753 >


Right now, perl.h has this, if USE_64_BIT_INT is not defined:

#  if IVSIZE == 8
#    define IV_IS_QUAD
#    define UV_IS_QUAD
#    ifndef HAS_QUAD
#      define HAS_QUAD
#    endif
#  else
#    undef IV_IS_QUAD
#    undef UV_IS_QUAD
#    undef HAS_QUAD
#  endif


The upshot is that HAS_QUAD is undefined if IVs are 32 bits
(and defined if IVs are 64 bits)

This completely ignores the description of HAS_QUAD

/* HAS_QUAD:
 *	This symbol, if defined, tells that there's a 64-bit integer type,
 *	Quad_t, and its unsigned counterpart, Uquad_t. QUADKIND will be one
 *	of QUAD_IS_INT, QUAD_IS_LONG, QUAD_IS_LONG_LONG, QUAD_IS_INT64_T,
 *	or QUAD_IS___INT64.
 */


because on any 32 bit platform with a long long type, HAS_QUAD is getting
undefined for a default build, even though there *is* a 64 bit type.

That line dates from the creation of HAS_QUAD:

commit de1c2614758a00c0d93fc45414417a54cdf923b3
Author: Jarkko Hietaniemi <jhi@iki.fi>
Date:   Sat Oct 30 12:41:50 1999 +0000

    Add HAS_QUAD ($Config{d_quad}); use it.
    
    p4raw-id: //depot/cfgperl@4497


It's never been changed. It feels like it's wrong as-is, in that we ought
to leave HAS_QUAD defined, so that XS code can take advantage of 64 bit types
if present.


Life is not that simple.

If you simply remove that line on a 32 bit platform built with -Duse64bitint,
the build fails. utf8.h makes the choice between 7 or 13 byte extended
sequences depending on HAS_QUAD, but the "HAS_QUAD" code then assumes a
different macro is defined, which is only defined for 64 bit UVs.

See http://perl5.git.perl.org/perl.git/commitdiff/aa9d3145482dc64c
(on the branch smoke-me/nicholas/keep-HAS_QUAD-for-use64bitint)

Fix that and the build completes, but tests for sprintf and pack fail.
The printf tests fail because printf's parsing of "long long" formats is
conditional on 64 bit support:

$ perl -MConfig -e 'printf "%d %lld\n", $Config{ivsize}, $Config{uvsize}'
8 8
$ ./perl -Ilib -MConfig -e 'printf "%d %lld\n", $Config{ivsize}, $Config{uvsize}'
4 %lld


(which I think is pretty ugly in the first place), and *that* is expressed
(badly) in terms of HAS_QUAD, which is being used as a proxy for "are IVs
64 bits". So in my branch I fixed that:

commit 186abcd33d91a27f2c796b5e49ade2d7db02c8af
Author: Nicholas Clark <nick@ccl4.org>
Date:   Wed Sep 11 12:09:25 2013 +0100

    Use IVSIZE not HAS_QUAD to enable "long long" formats in sv_vcatpvfn_flags()

    Without this, enabling HAS_QUAD on 32 bit IV systems causes a lot of test
    failures. Potentially those failures could be addressed, but it seems like
    a lot more work for little gain.


The pack failures are more frustrating still. The 'q' and 'Q' pack formats
are fatal errors on a perl with 32 bit IVs:

$ perl -le 'print length pack "q", ~0'
8
$ ./perl -le 'print length pack "q", ~0'
Invalid type 'q' in pack at -e line 1.

but the C code in pp_pack.c (again) is enabled with HAS_QUAD, not a test of
IVSIZE. The code itself is written on the assumption that it will be enabled
with 32 bit IVs, because it falls back to NVs. This works:

#ifdef HAS_QUAD
	case 'q':
	    while (len-- > 0) {
		Quad_t aquad;
                SHIFT_VAR(utf8, s, strend, aquad, datumtype, needs_swap);
		if (!checksum)
                    mPUSHs(aquad >= IV_MIN && aquad <= IV_MAX ?
			   newSViv((IV)aquad) : newSVnv((NV)aquad));
		else if (checksum > bits_in_uv)
		    cdouble += (NV)aquad;
		else
		    cuv += aquad;
	    }
	    break;


The problem is that the pack tests were never written to cope with this, and
fail. They could be fixed. More troubling though, is that t/op/64bitint.t
also fails, because it starts like this:

    eval { my $q = pack "q", 0 };
    skip_all('no 64-bit types') if $@;


It (sadly) seems to be a moderately common idiom on CPAN to determine whether
IVs are 64 bit by using pack 'q' rather than the (more) correct approach of
asking %Config. (Heck, or just comparing ~0 with 0xFFFFFFFF)

http://grep.cpan.me/?q=eval.%2Bpack%5B%5EA-Za-z0-9%5D%2B%5BqQ%5D

(not the best search term, but gets rid of most the false positives. Looks
like at least 10 modules use this approach, and would break)

So I think that 'q' and 'Q' support has to stay coupled to IV size. :-(

commit b657af64fed8879be76f4ad97527108350c0d559
Author: Nicholas Clark <nick@ccl4.org>
Date:   Wed Sep 11 12:12:25 2013 +0100

    Use IVSIZE not HAS_QUAD to enable 'q' and 'Q' formats in pack.

    Whilst the code for 'q' and 'Q' in pp_pack is itself well behaved if enabled
    on a perl with 32 bit IVs (using SvNV instead of SvIV and SvUV), the
    regression tests are not. Several tests use an eval of "pack 'q'" to
    determine if 64 bit integer support is available (instead of
    $Config{ivsize}), and t/op/pack.t fails many tests. While these could be
    fixed (or skipped), unfortunately the approach of evaling "pack 'q'" is
    fairly popular on CPAN, so the breakage isn't just in the perl core, and
    might also be present in code we can't see or submit patches for.


At which point you can keep HAS_QUAD and the test pass. As to CPAN...

There are around 20 distributions using HAS_QUAD in ways that might effect
their code:

http://grep.cpan.me/?q=HAS_QUAD%5Cb+-file%3Appport.h+-file%3Aconfig_H.gc

Most build and pass tests on an unmodified perl. Of those that do, two
fail if HAS_QUAD is enabled, Math::Prime::Util and Data::BitStream::XS.
(from the same author). Both seem to be carefully written, with the intent of
actually making use of the 64 bit type if present. Math::Prime::Util makes
the same mistake as the perl core repeats, and needs this change to work:

$ diff -u ptypes.h~ ptypes.h
--- ptypes.h~   2013-03-11 04:29:54.000000000 +0000
+++ ptypes.h    2013-09-11 09:03:57.000000000 +0100
@@ -50,7 +50,7 @@
 #endif
 
 
-#ifdef HAS_QUAD
+#if UVSIZE >= 8
   #define BITS_PER_WORD  64
   #define UVCONST(x)     U64_CONST(x)
 #else


Data::BitStream::XS defines a type WTYPE, which is intended to be the widest
type available. Unfortunately, if HAS_QUAD changes to being defined on an
installation with 32 bit UVs, it fails to compile due to a lot of prototype
mismatches. eg:

sequences.c:UV prime_count_lower(WTYPE x)
sequences.h:extern WTYPE prime_count_lower(WTYPE x);


This ought to be fixable by correcting all the function definitions to be
WTYPE, not UV. eg:

$ diff -u sequences.c~ sequences.c
--- sequences.c~        2012-06-02 04:40:04.000000000 +0100
+++ sequences.c 2013-09-11 09:12:52.000000000 +0100
@@ -225,7 +225,7 @@
 #define NPRIME_COUNT_SMALL  (sizeof(prime_count_small)/sizeof(prime_count_small[0]))
 
 static const float F1 = 1.0;
-UV prime_count_lower(WTYPE x)
+WTYPE prime_count_lower(WTYPE x)
 {
   float fx, flogx;
   float a = 1.80;

[and quite a lot more:
 sequences.c |   39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)
]

which fixes the build, but the tests fail, due (much like t/op/pack.t) to
assuming that Perl-space integer maths can be done to the same precision
as C maths. So the fix ends up being this:

$ diff -u wtype.h~ wtype.h
--- wtype.h~    2012-06-01 09:09:19.000000000 +0100
+++ wtype.h     2013-09-11 13:28:19.000000000 +0100
@@ -41,7 +41,12 @@
 #endif
 
 
-#ifdef HAS_QUAD
+/* Effectively this forces WTYPE to always be the same type as UV.
+   Given the current codebase, this isn't actually a constraint. The current
+   codebase has functions with prototypes which declare UV, but the definitions
+   use WTYPE, which means that the compilation already fails if the two types
+   differ.  */
+#if UVSIZE >= 8
   typedef U64TYPE WTYPE;
   #define W_CONST(c)  U64_CONST(c)
   #define WTYPE_IS_64BIT   1


With this, all CPAN modules that build on (unmodified) blead also build with
the change.


But whilst I think that it is worth bringing the other changes on that branch
back to blead, I'm not sure that it's worth actually doing this part:

commit 81775faa427fb3dcb687559381d9a5cb616d20b3
Author: Nicholas Clark <nick@ccl4.org>
Date:   Wed Sep 11 12:20:05 2013 +0100

    Don't undefine HAS_QUAD with 32 bit IVs on a platform with a 64 bit type.
    
    When commit de1c2614758a00c0 added support for $Config{d_quad} and HAS_QUAD
    in Oct 1999, it also undefined HAS_QUAD. The presence of a 64 bit type
    doesn't directly affect the choice of using 32 or 64 bit types for IVs, so
    undefining this simply throws away information which might be of use to C
    and XS code.

diff --git a/perl.h b/perl.h
index e50a4ce..4a59e94 100644
--- a/perl.h
+++ b/perl.h
@@ -1610,7 +1610,6 @@ typedef UVTYPE UV;
 #  else
 #    undef IV_IS_QUAD
 #    undef UV_IS_QUAD
-#    undef HAS_QUAD
 #  endif
 #endif
 


It would have been 10 years ago. But these days, pretty much everything which
wants 64 bit integer support is simply using long long:

http://grep.cpan.me/?q=long+long%5B%5E%27%5D%5B%5E%5Cn%5D*%3B+-file%3Appport.h

(233 distributions found, but a few are false positives)


So the change would break about 0.02% of CPAN distributions, for I'm not sure
how much gain. Do we actually want to make use of "Quad" in the core on 32
bit builds?

Nicholas Clark


Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About