Skip Menu |
Report information
Id: 132159
Status: pending release
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: atoomic [at] cpan.org
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



Date: Mon, 25 Sep 2017 12:16:13 -0600
From: "Nicolas R." <atoomic [...] cpan.org>
Subject: Use preprocessor check for some DEBUG_X_TEST calls in sv.c and toke.c
To: perlbug [...] perl.org
Download (untitled) / with headers
text/plain 3.5k
This is a bug report for perl from atoomic@cpan.org,
generated with the help of perlbug 1.40 running under perl 5.27.5.


-----------------------------------------------------------------
Most of the DEBUG_?_TEST calls are already protected
by one '#idef DEBUGGING', but noticed a few of them
which were not protected in sv.c and toke.c

We should avoid these extra 'if' statements if perl
is not compiled with debug: -DDEBUGGING.

Tested by running testsuite with/without -DDEBUGGING option
-----------------------------------------------------------------
---
Flags:
    category=core
    severity=low
---
Site configuration information for perl 5.27.5:

Configured by root at Mon Sep 25 12:23:53 CDT 2017.

Summary of my perl5 (revision 5 version 27 subversion 5) configuration:
  Commit id: e0ede300d5b7e50cc49cb1da9545c11bb14b6bec
  Platform:
    osname=linux
    osvers=3.10.0-693.2.2.el7.x86_64
    archname=x86_64-linux
    uname='linux nico-c7.dev.cpanel.net 3.10.0-693.2.2.el7.x86_64 #1 smp tue sep 12 22:26:13 utc 2017 x86_64 x86_64 x86_64 gnulinux '
    config_args='-Dusedevel -des'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    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-16)'
    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 -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    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'


---
@INC for perl 5.27.5:
    lib
    /root/.dotfiles/perl-must-have/lib
    /root/perl5/lib/perl5/
    /usr/local/lib/perl5/site_perl/5.27.5/x86_64-linux
    /usr/local/lib/perl5/site_perl/5.27.5
    /usr/local/lib/perl5/5.27.5/x86_64-linux
    /usr/local/lib/perl5/5.27.5

