Skip Menu |
Report information
Id: 129961
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: petdance <andy [at] petdance.com>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.24.0
Fixed In: (no value)



Date: Tue, 25 Oct 2016 00:44:40 -0400 (EDT)
From: andy [...] petdance.com
Subject: [PATCH] Remove some compiler hoops to jump through for EBCDIC
To: perlbug [...] perl.org
CC: andy [...] petdance.com
Download (untitled) / with headers
text/plain 4.2k
This is a bug report for perl from andy@petdance.com, generated with the help of perlbug 1.40 running under perl 5.24.0. ----------------------------------------------------------------- [Please describe your issue here] This patch simplifies two bits of code that I came across while working on supporting the clang -Weverything flag. The first, in Perl_validate_proto, removes unnecessary variable initialization if proto of NULL is passed. The second, in S_scan_const, rearranges some code and #ifdefs so that the convert_unicode and real_range_max variables are only declared if EBCDIC is set. This lets us no longer have to unnecessarily set useless variables to make the compiler happy, and it saves us from some unnecessary checks on "if (convert_unicode)". One of the comments says "(Compilers should optimize this out for non-EBCDIC)", but now the compiler won't even see these unnecessary variables or tests. This diff may be easier to understand if it's viewed as two side-by-side files, such as with vimdiff. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low Type=Patch PatchStatus=HasPatch --- Site configuration information for perl 5.24.0: Configured by andy at Sun Jun 5 23:28:46 CDT 2016. Summary of my perl5 (revision 5 version 24 subversion 0) configuration: Platform: osname=linux, osvers=3.10.0-327.18.2.el7.x86_64, archname=x86_64-linux uname='linux clifford 3.10.0-327.18.2.el7.x86_64 #1 smp thu may 12 11:03:55 utc 2016 x86_64 x86_64 x86_64 gnulinux ' config_args='-de -Dprefix=/home/andy/perl5/perlbrew/perls/perl-5.24.0 -Aeval:scriptdir=/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=define, use64bitall=define, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2', optimize='-O2', cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='', gccversion='4.8.5 20150623 (Red Hat 4.8.5-4)', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib /lib64 /usr/lib64 /usr/local/lib64 libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.17.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.17' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong' Locally applied patches: Devel::PatchPerl 1.38 --- @INC for perl 5.24.0: /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0/x86_64-linux /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0 /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0/x86_64-linux /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0 . --- Environment for perl 5.24.0: HOME=/home/andy LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin:/home/andy/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin PERLBREW_BASHRC_VERSION=0.75 PERLBREW_HOME=/home/andy/.perlbrew PERLBREW_MANPATH=/home/andy/perl5/perlbrew/perls/perl-5.24.0/man PERLBREW_PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin PERLBREW_PERL=perl-5.24.0 PERLBREW_ROOT=/home/andy/perl5/perlbrew PERLBREW_VERSION=0.75 PERLCRITIC=/home/andy/tw/Dev/perlcriticrc PERL_BADLANG (unset) SHELL=/bin/bash
Download toke-ebcdic.patch
text/plain 3.8k

