[PATCH] Make arguments to S_invlist_is_iterating and S_invlist_max be const

[p5pRT]

Migrated from rt.perl.org#130591 (status was 'open')

Searchable as RT130591$

[p5pRT]

From @petdance

Created by @petdance

Make arguments to S_invlist_is_iterating and S_invlist_max be const.

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

0001-Make-arguments-to-S_invlist_is_iterating-and-S_invli.patch

From a1c41da9a78a9cff927ec89161d6e81658d6e74d Mon Sep 17 00:00:00 2001
From: Andy Lester <andy@petdance.com>
Date: Thu, 19 Jan 2017 22:52:15 -0600
Subject: [PATCH] Make arguments to S_invlist_is_iterating and S_invlist_max be
 const

---
 embed.fnc | 4 ++--
 proto.h   | 4 ++--
 regcomp.c | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/embed.fnc b/embed.fnc
index 656afe5..af5e7fb 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -1635,10 +1635,10 @@ EXpM	|UV	|swash_fetch	|NN SV *swash|NN const U8 *ptr|bool do_utf8
 #ifdef PERL_IN_REGCOMP_C
 EiMR	|SV*	|add_cp_to_invlist	|NULLOK SV* invlist|const UV cp
 EiM	|void	|invlist_set_len|NN SV* const invlist|const UV len|const bool offset
-EiMRn	|bool	|invlist_is_iterating|NN SV* const invlist
+EiMRn	|bool	|invlist_is_iterating|NN const SV* const invlist
 #ifndef PERL_EXT_RE_BUILD
 EiMRn	|UV*	|_invlist_array_init	|NN SV* const invlist|const bool will_have_0
-EiMRn	|UV	|invlist_max	|NN SV* const invlist
+EiMRn	|UV	|invlist_max	|NN const SV* const invlist
 EsM	|void	|_append_range_to_invlist   |NN SV* const invlist|const UV start|const UV end
 EsM	|void	|invlist_extend    |NN SV* const invlist|const UV len
 EsM	|void	|invlist_replace_list_destroys_src|NN SV *dest|NN SV *src
diff --git a/proto.h b/proto.h
index 2fd8a51..62526cb 100644
--- a/proto.h
+++ b/proto.h
@@ -3840,7 +3840,7 @@ PERL_STATIC_INLINE void	S_invlist_clear(pTHX_ SV* invlist);
 STATIC void	S_invlist_extend(pTHX_ SV* const invlist, const UV len);
 #define PERL_ARGS_ASSERT_INVLIST_EXTEND	\
 	assert(invlist)
-PERL_STATIC_INLINE UV	S_invlist_max(SV* const invlist)
+PERL_STATIC_INLINE UV	S_invlist_max(const SV* const invlist)
 			__attribute__warn_unused_result__;
 #define PERL_ARGS_ASSERT_INVLIST_MAX	\
 	assert(invlist)
@@ -4966,7 +4966,7 @@ PERL_STATIC_INLINE UV	S_invlist_highest(SV* const invlist)
 #define PERL_ARGS_ASSERT_INVLIST_HIGHEST	\
 	assert(invlist)
 
-PERL_STATIC_INLINE bool	S_invlist_is_iterating(SV* const invlist)
+PERL_STATIC_INLINE bool	S_invlist_is_iterating(const SV* const invlist)
 			__attribute__warn_unused_result__;
 #define PERL_ARGS_ASSERT_INVLIST_IS_ITERATING	\
 	assert(invlist)
diff --git a/regcomp.c b/regcomp.c
index 97888ca..a5feadc 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -8579,17 +8579,17 @@ S_invlist_clear(pTHX_ SV* invlist)    /* Empty the inversion list */
 #endif /* ifndef PERL_IN_XSUB_RE */
 
 PERL_STATIC_INLINE bool
-S_invlist_is_iterating(SV* const invlist)
+S_invlist_is_iterating(const SV* const invlist)
 {
     PERL_ARGS_ASSERT_INVLIST_IS_ITERATING;
 
-    return *(get_invlist_iter_addr(invlist)) < (STRLEN) UV_MAX;
+    return ((XINVLIST*) SvANY(invlist))->iterator < (STRLEN)UV_MAX;
 }
 
 #ifndef PERL_IN_XSUB_RE
 
 PERL_STATIC_INLINE UV
