[PATCH] Clean up integer conversions

[p5pRT]

Migrated from rt.perl.org#129998 (status was 'rejected')

Searchable as RT129998$

[p5pRT]

From @petdance

Created by @petdance

As part of my work at making everything run clean under clang's
-Weverything, I've found some integers that can use some explicit casting,
and changed static function S_lop to take a U8 instead of an int to
avoid type conversions inside.

Perl Info

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

[p5pRT]

From @petdance

integer-types.diff

diff --git a/embed.fnc b/embed.fnc
index 5cc73b7..c659eff 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -2606,7 +2606,7 @@ s	|void	|force_ident_maybe_lex|char pit
 s	|void	|incline	|NN const char *s
 s	|int	|intuit_method	|NN char *s|NULLOK SV *ioname|NULLOK CV *cv
 s	|int	|intuit_more	|NN char *s
-s	|I32	|lop		|I32 f|int x|NN char *s
+s	|I32	|lop		|I32 f|U8 x|NN char *s
 rs	|void	|missingterm	|NULLOK char *s
 s	|void	|no_op		|NN const char *const what|NULLOK char *s
 s	|int	|pending_ident
diff --git a/proto.h b/proto.h
index 1d79c46..0243910 100644
--- a/proto.h
+++ b/proto.h
@@ -5488,7 +5488,7 @@ STATIC int	S_intuit_method(pTHX_ char *s, SV *ioname, CV *cv);
 STATIC int	S_intuit_more(pTHX_ char *s);
 #define PERL_ARGS_ASSERT_INTUIT_MORE	\
 	assert(s)
-STATIC I32	S_lop(pTHX_ I32 f, int x, char *s);
+STATIC I32	S_lop(pTHX_ I32 f, U8 x, char *s);
 #define PERL_ARGS_ASSERT_LOP	\
 	assert(s)
 PERL_STATIC_NO_RET void	S_missingterm(pTHX_ char *s)
diff --git a/toke.c b/toke.c
index ffac930..2495bc2 100644
--- a/toke.c
+++ b/toke.c
@@ -1890,7 +1890,7 @@ S_check_uni(pTHX)
  */
 
 STATIC I32