Message body is not shown because sender requested not to inline it.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 231b
On Mon Oct 24 21:45:04 2016, petdance wrote: Show quoted text
> This patch simplifies two bits of code that I came across while > working on supporting the clang -Weverything flag.
Thanks, applied as 11327fa125f019cb46cfadd1fcec077cbcb8e41f. Tony
From: Karl Williamson <public [...] khwilliamson.com>
Date: Sat, 26 Nov 2016 15:33:17 -0700
To: perl5-porters [...] perl.org
Subject: Re: [perl #129961] [PATCH] Remove some compiler hoops to jump through for EBCDIC
Download (untitled) / with headers
text/plain 5.6k
On 10/24/2016 10:45 PM, Andy Lester (via RT) wrote: Show quoted text
> # New Ticket Created by Andy Lester > # Please include the string: [perl #129961] > # in the subject line of all future correspondence about this issue. > # <URL: https://rt.perl.org/Ticket/Display.html?id=129961 > > > > This is a bug report for perl from andy@petdance.com, > generated with the help of perlbug 1.40 running under perl 5.24.0.
Show quoted text
> > > ----------------------------------------------------------------- > [Please describe your issue here] > > This patch simplifies two bits of code that I came across while > working on supporting the clang -Weverything flag. > > The first, in Perl_validate_proto, removes unnecessary variable > initialization if proto of NULL is passed. > > The second, in S_scan_const, rearranges some code and #ifdefs so that > the convert_unicode and real_range_max variables are only declared > if EBCDIC is set. This lets us no longer have to unnecessarily set > useless variables to make the compiler happy, and it saves us from some > unnecessary checks on "if (convert_unicode)". One of the comments says > "(Compilers should optimize this out for non-EBCDIC)", but now the > compiler won't even see these unnecessary variables or tests. >
Given that this code was generating compiler warnings was the reason to fix it. But I do want to clarify that he reason I wrote it the way it was, was to cut down on the number of #ifdef EBCDICs there are. Each added #ifdef is an added human maintenance cost. It makes the code harder to read, as it disrupts the flow of what is going on. It also means that some code won't even be compiled except under the right conditions, and that skipped code may not even be syntactically valid, which you won't find out until and unless there is some smoker (or worse, something from the field that we don't have the resources to reproduce) that has the correct configurations to trigger it. When I first started the modern EBCDIC port, there were a bunch of syntax errors to work through from code that had been modified, but didn't get compiled. By coding it the way it was, I made it easier to read, and hence maintain, and I knew it would compile on both types of machines. Show quoted text
> This diff may be easier to understand if it's viewed as two side-by-side > files, such as with vimdiff. > > [Please do not change anything below this line] > ----------------------------------------------------------------- > --- > Flags: > category=core > severity=low > Type=Patch > PatchStatus=HasPatch > --- > Site configuration information for perl 5.24.0: > > Configured by andy at Sun Jun 5 23:28:46 CDT 2016. > > Summary of my perl5 (revision 5 version 24 subversion 0) configuration: > > Platform: > osname=linux, osvers=3.10.0-327.18.2.el7.x86_64, archname=x86_64-linux > uname='linux clifford 3.10.0-327.18.2.el7.x86_64 #1 smp thu may 12 11:03:55 utc 2016 x86_64 x86_64 x86_64 gnulinux ' > config_args='-de -Dprefix=/home/andy/perl5/perlbrew/perls/perl-5.24.0 -Aeval:scriptdir=/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin' > hint=recommended, useposix=true, d_sigaction=define > useithreads=undef, usemultiplicity=undef > use64bitint=define, use64bitall=define, uselongdouble=undef > usemymalloc=n, bincompat5005=undef > Compiler: > cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2', > optimize='-O2', > cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' > ccversion='', gccversion='4.8.5 20150623 (Red Hat 4.8.5-4)', gccosandvers='' > intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3 > d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3 > ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 > alignbytes=8, prototype=define > Linker and Libraries: > ld='cc', ldflags =' -fstack-protector-strong -L/usr/local/lib' > libpth=/usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib /lib64 /usr/lib64 /usr/local/lib64 > libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc > perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc > libc=libc-2.17.so, so=so, useshrplib=false, libperl=libperl.a > gnulibc_version='2.17' > Dynamic Linking: > dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' > cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong' > > Locally applied patches: > Devel::PatchPerl 1.38 > > --- > @INC for perl 5.24.0: > /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0/x86_64-linux > /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0 > /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0/x86_64-linux > /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0 > . > > --- > Environment for perl 5.24.0: > HOME=/home/andy > LANG=en_US.UTF-8 > LANGUAGE (unset) > LD_LIBRARY_PATH (unset) > LOGDIR (unset) > PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin:/home/andy/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin > PERLBREW_BASHRC_VERSION=0.75 > PERLBREW_HOME=/home/andy/.perlbrew > PERLBREW_MANPATH=/home/andy/perl5/perlbrew/perls/perl-5.24.0/man > PERLBREW_PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin > PERLBREW_PERL=perl-5.24.0 > PERLBREW_ROOT=/home/andy/perl5/perlbrew > PERLBREW_VERSION=0.75 > PERLCRITIC=/home/andy/tw/Dev/perlcriticrc > PERL_BADLANG (unset) > SHELL=/bin/bash >
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 277b
Show quoted text
> > By coding it the way it was, I made it easier to read, and hence > maintain, and I knew it would compile on both types of machines
That was my intent, too, to make it easier to follow for the programmer by encapsulating the EBCDIC-only variables within the ifdef blocks.
From: Aaron Crane <arc [...] cpan.org>
To: James E Keenan via RT <perlbug-followup [...] perl.org>
Date: Sun, 27 Nov 2016 18:41:44 +0000
CC: Perl5 Porters <perl5-porters [...] perl.org>
Subject: Re: [perl #129961] [PATCH] Remove some compiler hoops to jump through for EBCDIC
Download (untitled) / with headers
text/plain 1.5k
Andy Lester via RT <perlbug-followup@perl.org> wrote: Show quoted text
>> By coding it the way it was, I made it easier to read, and hence >> maintain, and I knew it would compile on both types of machines
> > That was my intent, too, to make it easier to follow for the programmer by encapsulating the EBCDIC-only variables within the ifdef blocks.
(Apologies for the delay in this response; I meant to follow up on this ticket some weeks ago, but doing so apparently slipped my mind.) My own view is that the net consequence of this change is to make it harder rather than easier to follow the logic in question. My biggest concerns relate to code structured like this: #ifdef EBCDIC if (convert_unicode) { … } else #endif { … } This sort of code interleaves conditionals written in two very different languages. Correctly construing the meaning of this code for all paths requires a mental dance that effectively reimplements the various translation phases of a C compiler. I'm also aware that we have a lot of code with the sort of structure in question (and, indeed, some of it is even harder to follow than this example), but I'd strongly prefer simplifying what we already have, rather than letting these structures proliferate. I think the best course of action would be to revert 11327fa125f019cb46cfadd1fcec077cbcb8e41f (though I regret saying so, because I know that you submitted this patch in a good-faith attempt to improve Perl). -- Aaron Crane ** http://aaroncrane.co.uk/
CC: Perl5 Porters <perl5-porters [...] perl.org>
Subject: Re: [perl #129961] [PATCH] Remove some compiler hoops to jump through for EBCDIC
Date: Sun, 27 Nov 2016 21:41:55 -0700
To: Aaron Crane <arc [...] cpan.org>, James E Keenan via RT <perlbug-followup [...] perl.org>
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 2.4k
On 11/27/2016 11:41 AM, Aaron Crane wrote: Show quoted text
> Andy Lester via RT <perlbug-followup@perl.org> wrote:
>>> By coding it the way it was, I made it easier to read, and hence >>> maintain, and I knew it would compile on both types of machines
>> >> That was my intent, too, to make it easier to follow for the programmer by encapsulating the EBCDIC-only variables within the ifdef blocks.
> > (Apologies for the delay in this response; I meant to follow up on > this ticket some weeks ago, but doing so apparently slipped my mind.) > > My own view is that the net consequence of this change is to make it > harder rather than easier to follow the logic in question. My biggest > concerns relate to code structured like this: > > #ifdef EBCDIC > if (convert_unicode) { > … > } > else > #endif > { … } > > This sort of code interleaves conditionals written in two very > different languages. Correctly construing the meaning of this code for > all paths requires a mental dance that effectively reimplements the > various translation phases of a C compiler. > > I'm also aware that we have a lot of code with the sort of structure > in question (and, indeed, some of it is even harder to follow than > this example), but I'd strongly prefer simplifying what we already > have, rather than letting these structures proliferate. I think the > best course of action would be to revert > 11327fa125f019cb46cfadd1fcec077cbcb8e41f (though I regret saying so, > because I know that you submitted this patch in a good-faith attempt > to improve Perl). >
But, my understanding is that this patch silences compiler warnings that are not currently enabled, but that we are working to clean up the core so that they can be enabled, and if so, I think that trumps readability in this case. So the patch should remain, if my understanding is correct. And it's clear that what is the best readability is debatable, even though my taste agrees with Aaron in this case. I've spent quite a bit of effort to try to eliminate EBCDIC vs ASCII separate compilation paths, confining them to almost entirely utf8.h and utf8.c. The main exceptions are because in EBCDIC, the range [A-Z] includes more than 26 characters, and we have special handling to try to make that invisible to the Perl application. There are two affected files: toke.c because we can say tr/A-Z/foo/, and regcomp.c because of qr/[A-Z]/. This code is in one of those areas.
To: Karl Williamson <public [...] khwilliamson.com>
Date: Sun, 27 Nov 2016 22:52:49 -0600
From: Andy Lester <andy [...] petdance.com>
CC: Aaron Crane <arc [...] cpan.org>, James E Keenan via RT <perlbug-followup [...] perl.org>, Perl5 Porters <perl5-porters [...] perl.org>
Subject: Re: [perl #129961] [PATCH] Remove some compiler hoops to jump through for EBCDIC
Download (untitled) / with headers
text/plain 264b
Show quoted text
> But, my understanding is that this patch silences compiler warnings that are not currently enabled, but that we are working to clean up the core so that they can be enabled,
Yes, that is correct. It was uncovered on my branch to enable clang's -Weverything.
Download (untitled) / with headers
text/plain 313b
Thank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been resolved. Perl 5.26.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.26.0 If you find that the problem persists, feel free to reopen this ticket.


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