From @jhi

Minor nit noticed in passing​: does not make sense to use PTRSIZE in
runtime when we can do it in compiletime (perl.c​:perl_parse(), in the
computation of Perl_origalen)

Patch attached.

From @jhi

From 34182552ece7d209f632ba15d8feb44105da416a Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Wed, 30 Apr 2014 07:49:07 -0400
Subject: [PATCH] Use PTRSIZE in compiletime, not runtime.

---
 perl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/perl.c b/perl.c
index 27d0d9e..1b9b429 100644
--- a/perl.c
+++ b/perl.c
@@ -1496,8 +1496,7 @@ perl_parse(pTHXx_ XSINIT_t xsinit, int argc, char **argv, char **env)
 	 * --jhi */
 	 const char *s = NULL;
 	 int i;
-	 const UV mask =
-	   ~(UV)(PTRSIZE == 4 ? 3 : PTRSIZE == 8 ? 7 : PTRSIZE == 16 ? 15 : 0);
+	 const UV mask = ~(UV)(PTRSIZE-1);
          /* Do the mask check only if the args seem like aligned. */
 	 const UV aligned =
 	   (mask < ~(UV)0) && ((PTR2UV(argv[0]) & mask) == PTR2UV(argv[0]));
-- 
1.9.2

From @tonycoz

On Wed Apr 30 04​:54​:26 2014, jhi wrote​:

Minor nit noticed in passing​: does not make sense to use PTRSIZE in
runtime when we can do it in compiletime (perl.c​:perl_parse(), in the
computation of Perl_origalen)

Patch attached.

They're both compile-time, your version is certainly terser.

I'm not sure the alignments used here are correct, the preferred alignment for x86_64 (64-bit, 8 byte pointers) is 16 bytes[1], though whether anyone bothers to align argv[] on 16-byte boundaries, I don't know.

Tony

[1] some SSE instructions require 16-byte alignment

The RT System itself - Status changed from 'new' to 'open'

From @jhi

On Thursday-201405-01, 2​:18, Tony Cook via RT wrote​:

They're both compile-time, your version is certainly terser.

Argh, yes, right.

I'm not sure the alignments used here are correct, the preferred alignment for x86_64 (64-bit, 8 byte pointers) is 16 bytes[1], though whether anyone bothers to align argv[] on 16-byte boundaries, I don't know.

Well, my change is not at least not making the alignments different...
(except in the fallback case of not-4/8/16, which likely never to be
reached, unless someone tries to compile Perl on 6502/Z80...) I don't
know whether we have anything for "preferred alignment of pointers" in
our config vars.

That being said, the whole environment peeking/poking code is horrendous.

From @jhi

On Thursday-201405-01, 6​:43, Jarkko Hietaniemi wrote​:

On Thursday-201405-01, 2​:18, Tony Cook via RT wrote​:

They're both compile-time, your version is certainly terser.

Argh, yes, right.

I'm not sure the alignments used here are correct, the preferred alignment for x86_64 (64-bit, 8 byte pointers) is 16 bytes[1], though whether anyone bothers to align argv[] on 16-byte boundaries, I don't know.

Well, my change is not at least not making the alignments different...
(except in the fallback case of not-4/8/16, which likely never to be
reached, unless someone tries to compile Perl on 6502/Z80...) I don't
know whether we have anything for "preferred alignment of pointers" in
our config vars.

That being said, the whole environment peeking/poking code is horrendous.

Better described patch attached.

From @jhi

From a54bfd42793134ffec3b400cef07845fcf2c5f48 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Wed, 30 Apr 2014 07:49:07 -0400
Subject: [PATCH] For ptr masking use PTRSIZE-1, not nested conditional
 statement.

---
 perl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/perl.c b/perl.c
index 27d0d9e..1b9b429 100644
--- a/perl.c
+++ b/perl.c
@@ -1496,8 +1496,7 @@ perl_parse(pTHXx_ XSINIT_t xsinit, int argc, char **argv, char **env)
 	 * --jhi */
 	 const char *s = NULL;
 	 int i;
-	 const UV mask =
-	   ~(UV)(PTRSIZE == 4 ? 3 : PTRSIZE == 8 ? 7 : PTRSIZE == 16 ? 15 : 0);
+	 const UV mask = ~(UV)(PTRSIZE-1);
          /* Do the mask check only if the args seem like aligned. */
 	 const UV aligned =
 	   (mask < ~(UV)0) && ((PTR2UV(argv[0]) & mask) == PTR2UV(argv[0]));
-- 
1.9.2

From @tonycoz

On Thu May 01 17​:44​:59 2014, jhi wrote​:

Better described patch attached.

Added to the ever growing 5.21.1 blockers list

Tony

From @tsee

Thanks, second version of the patch applied as b7249aa.

@tsee - Status changed from 'open' to 'resolved'