-S_lop(pTHX_ I32 f, int x, char *s)
+S_lop(pTHX_ I32 f, U8 x, char *s)
 {
     PERL_ARGS_ASSERT_LOP;
 
diff --git a/op.c b/op.c
index cf1399e..f25a116 100644
--- a/op.c
+++ b/op.c
@@ -10103,7 +10103,7 @@ Perl_ck_fun(pTHX_ OP *o)
 			    {
 				GV * const gv = cGVOPx_gv(kUNOP->op_first);
 				name = GvNAME(gv);
-				len = GvNAMELEN(gv);
+				len = (STRLEN)GvNAMELEN(gv);
                                 name_utf8 = GvNAMEUTF8(gv) ? SVf_UTF8 : 0;
 			    }
 			    else if (kid->op_type == OP_AELEM
diff --git a/pad.c b/pad.c
index e810ccd..beff5a4 100644
--- a/pad.c
+++ b/pad.c
@@ -2746,7 +2746,7 @@ Perl_newPADNAMEpvn(const char *s, STRLEN len)
     PadnamePV(pn) = alloc->xpadn_str;
     Copy(s, PadnamePV(pn), len, char);
     *(PadnamePV(pn) + len) = '\0';
-    PadnameLEN(pn) = len;
+    PadnameLEN(pn) = (U8)len;
     return pn;
 }
 
diff --git a/pp_sort.c b/pp_sort.c
index 4f0553b..8736bc6 100644
--- a/pp_sort.c
+++ b/pp_sort.c
@@ -1802,7 +1802,7 @@ S_sortcv(pTHX_ SV *const a, SV *const b)
     /* entry zero of a stack is always PL_sv_undef, which
      * simplifies converting a '()' return into undef in scalar context */
     assert(PL_stack_sp > PL_stack_base || *PL_stack_base == &PL_sv_undef);
-    result = SvIV(*PL_stack_sp);
+    result = (I32)SvIV(*PL_stack_sp);
 
     LEAVE_SCOPE(oldsaveix);
     PL_curpm = pm;
@@ -1849,7 +1849,7 @@ S_sortcv_stacked(pTHX_ SV *const a, SV *const b)
     /* entry zero of a stack is always PL_sv_undef, which
      * simplifies converting a '()' return into undef in scalar context */
     assert(PL_stack_sp > PL_stack_base || *PL_stack_base == &PL_sv_undef);
-    result = SvIV(*PL_stack_sp);
+    result = (I32)SvIV(*PL_stack_sp);
 
     LEAVE_SCOPE(oldsaveix);
     PL_curpm = pm;
@@ -1877,7 +1877,7 @@ S_sortcv_xsub(pTHX_ SV *const a, SV *const b)
     /* entry zero of a stack is always PL_sv_undef, which
      * simplifies converting a '()' return into undef in scalar context */
     assert(PL_stack_sp > PL_stack_base || *PL_stack_base == &PL_sv_undef);
-    result = SvIV(*PL_stack_sp);
+    result = (I32)SvIV(*PL_stack_sp);
 
     LEAVE_SCOPE(oldsaveix);
     PL_curpm = pm;
@@ -1931,7 +1931,7 @@ S_amagic_ncmp(pTHX_ SV *const a, SV *const b)
 
     if (tmpsv) {
         if (SvIOK(tmpsv)) {
-            const I32 i = SvIVX(tmpsv);
+            const I32 i = (I32)SvIVX(tmpsv);
             return SORT_NORMAL_RETURN_VALUE(i);
         }
 	else {
@@ -1951,7 +1951,7 @@ S_amagic_i_ncmp(pTHX_ SV *const a, SV *const b)
 
     if (tmpsv) {
         if (SvIOK(tmpsv)) {
-            const I32 i = SvIVX(tmpsv);
+            const I32 i = (I32)SvIVX(tmpsv);
             return SORT_NORMAL_RETURN_VALUE(i);
         }
 	else {
@@ -1971,7 +1971,7 @@ S_amagic_cmp(pTHX_ SV *const str1, SV *const str2)
 
     if (tmpsv) {
         if (SvIOK(tmpsv)) {
-            const I32 i = SvIVX(tmpsv);
+            const I32 i = (I32)SvIVX(tmpsv);
             return SORT_NORMAL_RETURN_VALUE(i);
         }
 	else {
@@ -1993,7 +1993,7 @@ S_amagic_cmp_locale(pTHX_ SV *const str1, SV *const str2)
 
     if (tmpsv) {
         if (SvIOK(tmpsv)) {
-            const I32 i = SvIVX(tmpsv);
+            const I32 i = (I32)SvIVX(tmpsv);
             return SORT_NORMAL_RETURN_VALUE(i);
         }
 	else {

[p5pRT]

From @jkeenan

On Tue Nov 01 21​:30​:44 2016, petdance wrote​:

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]

As part of my work at making everything run clean under clang's
-Weverything, I've found some integers that can use some explicit
casting,
and changed static function S_lop to take a U8 instead of an int to
avoid type conversions inside.

What command-line options would we provide to ./Configure to test out this patch in a before/after scenario?

If you can provide that, I can test it out in my FreeBSD VMs, where clang is default C compiler.

Thank you very much.

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

[p5pRT]

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

[p5pRT]

From @petdance

There's nothing to test specifically. This patch is just adding some casts in response to some warnings that clang came up with under -Weverything.

[p5pRT]

From @khwilliamson

On 11/01/2016 10​:30 PM, Andy Lester (via RT) wrote​:

# New Ticket Created by Andy Lester
# Please include the string​: [perl #129998]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129998 >

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]

As part of my work at making everything run clean under clang's
-Weverything, I've found some integers that can use some explicit casting,
and changed static function S_lop to take a U8 instead of an int to
avoid type conversions inside.

Having merely briefly scanned the patch, so I don't know for certain,
but those I32 casts look suspicious. In general use of 'I32' is
considered harmful, and are supposed to be converted to IV as we go
along. Please double check

[p5pRT]

From @petdance

Having merely briefly scanned the patch, so I don't know for certain,
but those I32 casts look suspicious. In general use of 'I32' is
considered harmful, and are supposed to be converted to IV as we go
along. Please double check

I'm not sure what I would double check.

If they should be IV and not I32, then I'll look into doing it that way instead. I was just matching the target variable.

[p5pRT]

From @khwilliamson

On 11/02/2016 08​:57 AM, Andy Lester via RT wrote​:

Having merely briefly scanned the patch, so I don't know for certain,
but those I32 casts look suspicious. In general use of 'I32' is
considered harmful, and are supposed to be converted to IV as we go
along. Please double check

I'm not sure what I would double check.

If they should be IV and not I32, then I'll look into doing it that way instead. I was just matching the target variable.

Someone who actually knows about this should weigh in. If the target
variable is I32, then they should be I32. But maybe instead, the target
variable should be changed to IV while we're at it.

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129998

[p5pRT]

From @hvds

On Wed Nov 02 07​:57​:27 2016, petdance wrote​:

If they should be IV and not I32, then I'll look into doing it that
way instead. I was just matching the target variable.

That final comment makes me wonder what value is being added here.

Warnings are supposed to be useful by encouraging you to stop and think whether the code is really doing the right thing. If you're just mechanically adding casts to make the warning go away, I think we might well be better off without - I would normally take the existence of the cast as evidence that someone _has_ thought about it.

Hugo

[p5pRT]

From @cpansprout

On Wed Nov 02 08​:08​:19 2016, public@​khwilliamson.com wrote​:

On 11/02/2016 08​:57 AM, Andy Lester via RT wrote​:

Having merely briefly scanned the patch, so I don't know for
certain,
but those I32 casts look suspicious. In general use of 'I32' is
considered harmful, and are supposed to be converted to IV as we go
along. Please double check

I'm not sure what I would double check.

If they should be IV and not I32, then I'll look into doing it that
way instead. I was just matching the target variable.

Someone who actually knows about this should weigh in. If the target
variable is I32, then they should be I32. But maybe instead, the
target
variable should be changed to IV while we're at it.

(Caveat​: I have not looked at the code or the patch.)

If it measure the length of something stored in memory (string, array, etc.), then it should be SSize_t. Otherwise, it if measure something small that we *know* will not exceed 32 bits, then I32 is fine. Otherwise, it may need to hold a large number, so it should be IV (or UV if no negative numbers will be encountered [but changing signedness is an easy way to introduce bugs, so I32->IV is a safer change than I32->UV]).

--

Father Chrysostomos

[p5pRT]

From @cpansprout

On Wed Nov 02 15​:48​:40 2016, sprout wrote​:

If it measure

My s must not be working (or mut not be working).

the length of something stored in memory (string, array,
etc.), then it should be SSize_t. Otherwise, it if measure

Again.

(Or I could be using subjunctive. :-)

--

Father Chrysostomos

[p5pRT]

From @petdance

I'm going to reject this patch, and resubmit just the one change change that should be a nonissue. And then I will open the discussion of handling of ints that -Weverything complains about on p5p.

[p5pRT]

@petdance - Status changed from 'open' to 'rejected'

[p5pRT]

From @iabyn

On Tue, Nov 01, 2016 at 09​:30​:44PM -0700, Andy Lester wrote​:

-s |I32 |lop |I32 f|int x|NN char *s
+s |I32 |lop |I32 f|U8 x|NN char *s

This one looks good.

- len = GvNAMELEN(gv);
+ len = (STRLEN)GvNAMELEN(gv);

This is casting from I32 to STRLEN (aka Size_t), so a sign conversion.
Arguably it should have an assert(GvNAMELEN(gv) >= 0) just before it.
I presume the GvNAMELEN has to be signed since its really a HEK, and HEKs
can have a negative length to indicate things (IIRC).

- PadnameLEN(pn) = len;
+ PadnameLEN(pn) = (U8)len;

pad variable name lengths are U8, because the lexer croaks if identifier
names are too long.

I Think newPADNAMEpvn(const char *s, STRLEN len), which is an API
function, should check for len > (whatever the actual max is), and croak
with "Identifier too long". Then do the cast.

For info​:

  $ perl -le'print q{my $}, "a" x 252' | perl
  Identifier too long at - line 1.

--- a/pp_sort.c
+++ b/pp_sort.c
@​@​ -1802,7 +1802,7 @​@​ S_sortcv(pTHX_ SV *const a, SV *const b)
/* entry zero of a stack is always PL_sv_undef, which
* simplifies converting a '()' return into undef in scalar context */
assert(PL_stack_sp > PL_stack_base || *PL_stack_base == &PL_sv_undef);
- result = SvIV(*PL_stack_sp);
+ result = (I32)SvIV(*PL_stack_sp);

[and lots of of similar I32 casts related to returning an I32 (-1,0,-1)
comparison value].

I wonder whether instead we should change

  typedef I32 (*SVCOMPARE_t) (pTHX_ SV* const, SV* const);

to return an IV instead of an I32 and fix up all the static sort subs to
use IV instead.

--
Wesley Crusher gets beaten up by his classmates for being a smarmy git,
and consequently has a go at making some friends of his own age for a
change.
  -- Things That Never Happen in "Star Trek" #18