Skip Menu |

Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Fri, 4 Jan 2013 12:36:32 -0500
To: perlbug [...] perl.org
From: bulk88 <bulk88 [...] hotmail.com>
See attached patch.
From 7e84c731682a55cf3f8f88caa599c844c8151b8f Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Fri, 4 Jan 2013 12:34:33 -0500 Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v |SvPVX(vstringify(PL_patchlevel))| is the same string as |"v" PERL_VERSION_STRING|. No need to go through the overhead of using a version object. Instead of creating a SV, then further manipulating it. Create and manipulate it at the same time with Perl_newSVpvf_nocontext or newSVpvn. "num_len >= level_len " will be folded away by the compiler, previously, since both lengths came from SVs, they were not const technically. A very smart compiler might see strncmp/strnEQ takes all const arguments, and will optimize that away also. VC 2003 didnt do that. Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons. With the copyright lines, use more CPP cating. This reduced the number of instructions, and removes a couple alignment null in .rdata image section. UNDER_CE presents a challange because of the wce_hitreturn(); call. BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains a perlio printf call. So that can not be CPP catted unfortunatly. If a build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright line is catted with the license line. Prior to this patch, S_minus_v was 0x129 bytes long of x86-32 VC 2003 -O1 -GL machine code. After it is 0x70 bytes long. Although S_minus_v is not hot and is rarely used, making the interp image smaller, and therefore faster is a good idea. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. --- perl.c | 102 ++++++++++++++++++++++++++++++++-------------------------------- 1 files changed, 51 insertions(+), 51 deletions(-) diff --git a/perl.c b/perl.c index 0cfb73c..7589e1f 100644 --- a/perl.c +++ b/perl.c @@ -3414,32 +3414,38 @@ Perl_moreswitches(pTHX_ const char *s) STATIC void S_minus_v(pTHX) { + PerlIO * PIO_stdout; - if (!sv_derived_from(PL_patchlevel, "version")) - upg_version(PL_patchlevel, TRUE); #if !defined(DGUX) { - SV* level= vstringify(PL_patchlevel); + const char * const level_str = "v" PERL_VERSION_STRING; + const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1; #ifdef PERL_PATCHNUM + SV* level; # ifdef PERL_GIT_UNCOMMITTED_CHANGES - SV *num = newSVpvs(PERL_PATCHNUM "*"); + static const char num [] = PERL_PATCHNUM "*"; # else - SV *num = newSVpvs(PERL_PATCHNUM); + static const char num [] = PERL_PATCHNUM; # endif { - STRLEN level_len, num_len; - char * level_str, * num_str; - num_str = SvPV(num, num_len); - level_str = SvPV(level, level_len); - if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) { - SvREFCNT_dec(level); - level= num; + const STRLEN num_len = sizeof(num)-1; + /* A very advanced compiler would fold away the strnEQ + and this whole conditional, but most (all?) won't do it. + SV level could also be replaced by with preprocessor + catenation. + */ + if (num_len >= level_len && strnEQ(num,level_str,level_len)) { + /* per 46807d8e80, PERL_PATCHNUM is outside of the control + of the interp so it might contain format characters + */ + level = Perl_newSVpvf_nocontext("%s", num); } else { - Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num); - SvREFCNT_dec(num); + level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num); } } - #endif +#else + SV* level = newSVpvn(level_str, level_len); +#endif /* #ifdef PERL_PATCHNUM */ PIO_stdout = PerlIO_stdout(); PerlIO_printf(PIO_stdout, "\nThis is perl " STRINGIFY(PERL_REVISION) @@ -3447,14 +3453,13 @@ S_minus_v(pTHX) ", subversion " STRINGIFY(PERL_SUBVERSION) " (%"SVf") built for " ARCHNAME, level ); - SvREFCNT_dec(level); + SvREFCNT_dec_NN(level); } #else /* DGUX */ PIO_stdout = PerlIO_stdout(); /* Adjust verbose output as in the perl that ships with the DG/UX OS from EMC */ PerlIO_printf(PIO_stdout, - Perl_form(aTHX_ "\nThis is perl, %"SVf"\n", - SVfARG(vstringify(PL_patchlevel)))); + Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n"); PerlIO_printf(PIO_stdout, Perl_form(aTHX_ " built under %s at %s %s\n", OSNAME, __DATE__, __TIME__)); @@ -3472,47 +3477,42 @@ S_minus_v(pTHX) #endif PerlIO_printf(PIO_stdout, - "\n\nCopyright 1987-2012, Larry Wall\n"); -#ifdef MSDOS - PerlIO_printf(PIO_stdout, - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); -#endif -#ifdef DJGPP - PerlIO_printf(PIO_stdout, + "\n\nCopyright 1987-2012, Larry Wall\n" +#if defined(MSDOS) + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" +#elif defined(DJGPP) "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); -#endif -#ifdef OS2 - PerlIO_printf(PIO_stdout, + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" +#elif defined(OS2) "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); -#endif -#ifdef OEMVS - PerlIO_printf(PIO_stdout, - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); -#endif -#ifdef __VOS__ - PerlIO_printf(PIO_stdout, - "Stratus VOS port by Paul.Green@stratus.com, 1997-2002\n"); -#endif -#ifdef POSIX_BC - PerlIO_printf(PIO_stdout, - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); -#endif -#ifdef UNDER_CE - PerlIO_printf(PIO_stdout, + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" +#elif defined(__BEOS__) + "BeOS port Copyright Tom Spindler, 1997-1999\n" +#elif defined(OEMVS) + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" +#elif defined(__VOS__) + "Stratus VOS port by Paul.Green@stratus.com, 1997-2002\n" +#elif defined(POSIX_BC) + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" +/* UNDER_CE closes the printf call*/ +#elif defined(UNDER_CE) "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#elif defined(__SYMBIAN32__) + "Symbian port by Nokia, 2004-2005\n" +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif BINARY_BUILD_NOTICE; -#endif PerlIO_printf(PIO_stdout, +#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */ + PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ -- 1.7.9.msysgit.0
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Fri Jan 04 09:37:10 2013, bulk88 wrote: Show quoted text
> See attached patch.
This patch no longer applies. I can see a couple of places it could be improved: + level = Perl_newSVpvf_nocontext("%s", num); Couldn't this just be: level = newSVpvn(num, num_len); PerlIO_printf(PIO_stdout, - Perl_form(aTHX_ "\nThis is perl, %"SVf"\n", - SVfARG(vstringify(PL_patchlevel)))); + Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n"); Why is Perl_form() being called here? #elif defined(UNDER_CE) "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#elif defined(__SYMBIAN32__) + "Symbian port by Nokia, 2004-2005\n" +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif BINARY_BUILD_NOTICE; -#endif PerlIO_printf(PIO_stdout, +#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */ + PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif The contortions to handle UNDER_CE ending the PerlIO_printf() are confusing. Wouldn't it be better to put the wce_hitreturn() in it's own #ifdef UNDER_CE ? Tony
RT-Send-CC: perl5-porters [...] perl.org
On Fri Jul 05 00:01:09 2013, tonyc wrote: Show quoted text
> On Fri Jan 04 09:37:10 2013, bulk88 wrote:
> > See attached patch.
> > This patch no longer applies.
I'll have to work on it then. Show quoted text
> > I can see a couple of places it could be improved: > > + level = Perl_newSVpvf_nocontext("%s", num); > > Couldn't this just be: > > level = newSVpvn(num, num_len);
No for code size reasons. There is a 2nd Perl_newSVpvf_nocontext below in the other branch. Compiler will put params on C stack then jump to the "call Perl_newSVpvf_nocontext" instruction. The "call Perl_newSVpvf_nocontext" is shared by both branches. Perl_newSVpvf_nocontext takes 2 params, newSVpvn takes 3 params on ithreads. Show quoted text
> > PerlIO_printf(PIO_stdout, > - Perl_form(aTHX_ "\nThis is perl, %"SVf"\n", > - SVfARG(vstringify(PL_patchlevel)))); > + Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n"); > > Why is Perl_form() being called here?
Good catch. It is redundant. Show quoted text
> > #elif defined(UNDER_CE) > "WINCE port by Rainer Keuchel, 2001-2002\n" > "Built on " __DATE__ " " __TIME__ "\n\n"); > wce_hitreturn(); > -#endif > -#ifdef __SYMBIAN32__ > - PerlIO_printf(PIO_stdout, > - "Symbian port by Nokia, 2004-2005\n"); > -#endif > +#elif defined(__SYMBIAN32__) > + "Symbian port by Nokia, 2004-2005\n" > +#endif /* end of additional copyright lines */ > + > #ifdef BINARY_BUILD_NOTICE > +# ifndef UNDER_CE > + ); /* close Larry Wall copyright printf */ > +# endif > BINARY_BUILD_NOTICE; > -#endif > PerlIO_printf(PIO_stdout, > +#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */ > + PerlIO_printf(PIO_stdout, > +/* else CPP catenation continues from Larry Wall copyright printf */ > +#endif > > The contortions to handle UNDER_CE ending the PerlIO_printf() are > confusing. Wouldn't it be better to put the wce_hitreturn() in it's own > #ifdef UNDER_CE ?
So confusing I need more time to think up an answer. I have to try to get this to apply to blead so I will figure out what can be done about UNDER_CE when I try to compile the code. Show quoted text
> > Tony
-- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 808b
Patch rewritten for blead. Some changes from the last old one is the LOCAL_PATCH_COUNT stuff was changed, and I tried to minimize diff size by keeping the "#ifdef OSNAME #endif #ifdef OSNAME #endif" tree the same. In old patch the OS names were rearranged into "#if #elif #elif" tree. Also the whitespace/indents on the double quote lines was kept the same even if it was wrong. Symbian and Win CE conditionals were swapped in new patch to make more sense. The new LOCAL_PATCH_COUNT code is a compromise to not put the GPL and copyright strings into macros and having the GPL and copyright literals appear 3 times in preprocessed output (only the C compiler can pick which string to use since LOCAL_PATCH_COUNT isn't a preprocessor constant and takes the size of a struct in its expansion from patchlevel.h).
From 4acb8e4ecbbd1691ad8e83c1a8a8e57a94fd97d3 Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Fri, 5 Jul 2013 19:12:29 -0400 Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v |SvPVX(vstringify(PL_patchlevel))| is the same string as |"v" PERL_VERSION_STRING|. No need to go through the overhead of using a version object. Instead of creating a SV, then further manipulating it. Create and manipulate it at the same time with Perl_newSVpvf_nocontext or newSVpvn. "num_len >= level_len " will be folded away by the compiler, previously, since both lengths came from SVs, they were not const technically. A very smart compiler might see strncmp/strnEQ takes all const arguments, and will optimize that away also. VC 2003 didnt do that. Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons. With the copyright lines, use more CPP cating. This reduced the number of instructions, and removes a couple alignment null in .rdata image section. UNDER_CE presents a challange because of the wce_hitreturn(); call. BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains a perlio printf call. So that can not be CPP catted unfortunatly. If a build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright line is catted with the license line. Prior to this patch, S_minus_v was 0x13b bytes long of x86-32 VC 2003 -O1 -GL machine code. After it is 0x77 bytes long. Although S_minus_v is not hot and is rarely used, making the interp image smaller, and therefore faster is a good idea. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. --- perl.c | 95 ++++++++++++++++++++++++++++++++++----------------------------- 1 files changed, 51 insertions(+), 44 deletions(-) diff --git a/perl.c b/perl.c index bad66f5..2198205 100644 --- a/perl.c +++ b/perl.c @@ -3502,30 +3502,35 @@ STATIC void S_minus_v(pTHX) { PerlIO * PIO_stdout; - if (!sv_derived_from(PL_patchlevel, "version")) - upg_version(PL_patchlevel, TRUE); { - SV* level= vstringify(PL_patchlevel); + const char * const level_str = "v" PERL_VERSION_STRING; + const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1; #ifdef PERL_PATCHNUM + SV* level; # ifdef PERL_GIT_UNCOMMITTED_CHANGES - SV *num = newSVpvs(PERL_PATCHNUM "*"); + static const char num [] = PERL_PATCHNUM "*"; # else - SV *num = newSVpvs(PERL_PATCHNUM); + static const char num [] = PERL_PATCHNUM; # endif { - STRLEN level_len, num_len; - char * level_str, * num_str; - num_str = SvPV(num, num_len); - level_str = SvPV(level, level_len); - if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) { - SvREFCNT_dec(level); - level= num; + const STRLEN num_len = sizeof(num)-1; + /* A very advanced compiler would fold away the strnEQ + and this whole conditional, but most (all?) won't do it. + SV level could also be replaced by with preprocessor + catenation. + */ + if (num_len >= level_len && strnEQ(num,level_str,level_len)) { + /* per 46807d8e80, PERL_PATCHNUM is outside of the control + of the interp so it might contain format characters + */ + level = Perl_newSVpvf_nocontext("%s", num); } else { - Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num); - SvREFCNT_dec(num); + level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num); } } - #endif +#else + SV* level = newSVpvn(level_str, level_len); +#endif /* #ifdef PERL_PATCHNUM */ PIO_stdout = PerlIO_stdout(); PerlIO_printf(PIO_stdout, "\nThis is perl " STRINGIFY(PERL_REVISION) @@ -3533,59 +3538,61 @@ S_minus_v(pTHX) ", subversion " STRINGIFY(PERL_SUBVERSION) " (%"SVf") built for " ARCHNAME, level ); - SvREFCNT_dec(level); + SvREFCNT_dec_NN(level); } -#if defined(LOCAL_PATCH_COUNT) - if (LOCAL_PATCH_COUNT > 0) - PerlIO_printf(PIO_stdout, - "\n(with %d registered patch%s, " - "see perl -V for more detail)", - LOCAL_PATCH_COUNT, - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); -#endif - PerlIO_printf(PIO_stdout, - "\n\nCopyright 1987-2013, Larry Wall\n"); +#if defined(LOCAL_PATCH_COUNT) + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */ + LOCAL_PATCH_COUNT == 1 ? + "\n(with %d registered patch, see perl -V for more detail)%s" + : LOCAL_PATCH_COUNT > 0 ? + "\n(with %d registered patches, see perl -V for more detail)%s" + : "%.0s%s", + LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, +#endif + "\n\nCopyright 1987-2013, Larry Wall\n" #ifdef MSDOS - PerlIO_printf(PIO_stdout, - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" #endif #ifdef DJGPP - PerlIO_printf(PIO_stdout, "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" #endif #ifdef OS2 - PerlIO_printf(PIO_stdout, "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" #endif #ifdef OEMVS - PerlIO_printf(PIO_stdout, - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" #endif #ifdef __VOS__ - PerlIO_printf(PIO_stdout, - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" #endif #ifdef POSIX_BC - PerlIO_printf(PIO_stdout, - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" +#endif +#ifdef __SYMBIAN32__ + "Symbian port by Nokia, 2004-2005\n" #endif +/* UNDER_CE closes the printf call*/ #ifdef UNDER_CE - PerlIO_printf(PIO_stdout, "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ BINARY_BUILD_NOTICE; #endif +/* because of wce_hitreturn() open new printf for UNDER_CE */ +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ -- 1.7.9.msysgit.0
CC: perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Sat, 6 Jul 2013 15:43:56 +0100
To: bulk88 via RT <perlbug-followup [...] perl.org>
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 1.1k
On Fri, Jul 05, 2013 at 08:34:01AM -0700, bulk88 via RT wrote: Show quoted text
> On Fri Jul 05 00:01:09 2013, tonyc wrote:
> > I can see a couple of places it could be improved: > > > > + level = Perl_newSVpvf_nocontext("%s", num); > > > > Couldn't this just be: > > > > level = newSVpvn(num, num_len);
> > No for code size reasons. There is a 2nd Perl_newSVpvf_nocontext below > in the other branch. Compiler will put params on C stack then jump to > the "call Perl_newSVpvf_nocontext" instruction. The "call > Perl_newSVpvf_nocontext" is shared by both branches. > Perl_newSVpvf_nocontext takes 2 params, newSVpvn takes 3 params on ithreads.
I really don't like this part. Its just an excessive level of micro-optimisation for code that's only called once at most, and will have a negligible effect on the overall binary size. On x86_64 platforms with the "first 6 args are in registers" calling convention, using newSVpvn() will likely involve no more code, since my_perl will already be in the relevant register; while on x86, its likely just a push of a register value. The downside is that the src code becomes more bloated and confusing. -- Fire extinguisher (n) a device for holding open fire doors.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 5.1k
On Sat Jul 06 07:45:05 2013, davem wrote: Show quoted text
> On x86_64 platforms with the "first 6 args are in registers" calling > convention, using newSVpvn() will likely involve no more code, since > my_perl will already be in the relevant register; while on x86, its > likely > just a push of a register value. >
On Win64 that is wrong. my_perl has to be copied every time to the 1st callee register by the caller from the caller's non-vol register that it saved or caller's c stack auto. There will always be an assignment to first register (callee's my_perl) on x64 on cdecl for each perl func call. The calling convention does not say anywhere that a callee must keep incoming params in registers nonvolatile. What if the callee doesn't need to every use my_perl again until it returns and calls strlen which doesn't take a my_perl? The caller just lost their my_perl pointer. Keeping func params const on assembly level, to avoid copying in the caller for each call, would require a machine code JITer and/or random calling conventions. Random calling conventions are implemented in some C compilers for Windows, but only for Win32, not Win64. http://lists.cs.uiuc.edu/pipermail/llvmdev/2009-September/025927.html http://nondot.org/sabre/LLVMNotes/GlobalRegisterVariables.txt Here is the boot func from re.dll Show quoted text
_______________________________________________________________ 0000000010001B20 sub rsp,48h 0000000010001B24 mov rax,qword ptr [rcx+70h] 0000000010001B28 mov rdx,qword ptr [rcx+18h] 0000000010001B2C mov qword ptr [rsp+60h],rbx 0000000010001B31 mov rbx,qword ptr [rcx] 0000000010001B34 mov qword ptr [rsp+68h],rbp 0000000010001B39 mov qword ptr [rsp+40h],rsi <<<<<< save non vol 0000000010001B3E mov qword ptr [rsp+38h],rdi 0000000010001B43 movsxd rdi,dword ptr [rax] 0000000010001B46 add rax,0FFFFFFFFFFFFFFFCh 0000000010001B4A lea r9,[rdx+rdi*8] 0000000010001B4E mov qword ptr [rcx+70h],rax 0000000010001B52 inc edi 0000000010001B54 sub rbx,r9 0000000010001B57 movsxd rbp,edi 0000000010001B5A lea r8,[string "v5.14.0" (1003D378h)] 0000000010001B61 mov rdx,qword ptr [rdx+rbp*8] 0000000010001B65 mov r9d,7 0000000010001B6B mov rsi,rcx <<<<<<<<<<<<<<<<<<<<<<<<<< incoming param copied to non-vol reg 0000000010001B6E sar rbx,3 0000000010001B72 call Perl_xs_apiversion_bootcheck (1000135Ah) 0000000010001B77 lea r9,[string "0.18" (1003D36Ch)] 0000000010001B7E mov r8d,edi 0000000010001B81 mov edx,ebx 0000000010001B83 mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<< 0000000010001B86 mov qword ptr [rsp+20h],4 0000000010001B8F call Perl_xs_version_bootcheck (10001348h) 0000000010001B94 lea r9,[string "re.c" (1003D380h)] 0000000010001B9B lea r8,[XS_re_install (10001A50h)] 0000000010001BA2 lea rdx,[string "re::install" (1003D360h)] 0000000010001BA9 mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<< 0000000010001BAC call Perl_newXS (1000134Eh) 0000000010001BB1 lea rax,[string "$" (1003A7DCh)] 0000000010001BB8 lea r9,[string "re.c" (1003D380h)] 0000000010001BBF lea r8,[XS_re_regmust (10001860h)] 0000000010001BC6 lea rdx,[string "re::regmust" (1003D350h)] 0000000010001BCD mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<< 0000000010001BD0 mov dword ptr [rsp+28h],0 0000000010001BD8 mov qword ptr [rsp+20h],rax 0000000010001BDD call Perl_newXS_flags (10001366h) 0000000010001BE2 mov r8,qword ptr [rsi+5A8h] 0000000010001BE9 mov rdi,qword ptr [rsp+38h] 0000000010001BEE mov rbx,qword ptr [rsp+60h] 0000000010001BF3 test r8,r8 0000000010001BF6 je boot_re+0E3h (10001C03h) 0000000010001BF8 mov edx,dword ptr [rsi+38h] 0000000010001BFB mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<< 0000000010001BFE call Perl_call_list (10001354h) 0000000010001C03 mov rax,qword ptr [rsi+18h] 0000000010001C07 lea rcx,[rsi+0BF8h] 0000000010001C0E mov qword ptr [rax+rbp*8],rcx 0000000010001C12 mov rax,qword ptr [rsi+18h] 0000000010001C16 lea rcx,[rax+rbp*8] 0000000010001C1A mov rbp,qword ptr [rsp+68h] 0000000010001C1F mov qword ptr [rsi],rcx 0000000010001C22 mov rsi,qword ptr [rsp+40h] <<<<<<<<<restore non vol 0000000010001C27 add rsp,48h 0000000010001C2B ret
____________________________________________________________
> I really don't like this part. Its just an excessive level of > micro-optimisation for code that's only called once at most, and will > have > a negligible effect on the overall binary size.
................
> The downside is that the src code becomes more bloated and confusing.
And this is why people give up, switch languages, then tell all their friends Perl [5 or Perl world] is dead. If the git repo is locked, code will never become more confusing. The assumption that progress is for communists and hippies is the death of Perl 5. -- bulk88 ~ bulk88 at hotmail.com
CC: "OtherRecipients of perl Ticket #116296:;" [...] smtp.indra.com, perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Sat, 06 Jul 2013 22:59:51 -0600
To: perlbug-followup [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 1.3k
On 07/06/2013 10:10 AM, bulk88 via RT wrote: Show quoted text
>> >I really don't like this part. Its just an excessive level of >> >micro-optimisation for code that's only called once at most, and will >> >have >> >a negligible effect on the overall binary size.
> ................
>> >The downside is that the src code becomes more bloated and confusing.
> And this is why people give up, switch languages, then tell all their > friends Perl [5 or Perl world] is dead. If the git repo is locked, code > will never become more confusing. The assumption that progress is for > communists and hippies is the death of Perl 5.
I agree with Dave, and I believe it is counterproductive to write code based on how current particular hardware works, when that code will long outlive the hardware and likely the whole underlying mechanism. But suppose we are wrong; suppose it has to be implemented the way you did. It's not at all obvious that it has to be that way. Therefore there should be comments in the code as to your reasoning. If not, the next guy might come along and not realize that this implementation has to be this particular way, and change it out. If indeed it is required to be this way, that might introduce subtle failures that the accompanying tests don't catch. It's unfair to those who follow to not comment such subtleties.
CC: perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Mon, 8 Jul 2013 16:19:33 +1000
To: bulk88 via RT <perlbug-followup [...] perl.org>
From: Tony Cook <tony [...] develop-help.com>
Download (untitled) / with headers
text/plain 3.8k
On Sat, Jul 06, 2013 at 09:10:12AM -0700, bulk88 via RT wrote: Show quoted text
> On Sat Jul 06 07:45:05 2013, davem wrote:
> > On x86_64 platforms with the "first 6 args are in registers" calling > > convention, using newSVpvn() will likely involve no more code, since > > my_perl will already be in the relevant register; while on x86, its > > likely > > just a push of a register value. > >
> > On Win64 that is wrong. my_perl has to be copied every time to the 1st > callee register by the caller from the caller's non-vol register that it > saved or caller's c stack auto. There will always be an assignment to > first register (callee's my_perl) on x64 on cdecl for each perl func > call. The calling convention does not say anywhere that a callee must > keep incoming params in registers nonvolatile. What if the callee > doesn't need to every use my_perl again until it returns and calls > strlen which doesn't take a my_perl? The caller just lost their my_perl > pointer. > > Keeping func params const on assembly level, to avoid copying in the > caller for each call, would require a machine code JITer and/or random > calling conventions. Random calling conventions are implemented in some > C compilers for Windows, but only for Win32, not Win64.
While Dave was wrong about the registers, his general argument about the micro-optimization is correct, MSVC isn't the only compiler. With gcc/clang the change *costs* 5 bytes[1] - while making the code a little harder to read. Both gcc and clang optimize out the conditional. Show quoted text
> > I really don't like this part. Its just an excessive level of > > micro-optimisation for code that's only called once at most, and will > > have > > a negligible effect on the overall binary size.
> ................
> > The downside is that the src code becomes more bloated and confusing.
> > And this is why people give up, switch languages, then tell all their > friends Perl [5 or Perl world] is dead. If the git repo is locked, code > will never become more confusing. The assumption that progress is for > communists and hippies is the death of Perl 5.
I think it's great that you're enthusiastic about optimizing perl's implementation, but treating a comment about fairly questionable optimization as an attack on all changes is reaching. We optimize for several things, generated code size is important, but not more important than a) developer time - the poor sob who comes along in 6 months or 5 years and needs to understand the code, b) performance. And code size doesn't map one-to-one to performance - several performance optimizations produce significantly larger code. Performance doesn't matter much for S_minus_v(), but developer time matters as much there as it does anywhere else. Tony [1] gcc, default Configure set optimization, -Duseithreads, git_version.h edited to match a tagged release, first Perl_newSVpvf_no_context: 00000000000050a0 <S_minus_v>: 50a0: 41 54 push %r12 50a2: be 00 00 00 00 mov $0x0,%esi 50a3: R_X86_64_32 .rodata+0x8ad 50a7: 31 c0 xor %eax,%eax 50a9: 55 push %rbp 50aa: 48 89 fd mov %rdi,%rbp 50ad: bf 00 00 00 00 mov $0x0,%edi 50ae: R_X86_64_32 .rodata.str1.1+0x55 50b2: 53 push %rbx 50b3: e8 00 00 00 00 callq 50b8 <S_minus_v+0x18> 50b4: R_X86_64_PC32 Perl_newSVpvf_nocontext+0xfffffffffffffffc Then newSVpvn(): 00000000000050a0 <S_minus_v>: 50a0: 41 54 push %r12 50a2: 31 d2 xor %edx,%edx 50a4: be 00 00 00 00 mov $0x0,%esi 50a5: R_X86_64_32 .rodata+0x8ad 50a9: 55 push %rbp 50aa: 53 push %rbx 50ab: 48 89 fb mov %rdi,%rbx 50ae: e8 00 00 00 00 callq 50b3 <S_minus_v+0x13> 50af: R_X86_64_PC32 Perl_newSVpvn+0xfffffffffffffffc
CC: perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Mon, 8 Jul 2013 16:52:53 +1000
To: bulk88 via RT <perlbug-followup [...] perl.org>
From: Tony Cook <tony [...] develop-help.com>
Download (untitled) / with headers
text/plain 7.9k
On Fri, Jul 05, 2013 at 04:29:29PM -0700, bulk88 via RT wrote: Show quoted text
> Prior to this patch, S_minus_v was 0x13b bytes long of x86-32 VC 2003 -O1 > -GL machine code. After it is 0x77 bytes long. Although S_minus_v is not > hot and is rarely used, making the interp image smaller, and therefore > faster is a good idea. There should not be any user visible changes to -v > with this patch. switches.t contains a regexp for -v already so no further > tests were added. > --- > perl.c | 95 ++++++++++++++++++++++++++++++++++----------------------------- > 1 files changed, 51 insertions(+), 44 deletions(-) > > diff --git a/perl.c b/perl.c > index bad66f5..2198205 100644 > --- a/perl.c > +++ b/perl.c > @@ -3502,30 +3502,35 @@ STATIC void > S_minus_v(pTHX) > { > PerlIO * PIO_stdout; > - if (!sv_derived_from(PL_patchlevel, "version")) > - upg_version(PL_patchlevel, TRUE); > { > - SV* level= vstringify(PL_patchlevel); > + const char * const level_str = "v" PERL_VERSION_STRING; > + const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1; > #ifdef PERL_PATCHNUM > + SV* level; > # ifdef PERL_GIT_UNCOMMITTED_CHANGES > - SV *num = newSVpvs(PERL_PATCHNUM "*"); > + static const char num [] = PERL_PATCHNUM "*"; > # else > - SV *num = newSVpvs(PERL_PATCHNUM); > + static const char num [] = PERL_PATCHNUM; > # endif > { > - STRLEN level_len, num_len; > - char * level_str, * num_str; > - num_str = SvPV(num, num_len); > - level_str = SvPV(level, level_len); > - if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) { > - SvREFCNT_dec(level); > - level= num; > + const STRLEN num_len = sizeof(num)-1; > + /* A very advanced compiler would fold away the strnEQ > + and this whole conditional, but most (all?) won't do it. > + SV level could also be replaced by with preprocessor > + catenation. > + */ > + if (num_len >= level_len && strnEQ(num,level_str,level_len)) { > + /* per 46807d8e80, PERL_PATCHNUM is outside of the control > + of the interp so it might contain format characters > + */ > + level = Perl_newSVpvf_nocontext("%s", num); > } else { > - Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num); > - SvREFCNT_dec(num); > + level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num); > } > } > - #endif > +#else > + SV* level = newSVpvn(level_str, level_len); > +#endif /* #ifdef PERL_PATCHNUM */
Except (as discussed) the use of newSVpvn(), I think it's good up to here. Show quoted text
> PIO_stdout = PerlIO_stdout(); > PerlIO_printf(PIO_stdout, > "\nThis is perl " STRINGIFY(PERL_REVISION) > @@ -3533,59 +3538,61 @@ S_minus_v(pTHX) > ", subversion " STRINGIFY(PERL_SUBVERSION) > " (%"SVf") built for " ARCHNAME, level > ); > - SvREFCNT_dec(level); > + SvREFCNT_dec_NN(level);
That's fine too. Show quoted text
> } > -#if defined(LOCAL_PATCH_COUNT) > - if (LOCAL_PATCH_COUNT > 0) > - PerlIO_printf(PIO_stdout, > - "\n(with %d registered patch%s, " > - "see perl -V for more detail)", > - LOCAL_PATCH_COUNT, > - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); > -#endif > - > PerlIO_printf(PIO_stdout, > - "\n\nCopyright 1987-2013, Larry Wall\n"); > +#if defined(LOCAL_PATCH_COUNT) > + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */ > + LOCAL_PATCH_COUNT == 1 ? > + "\n(with %d registered patch, see perl -V for more detail)%s" > + : LOCAL_PATCH_COUNT > 0 ? > + "\n(with %d registered patches, see perl -V for more detail)%s" > + : "%.0s%s", > + LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, > +#endif
This code is incorrect. For some compilers NULL is ((void *)0) - a pointer, not an integer. Consider - what's the type of the expression: LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, It's also unnecessarily duplicative, in that if the registered patches note needed to change, it will need to be changed in two places. gcc and clang complain about the type too, gcc: perl.c: In function ‘S_minus_v’: perl.c:3551:39: warning: pointer/integer type mismatch in conditional expression [enabled by default] perl.c:3596:9: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘void *’ [-Wformat] clang produces a more understandable message: perl.c:3551:9: warning: format specifies type 'int' but the argument has type 'void *' [-Wformat] LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./patchlevel.h:147:2: note: expanded from macro 'LOCAL_PATCH_COUNT' ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2)) ^ perl.c:3551:9: warning: format specifies type 'int' but the argument has type 'void *' [-Wformat] LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./patchlevel.h:147:2: note: expanded from macro 'LOCAL_PATCH_COUNT' ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2)) (I think it warns twice because you provide three different format strings, two of which don't match.) The original code is clearer I think. Show quoted text
> + "\n\nCopyright 1987-2013, Larry Wall\n" > #ifdef MSDOS > - PerlIO_printf(PIO_stdout, > - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); > + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" > #endif > #ifdef DJGPP > - PerlIO_printf(PIO_stdout, > "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" > - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); > + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" > #endif > #ifdef OS2 > - PerlIO_printf(PIO_stdout, > "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" > - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); > + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" > #endif > #ifdef OEMVS > - PerlIO_printf(PIO_stdout, > - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); > + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" > #endif > #ifdef __VOS__ > - PerlIO_printf(PIO_stdout, > - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); > + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" > #endif > #ifdef POSIX_BC > - PerlIO_printf(PIO_stdout, > - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); > + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" > +#endif > +#ifdef __SYMBIAN32__ > + "Symbian port by Nokia, 2004-2005\n" > #endif > +/* UNDER_CE closes the printf call*/ > #ifdef UNDER_CE > - PerlIO_printf(PIO_stdout, > "WINCE port by Rainer Keuchel, 2001-2002\n" > "Built on " __DATE__ " " __TIME__ "\n\n"); > wce_hitreturn(); > -#endif > -#ifdef __SYMBIAN32__ > - PerlIO_printf(PIO_stdout, > - "Symbian port by Nokia, 2004-2005\n"); > -#endif > +#endif /* end of additional copyright lines */ > + > #ifdef BINARY_BUILD_NOTICE > +# ifndef UNDER_CE > + ); /* close Larry Wall copyright printf */ > +# endif > +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ > BINARY_BUILD_NOTICE; > #endif > +/* because of wce_hitreturn() open new printf for UNDER_CE */ > +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) > PerlIO_printf(PIO_stdout, > +/* else CPP catenation continues from Larry Wall copyright printf */ > +#endif > "\n\ > Perl may be copied only under the terms of either the Artistic License or the\n\ > GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
Rather than trying to build it all into one printf, I think it would be more understandable if the printf() started at the Larry Wall line, and the wce_hitreturn() got its own extra #ifdef UNDER_CE, so: PerlIO_printf(PIO_stdout, larry's line #ifdef platform platform line #endif ... ); #ifdef UNDER_CE wce_hitreturn(); #endif Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 9.4k
On Sat Jul 06 22:00:36 2013, public@khwilliamson.com wrote: Show quoted text
> > I agree with Dave, and I believe it is counterproductive to write code > based on how current particular hardware works, when that code will long > outlive the hardware and likely the whole underlying mechanism. >
Does that apply to Perl? http://perl5.git.perl.org/perl.git/commit/99ee1dcd0469086e91a96e31a9b9ea27bb7f0c7e That commit message makes me think you (khw) do believe in writing code for current hardware. Some commits (all authors) talk about cache alignment and struct alignment. Perl already has a bias towards current hardware and not theoretical hardware from the future, and a strong "there is no C compiler but GCC bias" (why all the PERL_GCC_BRACE_GROUPS_FORBIDDEN code?). Arguing over what the future hardware will look like isn't productive so I won't go down that path. My point is Perl is biased towards today's processors, and today's OSes, and today's compilers, and biased towards different ones at the same time. When I hear "your favorites aren't my favorites so burn in hell", I see that commenter is wearing rose glasses and it is political/religious BS. Show quoted text
> But suppose we are wrong; suppose it has to be implemented the way you > did. It's not at all obvious that it has to be that way. Therefore > there should be comments in the code as to your reasoning. If not, the > next guy might come along and not realize that this implementation has > to be this particular way, and change it out. If indeed it is required > to be this way, that might introduce subtle failures that the > accompanying tests don't catch. It's unfair to those who follow to not > comment such subtleties. >
There is a comment that explains what is going on "+/* else CPP catenation continues from Larry Wall copyright printf */". That explains the design of what the CPP conditionals are doing and suggests to keep the CPP concating. On Sun Jul 07 23:20:13 2013, tonyc wrote: Show quoted text
> > While Dave was wrong about the registers, his general argument about > the micro-optimization is correct, MSVC isn't the only compiler. > > With gcc/clang the change *costs* 5 bytes[1] - while making the code a > little harder to read. Both gcc and clang optimize out the > conditional. >
Those are good compilers if they do that then but gcc/clang aren't the whole world. I will change the "level = Perl_newSVpvf_nocontext("%s", num);" branch to newSVpvn. Show quoted text
> > > The downside is that the src code becomes more bloated and confusing.
> > > > And this is why people give up, switch languages, then tell all their > > friends Perl [5 or Perl world] is dead. If the git repo is locked, code > > will never become more confusing. The assumption that progress is for > > communists and hippies is the death of Perl 5.
> > I think it's great that you're enthusiastic about optimizing perl's > implementation, but treating a comment about fairly questionable > optimization as an attack on all changes is reaching.
The same opinions are coming out again, self-righteous "if I wouldn't change it, you have no right to either" https://rt.perl.org/rt3/Ticket/Display.html?id=116443 https://rt.perl.org/rt3/Ticket/Display.html?id=115894 https://rt.perl.org/rt3/Ticket/Display.html?id=116188 Show quoted text
> > We optimize for several things, generated code size is important, but > not more important than a) developer time - the poor sob who comes > along in 6 months or 5 years and needs to understand the code, b) > performance.
Who is "the poor sob who comes along in 6 months or 5 years and needs to understand the code"? Commiters are held to no standard of code clarity or documentation. I've never seen a revert commit to revert something for lack of comments or clarity. Why is it said by others in the Perl world (YAPCNA) only a dozen people on earth can hack the C guts? a recent one http://perl5.git.perl.org/perl.git/commitdiff/0c0d42fffa5c2ddd284610f681b65d355471189b What systems? Tested on any? Seen it in a man page? http://perl5.git.perl.org/perl.git/commit/28e464fba839d02f376952199261fc8b7d58a0d8 nothing! get the Ouija board out! http://perl5.git.perl.org/perl.git/commit/88675427278c9e6329fba9382072cae9dd00c163 what commit? http://perl5.git.perl.org/perl.git/commit/175bc858cac2126947f4b8fa838835417c1430d1 faster? slower? https://rt.perl.org/rt3/Ticket/Display.html?id=116925 when will THINKFIRST be documented? Show quoted text
> > And code size doesn't map one-to-one to performance - several > performance optimizations produce significantly larger code. > > Performance doesn't matter much for S_minus_v(), but developer time > matters as much there as it does anywhere else. >
S_minus_v never gets changed except for copyright dates. Ideally it should be a single string written by some .pl script into a .h during the build process, but that is too complex for me to implement (and will break every build except Windows if I try), plus removing BINARY_BUILD_NOTICE and getting AS and DARKPAN to not use it and introducing a BINARY_BUILD_NOTICE_S macro and "#ifdef BINARY_BUILD_NOTICE\n#error BINARY_BUILD_NOTICE has been removed, use BINARY_BUILD_NOTICE_S." is too difficult for me. On Sun Jul 07 23:53:42 2013, tonyc wrote: Show quoted text
> > - #endif > > +#else > > + SV* level = newSVpvn(level_str, level_len); > > +#endif /* #ifdef PERL_PATCHNUM */
> > Except (as discussed) the use of newSVpvn(), I think it's good up to > here.
Will switch to newSVpvn as said above. Show quoted text
> > } > > -#if defined(LOCAL_PATCH_COUNT) > > - if (LOCAL_PATCH_COUNT > 0) > > - PerlIO_printf(PIO_stdout, > > - "\n(with %d registered patch%s, " > > - "see perl -V for more detail)", > > - LOCAL_PATCH_COUNT, > > - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); > > -#endif > > - > > PerlIO_printf(PIO_stdout, > > - "\n\nCopyright 1987-2013, Larry Wall\n"); > > +#if defined(LOCAL_PATCH_COUNT) > > + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor
> constant */
> > + LOCAL_PATCH_COUNT == 1 ? > > + "\n(with %d registered patch, see perl -V for more
> detail)%s"
> > + : LOCAL_PATCH_COUNT > 0 ? > > + "\n(with %d registered patches, see perl -V for
> more detail)%s"
> > + : "%.0s%s", > > + LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, > > +#endif
> > This code is incorrect. > > For some compilers NULL is ((void *)0) - a pointer, not an integer. > > Consider - what's the type of the expression: > > LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
I knew that would be a portability problem, so the NULL went there. I didn't want to try to use more complicated features of sv_vcatpvfn_flags (t or z letter) due to sv_vcatpvfn_flags crashing on me when using fancy features (bug filed). Google says implicit type promotion so I guess the size letters have to go in. "%.0s" produces no output if the param is null, and therefore skips the param. "%.0s" is a work around to my original idea which became bug #118775. Show quoted text
> > It's also unnecessarily duplicative, in that if the registered patches > note needed to change, it will need to be changed in two places.
The alternative was more complicated (putting each litteral into a macro, then putting many instances of each macro). The 2 lines are next to each other, nobody is likely to change them. If they do, they can see it on the same screen. 3 seconds of work is nothing. Show quoted text
> > gcc and clang complain about the type too, gcc: > > perl.c: In function S_minus_v: > perl.c:3551:39: warning: pointer/integer type mismatch in conditional > expression [enabled by default] > perl.c:3596:9: warning: format %d expects argument of type int, > but argument 3 has type void * [-Wformat] > > clang produces a more understandable message: > > perl.c:3551:9: warning: format specifies type 'int' but the argument > has type > 'void *' [-Wformat] > LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./patchlevel.h:147:2: note: expanded from macro 'LOCAL_PATCH_COUNT' > ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2)) > ^ > perl.c:3551:9: warning: format specifies type 'int' but the argument > has type > 'void *' [-Wformat] > LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./patchlevel.h:147:2: note: expanded from macro 'LOCAL_PATCH_COUNT' > ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2)) > > (I think it warns twice because you provide three different format > strings, two of which don't match.) > > The original code is clearer I think. >
I could break out the LOCAL_PATCH_COUNT stuff into a separate printf/remove it (as it was originally). Show quoted text
> > Rather than trying to build it all into one printf, I think it would > be more understandable if the printf() started at the Larry Wall line, > and the wce_hitreturn() got its own extra #ifdef UNDER_CE, so: > > PerlIO_printf(PIO_stdout, > larry's line > #ifdef platform > platform line > #endif > ... > ); > #ifdef UNDER_CE > wce_hitreturn(); > #endif > > Tony >
The GPL line wont be concatted with the larry CR and platform CR in that case("> );"). Leaving the only optimization being dec_NN and concating of larry CR with platform CR. http://perl5.git.perl.org/perl.git/commit/e1caacb4fdb62cb28dc825ca0115faf95e569339?f=perl.c I think the wce_hitreturn (implemented in wince.c) has a screen pager purpose due to the tiny screen size of a WinCE device, so it has to be between the larry CR/platform CR and the GPL part on WinCE. -- bulk88 ~ bulk88 at hotmail.com
CC: perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Tue, 9 Jul 2013 11:22:12 -0400
To: "\"bulk88 via RT\"" <perlbug-followup [...] perl.org>
From: Andy Dougherty <doughera [...] lafayette.edu>
Download (untitled) / with headers
text/plain 552b
On Mon, Jul 08, 2013 at 03:36:35PM -0700, "bulk88 via RT" wrote: Show quoted text
> The same opinions are coming out again, self-righteous "if I wouldn't > change it, you have no right to either" > https://rt.perl.org/rt3/Ticket/Display.html?id=116443 > https://rt.perl.org/rt3/Ticket/Display.html?id=115894 > https://rt.perl.org/rt3/Ticket/Display.html?id=116188
I think that is an unfair characterization of those discussions, especially when you consider that in two out of the three cases, your patch was applied. -- Andy Dougherty doughera@lafayette.edu
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Tue, 9 Jul 2013 18:59:03 -0400
To: perl5-porters [...] perl.org
From: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
* bulk88 via RT <perlbug-followup@perl.org> [2013-07-08T18:36:35] Show quoted text
> When I hear "your favorites aren't my favorites so burn in hell", I see > that commenter is wearing rose glasses and it is political/religious BS. > [ ⋮ ] > The same opinions are coming out again, self-righteous "if I wouldn't > change it, you have no right to either"
I think that this is not only hyperbole (which is one thing), but also unfair and, possibly worst of all, not likely to convince anybody of anything helpful. In every case that I've seen, the issue has not been "I don't care about your compiler," but "this optimization is only an optimization sometimes, and other times a cost or no change, but carries a code complexity cost." Show quoted text
> Commiters are held to no standard of code clarity or documentation. I've > never seen a revert commit to revert something for lack of comments or > clarity.
I encourage you to politely point out such shortcomings as they occur, to remind other contributors to improve their comments post facto and to improve their commit messages next time. I think it would be in some ways nice, and would have some positive effects, to have a policy stating that a commiter may not, barring exception circumstances, apply his own code to blead, but must have a second set of eyes in the form of another committer who applies the author's branch. Unfortunately, I think that this is going to be a hard sell, at best. We need a better culture of code review, I agree. I also agree that it would benefit everyone were that code review applied not only to code coming from non-committers, but also within the group of committers. That said, I don't, of course, think the solution is to have no standards at all. That's why I hope you will, in the future, politely point out when commits have been made that are of unclear purpose or are under-explained. If the commits are on a branch before merging and you have seen it, all the better, so it can be fixed before it hits blead. If not, it's something for the author to keep in mind for next time. -- rjbs
Download signature.asc
application/pgp-signature 490b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 197b
On Sun Jul 07 23:53:42 2013, tonyc wrote: Show quoted text
> > Tony >
Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using GCC) stop type mismatch warning. -- bulk88 ~ bulk88 at hotmail.com
From 272f571b0f05ee9f86e59b3560daf73a5a976e0a Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Thu, 18 Jul 2013 14:55:37 -0400 Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v |SvPVX(vstringify(PL_patchlevel))| is the same string as |"v" PERL_VERSION_STRING|. No need to go through the overhead of using a version object. Instead of creating a SV, then further manipulating it. Create and manipulate it at the same time with Perl_newSVpvf_nocontext or newSVpvn. "num_len >= level_len " will be folded away by the compiler, previously, since both lengths came from SVs, they were not const technically. A very smart compiler might see strncmp/strnEQ takes all const arguments, and will optimize that away also. VC 2003 didnt do that. Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons. With the copyright lines, use more CPP cating. This reduced the number of instructions, and removes a couple alignment null in .rdata image section. UNDER_CE presents a challange because of the wce_hitreturn(); call. BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains a perlio printf call. So that can not be CPP catted unfortunatly. If a build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright line is catted with the license line. Although S_minus_v is not hot and is rarely used, making the interp image smaller, and therefore faster is a good idea. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. --- perl.c | 95 ++++++++++++++++++++++++++++++++++----------------------------- 1 files changed, 51 insertions(+), 44 deletions(-) diff --git a/perl.c b/perl.c index bad66f5..2c52a6e 100644 --- a/perl.c +++ b/perl.c @@ -3502,30 +3502,35 @@ STATIC void S_minus_v(pTHX) { PerlIO * PIO_stdout; - if (!sv_derived_from(PL_patchlevel, "version")) - upg_version(PL_patchlevel, TRUE); { - SV* level= vstringify(PL_patchlevel); + const char * const level_str = "v" PERL_VERSION_STRING; + const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1; #ifdef PERL_PATCHNUM + SV* level; # ifdef PERL_GIT_UNCOMMITTED_CHANGES - SV *num = newSVpvs(PERL_PATCHNUM "*"); + static const char num [] = PERL_PATCHNUM "*"; # else - SV *num = newSVpvs(PERL_PATCHNUM); + static const char num [] = PERL_PATCHNUM; # endif { - STRLEN level_len, num_len; - char * level_str, * num_str; - num_str = SvPV(num, num_len); - level_str = SvPV(level, level_len); - if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) { - SvREFCNT_dec(level); - level= num; + const STRLEN num_len = sizeof(num)-1; + /* A very advanced compiler would fold away the strnEQ + and this whole conditional, but most (all?) won't do it. + SV level could also be replaced by with preprocessor + catenation. + */ + if (num_len >= level_len && strnEQ(num,level_str,level_len)) { + /* per 46807d8e80, PERL_PATCHNUM is outside of the control + of the interp so it might contain format characters + */ + level = newSVpvn(num, num_len); } else { - Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num); - SvREFCNT_dec(num); + level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num); } } - #endif +#else + SV* level = newSVpvn(level_str, level_len); +#endif /* #ifdef PERL_PATCHNUM */ PIO_stdout = PerlIO_stdout(); PerlIO_printf(PIO_stdout, "\nThis is perl " STRINGIFY(PERL_REVISION) @@ -3533,59 +3538,61 @@ S_minus_v(pTHX) ", subversion " STRINGIFY(PERL_SUBVERSION) " (%"SVf") built for " ARCHNAME, level ); - SvREFCNT_dec(level); + SvREFCNT_dec_NN(level); } -#if defined(LOCAL_PATCH_COUNT) - if (LOCAL_PATCH_COUNT > 0) - PerlIO_printf(PIO_stdout, - "\n(with %d registered patch%s, " - "see perl -V for more detail)", - LOCAL_PATCH_COUNT, - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); -#endif - PerlIO_printf(PIO_stdout, - "\n\nCopyright 1987-2013, Larry Wall\n"); +#if defined(LOCAL_PATCH_COUNT) + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */ + LOCAL_PATCH_COUNT == 1 ? + "\n(with %zu registered patch, see perl -V for more detail)%s" + : LOCAL_PATCH_COUNT > 0 ? + "\n(with %zu registered patches, see perl -V for more detail)%s" + : "%.0s%s", + INT2PTR(size_t, LOCAL_PATCH_COUNT), +#endif + "\n\nCopyright 1987-2013, Larry Wall\n" #ifdef MSDOS - PerlIO_printf(PIO_stdout, - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" #endif #ifdef DJGPP - PerlIO_printf(PIO_stdout, "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" #endif #ifdef OS2 - PerlIO_printf(PIO_stdout, "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" #endif #ifdef OEMVS - PerlIO_printf(PIO_stdout, - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" #endif #ifdef __VOS__ - PerlIO_printf(PIO_stdout, - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" #endif #ifdef POSIX_BC - PerlIO_printf(PIO_stdout, - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" +#endif +#ifdef __SYMBIAN32__ + "Symbian port by Nokia, 2004-2005\n" #endif +/* UNDER_CE closes the printf call*/ #ifdef UNDER_CE - PerlIO_printf(PIO_stdout, "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ BINARY_BUILD_NOTICE; #endif +/* because of wce_hitreturn() open new printf for UNDER_CE */ +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ -- 1.7.9.msysgit.0
CC: perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Thu, 18 Jul 2013 20:34:35 +0100
To: bulk88 via RT <perlbug-followup [...] perl.org>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 1.6k
On Thu, Jul 18, 2013 at 11:57:37AM -0700, bulk88 via RT wrote: Show quoted text
> On Sun Jul 07 23:53:42 2013, tonyc wrote:
> > > > Tony > >
> > Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using > GCC) stop type mismatch warning.
I'm not sure if everything likes %zu. Particularly older SysV vendor compilers. Need to check. Show quoted text
> With the copyright lines, use more CPP cating. This reduced the number of > instructions, and removes a couple alignment null in .rdata image section.
I'm pretty sure that mixing #ifdef and pre-processor string concatenation has upset some compilers in the past. Whilst no particular system is going to have more than one macro defined: Show quoted text
> #ifdef OEMVS > - PerlIO_printf(PIO_stdout, > - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); > + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" > #endif > #ifdef __VOS__ > - PerlIO_printf(PIO_stdout, > - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); > + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" > #endif > #ifdef POSIX_BC > - PerlIO_printf(PIO_stdout, > - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); > + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" > +#endif
the construction looks like it could cause compilers to choke (So we need to check before changing this) Also, what's the maximum length from concatenating string literals doing it this way? IIRC ANSI's minimum limit was something puny like 502 characters. I don't think that any compiler we encountered was that unforgiving, but something (IBM's compiler) maxed out at about 2000 characters, which we hit at times. Nicholas Clark
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.7k
On Thu Jul 18 12:35:21 2013, nicholas wrote: Show quoted text
> On Thu, Jul 18, 2013 at 11:57:37AM -0700, bulk88 via RT wrote:
> > Patch remade, newSVpvn and %d replaced with %zu to hopefully (not
> using
> > GCC) stop type mismatch warning.
> > I'm not sure if everything likes %zu. Particularly older SysV vendor > compilers. > Need to check.
OS C library is irrelevant. The format string is parsed by Perl itself (Perl_sv_vcatpvfn_flags). Show quoted text
>
> > With the copyright lines, use more CPP cating. This reduced the
> number of
> > instructions, and removes a couple alignment null in .rdata image
> section. > > I'm pretty sure that mixing #ifdef and pre-processor string > concatenation > has upset some compilers in the past. Whilst no particular system is > going to > have more than one macro defined:
The current preproc conditional code could in theory allow 2 different OSes to match and both of their strings are included. I think I above asked whether to #elsif the OS lines but got no comments. In my original patch I made only 1 OS line to be picked. To reduce controversy, I reverted it back to the original/current 1 or more OS lines code. Show quoted text
>
> > #ifdef OEMVS > > - PerlIO_printf(PIO_stdout, > > - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); > > + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" > > #endif > > #ifdef __VOS__ > > - PerlIO_printf(PIO_stdout, > > - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-
> 2013\n");
> > + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-
> 2013\n"
> > #endif > > #ifdef POSIX_BC > > - PerlIO_printf(PIO_stdout, > > - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); > > + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" > > +#endif
> > the construction looks like it could cause compilers to choke > (So we need to check before changing this) > > Also, what's the maximum length from concatenating string literals > doing it > this way? IIRC ANSI's minimum limit was something puny like 502 > characters. > I don't think that any compiler we encountered was that unforgiving, > but > something (IBM's compiler) maxed out at about 2000 characters, which > we hit > at times. > > Nicholas Clark >
larry wall copy right+gpl string came out to 411 bytes+alignment padding (upto 8 more bytes) in my Win32 binary image using a disassembler. If CPP token catting doesn't work, I dont think Perl will compile because of code like Perl_croak(aTHX_ "Can't locate object method \"%"UTF8f "\" via package \"%"SVf"\"" " (perhaps you forgot to load \"%"SVf"\"?)", ifdefs evaled before string litteral catting, http://stackoverflow.com/questions/1476892/poster-with-the-8-phases-of-translation-in-the-c-language -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 3.4k
On Thu Jul 18 11:57:36 2013, bulk88 wrote: Show quoted text
> On Sun Jul 07 23:53:42 2013, tonyc wrote:
> > > > Tony > >
> > Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using > GCC) stop type mismatch warning. >
Thanks for the newSVpvn change, unfortunately the format string error remains: CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat -Wwrite-strings perl.c: In function ‘S_minus_v’: perl.c:3600:9: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘long unsigned int’ [-Wformat] From clang: perl.c:3555:9: warning: format specifies type 'char *' but the argument has type 'size_t' (aka 'unsigned long') [-Wformat] INT2PTR(size_t, LOCAL_PATCH_COUNT), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./perl.h:1630:26: note: expanded from macro 'INT2PTR' # define INT2PTR(any,d) (any)(d) ^~~~~~~~ Note that ANSI C doesn't guarantee that size_t and pointers are the same size, though I can only think an obsolete platform (Win16 large model) where they are different sizes. About the only way I can see to avoid this type of error would be something like: const char copyrights[] = ... #ifdefs for platform messages ... LOCAL_PATCH_COUNT == 0 ? PerlIO_printf(PIO_stdout, larry message "%s", copyrights) : LOCAL_PATCH_COUNT == 1 ? PerlIO_printf(PIO_stdout, larry message "with 1 registered patch\n%s", copyrights) : PerlIO_printf(PIO_stdout, larry message "with %d registered patches\n%s", LOCAL_PATCH_COUNT, copyrights); The compiler should optimize away all but one branch, but it's ridiculous code for the purpose (and doesn't belong in perl.) This updated patch does nothing about the confusing "where's the end of the printf" issue which I should probably have been more explicit about in an earlier message. (I almost posted about %zu too, since that's not C89, but you're right, perl itself handles the format.) WRT your response to Nicholas: Show quoted text
> larry wall copy right+gpl string came out to 411 bytes+alignment > padding (upto 8 more bytes) in my Win32 binary image using a > disassembler.
The C89 limit is 509 characters, and it's a limit on string literals in general, not concatenation in particular. C99 is more generous allowing 4096 characters. So, as you said, we're fine here. Show quoted text
> If CPP token catting doesn't work, I dont think Perl will compile > because of code like > > Perl_croak(aTHX_ > "Can't locate object method \"%"UTF8f > "\" via package \"%"SVf"\"" > " (perhaps you forgot to load \"%"SVf"\"?)", > > ifdefs evaled before string litteral catting, >
http://stackoverflow.com/questions/1476892/poster-with-the-8-phases-of-translation-in-the-c-language While you're right about the behaviour the C specification requires, some compilers are unfortunately buggy. You're also comparing two different code constructs here, Nicholas said that: "literal1" #ifdef SOMETHING "literal2" #endif may cause a compiler to choke (such a compiler is buggy, string literal concatenation has nothing to do with the preprocessor.) Your example contains no preprocessor directives, just macro replacements. I wonder if we should have a document describing such compiler bugs that we've bothered working around in the past. Tony
CC: perl5-porters [...] perl.org
Subject: Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Date: Fri, 19 Jul 2013 13:45:31 -0400
To: "\"Tony Cook via RT\"" <perlbug-followup [...] perl.org>
From: Andy Dougherty <doughera [...] lafayette.edu>
Download (untitled) / with headers
text/plain 1.6k
On Thu, Jul 18, 2013 at 06:32:43PM -0700, "Tony Cook via RT" wrote: Show quoted text
> On Thu Jul 18 11:57:36 2013, bulk88 wrote:
> > On Sun Jul 07 23:53:42 2013, tonyc wrote:
> > > > > > Tony > > >
> > > > Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using > > GCC) stop type mismatch warning. > >
> > Thanks for the newSVpvn change, unfortunately the format string error > remains: > > CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE > -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include > -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi -W > -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat > -Wwrite-strings > perl.c: In function ‘S_minus_v’: > perl.c:3600:9: warning: format ‘%s’ expects argument of type ‘char *’, > but argument 3 has type ‘long unsigned int’ [-Wformat] >
> >From clang:
> > perl.c:3555:9: warning: format specifies type 'char *' but the argument > has type > 'size_t' (aka 'unsigned long') [-Wformat] > INT2PTR(size_t, LOCAL_PATCH_COUNT), > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./perl.h:1630:26: note: expanded from macro 'INT2PTR' > # define INT2PTR(any,d) (any)(d) > ^~~~~~~~ > Note that ANSI C doesn't guarantee that size_t and pointers are the same > size, though I can only think an obsolete platform (Win16 large model) > where they are different sizes.
I confess I didn't dig through the code to figure out why this was a problem, but in my view, for simply printing out copyright messages, I don't see the value in fighting portability battles. Just keep it simple. -- Andy Dougherty doughera@lafayette.edu
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.6k
On Thu Jul 18 18:32:42 2013, tonyc wrote: Show quoted text
> On Thu Jul 18 11:57:36 2013, bulk88 wrote:
> > On Sun Jul 07 23:53:42 2013, tonyc wrote:
> > > > > > Tony > > >
> > > > Patch remade, newSVpvn and %d replaced with %zu to hopefully (not > > using > > GCC) stop type mismatch warning. > >
> > Thanks for the newSVpvn change, unfortunately the format string error > remains: > > CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE > -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include > -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi > -W > -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat > -Wwrite-strings > perl.c: In function ‘S_minus_v’: > perl.c:3600:9: warning: format ‘%s’ expects argument of type ‘char *’, > but argument 3 has type ‘long unsigned int’ [-Wformat] > > From clang: > > perl.c:3555:9: warning: format specifies type 'char *' but the > argument > has type > 'size_t' (aka 'unsigned long') [-Wformat] > INT2PTR(size_t, LOCAL_PATCH_COUNT), > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./perl.h:1630:26: note: expanded from macro 'INT2PTR' > # define INT2PTR(any,d) (any)(d) > ^~~~~~~~ > Note that ANSI C doesn't guarantee that size_t and pointers are the > same > size, though I can only think an obsolete platform (Win16 large model) > where they are different sizes.
Patch remade, warning silenced on my clang 3.1 and gcc 4.5.3. I thought of doing a "#pragma clang diagnostic ignored "-Wformat"" also, but it seems unnecessary since clang implemented enough of gcc warnings pragma to work. -- bulk88 ~ bulk88 at hotmail.com
Subject: 0001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patch
From ef64b0eddac3937d643ae3f0ec79de8648ec9b72 Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Tue, 29 Oct 2013 18:01:09 -0400 Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v |SvPVX(vstringify(PL_patchlevel))| is the same string as |"v" PERL_VERSION_STRING|. No need to go through the overhead of using a version object. Instead of creating a SV, then further manipulating it. Create and manipulate it at the same time with Perl_newSVpvf_nocontext or newSVpvn. "num_len >= level_len " will be folded away by the compiler, previously, since both lengths came from SVs, they were not const technically. A very smart compiler might see strncmp/strnEQ takes all const arguments, and will optimize that away also. VC 2003 didnt do that. Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons. With the copyright lines, use more CPP cating. This reduced the number of instructions, and removes a couple alignment null in .rdata image section. UNDER_CE presents a challange because of the wce_hitreturn(); call. BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains a perlio printf call. So that can not be CPP catted unfortunatly. If a build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright line is catted with the license line. Although S_minus_v is not hot and is rarely used, making the interp image smaller, and therefore faster is a good idea. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. --- perl.c | 113 ++++++++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 44 deletions(-) diff --git a/perl.c b/perl.c index 4c80edb..2dbc9ee 100644 --- a/perl.c +++ b/perl.c @@ -3525,35 +3525,47 @@ Perl_moreswitches(pTHX_ const char *s) return NULL; } - +#ifdef __GNUC__ +# if __GNUC__ == 4 && __GNUC_MINOR__ >= 6 || __GNUC__ > 4 /* 4.6 -> */ +# pragma GCC diagnostic push +# endif +# if __GNUC__ == 4 && __GNUC_MINOR__ >= 2 || __GNUC__ > 4 /* 4.2 -> */ +# pragma GCC diagnostic ignored "-Wformat" +# endif +#endif STATIC void S_minus_v(pTHX) { PerlIO * PIO_stdout; - if (!sv_derived_from(PL_patchlevel, "version")) - upg_version(PL_patchlevel, TRUE); { - SV* level= vstringify(PL_patchlevel); + const char * const level_str = "v" PERL_VERSION_STRING; + const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1; #ifdef PERL_PATCHNUM + SV* level; # ifdef PERL_GIT_UNCOMMITTED_CHANGES - SV *num = newSVpvs(PERL_PATCHNUM "*"); + static const char num [] = PERL_PATCHNUM "*"; # else - SV *num = newSVpvs(PERL_PATCHNUM); + static const char num [] = PERL_PATCHNUM; # endif { - STRLEN level_len, num_len; - char * level_str, * num_str; - num_str = SvPV(num, num_len); - level_str = SvPV(level, level_len); - if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) { - SvREFCNT_dec(level); - level= num; + const STRLEN num_len = sizeof(num)-1; + /* A very advanced compiler would fold away the strnEQ + and this whole conditional, but most (all?) won't do it. + SV level could also be replaced by with preprocessor + catenation. + */ + if (num_len >= level_len && strnEQ(num,level_str,level_len)) { + /* per 46807d8e80, PERL_PATCHNUM is outside of the control + of the interp so it might contain format characters + */ + level = newSVpvn(num, num_len); } else { - Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num); - SvREFCNT_dec(num); + level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num); } } - #endif +#else + SV* level = newSVpvn(level_str, level_len); +#endif /* #ifdef PERL_PATCHNUM */ PIO_stdout = PerlIO_stdout(); PerlIO_printf(PIO_stdout, "\nThis is perl " STRINGIFY(PERL_REVISION) @@ -3561,59 +3573,65 @@ S_minus_v(pTHX) ", subversion " STRINGIFY(PERL_SUBVERSION) " (%"SVf") built for " ARCHNAME, level ); - SvREFCNT_dec(level); + SvREFCNT_dec_NN(level); } -#if defined(LOCAL_PATCH_COUNT) - if (LOCAL_PATCH_COUNT > 0) - PerlIO_printf(PIO_stdout, - "\n(with %d registered patch%s, " - "see perl -V for more detail)", - LOCAL_PATCH_COUNT, - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); -#endif +/* gcc pragma above silences "perl.c: In function 'S_minus_v': + warning: format '%.0s' expects type 'char *', but argument 3 has type + 'unsigned int'" below */ PerlIO_printf(PIO_stdout, - "\n\nCopyright 1987-2013, Larry Wall\n"); +#if defined(LOCAL_PATCH_COUNT) + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */ + LOCAL_PATCH_COUNT == 1 ? + "\n(with %zu registered patch, see perl -V for more detail)%s" + : LOCAL_PATCH_COUNT > 0 ? + "\n(with %zu registered patches, see perl -V for more detail)%s" + : "%.0s%s", + INT2PTR(size_t, LOCAL_PATCH_COUNT), +#endif + "\n\nCopyright 1987-2013, Larry Wall\n" #ifdef MSDOS - PerlIO_printf(PIO_stdout, - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" #endif #ifdef DJGPP - PerlIO_printf(PIO_stdout, "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" #endif #ifdef OS2 - PerlIO_printf(PIO_stdout, "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" #endif #ifdef OEMVS - PerlIO_printf(PIO_stdout, - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" #endif #ifdef __VOS__ - PerlIO_printf(PIO_stdout, - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" #endif #ifdef POSIX_BC - PerlIO_printf(PIO_stdout, - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" +#endif +#ifdef __SYMBIAN32__ + "Symbian port by Nokia, 2004-2005\n" #endif +/* UNDER_CE closes the printf call*/ #ifdef UNDER_CE - PerlIO_printf(PIO_stdout, "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ BINARY_BUILD_NOTICE; #endif +/* because of wce_hitreturn() open new printf for UNDER_CE */ +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ @@ -3622,6 +3640,13 @@ this system using \"man perl\" or \"perldoc perl\". If you have access to the\n Internet, point your browser at http://www.perl.org/, the Perl Home Page.\n\n"); my_exit(0); } +#ifdef __GNUC__ +# if __GNUC__ == 4 && __GNUC_MINOR__ >= 6 || __GNUC__ > 4 /* 4.6 -> */ +# pragma GCC diagnostic pop +# elif __GNUC__ == 4 && __GNUC_MINOR__ >= 2 || __GNUC__ > 4 /* 4.2 -> */ +# pragma GCC diagnostic warning "-Wformat" +# endif +#endif /* compliments of Tom Christiansen */ -- 1.8.0.msysgit.0
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.6k
On Tue Oct 29 15:07:46 2013, bulk88 wrote: Show quoted text
> On Thu Jul 18 18:32:42 2013, tonyc wrote:
> > On Thu Jul 18 11:57:36 2013, bulk88 wrote:
> > > On Sun Jul 07 23:53:42 2013, tonyc wrote:
> > > > > > > > Tony > > > >
> > > > > > Patch remade, newSVpvn and %d replaced with %zu to hopefully (not > > > using > > > GCC) stop type mismatch warning. > > >
> > > > Thanks for the newSVpvn change, unfortunately the format string error > > remains: > > > > CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE > > -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include > > -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi > > -W > > -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat > > -Wwrite-strings > > perl.c: In function ‘S_minus_v’: > > perl.c:3600:9: warning: format ‘%s’ expects argument of type ‘char > > *’, > > but argument 3 has type ‘long unsigned int’ [-Wformat] > > > > From clang: > > > > perl.c:3555:9: warning: format specifies type 'char *' but the > > argument > > has type > > 'size_t' (aka 'unsigned long') [-Wformat] > > INT2PTR(size_t, LOCAL_PATCH_COUNT), > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ./perl.h:1630:26: note: expanded from macro 'INT2PTR' > > # define INT2PTR(any,d) (any)(d) > > ^~~~~~~~ > > Note that ANSI C doesn't guarantee that size_t and pointers are the > > same > > size, though I can only think an obsolete platform (Win16 large > > model) > > where they are different sizes.
> > Patch remade, warning silenced on my clang 3.1 and gcc 4.5.3. I > thought of doing a "#pragma clang diagnostic ignored "-Wformat"" also, > but it seems unnecessary since clang implemented enough of gcc > warnings pragma to work.
Silencing the warning does not make the code correct. It also doesn't fix the painful "where does this statement" end that was introduced in the original patch. To try and move forward: a) I'd love a patch with just the first part of the change - everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest patch. b) to make progress on the consolidation of the string literals I'd like to suggest the following changes to your approach: 1) retain the current #if defined(LOCAL_PATCH_COUNT) block, trying to fiddle with that in the way you have been is just too non-C. An optimizing compiler will remove the block when LOCAL_PATCH_COUNT is zero anyway 2) don't try to consolidate the "Perl may be copied ..." printf(), it's the main readability issue with your changes. 3) use a second #ifdef UNDER_CE for the wce_hitreturn rather than trying to close and open the printf around it. Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 544b
On Mon Nov 11 16:53:38 2013, tonyc wrote: Show quoted text
> To try and move forward: > > a) I'd love a patch with just the first part of the change - > everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest > patch. >
1st half split out. Patch attached. I need more time to address the rest of your last post. The title was change, so the next half will be tentatively named "optimize the function " and not "remove the Foo from the function" which is this patch. Also RT # added in body of part 1/this patch. -- bulk88 ~ bulk88 at hotmail.com
Subject: 0001-remove-PL_patchlevel-from-S_minus_v.patch
From e8d5cdcfc749ebb9541705427c7eec832f432ace Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Tue, 12 Nov 2013 23:07:16 -0500 Subject: [PATCH] remove PL_patchlevel from S_minus_v |SvPVX(vstringify(PL_patchlevel))| is the same string as |"v" PERL_VERSION_STRING|. No need to go through the overhead of using a version object. Instead of creating a SV, then further manipulating it. Create and manipulate it at the same time with Perl_newSVpvf_nocontext or newSVpvn. "num_len >= level_len " will be folded away by the compiler, previously, since both lengths came from SVs, they were not const technically. A very smart compiler might see strncmp/strnEQ takes all const arguments, and will optimize that away also. VC 2003 didnt do that. Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. This patch is part of #116296. --- perl.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/perl.c b/perl.c index 2892a87..e4310cd 100644 --- a/perl.c +++ b/perl.c @@ -3443,30 +3443,35 @@ STATIC void S_minus_v(pTHX) { PerlIO * PIO_stdout; - if (!sv_derived_from(PL_patchlevel, "version")) - upg_version(PL_patchlevel, TRUE); { - SV* level= vstringify(PL_patchlevel); + const char * const level_str = "v" PERL_VERSION_STRING; + const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1; #ifdef PERL_PATCHNUM + SV* level; # ifdef PERL_GIT_UNCOMMITTED_CHANGES - SV *num = newSVpvs(PERL_PATCHNUM "*"); + static const char num [] = PERL_PATCHNUM "*"; # else - SV *num = newSVpvs(PERL_PATCHNUM); + static const char num [] = PERL_PATCHNUM; # endif { - STRLEN level_len, num_len; - char * level_str, * num_str; - num_str = SvPV(num, num_len); - level_str = SvPV(level, level_len); - if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) { - SvREFCNT_dec(level); - level= num; + const STRLEN num_len = sizeof(num)-1; + /* A very advanced compiler would fold away the strnEQ + and this whole conditional, but most (all?) won't do it. + SV level could also be replaced by with preprocessor + catenation. + */ + if (num_len >= level_len && strnEQ(num,level_str,level_len)) { + /* per 46807d8e80, PERL_PATCHNUM is outside of the control + of the interp so it might contain format characters + */ + level = newSVpvn(num, num_len); } else { - Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num); - SvREFCNT_dec(num); + level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num); } } - #endif +#else + SV* level = newSVpvn(level_str, level_len); +#endif /* #ifdef PERL_PATCHNUM */ PIO_stdout = PerlIO_stdout(); PerlIO_printf(PIO_stdout, "\nThis is perl " STRINGIFY(PERL_REVISION) @@ -3474,7 +3479,7 @@ S_minus_v(pTHX) ", subversion " STRINGIFY(PERL_SUBVERSION) " (%"SVf") built for " ARCHNAME, level ); - SvREFCNT_dec(level); + SvREFCNT_dec_NN(level); } #if defined(LOCAL_PATCH_COUNT) if (LOCAL_PATCH_COUNT > 0) -- 1.8.0.msysgit.0
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 645b
On Tue Nov 12 20:15:03 2013, bulk88 wrote: Show quoted text
> On Mon Nov 11 16:53:38 2013, tonyc wrote:
> > To try and move forward: > > > > a) I'd love a patch with just the first part of the change - > > everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest > > patch. > >
> > 1st half split out. Patch attached. I need more time to address the > rest of your last post. The title was change, so the next half will be > tentatively named "optimize the function " and not "remove the Foo > from the function" which is this patch. Also RT # added in body of > part 1/this patch.
Thanks, applied as 709aee9447f22f3f59d548d44298cca98a023f80. Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 542b
On Mon Nov 11 16:53:38 2013, tonyc wrote: Show quoted text
> > Silencing the warning does not make the code correct. >
CC warning free now, is it a ptr or is it an int in the format string design is gone. Show quoted text
> It also doesn't fix the painful "where does this statement" end that > was introduced in the original patch. >
....... Show quoted text
> > 2) don't try to consolidate the "Perl may be copied ..." printf(), > it's the main readability issue with your changes.
I changed the comments to be clearer where the concatenation goes. -- bulk88 ~ bulk88 at hotmail.com
Subject: 0001-optimize-S_minus_v.patch
From 407983156b9b3f0f58e179b653d900df7adb3b08 Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Sun, 22 Dec 2013 12:19:34 -0500 Subject: [PATCH] optimize S_minus_v With the copyright lines, use more CPP cating. This reduced the number of instructions, and removes a couple alignment null in .rdata image section. UNDER_CE presents a challange because of the wce_hitreturn(); call. BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains a perlio printf call. So that can not be CPP catted unfortunatly. If a build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright line is catted with the license line. LOCAL_PATCH_COUNT is a C expression, like offsetof, which is not a preprocessor constant, so add PERL_GIT_UNPUSHED_COMMITS_LEN and LOCAL_PATCH_COUNT_CONST which are pure CPP constants. LOCAL_PATCH_COUNT can be changed to LOCAL_PATCH_COUNT_CONST in the future. Although S_minus_v is not hot and is rarely used, making the interp image smaller, and therefore faster is a good idea. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. This patch is part of #116296. --- make_patchnum.pl | 9 ++++--- patchlevel.h | 10 +++++++++ perl.c | 60 ++++++++++++++++++++++++++++++----------------------- 3 files changed, 49 insertions(+), 30 deletions(-) diff --git a/make_patchnum.pl b/make_patchnum.pl index 3f857b5..82ec266 100644 --- a/make_patchnum.pl +++ b/make_patchnum.pl @@ -122,6 +122,7 @@ sub write_files { return 0; } +my @unpushed_commits; my $unpushed_commits = ' '; my ($read, $branch, $snapshot_created, $commit_id, $describe)= ("") x 5; my ($changed, $extra_info, $commit_title)= ("") x 3; @@ -155,14 +156,13 @@ elsif (-d "$srcdir/.git") { } if (length $branch && length $remote) { + @unpushed_commits = grep {/\+/} backtick("git cherry $remote/$merge"); # git cherry $remote/$branch | awk 'BEGIN{ORS=","} /\+/ {print $2}' | sed -e 's/,$//' my $unpushed_commit_list = - join ",", map { (split /\s/, $_)[1] } - grep {/\+/} backtick("git cherry $remote/$merge"); + join ",", map { (split /\s/, $_)[1] } @unpushed_commits; # git cherry $remote/$branch | awk 'BEGIN{ORS="\t\\\\\n"} /\+/ {print ",\"" $2 "\""}' $unpushed_commits = - join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } - grep {/\+/} backtick("git cherry $remote/$merge"); + join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } @unpushed_commits; if (length $unpushed_commits) { $commit_title = "Local Commit:"; my $ancestor = backtick("git rev-parse $remote/$merge"); @@ -185,6 +185,7 @@ write_files(<<"EOF_HEADER", <<"EOF_CONFIG"); * DO NOT EDIT DIRECTLY - edit make_patchnum.pl instead ***************************************************************************/ @{[$describe ? "#define PERL_PATCHNUM \"$describe\"" : ()]} +#define PERL_GIT_UNPUSHED_COMMITS_LEN ${\scalar(@unpushed_commits)} #define PERL_GIT_UNPUSHED_COMMITS\t\t\\ $unpushed_commits/*leave-this-comment*/ @{[$changed ? "#define PERL_GIT_UNCOMMITTED_CHANGES" : ()]} diff --git a/patchlevel.h b/patchlevel.h index a7e8dac..cd60a71 100644 --- a/patchlevel.h +++ b/patchlevel.h @@ -125,9 +125,11 @@ hunk. # if defined(PERL_IS_MINIPERL) # define PERL_PATCHNUM "UNKNOWN-miniperl" # define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/ +# define PERL_GIT_UNPUSHED_COMMITS_LEN 0 # elif defined(PERL_MICRO) # define PERL_PATCHNUM "UNKNOWN-microperl" # define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/ +# define PERL_GIT_UNPUSHED_COMMITS_LEN 0 # else #include "git_version.h" # endif @@ -146,6 +148,14 @@ static const char * const local_patches[] = { # define LOCAL_PATCH_COUNT \ ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2)) +#ifdef PERL_GIT_UNCOMMITTED_CHANGES +# define LOCAL_PATCH_COUNT_CONST \ + (PERL_GIT_UNPUSHED_COMMITS_LEN+1) +#else +# define LOCAL_PATCH_COUNT_CONST \ + (PERL_GIT_UNPUSHED_COMMITS_LEN) +#endif + /* the old terms of reference, add them only when explicitly included */ #define PATCHLEVEL PERL_VERSION #undef SUBVERSION /* OS/390 has a SUBVERSION in a system header */ diff --git a/perl.c b/perl.c index 8271915..ad7db6a 100644 --- a/perl.c +++ b/perl.c @@ -3480,56 +3480,64 @@ S_minus_v(pTHX) SvREFCNT_dec_NN(level); } #if defined(LOCAL_PATCH_COUNT) - if (LOCAL_PATCH_COUNT > 0) - PerlIO_printf(PIO_stdout, - "\n(with %d registered patch%s, " - "see perl -V for more detail)", - LOCAL_PATCH_COUNT, - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); + assert(LOCAL_PATCH_COUNT_CONST == LOCAL_PATCH_COUNT); #endif - +/* start of Larry Wall copyright printf */ PerlIO_printf(PIO_stdout, - "\n\nCopyright 1987-2013, Larry Wall\n"); +#if defined(LOCAL_PATCH_COUNT) && LOCAL_PATCH_COUNT_CONST != 0 + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */ +# if LOCAL_PATCH_COUNT_CONST == 1 + "\n(with %d registered patch, see perl -V for more detail)%s" +# else + "\n(with %d registered patches, see perl -V for more detail)%s" +# endif + ,LOCAL_PATCH_COUNT_CONST, +#else + /* use copyrights for format string */ +#endif + "\n\nCopyright 1987-2013, Larry Wall\n" #ifdef MSDOS - PerlIO_printf(PIO_stdout, - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" #endif #ifdef DJGPP - PerlIO_printf(PIO_stdout, "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" #endif #ifdef OS2 - PerlIO_printf(PIO_stdout, "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" #endif #ifdef OEMVS - PerlIO_printf(PIO_stdout, - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" #endif #ifdef __VOS__ - PerlIO_printf(PIO_stdout, - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" #endif #ifdef POSIX_BC - PerlIO_printf(PIO_stdout, - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" +#endif +#ifdef __SYMBIAN32__ + "Symbian port by Nokia, 2004-2005\n" #endif +/* UNDER_CE closes Larry Wall copyright printf */ #ifdef UNDER_CE - PerlIO_printf(PIO_stdout, "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ BINARY_BUILD_NOTICE; #endif +/* because of wce_hitreturn() open new printf for UNDER_CE */ +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ -- 1.7.9.msysgit.0
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 654b
On Sun Dec 22 09:23:16 2013, bulk88 wrote: Show quoted text
> On Mon Nov 11 16:53:38 2013, tonyc wrote:
> > > > Silencing the warning does not make the code correct. > >
> > CC warning free now, is it a ptr or is it an int in the format string > design is gone. >
> > It also doesn't fix the painful "where does this statement" end that > > was introduced in the original patch. > >
> .......
> > > > 2) don't try to consolidate the "Perl may be copied ..." printf(), > > it's the main readability issue with your changes.
> > I changed the comments to be clearer where the concatenation goes.
Bump. Patch needs review and applying. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 158b
On Wed Mar 05 04:49:43 2014, bulk88 wrote: Show quoted text
> Bump. Patch needs review and applying.
This fails to apply to blead, I haven't tried to apply it manually. Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 337b
On Fri Mar 07 19:00:30 2014, tonyc wrote: Show quoted text
> On Wed Mar 05 04:49:43 2014, bulk88 wrote:
> > Bump. Patch needs review and applying.
> > This fails to apply to blead, I haven't tried to apply it manually. > > Tony
I fixed the conflicts and created a new patch. Patch is testing right now on my system. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 429b
On Fri Mar 07 20:09:09 2014, bulk88 wrote: Show quoted text
> On Fri Mar 07 19:00:30 2014, tonyc wrote:
> > On Wed Mar 05 04:49:43 2014, bulk88 wrote:
> > > Bump. Patch needs review and applying.
> > > > This fails to apply to blead, I haven't tried to apply it manually. > > > > Tony
> > I fixed the conflicts and created a new patch. Patch is testing right > now on my system.
Will attach when that is done. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 146b
On Fri Mar 07 20:09:36 2014, bulk88 wrote: Show quoted text
> Will attach when that is done.
New patch against blead attached. -- bulk88 ~ bulk88 at hotmail.com
Subject: 0001-optimize-S_minus_v.patch
From b5787d4e10533e116b931570f821bb7254fe2721 Mon Sep 17 00:00:00 2001 From: Daniel Dragan <bulk88@hotmail.com> Date: Sun, 22 Dec 2013 12:19:34 -0500 Subject: [PATCH] optimize S_minus_v With the copyright lines, use more CPP cating. This reduced the number of instructions, and removes a couple alignment null in .rdata image section. UNDER_CE presents a challange because of the wce_hitreturn(); call. BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains a perlio printf call. So that can not be CPP catted unfortunatly. If a build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright line is catted with the license line. LOCAL_PATCH_COUNT is a C expression, like offsetof, which is not a preprocessor constant, so add PERL_GIT_UNPUSHED_COMMITS_LEN and LOCAL_PATCH_COUNT_CONST which are pure CPP constants. LOCAL_PATCH_COUNT can be changed to LOCAL_PATCH_COUNT_CONST in the future. Although S_minus_v is not hot and is rarely used, making the interp image smaller, and therefore faster is a good idea. There should not be any user visible changes to -v with this patch. switches.t contains a regexp for -v already so no further tests were added. This patch is part of #116296. --- make_patchnum.pl | 9 ++++--- patchlevel.h | 10 +++++++++ perl.c | 60 ++++++++++++++++++++++++++++++----------------------- 3 files changed, 49 insertions(+), 30 deletions(-) diff --git a/make_patchnum.pl b/make_patchnum.pl index 3f857b5..82ec266 100644 --- a/make_patchnum.pl +++ b/make_patchnum.pl @@ -122,6 +122,7 @@ sub write_files { return 0; } +my @unpushed_commits; my $unpushed_commits = ' '; my ($read, $branch, $snapshot_created, $commit_id, $describe)= ("") x 5; my ($changed, $extra_info, $commit_title)= ("") x 3; @@ -155,14 +156,13 @@ elsif (-d "$srcdir/.git") { } if (length $branch && length $remote) { + @unpushed_commits = grep {/\+/} backtick("git cherry $remote/$merge"); # git cherry $remote/$branch | awk 'BEGIN{ORS=","} /\+/ {print $2}' | sed -e 's/,$//' my $unpushed_commit_list = - join ",", map { (split /\s/, $_)[1] } - grep {/\+/} backtick("git cherry $remote/$merge"); + join ",", map { (split /\s/, $_)[1] } @unpushed_commits; # git cherry $remote/$branch | awk 'BEGIN{ORS="\t\\\\\n"} /\+/ {print ",\"" $2 "\""}' $unpushed_commits = - join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } - grep {/\+/} backtick("git cherry $remote/$merge"); + join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } @unpushed_commits; if (length $unpushed_commits) { $commit_title = "Local Commit:"; my $ancestor = backtick("git rev-parse $remote/$merge"); @@ -185,6 +185,7 @@ write_files(<<"EOF_HEADER", <<"EOF_CONFIG"); * DO NOT EDIT DIRECTLY - edit make_patchnum.pl instead ***************************************************************************/ @{[$describe ? "#define PERL_PATCHNUM \"$describe\"" : ()]} +#define PERL_GIT_UNPUSHED_COMMITS_LEN ${\scalar(@unpushed_commits)} #define PERL_GIT_UNPUSHED_COMMITS\t\t\\ $unpushed_commits/*leave-this-comment*/ @{[$changed ? "#define PERL_GIT_UNCOMMITTED_CHANGES" : ()]} diff --git a/patchlevel.h b/patchlevel.h index 37dda6a..39191fb 100644 --- a/patchlevel.h +++ b/patchlevel.h @@ -125,9 +125,11 @@ hunk. # if defined(PERL_IS_MINIPERL) # define PERL_PATCHNUM "UNKNOWN-miniperl" # define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/ +# define PERL_GIT_UNPUSHED_COMMITS_LEN 0 # elif defined(PERL_MICRO) # define PERL_PATCHNUM "UNKNOWN-microperl" # define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/ +# define PERL_GIT_UNPUSHED_COMMITS_LEN 0 # else #include "git_version.h" # endif @@ -146,6 +148,14 @@ static const char * const local_patches[] = { # define LOCAL_PATCH_COUNT \ ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2)) +#ifdef PERL_GIT_UNCOMMITTED_CHANGES +# define LOCAL_PATCH_COUNT_CONST \ + (PERL_GIT_UNPUSHED_COMMITS_LEN+1) +#else +# define LOCAL_PATCH_COUNT_CONST \ + (PERL_GIT_UNPUSHED_COMMITS_LEN) +#endif + /* the old terms of reference, add them only when explicitly included */ #define PATCHLEVEL PERL_VERSION #undef SUBVERSION /* OS/390 has a SUBVERSION in a system header */ diff --git a/perl.c b/perl.c index 36d33d9..cb7d62a 100644 --- a/perl.c +++ b/perl.c @@ -3498,56 +3498,64 @@ S_minus_v(pTHX) SvREFCNT_dec_NN(level); } #if defined(LOCAL_PATCH_COUNT) - if (LOCAL_PATCH_COUNT > 0) - PerlIO_printf(PIO_stdout, - "\n(with %d registered patch%s, " - "see perl -V for more detail)", - LOCAL_PATCH_COUNT, - (LOCAL_PATCH_COUNT!=1) ? "es" : ""); + assert(LOCAL_PATCH_COUNT_CONST == LOCAL_PATCH_COUNT); #endif - +/* start of Larry Wall copyright printf */ PerlIO_printf(PIO_stdout, - "\n\nCopyright 1987-2014, Larry Wall\n"); +#if defined(LOCAL_PATCH_COUNT) && LOCAL_PATCH_COUNT_CONST != 0 + /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */ +# if LOCAL_PATCH_COUNT_CONST == 1 + "\n(with %d registered patch, see perl -V for more detail)%s" +# else + "\n(with %d registered patches, see perl -V for more detail)%s" +# endif + ,LOCAL_PATCH_COUNT_CONST, +#else + /* use copyrights for format string */ +#endif + "\n\nCopyright 1987-2014, Larry Wall\n" #ifdef MSDOS - PerlIO_printf(PIO_stdout, - "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"); + "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n" #endif #ifdef DJGPP - PerlIO_printf(PIO_stdout, "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n" - "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"); + "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n" #endif #ifdef OS2 - PerlIO_printf(PIO_stdout, "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n" - "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"); + "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n" #endif #ifdef OEMVS - PerlIO_printf(PIO_stdout, - "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"); + "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n" #endif #ifdef __VOS__ - PerlIO_printf(PIO_stdout, - "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"); + "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n" #endif #ifdef POSIX_BC - PerlIO_printf(PIO_stdout, - "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"); + "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n" +#endif +#ifdef __SYMBIAN32__ + "Symbian port by Nokia, 2004-2005\n" #endif +/* UNDER_CE closes Larry Wall copyright printf */ #ifdef UNDER_CE - PerlIO_printf(PIO_stdout, "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ +# endif +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ BINARY_BUILD_NOTICE; #endif +/* because of wce_hitreturn() open new printf for UNDER_CE */ +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ -- 1.7.9.msysgit.0
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 204b
On Fri Mar 07 22:02:52 2014, bulk88 wrote: Show quoted text
> On Fri Mar 07 20:09:36 2014, bulk88 wrote:
> > Will attach when that is done.
> > New patch against blead attached.
Bump. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 104b
While I'd like to see this ticket reviewed, I am declining to mark it as a blocker for 5.20.0. -- rjbs
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 182b
On Mon Mar 24 07:16:06 2014, rjbs wrote: Show quoted text
> While I'd like to see this ticket reviewed, I am declining to mark it > as a blocker for 5.20.0.
Bump. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Thu Apr 03 23:23:16 2014, bulk88 wrote: Show quoted text
> On Mon Mar 24 07:16:06 2014, rjbs wrote:
> > While I'd like to see this ticket reviewed, I am declining to mark it > > as a blocker for 5.20.0.
> > Bump.
I won't accept it while that printf() can end in more than one place. Right now it can end in three different places: #ifdef UNDER_CE - PerlIO_printf(PIO_stdout, "WINCE port by Rainer Keuchel, 2001-2002\n" "Built on " __DATE__ " " __TIME__ "\n\n"); **HERE wce_hitreturn(); -#endif -#ifdef __SYMBIAN32__ - PerlIO_printf(PIO_stdout, - "Symbian port by Nokia, 2004-2005\n"); -#endif +#endif /* end of additional copyright lines */ + #ifdef BINARY_BUILD_NOTICE +# ifndef UNDER_CE + ); /* close Larry Wall copyright printf */ **HERE +# endif +/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */ BINARY_BUILD_NOTICE; #endif +/* because of wce_hitreturn() open new printf for UNDER_CE */ +#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE) PerlIO_printf(PIO_stdout, +/* else CPP catenation continues from Larry Wall copyright printf */ +#endif "\n\ Perl may be copied only under the terms of either the Artistic License or the\n\ GNU General Public License, which may be found in the Perl 5 source kit.\n\n\ ** SOMEWHERE BELOW HERE Unfortunately I expect that will remove the rather trivial space improvement the patch brings. Otherwise, I'm worried about the issue Nicholas mentioned, where some compilers might choke on the mix of pre-processor directives and string literal concatenation. Tony


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org