-S_invlist_max(SV* const invlist)
+S_invlist_max(const SV* const invlist)
 {
     /* Returns the maximum number of elements storable in the inversion list's
      * array, without having to realloc() */
-- 
1.8.3.1

[p5pRT]

From @hvds

Andy Lester (via RT) <perlbug-followup@​perl.org> wrote​:
:Make arguments to S_invlist_is_iterating and S_invlist_max be const.
[...]
: PERL_STATIC_INLINE bool
:-S_invlist_is_iterating(SV* const invlist)
:+S_invlist_is_iterating(const SV* const invlist)
: {
: PERL_ARGS_ASSERT_INVLIST_IS_ITERATING;
:
:- return *(get_invlist_iter_addr(invlist)) < (STRLEN) UV_MAX;
:+ return ((XINVLIST*) SvANY(invlist))->iterator < (STRLEN)UV_MAX;
: }

Why the additional change here, was it somehow required for the consting?
It looks like care had been taken to abstract all references to iterator
behind get_invlist_iter_addr(), this would violate that abstraction.

Hugo

[p5pRT]

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

[p5pRT]

From @petdance

On Jan 20, 2017, at 3​:29 AM, hv@​crypt.org wrote​:

Why the additional change here, was it somehow required for the consting?

Yes.

--
Andy Lester => www.petdance.com

[p5pRT]

From @hvds

On Fri, 20 Jan 2017 07​:02​:54 -0800, petdance wrote​:

On Jan 20, 2017, at 3​:29 AM, hv@​crypt.org wrote​:
Why the additional change here, was it somehow required for the consting?

Yes.

Thanks, but I had hoped for a more engaged response.

I had also written​:

It looks like care had been taken to abstract all references to iterator
behind get_invlist_iter_addr(), this would violate that abstraction.

I understand and value the benefits of abstraction and encapsulation in a complex codebase, and find them all too rarely in perl's. I'm much vaguer on the benefits of consting - they seem pretty minor, and regularly outweighed by bugs introduced - so my default preference would be to retain the former and reject this patch.

However it might also be possible to provide the benefits of both, eg by splitting the r/w accessor into separate rvalue and lvalue variants.

Karl, I think you're the one most involved with this code, do you have any opinion?

Hugo

[p5pRT]

From @petdance

Why the additional change here, was it somehow required for the consting?

Yes.

Thanks, but I had hoped for a more engaged response.

I'm sorry. I'd answered your question and didn't have anything else to add to your obvservation about abstractions.

I understand and value the benefits of abstraction and encapsulation in a complex codebase, and find them all too rarely in perl's. I'm much vaguer on the benefits of consting - they seem pretty minor, and regularly outweighed by bugs introduced - so my default preference would be to retain the former and reject this patch.

I'm curious what bugs have been introduced by consting. I haven't seen any in the decade since I started working on using const in the Perl codebase. I'm curious what's gone wrong with it.

I work hard to get the compiler and other tools do as much keeping an eye on us fallible humans as possible. The big point of consting function pointer parameters is to let the compiler know what goes on inside a function. Consider this code​:

  char p[100];
  int n = strlen(p);

You know and I know that that's bad, because p has not been initialized. The compiler will throw a warning that p is being used without being initialized. The compiler only knows that because strlen is defined strlen(const char *p). If not for that const on the char *p, the compiler has to assume that it's possible that strlen() is modifying the contents of p. So we need to have as many of our functions declared in the same way so the compiler can do its job better.

So if S_invlist_max is not declared with a const pointer argument, the compiler may think that S_invlist_max is actually writing to the pointer contents.

The other point of consting, in consting variables, is that it gives the reader a clue as to the intent of a variable.

  const int len = this + that + other;
  int len = this + that + other;

The first tells the reader that you're calculating this once, at the top of the block, and you're not going to modify it later on. In the second, the reader is now notified that this len is going to change later on in the block. And, if you do modify the len and get a compiler warning, the person making the change now needs to wonder "Should I really be modifying this value if that's not it's intent?"

However it might also be possible to provide the benefits of both, eg by splitting the r/w accessor into separate rvalue and lvalue variants.

I think that's an excellent idea. Two thumbs up from me. Thanks for pursuing it.

--
Andy Lester => www.petdance.com

[p5pRT]

From @craigberry

On Sat, Jan 21, 2017 at 10​:15 PM, Andy Lester <andy@​petdance.com> wrote​:

I'm curious what bugs have been introduced by consting. I haven't seen any in the decade since I started working on using const in the Perl codebase. I'm curious what's gone wrong with it.

I've seen estimates of something like 8-10 bugs introduced into the
Perl core by previous rounds of consting patches, sometimes by the
refactoring that came with them rather than the consting per se. The
only one I found a specific reference to was​:

<https://rt-archive.perl.org/perl5/Ticket/Display.html?id=72426>

which was a segfault cause by​:

<https://perl5.git.perl.org/perl.git/commitdiff/890ce7af62ab97fd07b5b49562f13e94286469fb>

So there was an easy segfault (C<$ perl -etell>) in the wild for 5
years before it was caught and fixed. There are some benefits to
consting, but it's not always worth the risk.

[p5pRT]

From @petdance

I've seen estimates of something like 8-10 bugs introduced into the
Perl core by previous rounds of consting patches, sometimes by the
refactoring that came with them rather than the consting per se.

You say "sometimes by the refactoring" but I suspect that they've all been because of refactoring, and not because of the consting.

The only one I found a specific reference to was​:

<https://rt-archive.perl.org/perl5/Ticket/Display.html?id=72426>

which was a segfault cause by​:

<https://perl5.git.perl.org/perl.git/commitdiff/890ce7af62ab97fd07b5b49562f13e94286469fb>

Interesting, thanks for digging it up. That patch from 2005 is way too big, and I should have been broken it up into smaller more discrete patches.

I haven't dug into it, but my guess is that it's the NN/NULLOK getting added to the parameter pointers that probably caused the problems. Jarkko pointed out a while back the problems with the notnull attributes and how they change the code generated, and we've since eliminated those from the codebase.

--
Andy Lester => www.petdance.com

[p5pRT]

From @cpansprout

On Sun, 22 Jan 2017 10​:48​:27 -0800, petdance wrote​:

I've seen estimates of something like 8-10 bugs introduced into the
Perl core by previous rounds of consting patches, sometimes by the
refactoring that came with them rather than the consting per se.

You say "sometimes by the refactoring" but I suspect that they've all
been because of refactoring, and not because of the consting.

92adfbd is an example of refactoring that was solely for the sake of consting (according to <20060308064529.GA16499@​petdance.com>). It caused bug #48332.

--

Father Chrysostomos

[p5pRT]

From @khwilliamson

On Sat, 21 Jan 2017 01​:11​:24 -0800, hv wrote​:

On Fri, 20 Jan 2017 07​:02​:54 -0800, petdance wrote​:

On Jan 20, 2017, at 3​:29 AM, hv@​crypt.org wrote​:
Why the additional change here, was it somehow required for the
consting?

Yes.

Thanks, but I had hoped for a more engaged response.

I had also written​:

It looks like care had been taken to abstract all references to
iterator
behind get_invlist_iter_addr(), this would violate that abstraction.

I understand and value the benefits of abstraction and encapsulation
in a complex codebase, and find them all too rarely in perl's. I'm
much vaguer on the benefits of consting - they seem pretty minor, and
regularly outweighed by bugs introduced - so my default preference
would be to retain the former and reject this patch.

However it might also be possible to provide the benefits of both, eg
by splitting the r/w accessor into separate rvalue and lvalue
variants.

Karl, I think you're the one most involved with this code, do you have
any opinion?

I strongly prefer keeping the encapsulation.

As many others have said (I'm guessing), I wish this had been written in C++, and then people wouldn't even be tempted to break it.

The underlying implementation of inversion lists was changed once already, and the effects were limited because of the encapsulation. We may change it again, so that it isn't an SV type, and this would break at that point.

My position on consting is that I try to use it, but if it leads to any compile issues, I immediately abandon it.

Hugo

--
Karl Williamson

[p5pRT]

From @demerphq

On 26 January 2017 at 06​:03, Karl Williamson via RT
<perlbug-followup@​perl.org> wrote​:

My position on consting is that I try to use it, but if it leads to any compile issues, I immediately abandon it.

Me too. All too often it seems to be that adding a const attribute to
something triggers a cascade of secondary changes. Somehow I feel like
C does it wrong, although i have no idea what doing it right looks
like.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

[p5pRT]

From @khwilliamson

On 01/27/2017 10​:39 AM, demerphq wrote​:

On 26 January 2017 at 06​:03, Karl Williamson via RT
<perlbug-followup@​perl.org> wrote​:

My position on consting is that I try to use it, but if it leads to any compile issues, I immediately abandon it.

Me too. All too often it seems to be that adding a const attribute to
something triggers a cascade of secondary changes. Somehow I feel like
C does it wrong, although i have no idea what doing it right looks
like.

Yves

I've thought if things had been designed properly to begin with that
const would fall out without a problem. But I haven't verified that.
I've also thought that if one started down at the lowest level, one
could add a bunch of consts that could be propagated up. I don't know
if the example in this ticket is a counter example or not.

Also, note that const was not in the original C language. I programmed
for years in C without there being a const, and I'm not sure I still
fully do. When it became available, I believe it was at the same time
as enum and better function declarations, which overshadowed it as
something we desired.

[p5pRT]

From @petdance

On Jan 27, 2017, at 12​:13 PM, Karl Williamson <public@​khwilliamson.com> wrote​:

I've also thought that if one started down at the lowest level, one could add a bunch of consts that could be propagated up.

I’ve gone down that road before, at least twice, over the years. There’s a ton we could do, but as you suggest, it would have to be at the lowest level and worked upward. I always wound up abandoning my attempt branches.

--
Andy Lester => www.petdance.com

[p5pRT]

From @tonycoz

On Wed, 25 Jan 2017 21​:03​:33 -0800, khw wrote​:

On Sat, 21 Jan 2017 01​:11​:24 -0800, hv wrote​:

On Fri, 20 Jan 2017 07​:02​:54 -0800, petdance wrote​:

On Jan 20, 2017, at 3​:29 AM, hv@​crypt.org wrote​:
Why the additional change here, was it somehow required for the
consting?

Yes.

Thanks, but I had hoped for a more engaged response.

I had also written​:

It looks like care had been taken to abstract all references to
iterator
behind get_invlist_iter_addr(), this would violate that
abstraction.

I understand and value the benefits of abstraction and encapsulation
in a complex codebase, and find them all too rarely in perl's. I'm
much vaguer on the benefits of consting - they seem pretty minor, and
regularly outweighed by bugs introduced - so my default preference
would be to retain the former and reject this patch.

However it might also be possible to provide the benefits of both, eg
by splitting the r/w accessor into separate rvalue and lvalue
variants.

Karl, I think you're the one most involved with this code, do you
have
any opinion?

I strongly prefer keeping the encapsulation.

I agree, I wouldn't apply this patch as is.

This case

PERL_STATIC_INLINE bool
-S_invlist_is_iterating(SV* const invlist)
+S_invlist_is_iterating(const SV* const invlist)
{
  PERL_ARGS_ASSERT_INVLIST_IS_ITERATING;

- return *(get_invlist_iter_addr(invlist)) < (STRLEN) UV_MAX;
+ return ((XINVLIST*) SvANY(invlist))->iterator < (STRLEN)UV_MAX;
}

is one where I think you'd be justified (in C) in casting away const,
so something like​:

+ return *(get_invlist_iter_addr((SV*)invlist)) < (STRLEN) UV_MAX;

since invlist_is_iterating() doesn't modify the iter value.

get_invlist_iter_addr() itself shouldn't be const, since its return value can be
used to modify the iter value.

In C++ you'd just write const and non-const get_invlist_iter_addr() accessors.

Tony