Improving Perl 5: Grant Report for Month 11
Mon, 03-Sep-2012 by
Karen Pauley
edit post
<![CDATA[
_Nicholas Clark writes:_
More fun with compilers this month, with make utilities joining in. H. Merijn Brand and I identified this problem with HP's compiler a while back, but I'd not yet had time to fix it:
bc. $ cc -DPERL_CORE -c -Ae -D_HPUX_SOURCE -Wl,+vnocompatwarnings +DD64 -
DDEBUGGING -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 +O
2 +Onolimit -g pp_sys.c
cc: line 2976: panic 5172: Backend Assert ** Unimplemented CVT. (5172)
$ echo $?
1
Of course, HP's make just has to be helpfully "special" and despite noticing the failure exit code does not remove build products from the failed step:
bc. $ make pp_sys.o
`sh cflags "optimize='+O2 +Onolimit -g'" pp_sys.o` pp_sys.c
CCCMD = ccache cc -DPERL_CORE -c -Ae -D_HPUX_SOURCE -Wl,+vnocompatwarnings
+DD64 -DDEBUGGING -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 +O2 +Onolimit -g
cc: warning 404: Pre-processor not invoked, options ignored.
cc: line 2976: panic 5172: Backend Assert ** Unimplemented CVT. (5172)
*** Error exit code 1
bc. Stop.
$ ls -l pp_sys.o
-rw-r--r-- 1 nick perl 53592 Jul 30 12:04 pp_sys.o
$ file pp_sys.o
pp_sys.o: awk program text
$ less pp_sys.o
"pp_sys.o" may be a binary file. See it anyway?
which of course means that if you re-run make it then assumes that pp_sys.o is up to date, carries on and then fails at the link. "Special", as I said.
The compiler is failing to deal with this (valid) macro after the refactorings of commits d2c4d2d1e22d3125 and 8d7906e182f93e18:
bc. #define FT_RETURN_TRUE(X) \
RETURNX((void)( \
PL_op->op_flags & OPf_REF \
? (bool)XPUSHs( \
PL_op->op_private & OPpFT_STACKING ? (SV *)cGVOP_gv : (X) \
) \
: (PL_op->op_private & OPpFT_STACKING || SETs(X)) \
))
part of a sequence where Father Chrysostomos considerably simplified the filetest ops so that they are consistent and clear in when they manipulate the perl stack. Fortunately his simplification also made it possible for me to see a way to refactor things a bit further to reduce the complexity of the code generally, and the macros in particular. The diff is slightly deceptive
bc. $ git diff -R --stat 4c21785fe645f05e
pp_sys.c | 80 ++++++++++++++++++++++++++++++++--------------------------------
1 file changed, 40 insertions(+), 40 deletions(-)
because the forty lines of additions include 6 lines of documenting comments and 3 lines of whitespace.
So, on the subject of HP-UX make, what would one expect this Makefile to do?
bc. $ cat Makefile
bar: foo
cp $? $@
bc. baz: foo
ln -s $? $@
Here's AIX, not known for being the most flexible platform:
bc. $ touch foo
$ make bar
cp foo bar
$ make baz
ln -s foo baz
$ make bar
and repeat:
bc. Target "bar" is up to date.
$ make baz
Target "baz" is up to date.
Every other make I tried behaves the same. *Except* HP-UX:
bc. $ touch foo
$ make bar
cp foo bar
$ make baz
ln -s foo baz
$ make bar
`bar' is up to date.
$ make baz
ln -s foo baz
ln: baz exists
*** Error exit code 1
Stop.
Special. So the symlink is never up to date (and one has to work round this by always deleting it). I kiss you!
(And don't let's talk about HP-UX make's "support" for parallel makes.)
Better news - while working on the filetest operators, I noticed this piece of code just below them:
bc. #if defined(atarist) /* this will work with atariST. Configure will
make guesses for other systems. */
# define FILE_base(f) ((f)->_base)
# define FILE_ptr(f) ((f)->_ptr)
# define FILE_cnt(f) ((f)->_cnt)
# define FILE_bufsiz(f) ((f)->_cnt + ((f)->_ptr - (f)->_base))
#endif
Atari STs - I remember them. 16 bit machines from 25 years ago. So does perl 5 really build on the Atari ST? Seems unlikely - so is that another platform we need to add to the list of "soon to be culled":
"https://metacpan.org/module/perldelta#Platforms-with-no-supporting-programmers:":https://metacpan.org/module/perldelta#Platforms-with-no-supporting-programmers:
So with the help of git blame, I went digging...
"http://perl5.git.perl.org/perl.git/blame/v5.17.2:/pp_sys.c#l3306":http://perl5.git.perl.org/perl.git/blame/v5.17.2:/pp_sys.c#l3306
The code was refactored with "patch.1i for perl5.001", but has been in pp_sys.c since it first appeared with perl-5.000 alpha 2. So where as it in 4.036? doio.c:
"http://perl5.git.perl.org/perl.git/blame/perl-4.0.36:doio.c#l1192":http://perl5.git.perl.org/perl.git/blame/perl-4.0.36:doio.c#l1192
and it was added as part of patch #20, a rather big patch from June 1992. 1 line in that says:
Subject: added Atari ST portability
What was *also* added in that patch was an atarist/ directory, containing various files relevant to the port. We don't have an atarist/ directory now, so where did that go? Turns out it was removed on the release of perl-5.000. That rang a bell - there was also an msdos/ directory in perl 4 (added by perl 3 patch #16), but that was removed with perl-5.000.
So the atarist port is like the old perl 4 MSDOS port - the port specific files were removed with perl 5, but the tentacles left in the main parts of the code were not excised.
This I have now done.
And in the process it turned up a bunch of code for I286 support dating from perl 3 times, mostly specifically coping with 16 bit memory models, and using %ld instead of %d (because sizeof(int) is 2). So that's gone too now, and the world is a bit tidier.
This month I fixed some problems with the git bisect wrapper which were preventing me from bisecting to find the cause of other problems. :-/
The bisect wrapper is designed to sensibly default as much as possible, and its approach for defaulting the revision for the start of the bisect run is to try stable (.0) releases old to new until it finds one which can run the test code correctly. It had been using a hardcoded list, which still had 5.14.0 as the most recent stable release. It now uses @`git tag -l`@ to get the list of stable releases. The default for the end of the bisect used to be 'blead'. Now if there is no 'blead' branch, bisect.pl now uses a suitable alternative - if HEAD is more recent than the last stable release, use HEAD, else use the last stable tag. Also, when it wanted to check out a known good recent version of a file (such as makedepend.SH) it would check out the revision from blead. It now uses the most recent tagged stable release for this.
We've had quite a bit of fun with Debian and Ubuntu's switch to a multiarch setup. This results in important libraries (such as libm.so) moving from the well known /usr/lib to an architecture specific directory. Without knowing where they are, perl won't build. As of 5.14.0 (and 5.12.4), the hints for Configure have been updated to get the correct library paths from gcc, and I thought that I'd correctly put the analogous changes into the bisect wrapper. However, it turned out that it was only correct on x86_64. On other Linux architectures it failed to pass the multiarch locations to Configure. That is now fixed.
I also added a --timeout feature to permit the bisect runner to time out (and kill) the user's test case if it takes longer than the specified time to run. With this I was able to bisect a problem that I'd noticed had appeared recently with debugging builds seeming to hang if PERL_DESTRUCT_LEVEL=2 is set in the environment (RT #114356)
I spent a while digging into the pre-history of the various scalar flags,trying to make sense of how we got to where we are, and why Chip's patch to magic flags makes sense. The full conclusions are here
"http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2012-07/msg00826.html":http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2012-07/msg00826.html
but the question comes down to an inconsistency - there are both "public" and "private" flag bits for integers (I), floating point values (N) and strings (P), but there is only one flag for references (R). This seems wrong - why is this?
It turns out that public and private flags were added by 5.000 alpha 4, as part of implementing magic on scalars. Prior to that version, tainting was implemented by building a separate taintperl binary. Magic enabled tainting to be implemented at runtime (with the -T command line option) in the same binary as the regular perl, without a significant speed hit. Magic also permitted the implementation of tie and untie. However at that time there was no SVf_ROK(), or SvROK(). References could *only* be in SVs of type SVt_REF, and the code in sv_setsv() downgrades the destination SV to type to SVt_REF if needed. Note that one *can't* get a reference from the environment, so a reference can never be tainted.
Once the alpha went out into the wild, people discovered that this meant that also a reference could not be assigned to a tied variable, as noted in this thread from 1994:
"https://groups.google.com/forum/?fromgroups#!msg/comp.lang.perl/TlLd6ttq4o4/-3YuF4n9UysJ":https://groups.google.com/forum/?fromgroups#!msg/comp.lang.perl/TlLd6ttq4o4/-3YuF4n9UysJ
to which Larry replies "I'll fix it. Sounds like we'll want an alpha 5 pretty quick."
And so alpha 5 appeared, and changed SVt_REF to SVt_RV, added SVf_ROK, SvROK() and and SvRV(), thus (pretty much) promoting references to first class scalars with the same semantics as I, N and P.
Alpha 5 also contained a file internals,
"http://perl5.git.perl.org/perl.git/blob/ed6116ce9b9d:/internals":http://perl5.git.perl.org/perl.git/blob/ed6116ce9b9d:/internals
which describes the public flags like this:
bq. These tell whether an integer, double or string value is immediately available without further consideration. All tainting and magic (but not objecthood) works by turning off these bits and forcing a routine to be executed to discover the real value. The SvIV(), SvNV() and SvPV() macros that fetch values are smart about all this, and should always be used if possible.
and the private flags:
bq. These shadow the bits in sv_flags for tainted variables, indicated that there really is a valid value available, but you have to set the global tainted flag if you acces them.
which suggests that the lack of public and private flags for references was a mistake. The scheme was designed for tainting and tie, or designed for tainting and extended to tie, and references weren't quite first class then. References became first class one alpha too late, and that's why they never had the proper split public and private flags. Probably it wasn't noticed because references weren't tainted, and most early uses of references were effectively idempotent, with the result that as long as code was called, it didn't notice if it was called multiple times instead of once.
And yes, this does mean that every version from 5.000 to maint-5.16 has been subtly buggy.
This month finally something clicked and I finally understood the subtleties and assumptions of fold_constants(), op_linklist(), which it calls.
fold_constants() dates back to perl 5.000, but really it would have been better named "various mandatory and optional optree fixups that we need to do at this point", which is neither very terse, nor shorter than the 32 characters that ANSI guarantees as acceptable for a symbol. Since 5.000, various refactorings have moved the code unrelated to constant folding to other routines, leaving fold_constants() pretty much true to its name.
However, fold_constants() is not as general as its name might suggest. It is only able to analyse and fold a tree consisting of just a single op and entirely constant arguments. It can't fold anything more complex, such as this optree:
bc. +
/ \
1 *
/ \
2 3
This doesn't matter to perl's parser, as the optree is constructed from the bottom up, with fold_constants() is called immediately as each op is built, hence the above would be folded as 2 * 3 and then 1 + 6. But this does mean that fold_constants() isn't that useful as a general-purpose constant folding API.
As part of this enlightenment, I refactored op_linklist() to be slightly terser, improved the documentation of its wrapper macro LINKLIST(), and removed needless duplicate calls from fold_constants() to LINKLIST(). I've removed 9 gotos (all vestigial) from fold_constants(), and documented it. I also spotted a long-standing error in perlguts.pod, and fixed that too.
It looked fairly easy to write tests for the documented behaviour, and add it to the public API. It seemed pretty clear that it can return two types of OPs, so both would need testing:
bc. if (type == OP_RV2GV)
newop = newGVOP(OP_GV, 0, MUTABLE_GV(sv));
else
newop = newSVOP(OP_CONST, OPpCONST_FOLDED<<8, MUTABLE_SV(sv));
op_getmad(o,newop,'f');
return newop;
It's clear that the OP_CONST is the common case, and it's obvious how to test it, but what about that newGVOP? I had no idea what called that, so took the brute force approach of replacing it with an abort(), and running a full build and test cycle. (Parallel build and tests mean this takes less than 5 minutes. It's often faster than any other approach if it's not immediately obvious how to reach some code.)
Nothing failed.
Interesting...
So what's going on here? type is the type of the original op was folded. So the newGVOP route can only be reached if fold_constants() completes for an op of type OP_RV2GV. But fold_constants() will never complete for an op of type OP_RV2GV, as it will return almost immediately:
bc. if (!(PL_opargs[type] & OA_FOLDCONST))
return o;
as only ops with the OA_FOLDCONST bit set can be folded. That is set if the op is flagged as 'f' in regen/opcodes, and rv2gv doesn't have the flag. So, did it use to? It turns out that it never had it. The opcode data has moved around a bit in the history of perl, but even back in the earliest revision of perl 5 in git, alpha 2, rv2gv isn't flagged as 'f':
"http://perl5.git.perl.org/perl.git/blame/perl-5a2:/opcode.pl#l176":http://perl5.git.perl.org/perl.git/blame/perl-5a2:/opcode.pl#l176
So the code to return newGVOP, also added in alpha 2:
"http://perl5.git.perl.org/perl.git/blame/perl-5a2:/op.c#l714":http://perl5.git.perl.org/perl.git/blame/perl-5a2:/op.c#l714
has always been dead code. So on the branch, it's gone.
Which only leaves the obviously testable code. "obvious" - always a danger sign.
Turns out that the first problem with testing the folding of constants is that if you try to build an optree ready to fold, the op constructor functions such as newBINOP() spot this and helpfully fold it for you, returning a single OP_CONST, instead of the tree you were hoping for. So you have to subvert their efficiency by lying to them - build OP_NULL instead of the op you really want to fold, then replace the op_type and op_ppaddr values after its returned.
So now you have your tree ready to fold, and you pass it to fold_constants(). At which point you hit the second problem - nothing happens. It turns out that when it executes the ops in order to get the result, the BINOP I was using (OP_MULTIPLY) panics because it doesn't have a target allocated. OP_NULL doesn't need a target, so newBINOP() doesn't create one needlessly. However, no error report escapes, because constant folding runs with all warnings and exceptions trapped, and if anything goes wrong, constant folding is abandoned and the original optree remains. So, also allocate a pad slot, and all is happy.
Except that writing more tests reveals that it's not. SEGVs, wrong numbers of tests run, and "interesting" things like that, which valgrind reveals is due to a read from freed memory in pp_iternext(), the implementation of the looping part of for(). The problem turns out to be allocating that pad slot, however it's done, newBINOP() or the XS test code. It's all because XS code doesn't have its own pad - so at the time of the C calls the current pad is that of the calling Perl subroutine. The *running* subroutine. The problems happen when the pad gets moved as a side effect of being extended to accommodate allocating another slot in it, because the runtime for for() has taken the address of a location within the pad, never expecting it to move. This is a totally reasonable assumption, because the pad moving at runtime simply doesn't happen within the perl interpreter itself - once a subroutine is compiled to ops, neither the optree nor the pad changes again. I don't know how much of the runtime code makes assumptions such as these, but it suggests that the level at which the optree construction functions act doesn't make a good API to call near directly from Perl space.
Rather less successful was an attempt to untangle a bit more of the build process. The distribution 'version' on CPAN ships XS code for dealing with version objects and parsing v-syntax essentially identical to the code in the perl core. However, because that code is needed early in the core bootstrap (specifically, for miniperl, so before the core is able to process XS code), the code for version objects etc has to be shipped as C code. The code in question is within util.c and universal.c, which obviously makes it a real pain for keeping it in sync with the CPAN version.
I had a possible insight - the version distribution also ships with a pure perl implementation, version::vpp - could we use that to bootstrap miniperl? Sadly the answer is no, because "pure perl" has its usual CPAN meaning - "no need to install XS code". version::vpp uses B to extract raw version object metadata from magic attached to the scalar. Even trying to bodge things by forcing that code to return "not found" doesn't work - the build process grinds to a halt with a failure whilst it tries to build lib/Config.pm - ie pretty much the first step after miniperl is ready to run. So, this isn't going to work out. However, it looks like it might be possible to disentangle things using a potentially simpler approach - refactor the version distribution's XS code to split the pertinent C out into separate files, and then #include those directly from universal.c and util.c. John Peacock hopes to be able to look into this further, but can't currently as his time is otherwise spoken for.
A more detailed breakdown summarised from the weekly reports. In these:
16 hex digits refer to commits in "http://perl5.git.perl.org/perl.git":http://perl5.git.perl.org/perl.git
RT #... is a bug in "https://rt.perl.org/rt3/":https://rt.perl.org/rt3/
CPAN #... is a bug in "https://rt.cpan.org/Public/":https://rt.cpan.org/Public/
BBC is "bleadperl breaks CPAN" - Andreas König's test reports for CPAN modules
ID YYYYMMDD.### is an bug number in the old bug system. The RT # is given
afterwards. You can look up the old IDs at https://rt.perl.org/perlbug/
|Hours| |Activity|
|1.50 | BBC (8be227ab5eaa23f2)|
| 0.50 | Dumper.xs on non-gcc|
| 3.25 | HP-UX compiler chokes|
| 0.50 | HP-UX make and symlink targets |
| 0.50 | ID 20001202.002 (RT #4821)|
| 0.50 | Moose & MOP|
| 0.50 | NetWare|
| 0.25 | OS X non-UTF-8 filenames|
| 5.75 | Old RT tickets|
| 1.50 | PL_main_start/PL_main_root|
| 3.25 | PV in %ENV|
| 0.50 | RT #113786|
| 2.00 | RT #113856 |
| 0.25 | RT #113930 |
| 0.50 | RT #113980 |
| 0.25 | RT #114022 |
| 0.50 | RT #114128 |
| 3.00 | RT #114142 |
| 2.00 | RT #114356 |
| 0.50 | RT #22375 |
| 0.25 | RT #24652 |
| 0.25 | RT #66092 |
| 3.00 | RT #77536 |
| 0.75 | array test failure on ARM with -Duse64bitint |
| 2.50 | atarist |
| 12.50 | bisect.pl |
| | bisect.pl (Debian multiarch) |
| | bisect.pl (for RT #114356) |
| 3.00 | build process |
| 0.25 | ck_select |
| 2.25 | code review |
| 1.25 | cross compilation |
| 1.75 | decoupling version |
| 8.00 | fold_constants |
| 0.50 | given/when/smartmatch |
| 0.25 | i5 query |
| 1.00 | investigating security tickets |
| 3.00 | linklist |
| 7.50 | magicflags |
| 0.25 | pp_require|
| 2.25 | process, scalability, mentoring |
| 36.00 | reading/responding to list mail |
| 0.50 | scalarvoid |
| 0.50 | smoke-me/magic_setenv |
| 7.75 | smoke-me/require |
| 0.75 | t/re/reg_posixcc.t |
**123.50 hours total**
]]>
Comments (3)
Thanks for writing these reports. I learn so much from them (even if I don't always understand every word). Keep it up, and keep up the good work on the core!
Bother. The link to the "Platforms soon to be culled" now doesn't work. The "permanent" version should be
https://metacpan.org/module/RJBS/perl-5.16.0/pod/perldelta.pod#Platforms-with-no-supporting-programmers:
"soon" has started - UTS support was removed before v5.17.3, VM/ESA will be gone in v5.17.4, and the plan is to continue removing one per dev release until the code freeze.
link above works now, but AtariST is not on it.