---
Environment for perl 5.27.5:
    HOME=/root
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/local/cpanel/3rdparty/perl/526/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/3rdparty/perl/522/bin:/usr/local/cpanel/3rdparty/perl/514/bin:/usr/local/cpanel/3rdparty/bin:/root/bin/:/opt/local/bin:/opt/local/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/opt/cpanel/composer/bin:/root/.dotfiles/bin:/root/perl5/bin:/root/.rvm/bin:/root/bin
    PERL5DB=use Devel::NYTProf
    PERL5LIB=/root/.dotfiles/perl-must-have/lib::/root/perl5/lib/perl5/
    PERL_BADLANG (unset)
    PERL_CPANM_OPT=--quiet
    SHELL=/bin/bash

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 3.9k
add inline patch On Mon, 25 Sep 2017 11:16:33 -0700, atoomic@cpan.org wrote: Show quoted text
> This is a bug report for perl from atoomic@cpan.org, > generated with the help of perlbug 1.40 running under perl 5.27.5. > > > ----------------------------------------------------------------- > Most of the DEBUG_?_TEST calls are already protected > by one '#idef DEBUGGING', but noticed a few of them > which were not protected in sv.c and toke.c > > We should avoid these extra 'if' statements if perl > is not compiled with debug: -DDEBUGGING. > > Tested by running testsuite with/without -DDEBUGGING option > ----------------------------------------------------------------- > --- > Flags: > category=core > severity=low > --- > Site configuration information for perl 5.27.5: > > Configured by root at Mon Sep 25 12:23:53 CDT 2017. > > Summary of my perl5 (revision 5 version 27 subversion 5) > configuration: > Commit id: e0ede300d5b7e50cc49cb1da9545c11bb14b6bec > Platform: > osname=linux > osvers=3.10.0-693.2.2.el7.x86_64 > archname=x86_64-linux > uname='linux nico-c7.dev.cpanel.net 3.10.0-693.2.2.el7.x86_64 #1 > smp > tue sep 12 22:26:13 utc 2017 x86_64 x86_64 x86_64 gnulinux ' > config_args='-Dusedevel -des' > hint=recommended > useposix=true > d_sigaction=define > useithreads=undef > usemultiplicity=undef > use64bitint=define > use64bitall=define > uselongdouble=undef > usemymalloc=n > default_inc_excludes_dot=define > 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-16)' > 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 -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc > -lgdbm_compat > 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' > > > --- > @INC for perl 5.27.5: > lib > /root/.dotfiles/perl-must-have/lib > /root/perl5/lib/perl5/ > /usr/local/lib/perl5/site_perl/5.27.5/x86_64-linux > /usr/local/lib/perl5/site_perl/5.27.5 > /usr/local/lib/perl5/5.27.5/x86_64-linux > /usr/local/lib/perl5/5.27.5 > > --- > Environment for perl 5.27.5: > HOME=/root > LANG=en_US.UTF-8 > LANGUAGE (unset) > LD_LIBRARY_PATH (unset) > LOGDIR (unset) > > PATH=/usr/local/cpanel/3rdparty/perl/526/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/3rdparty/perl/522/bin:/usr/local/cpanel/3rdparty/perl/514/bin:/usr/local/cpanel/3rdparty/bin:/root/bin/:/opt/local/bin:/opt/local/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/opt/cpanel/composer/bin:/root/.dotfiles/bin:/root/perl5/bin:/root/.rvm/bin:/root/bin > PERL5DB=use Devel::NYTProf > PERL5LIB=/root/.dotfiles/perl-must- > have/lib::/root/perl5/lib/perl5/ > PERL_BADLANG (unset) > PERL_CPANM_OPT=--quiet > SHELL=/bin/bash
Subject: 0001-Use-preprocessor-check-for-some-DEBUG_X_TEST.patch
From e0ede300d5b7e50cc49cb1da9545c11bb14b6bec Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Mon, 25 Sep 2017 12:18:28 -0500 Subject: [PATCH] Use preprocessor check for some DEBUG_X_TEST Most of the DEBUG_?_TEST calls are already protected by one '#idef DEBUGGING' check, but noticed a few of them which were not protected in sv.c and toke.c We should avoid these extra 'if' statements if perl is not compiled with debug option: -DDEBUGGING. --- sv.c | 22 +++++++++++++++------- toke.c | 10 ++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/sv.c b/sv.c index 6dcd99ae59..9d8b00bf4b 100644 --- a/sv.c +++ b/sv.c @@ -4694,11 +4694,13 @@ Perl_sv_setsv_flags(pTHX_ SV *dstr, SV* sstr, const I32 flags) ) { /* Either it's a shared hash key, or it's suitable for copy-on-write. */ +#ifdef DEBUGGING if (DEBUG_C_TEST) { PerlIO_printf(Perl_debug_log, "Copy on write: sstr --> dstr\n"); sv_dump(sstr); sv_dump(dstr); } +#endif #ifdef PERL_ANY_COW if (!(sflags & SVf_IsCOW)) { SvIsCOW_on(sstr); @@ -4872,7 +4874,7 @@ Perl_sv_setsv_cow(pTHX_ SV *dstr, SV *sstr) #endif PERL_ARGS_ASSERT_SV_SETSV_COW; - +#ifdef DEBUGGING if (DEBUG_C_TEST) { PerlIO_printf(Perl_debug_log, "Fast copy on write: %p -> %p\n", (void*)sstr, (void*)dstr); @@ -4880,7 +4882,7 @@ Perl_sv_setsv_cow(pTHX_ SV *dstr, SV *sstr) if (dstr) sv_dump(dstr); } - +#endif if (dstr) { if (SvTHINKFIRST(dstr)) sv_force_normal_flags(dstr, SV_COW_DROP_PV); @@ -4927,9 +4929,10 @@ Perl_sv_setsv_cow(pTHX_ SV *dstr, SV *sstr) SvUTF8_on(dstr); SvLEN_set(dstr, len); SvCUR_set(dstr, cur); - if (DEBUG_C_TEST) { - sv_dump(dstr); - } +#ifdef DEBUGGING + if (DEBUG_C_TEST) + sv_dump(dstr); +#endif return dstr; } #endif @@ -5215,12 +5218,14 @@ S_sv_uncow(pTHX_ SV * const sv, const U32 flags) const STRLEN len = SvLEN(sv); const STRLEN cur = SvCUR(sv); +#ifdef DEBUGGING if (DEBUG_C_TEST) { PerlIO_printf(Perl_debug_log, "Copy on write: Force normal %ld\n", (long) flags); sv_dump(sv); } +#endif SvIsCOW_off(sv); # ifdef PERL_COPY_ON_WRITE if (len) { @@ -5260,9 +5265,10 @@ S_sv_uncow(pTHX_ SV * const sv, const U32 flags) } else { unshare_hek(SvSHARED_HEK_FROM_PV(pvx)); } - if (DEBUG_C_TEST) { +#ifdef DEBUGGING + if (DEBUG_C_TEST) sv_dump(sv); - } +#endif } #else const char * const pvx = SvPVX_const(sv); @@ -6805,10 +6811,12 @@ Perl_sv_clear(pTHX_ SV *const orig_sv) && !(IoFLAGS(sv) & IOf_FAKE_DIRP))) { if (SvIsCOW(sv)) { +#ifdef DEBUGGING if (DEBUG_C_TEST) { PerlIO_printf(Perl_debug_log, "Copy on write: clear\n"); sv_dump(sv); } +#endif if (SvLEN(sv)) { if (CowREFCNT(sv)) { sv_buf_to_rw(sv); diff --git a/toke.c b/toke.c index a91a4fcfbe..6ae26801a6 100644 --- a/toke.c +++ b/toke.c @@ -11698,7 +11698,9 @@ S_swallow_bom(pTHX_ U8 *s) /* diag_listed_as: Unsupported script encoding %s */ Perl_croak(aTHX_ "Unsupported script encoding UTF-32LE"); #ifndef PERL_NO_UTF16_FILTER +#ifdef DEBUGGING if (DEBUG_p_TEST || DEBUG_T_TEST) PerlIO_printf(Perl_debug_log, "UTF-16LE script encoding (BOM)\n"); +#endif s += 2; if (PL_bufend > (char*)s) { s = add_utf16_textfilter(s, TRUE); @@ -11712,7 +11714,9 @@ S_swallow_bom(pTHX_ U8 *s) case 0xFE: if (s[1] == 0xFF) { /* UTF-16 big-endian? */ #ifndef PERL_NO_UTF16_FILTER +#ifdef DEBUGGING if (DEBUG_p_TEST || DEBUG_T_TEST) PerlIO_printf(Perl_debug_log, "UTF-16BE script encoding (BOM)\n"); +#endif s += 2; if (PL_bufend > (char *)s) { s = add_utf16_textfilter(s, FALSE); @@ -11726,7 +11730,9 @@ S_swallow_bom(pTHX_ U8 *s) case BOM_UTF8_FIRST_BYTE: { const STRLEN len = sizeof(BOM_UTF8_TAIL) - 1; /* Exclude trailing NUL */ if (slen > len && memEQ(s+1, BOM_UTF8_TAIL, len)) { +#ifdef DEBUGGING if (DEBUG_p_TEST || DEBUG_T_TEST) PerlIO_printf(Perl_debug_log, "UTF-8 script encoding (BOM)\n"); +#endif s += len + 1; /* UTF-8 */ } break; @@ -11745,7 +11751,9 @@ S_swallow_bom(pTHX_ U8 *s) * 00 xx 00 xx * are a good indicator of UTF-16BE. */ #ifndef PERL_NO_UTF16_FILTER +#ifdef DEBUGGING if (DEBUG_p_TEST || DEBUG_T_TEST) PerlIO_printf(Perl_debug_log, "UTF-16BE script encoding (no BOM)\n"); +#endif s = add_utf16_textfilter(s, FALSE); #else /* diag_listed_as: Unsupported script encoding %s */ @@ -11761,7 +11769,9 @@ S_swallow_bom(pTHX_ U8 *s) * xx 00 xx 00 * are a good indicator of UTF-16LE. */ #ifndef PERL_NO_UTF16_FILTER +#ifdef DEBUGGING if (DEBUG_p_TEST || DEBUG_T_TEST) PerlIO_printf(Perl_debug_log, "UTF-16LE script encoding (no BOM)\n"); +#endif s = add_utf16_textfilter(s, TRUE); #else /* diag_listed_as: Unsupported script encoding %s */ -- 2.14.1
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 556b
On Mon, 25 Sep 2017 11:16:33 -0700, atoomic@cpan.org wrote: Show quoted text
> Most of the DEBUG_?_TEST calls are already protected > by one '#idef DEBUGGING', but noticed a few of them > which were not protected in sv.c and toke.c > > We should avoid these extra 'if' statements if perl > is not compiled with debug: -DDEBUGGING.
I don't think these changes are necessary. In non-DEBUGGING builds the DEBUG_?_TEST macros are replaced with (0) so the compiler should be optimizing them away. Are you seeing warnings or something that requires this change? Thanks, Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
Hi Tony, I agree with you this is [probably] not necessary but would mark it as 'good to have'. I would also assume that during compilation optimizations can discard 'if (0)' blocks. But in my understanding, this might depend on your compiler and options you used. Enforcing the pre-processor check guarantee that it always behave the same, and is consistent with the existing code. If you check the codebase, most of other places consuming these DEBUG macros (to do not say all) are already protected by a '#ifdef DEBUGGING' check. I saw no warnings and just noticed it while reviewing a case. The other argument for it, I have for it is when compiling a non-DEBUG perl but with symbols: ./Configure -Dusedevel -Doptimize=-g3 -des I do not want to view non-existing blocks during my gdb sessions. thanks nicolas On Tue, 26 Sep 2017 18:44:01 -0700, tonyc wrote: Show quoted text
> On Mon, 25 Sep 2017 11:16:33 -0700, atoomic@cpan.org wrote:
> > Most of the DEBUG_?_TEST calls are already protected > > by one '#idef DEBUGGING', but noticed a few of them > > which were not protected in sv.c and toke.c > > > > We should avoid these extra 'if' statements if perl > > is not compiled with debug: -DDEBUGGING.
> > I don't think these changes are necessary. > > In non-DEBUGGING builds the DEBUG_?_TEST macros are replaced with (0) > so the compiler should be optimizing them away. > > Are you seeing warnings or something that requires this change? > > Thanks, > Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 979b
On Wed, 27 Sep 2017 09:34:18 -0700, atoomic wrote: Show quoted text
> Hi Tony, I agree with you this is [probably] not necessary but would > mark it as 'good to have'. > > I would also assume that during compilation optimizations can discard > 'if (0)' blocks. But in my understanding, this might depend on your > compiler and options you used. > > Enforcing the pre-processor check guarantee that it always behave the > same, and is consistent with the existing code. If you check the > codebase, most of other places consuming these DEBUG macros (to do not > say all) are already protected by a '#ifdef DEBUGGING' check. > > I saw no warnings and just noticed it while reviewing a case. > > The other argument for it, I have for it is when compiling a non-DEBUG > perl but with symbols: ./Configure -Dusedevel -Doptimize=-g3 -des > > I do not want to view non-existing blocks during my gdb sessions.
You've convinced me, thanks, applied as f0e51ad56d2b90cda983b65e21526fd1bdf422b6. 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