Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LGTM-derived analysis of alerts and patches #16765

Closed
p5pRT opened this issue Nov 25, 2018 · 22 comments
Closed

LGTM-derived analysis of alerts and patches #16765

p5pRT opened this issue Nov 25, 2018 · 22 comments

Comments

@p5pRT
Copy link

p5pRT commented Nov 25, 2018

Migrated from rt.perl.org#133686 (status was 'resolved')

Searchable as RT133686$

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

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.c

No 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.c

No 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.c

0002-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.c

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-comparison

Though 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 Keenan

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

From @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

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

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.5

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

From @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.c

I 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-comparison

Though 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

@p5pRT
Copy link
Author

p5pRT commented Nov 25, 2018

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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.c

No 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.c

No 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.c

While 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 no

See 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-comparison

Though 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

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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.c

No 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.c

No 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.c

While 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 no

See 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-comparison

Though 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

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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.c

No 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.c

While 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 no

See 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.

Applied in commit 3a56a99.

Everything else still pends.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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.c

No 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.patch

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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.c

No 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.patch

Thank you very much.

Fine by me

@p5pRT
Copy link
Author

p5pRT commented Nov 26, 2018

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.c

No 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.patch

Thank you very much.

Fine by me

Those two patches have been merged into blead.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2018

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)

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2018

@jkeenan - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

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.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant