Repository navigation
LGTM-derived analysis of alerts and patches #16765
Description
Activity
From @jkeenan
Last week Dominic Hargreaves provided patches which enable Perl 5 source
code to be analyzed by LGTM.com. Dominic described LGTM as "... a code
analysis tool which offers a powerful framework for software analysis,
with a growing focus on security analysis and customisable queries."Over the past several days I looked at the LGTM-analysis of our codebase
at https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree, which, as
of this moment is reporting "141 alerts: 3 Errors, 104 Warnings, 34
Recommendations." I decided to see if any of these alerts were such
low-hanging fruit that even I could pluck them. In this RT I comment on
4 of those alert categories and provide patches for 3 of them.I.
Recommendation: Empty branch of conditional
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803
branch: smoke-me/jkeenan/empty-conditional-branch
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/empty-conditional-branch
patch: 0001-Eliminate-empty-conditional-branch.patch # sv.cNo smoke-test failures attributable to this patch (attached). Please
review.II.
Warning: local variable hides global variable
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
branch: smoke-me/jkeenan/local-hides-global
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/local-hides-global
patch: 0001-Rename-local-variables-to-prevent-confusion-with-glo.patch #
locale.c universal.cNo smoke-test failures attributable to this patch (attached). Please
review.III.
Warning: Comparison result is always the same
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
branch: smoke-me/jkeenan/compare-result-always-same
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same
patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch
# regexec.c0002-Remove-2-numerical-comparisons-which-are-always-true.patch #
numeric.c # TonyC says no
0003-Remove-1-comparison-whose-result-is-always-the-same.patch
# pp_sys.cNo smoke-test failures attributable to these patches (attached).
However, on #p5p Tony Cook advised against applying the 0002 patch for
numeric.c: "If the numeric.c changes don't result in a failure, maybe
we're missing a test or two. ... [T]hose numeric.c functions don't seem
to be used in core, but they're part of the API. [S]o they could be
tested in XS-APItest." Please review the other two.IV.
Error: Assignment where comparison was intended
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641
branch: jkeenan/assignment-vs-comparison
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparisonThough lgtm characterizes this as an error, "fixing" it causes extensive
test failures in t/re/fold_grind.t. Hence, I did not submit the branch
for smoking and am not attaching any patch.Thank you very much.
Jim KeenanFrom @jkeenan
0001-Rename-local-variables-to-prevent-confusion-with-glo.patch
From ac5465a8c22263a206c773b7396c35710b520416 Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Fri, 23 Nov 2018 12:51:44 -0500 Subject: [PATCH] Rename local variables to prevent confusion with globals. Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 --- locale.c | 6 +++--- universal.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/locale.c b/locale.c index ff6322fdc3..c32dd3f6ed 100644 --- a/locale.c +++ b/locale.c @@ -5050,15 +5050,15 @@ Perl__is_in_locale_category(pTHX_ const bool compiling, const int category) const COP * const cop = (compiling) ? &PL_compiling : PL_curcop; - SV *categories = cop_hints_fetch_pvs(cop, "locale", 0); - if (! categories || categories == &PL_sv_placeholder) { + SV *these_categories = cop_hints_fetch_pvs(cop, "locale", 0); + if (! these_categories || these_categories == &PL_sv_placeholder) { return FALSE; } /* The pseudo-category 'not_characters' is -1, so just add 1 to each to get * a valid unsigned */ assert(category >= -1); - return cBOOL(SvUV(categories) & (1U << (category + 1))); + return cBOOL(SvUV(these_categories) & (1U << (category + 1))); } char * diff --git a/universal.c b/universal.c index 2262939b8d..5a74722b30 100644 --- a/universal.c +++ b/universal.c @@ -655,7 +655,7 @@ XS(XS_PerlIO_get_layers) GV * gv; IO * io; bool input = TRUE; - bool details = FALSE; + bool these_details = FALSE; if (items > 1) { SV * const *svp; @@ -680,7 +680,7 @@ XS(XS_PerlIO_get_layers) goto fail; case 'd': if (memEQs(key, klen, "details")) { - details = SvTRUE(*valp); + these_details = SvTRUE(*valp); break; } goto fail; @@ -718,7 +718,7 @@ XS(XS_PerlIO_get_layers) const bool flgok = flgsvp && *flgsvp && SvIOK(*flgsvp); EXTEND(SP, 3); /* Three is the max in all branches: better check just once */ - if (details) { + if (these_details) { /* Indents of 5? Yuck. */ /* We know that PerlIO_get_layers creates a new SV for the name and flags, so we can just take a reference -- 2.17.1
From @jkeenan
0001-Eliminate-empty-conditional-branch.patch
From 3e8060643f957359e16cb8e39c5b6cbee09e0cf6 Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Fri, 23 Nov 2018 13:45:25 -0500 Subject: [PATCH] Eliminate empty conditional branch Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803 --- sv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sv.c b/sv.c index b10f205033..826228bc92 100644 --- a/sv.c +++ b/sv.c @@ -5164,9 +5164,8 @@ S_sv_uncow(pTHX_ SV * const sv, const U32 flags) SvCUR_set(sv, cur); *SvEND(sv) = '\0'; } - if (len) { - } else { - unshare_hek(SvSHARED_HEK_FROM_PV(pvx)); + if (! len) { + unshare_hek(SvSHARED_HEK_FROM_PV(pvx)); } #ifdef DEBUGGING if (DEBUG_C_TEST) -- 2.17.1
From @jkeenan
0001-Remove-2-numerical-comparisons-which-are-always-true.patch
From e5151f105a0a15ec8086bbc68738c55c24bf052e Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Fri, 23 Nov 2018 10:37:37 -0500 Subject: [PATCH 1/3] Remove 2 numerical comparisons which are always true. Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 --- regexec.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/regexec.c b/regexec.c index be1f5aece3..52d9a64232 100644 --- a/regexec.c +++ b/regexec.c @@ -9753,13 +9753,11 @@ S_reginclass(pTHX_ regexp * const prog, const regnode * const n, const U8* const } else if (flags & ANYOF_LOCALE_FLAGS) { if ((flags & ANYOFL_FOLD) - && c < 256 && ANYOF_BITMAP_TEST(n, PL_fold_locale[c])) { match = TRUE; } else if (ANYOF_POSIXL_TEST_ANY_SET(n) - && c < 256 ) { /* The data structure is arranged so bits 0, 2, 4, ... are set -- 2.17.1
From @jkeenan
0002-Remove-2-numerical-comparisons-which-are-always-true.patch
From daaf264cc2c8a24a2b80a6482c48d6147946f86a Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Fri, 23 Nov 2018 10:51:21 -0500 Subject: [PATCH 2/3] Remove 2 numerical comparisons which are always true. Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 --- numeric.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/numeric.c b/numeric.c index e5e08cb241..f153f9924a 100644 --- a/numeric.c +++ b/numeric.c @@ -44,7 +44,7 @@ Perl_cast_ulong(NV f) return (U32) f; #endif } - return f > 0 ? U32_MAX : 0 /* NaN */; + return U32_MAX /* NaN */; } I32 @@ -62,7 +62,7 @@ Perl_cast_i32(NV f) return (I32)(U32) f; #endif } - return f > 0 ? (I32)U32_MAX : 0 /* NaN */; + return (I32)U32_MAX /* NaN */; } IV -- 2.17.1
From @jkeenan
0003-Remove-1-comparison-whose-result-is-always-the-same.patch
From f98cbe2b272b02a5937b8a1020894cfa088919eb Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Fri, 23 Nov 2018 11:49:03 -0500 Subject: [PATCH 3/3] Remove 1 comparison whose result is always the same. Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 --- pp_sys.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pp_sys.c b/pp_sys.c index ab28ed65d9..5dc20b14f0 100644 --- a/pp_sys.c +++ b/pp_sys.c @@ -3824,8 +3824,7 @@ PP(pp_readlink) len = readlink(tmps, buf, sizeof(buf) - 1); if (len < 0) RETPUSHUNDEF; - if (len != -1) - buf[len] = '\0'; + buf[len] = '\0'; PUSHp(buf, len); RETURN; #else -- 2.17.1
From @jkeenan
Summary of my perl5 (revision 5 version 29 subversion 5) configuration:
Commit id: 3db0bcc
Platform:
osname=linux
osvers=4.15.0-39-generic
archname=x86_64-linux
uname='linux zareason 4.15.0-39-generic #42-ubuntu smp tue oct 23 15:48:01 utc 2018 x86_64 x86_64 x86_64 gnulinux '
config_args='-des -Dusedevel'
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'
optimize='-O2'
cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
ccversion=''
gccversion='7.3.0'
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/gcc/x86_64-linux-gnu/7/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /lib64 /usr/lib64
libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
libc=libc-2.27.so
so=so
useshrplib=false
libperl=libperl.a
gnulibc_version='2.27'
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'Characteristics of this binary (from libperl):
Compile-time options:
HAS_TIMES
PERLIO_LAYERS
PERL_COPY_ON_WRITE
PERL_DONT_CREATE_GVSV
PERL_MALLOC_WRAP
PERL_OP_PARENT
PERL_PRESERVE_IVUV
PERL_USE_DEVEL
USE_64_BIT_ALL
USE_64_BIT_INT
USE_LARGE_FILES
USE_LOCALE
USE_LOCALE_COLLATE
USE_LOCALE_CTYPE
USE_LOCALE_NUMERIC
USE_LOCALE_TIME
USE_PERLIO
USE_PERL_ATOF
Built under linux
Compiled at Nov 17 2018 17:14:01
%ENV:
PERL2DIR="/home/jkeenan/gitwork/perl2"
PERLBREW_HOME="/home/jkeenan/.perlbrew"
PERLBREW_MANPATH="/home/jkeenan/perl5/perlbrew/perls/perl-5.28.0/man"
PERLBREW_PATH="/home/jkeenan/perl5/perlbrew/bin:/home/jkeenan/perl5/perlbrew/perls/perl-5.28.0/bin"
PERLBREW_PERL="perl-5.28.0"
PERLBREW_ROOT="/home/jkeenan/perl5/perlbrew"
PERLBREW_SHELLRC_VERSION="0.84"
PERLBREW_VERSION="0.84"
PERL_WORKDIR="/home/jkeenan/gitwork/perl"
@INC:
lib
/usr/local/lib/perl5/site_perl/5.29.5/x86_64-linux
/usr/local/lib/perl5/site_perl/5.29.5
/usr/local/lib/perl5/5.29.5/x86_64-linux
/usr/local/lib/perl5/5.29.5From @Leont
On Sun, Nov 25, 2018 at 10:26 PM James E Keenan (via RT)
<perlbug-followup@perl.org> wrote:III.
Warning: Comparison result is always the same
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
branch: smoke-me/jkeenan/compare-result-always-same
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same
patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch
# regexec.cI suspect Karl needs to look at that before we apply. If that is a
left-over that really should have been NUM_ANYOF_CODE_POINTS then
indeed it can go.IV.
Error: Assignment where comparison was intended
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641
branch: jkeenan/assignment-vs-comparison
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparisonThough lgtm characterizes this as an error, "fixing" it causes extensive
test failures in t/re/fold_grind.t. Hence, I did not submit the branch
for smoking and am not attaching any patch.Yeah that looks quite intentional to me.
Leon
The RT System itself - Status changed from 'new' to 'open'
From @tonycoz
Last week Dominic Hargreaves provided patches which enable Perl 5 source
code to be analyzed by LGTM.com. Dominic described LGTM as "... a code
analysis tool which offers a powerful framework for software analysis,
with a growing focus on security analysis and customisable queries."Over the past several days I looked at the LGTM-analysis of our codebase
at https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree, which, as
of this moment is reporting "141 alerts: 3 Errors, 104 Warnings, 34
Recommendations." I decided to see if any of these alerts were such
low-hanging fruit that even I could pluck them. In this RT I comment on
4 of those alert categories and provide patches for 3 of them.I.
Recommendation: Empty branch of conditional
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803
branch: smoke-me/jkeenan/empty-conditional-branch
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/empty-conditional-branch
patch: 0001-Eliminate-empty-conditional-branch.patch # sv.cNo smoke-test failures attributable to this patch (attached). Please
review.This patch is reasonable I think.
Warning: local variable hides global variable
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
branch: smoke-me/jkeenan/local-hides-global
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/local-hides-global
patch: 0001-Rename-local-variables-to-prevent-confusion-with-glo.patch #
locale.c universal.cNo smoke-test failures attributable to this patch (attached). Please
review.Renaming the global (which is actually a static) for universal.c makes
more sense to me.Similarly for locale.c, though you might want to pass that by khw.
III.
Warning: Comparison result is always the same
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
branch: smoke-me/jkeenan/compare-result-always-same
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same
patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch
# regexec.cWhile this isn't exactly a false positive, as discussed in #p5p, it
would break using a larger value for NUM_ANYOF_CODE_POINTS, so it is a
false positive for our purposes.Leon suggested that the c < 256 comparison might be incorrect, but no,
it's checking whether the character is foldable with the
PL_fold_locale[] lookup table.So patching out this comparison would be incorrect.
0002-Remove-2-numerical-comparisons-which-are-always-true.patch #
numeric.c # TonyC says noSee the attached patch for tests that demonstrate why this is a false
positive.Also:
https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567
0003\-Remove\-1\-comparison\-whose\-result\-is\-always\-the\-same\.patch# pp_sys.c
This one (in pp_readlink) seems reasonable.
No smoke-test failures attributable to these patches (attached).
However, on #p5p Tony Cook advised against applying the 0002 patch for
numeric.c: "If the numeric.c changes don't result in a failure, maybe
we're missing a test or two. ... [T]hose numeric.c functions don't seem
to be used in core, but they're part of the API. [S]o they could be
tested in XS-APItest." Please review the other two.IV.
Error: Assignment where comparison was intended
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641
branch: jkeenan/assignment-vs-comparison
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparisonThough lgtm characterizes this as an error, "fixing" it causes extensive
test failures in t/re/fold_grind.t. Hence, I did not submit the branch
for smoking and am not attaching any patch.Coverity picked this up too, and we suppressed it (the code is correct.)
Tony
From @tonycoz
0001-perl-133686-add-tests-for-the-Perl_cast_-functions.patch
From 5e07ce045e5fb6e31d7cbedec9e19530d1f0363a Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Mon, 26 Nov 2018 11:15:58 +1100 Subject: (perl #133686) add tests for the Perl_cast_* functions Prompted by a false positive by lgtm, which doesn't appear to know about NaN. --- MANIFEST | 1 + ext/XS-APItest/numeric.xs | 12 ++++++++++ ext/XS-APItest/t/numeric.t | 59 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 ext/XS-APItest/t/numeric.t diff --git a/MANIFEST b/MANIFEST index 3fdf274cf4..6c4ed65497 100644 --- a/MANIFEST +++ b/MANIFEST @@ -4321,6 +4321,7 @@ ext/XS-APItest/t/my_exit.t XS::APItest: test my_exit ext/XS-APItest/t/newCONSTSUB.t XS::APItest: test newCONSTSUB(_flags) ext/XS-APItest/t/newDEFSVOP.t XS::APItest: test newDEFSVOP ext/XS-APItest/t/Null.pm Helper for ./blockhooks.t +ext/XS-APItest/t/numeric.c XS::APItest: test numeric.c utility functions ext/XS-APItest/t/op.t XS::APItest: tests for OP related APIs ext/XS-APItest/t/op_contextualize.t test op_contextualize() API ext/XS-APItest/t/op_list.t test OP list construction API diff --git a/ext/XS-APItest/numeric.xs b/ext/XS-APItest/numeric.xs index 847eb75d7c..22e9034661 100644 --- a/ext/XS-APItest/numeric.xs +++ b/ext/XS-APItest/numeric.xs @@ -59,3 +59,15 @@ grok_atoUV(number, endsv) PUSHs(sv_2mortal(newSViv(0))); } } + +UV +U_32(NV number) + +UV +U_V(NV number) + +IV +I_32(NV number) + +IV +I_V(NV number) diff --git a/ext/XS-APItest/t/numeric.t b/ext/XS-APItest/t/numeric.t new file mode 100644 index 0000000000..71165d6ba9 --- /dev/null +++ b/ext/XS-APItest/t/numeric.t @@ -0,0 +1,59 @@ +#!perl +use strict; +use Test::More; + +use XS::APItest; +use Config; + +unless ($Config{d_double_has_inf} && $Config{d_double_has_nan}) { + plan skip_all => "the doublekind $Config{doublekind} does not have inf/nan"; +} + +my $nan = "NaN" + 0; +my $large = 256 ** ($Config{ivsize}+1); +my $small = -$large; +my $max = int(~0 / 2); +my $min = -$max - 1; +my $umax = ~0; +my $i32_max = 0x7fffffff; +my $i32_min = -$i32_max - 1; +my $u32_max = 0xffffffff; +my $i32_min_as_u32 = 0x80000000; +my $iv_min_as_uv = 0x1 << ($Config{ivsize} * 8 - 1); + +*U_32 = \&XS::APItest::numeric::U_32; +*I_32 = \&XS::APItest::numeric::I_32; +*U_V = \&XS::APItest::numeric::U_V; +*I_V = \&XS::APItest::numeric::I_V; + +is(U_32(0.0), 0, "zero to u32"); +is(U_32($nan), 0, "nan to u32"); +is(U_32($large), $u32_max, "large to u32"); +is(U_32($min), $i32_min_as_u32, "small to u32"); +is(U_32($u32_max), $u32_max, "u32_max to u32"); + +is(I_32(0.0), 0, "zero to i32"); +is(I_32($nan), 0, "nan to i32"); +# this surprised me, but printf("%d"), which does iv = nv at the +# end produces the same value +is(I_32($large), -1, "large ($large) to i32"); +is(I_32($min), $i32_min, "small to i32"); +is(I_32($i32_max), $i32_max, "i32_max to i32"); + +is(U_V(0.0), 0, "zero to uv"); +is(U_V($nan), 0, "nan to uv"); +is(U_V($large), $umax, "large to uv"); +is(U_V($min), $iv_min_as_uv, "small to uv"); +# a large UV might not be exactly representable in a double +#is(U_V($umax), $umax, "u32_max to uv"); + +is(I_V(0.0), 0, "zero to iv"); +is(I_V($nan), 0, "nan to iv"); +# this surprised me, but printf("%d"), which does iv = nv at the +# end produces the same value +is(I_V($large), -1, "large ($large) to iv"); +is(I_V($min), $iv_min, "small to iv"); +# a large IV might not be exactly representable in a double +#is(I_V($iv_max), $iv_max, "iv_max to iv"); + +done_testing(); -- 2.11.0
From @khwilliamson
On 11/25/18 5:54 PM, Tony Cook wrote:
Last week Dominic Hargreaves provided patches which enable Perl 5 source
code to be analyzed by LGTM.com. Dominic described LGTM as "... a code
analysis tool which offers a powerful framework for software analysis,
with a growing focus on security analysis and customisable queries."Over the past several days I looked at the LGTM-analysis of our codebase
at https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree, which, as
of this moment is reporting "141 alerts: 3 Errors, 104 Warnings, 34
Recommendations." I decided to see if any of these alerts were such
low-hanging fruit that even I could pluck them. In this RT I comment on
4 of those alert categories and provide patches for 3 of them.I.
Recommendation: Empty branch of conditional
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803
branch: smoke-me/jkeenan/empty-conditional-branch
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/empty-conditional-branch
patch: 0001-Eliminate-empty-conditional-branch.patch # sv.cNo smoke-test failures attributable to this patch (attached). Please
review.This patch is reasonable I think.
Warning: local variable hides global variable
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
branch: smoke-me/jkeenan/local-hides-global
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/local-hides-global
patch: 0001-Rename-local-variables-to-prevent-confusion-with-glo.patch #
locale.c universal.cNo smoke-test failures attributable to this patch (attached). Please
review.Renaming the global (which is actually a static) for universal.c makes
more sense to me.Similarly for locale.c, though you might want to pass that by khw.
Without even reviewing the code, I think hiding a global is poor
practice, so should be fixed.III.
Warning: Comparison result is always the same
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
branch: smoke-me/jkeenan/compare-result-always-same
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same
patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch
# regexec.cWhile this isn't exactly a false positive, as discussed in #p5p, it
would break using a larger value for NUM_ANYOF_CODE_POINTS, so it is a
false positive for our purposes.Leon suggested that the c < 256 comparison might be incorrect, but no,
it's checking whether the character is foldable with the
PL_fold_locale[] lookup table.And the other is checking that the character fits in the parameter size
passed to a function. They could be #ifdef'd out, as is another
instance of this in regexec.c (probably to silence a compiler warning),
but I avoid #ifdef's when possible, as it makes the code harder to read.
But it could be done for these if people think it's warranted.So patching out this comparison would be incorrect.
0002-Remove-2-numerical-comparisons-which-are-always-true.patch #
numeric.c # TonyC says noSee the attached patch for tests that demonstrate why this is a false
positive.Also:
https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567
0003\-Remove\-1\-comparison\-whose\-result\-is\-always\-the\-same\.patch# pp_sys.c
This one (in pp_readlink) seems reasonable.
No smoke-test failures attributable to these patches (attached).
However, on #p5p Tony Cook advised against applying the 0002 patch for
numeric.c: "If the numeric.c changes don't result in a failure, maybe
we're missing a test or two. ... [T]hose numeric.c functions don't seem
to be used in core, but they're part of the API. [S]o they could be
tested in XS-APItest." Please review the other two.IV.
Error: Assignment where comparison was intended
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641
branch: jkeenan/assignment-vs-comparison
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparisonThough lgtm characterizes this as an error, "fixing" it causes extensive
test failures in t/re/fold_grind.t. Hence, I did not submit the branch
for smoking and am not attaching any patch.Coverity picked this up too, and we suppressed it (the code is correct.)
Tony
From @jkeenan
On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
I.
Recommendation: Empty branch of conditional
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803
branch: smoke-me/jkeenan/empty-conditional-branch
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-
me/jkeenan/empty-conditional-branch
patch: 0001-Eliminate-empty-conditional-branch.patch # sv.cNo smoke-test failures attributable to this patch (attached). Please
review.This patch is reasonable I think.
Applied in commit 41b654e.
[snip]
III.
Warning: Comparison result is always the same
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
branch: smoke-me/jkeenan/compare-result-always-same
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-
me/jkeenan/compare-result-always-same
patches: 0001-Remove-2-numerical-comparisons-which-are-always-
true.patch
# regexec.cWhile this isn't exactly a false positive, as discussed in #p5p, it
would break using a larger value for NUM_ANYOF_CODE_POINTS, so it is a
false positive for our purposes.Leon suggested that the c < 256 comparison might be incorrect, but no,
it's checking whether the character is foldable with the
PL_fold_locale[] lookup table.So patching out this comparison would be incorrect.
0002-Remove-2-numerical-comparisons-which-are-always-true.patch #
numeric.c # TonyC says noSee the attached patch for tests that demonstrate why this is a false
positive.Also:
https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567
0003-Remove-1-comparison-whose-result-is-always-the-same.patch
# pp_sys.cThis one (in pp_readlink) seems reasonable.
Applied in commit 3a56a99.
Everything else still pends.
--
James E Keenan (jkeenan@cpan.org)From @jkeenan
On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
Warning: local variable hides global variable
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
branch: smoke-me/jkeenan/local-hides-global
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-
me/jkeenan/local-hides-global
patch: 0001-Rename-local-variables-to-prevent-confusion-with-
glo.patch #
locale.c universal.cNo smoke-test failures attributable to this patch (attached). Please
review.Renaming the global (which is actually a static) for universal.c makes
more sense to me.Similarly for locale.c, though you might want to pass that by khw.
I agree with you in the case of universal.c. However, in the case of locale.c, there are many instances of the the global and the string 'categories' is frequently used in internal comments. So in this case confining the renaming to a small scope (the "local" instance) seems more prudent.
See this new branch:
smoke-me/jkeenan/133686-local-hides-global-2nd
2 new patches attached:
0001-Rename-global-variable-to-prevent-confusion-with-loc.patch
0002-Rename-local-variable-to-prevent-confusion-with-glob.patchThank you very much.
--
James E Keenan (jkeenan@cpan.org)From @jkeenan
0001-Rename-global-variable-to-prevent-confusion-with-loc.patch
From 419f8ba5087ee6baa2281aa8c355fc66a0fe3851 Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Mon, 26 Nov 2018 12:14:51 -0500 Subject: [PATCH 1/2] Rename global variable to prevent confusion with local Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 --- universal.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/universal.c b/universal.c index 2262939b8d..c1b5dd4b14 100644 --- a/universal.c +++ b/universal.c @@ -995,7 +995,7 @@ struct xsub_details { const char *proto; }; -static const struct xsub_details details[] = { +static const struct xsub_details these_details[] = { {"UNIVERSAL::isa", XS_UNIVERSAL_isa, NULL}, {"UNIVERSAL::can", XS_UNIVERSAL_can, NULL}, {"UNIVERSAL::DOES", XS_UNIVERSAL_DOES, NULL}, @@ -1075,8 +1075,8 @@ void Perl_boot_core_UNIVERSAL(pTHX) { static const char file[] = __FILE__; - const struct xsub_details *xsub = details; - const struct xsub_details *end = C_ARRAY_END(details); + const struct xsub_details *xsub = these_details; + const struct xsub_details *end = C_ARRAY_END(these_details); do { newXS_flags(xsub->name, xsub->xsub, file, xsub->proto, 0); -- 2.17.1
From @jkeenan
0002-Rename-local-variable-to-prevent-confusion-with-glob.patch
From 363d7d4a2b1a2867eccb5132540f9055aca201ef Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Mon, 26 Nov 2018 12:29:03 -0500 Subject: [PATCH 2/2] Rename local variable to prevent confusion with global Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 For: RT # 133686 (partial) --- locale.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/locale.c b/locale.c index ff6322fdc3..c32dd3f6ed 100644 --- a/locale.c +++ b/locale.c @@ -5050,15 +5050,15 @@ Perl__is_in_locale_category(pTHX_ const bool compiling, const int category) const COP * const cop = (compiling) ? &PL_compiling : PL_curcop; - SV *categories = cop_hints_fetch_pvs(cop, "locale", 0); - if (! categories || categories == &PL_sv_placeholder) { + SV *these_categories = cop_hints_fetch_pvs(cop, "locale", 0); + if (! these_categories || these_categories == &PL_sv_placeholder) { return FALSE; } /* The pseudo-category 'not_characters' is -1, so just add 1 to each to get * a valid unsigned */ assert(category >= -1); - return cBOOL(SvUV(categories) & (1U << (category + 1))); + return cBOOL(SvUV(these_categories) & (1U << (category + 1))); } char * -- 2.17.1
From @khwilliamson
On 11/26/18 10:39 AM, James E Keenan via RT wrote:
On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
Warning: local variable hides global variable
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
branch: smoke-me/jkeenan/local-hides-global
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-
me/jkeenan/local-hides-global
patch: 0001-Rename-local-variables-to-prevent-confusion-with-
glo.patch #
locale.c universal.cNo smoke-test failures attributable to this patch (attached). Please
review.Renaming the global (which is actually a static) for universal.c makes
more sense to me.Similarly for locale.c, though you might want to pass that by khw.
I agree with you in the case of universal.c. However, in the case of locale.c, there are many instances of the the global and the string 'categories' is frequently used in internal comments. So in this case confining the renaming to a small scope (the "local" instance) seems more prudent.
See this new branch:
smoke-me/jkeenan/133686-local-hides-global-2nd
2 new patches attached:
0001-Rename-global-variable-to-prevent-confusion-with-loc.patch
0002-Rename-local-variable-to-prevent-confusion-with-glob.patchThank you very much.
Fine by me
From @jkeenan
On Mon, 26 Nov 2018 17:48:21 GMT, public@khwilliamson.com wrote:
On 11/26/18 10:39 AM, James E Keenan via RT wrote:
On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
Warning: local variable hides global variable
lgtm url:
https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
branch: smoke-me/jkeenan/local-hides-global
branch url:
https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-
me/jkeenan/local-hides-global
patch: 0001-Rename-local-variables-to-prevent-confusion-with-
glo.patch #
locale.c universal.cNo smoke-test failures attributable to this patch (attached).
Please
review.Renaming the global (which is actually a static) for universal.c
makes
more sense to me.Similarly for locale.c, though you might want to pass that by khw.
I agree with you in the case of universal.c. However, in the case of
locale.c, there are many instances of the the global and the string
'categories' is frequently used in internal comments. So in this
case confining the renaming to a small scope (the "local" instance)
seems more prudent.See this new branch:
smoke-me/jkeenan/133686-local-hides-global-2nd
2 new patches attached:
0001-Rename-global-variable-to-prevent-confusion-with-loc.patch
0002-Rename-local-variable-to-prevent-confusion-with-glob.patchThank you very much.
Fine by me
Those two patches have been merged into blead.
--
James E Keenan (jkeenan@cpan.org)From @jkeenan
I'm going to resolve this ticket because it appears that all the patches that can usefully be applied to blead have been applied.
Also, going forward, for clarity in discussion we should probably open one RT for each combination of LGTM-warning and source-code file.
Thank you very much.
--
James E Keenan (jkeenan@cpan.org)From @khwilliamson
Thank you for filing this report. You have helped make Perl better.
With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.Perl 5.30.0 may be downloaded via:
https://metacpan.org/release/XSAWYERX/perl-5.30.0If you find that the problem persists, feel free to reopen this ticket.
@khwilliamson - Status changed from 'pending release' to 'resolved'
Migrated from rt.perl.org#133686 (status was 'resolved')
Searchable as RT133686$