From @demerphq

I consider this a serious bug in 5.14

$ perl5.14.2 -MDevel​::Peek -le'print $]; my %hash=("A".."D"); @​x= keys
%hash; Dump($x[0]);'
5.014002
SV = PV(0x1ace1e0) at 0x1ae27f0
  REFCNT = 1
  FLAGS = (POK,FAKE,READONLY,pPOK)
  PV = 0x1af3290 "A"
  CUR = 1
  LEN = 0

So why would a COPY of a key be READONLY?

In blead​:

$ ./perl -Ilib -MDevel​::Peek -le'print $]; my %hash=("A".."D"); @​x=
keys %hash; Dump($x[0]);'
5.017010
SV = PV(0xdf5bd0) at 0xe12398
  REFCNT = 1
  FLAGS = (POK,IsCOW,pPOK)
  PV = 0xe159e0 "C"
  CUR = 1
  LEN = 0

Is anyone familiar with this issue? Should I file a ticket?

Yves

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

From zefram@fysh.org

yves orton wrote​:

FLAGS = (POK,FAKE,READONLY,pPOK)

FAKE+READONLY means it's not actually unwritable, it's copy-on-write.
From sv.h​:

#define SVf_FAKE 0x01000000 /* 0​: glob or lexical is just a copy
  1​: SV head arena wasn't malloc()ed
  2​: in conjunction with SVf_READONLY
  marks a shared hash key scalar
  (SvLEN == 0) or a copy on write
  string (SvLEN != 0) [SvIsCOW(sv)]

In your case, SvLEN == 0, marking it as a shared hash key.

$ perl5.14.2 -MDevel​::Peek -le'print $]; my %hash=("A".."D"); @​x= keys %hash; Dump($x[0]); $x[0] = "foo"; Dump($x[0])'
5.014002
SV = PV(0x83d2100) at 0x83e2cc0
  REFCNT = 1
  FLAGS = (POK,FAKE,READONLY,pPOK)
  PV = 0x83ed36c "A"
  CUR = 1
  LEN = 0
SV = PV(0x83d2100) at 0x83e2cc0
  REFCNT = 1
  FLAGS = (POK,pPOK)
  PV = 0x83d8330 "foo"\0
  CUR = 3
  LEN = 12

-zefram

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

From @demerphq

On 22 March 2013 17​:54, Zefram via RT <perlbug-followup@​perl.org> wrote​:

yves orton wrote​:

FLAGS = (POK,FAKE,READONLY,pPOK)

FAKE+READONLY means it's not actually unwritable, it's copy-on-write.
From sv.h​:

#define SVf_FAKE 0x01000000 /* 0​: glob or lexical is just a copy
1​: SV head arena wasn't malloc()ed
2​: in conjunction with SVf_READONLY
marks a shared hash key scalar
(SvLEN == 0) or a copy on write
string (SvLEN != 0) [SvIsCOW(sv)]

In your case, SvLEN == 0, marking it as a shared hash key.

$ perl5.14.2 -MDevel​::Peek -le'print $]; my %hash=("A".."D"); @​x= keys %hash; Dump($x[0]); $x[0] = "foo"; Dump($x[0])'
5.014002
SV = PV(0x83d2100) at 0x83e2cc0
REFCNT = 1
FLAGS = (POK,FAKE,READONLY,pPOK)
PV = 0x83ed36c "A"
CUR = 1
LEN = 0
SV = PV(0x83d2100) at 0x83e2cc0
REFCNT = 1
FLAGS = (POK,pPOK)
PV = 0x83d8330 "foo"\0
CUR = 3
LEN = 12

Ah. I see. So the bug is in HTML​::Entities.

The actual code that kicked this off was this​:

$ booking-perl5.14.2 -MDevel​::Peek -MHTML​::Entities -le'print $]; my
%hash=("A".."D"); @​x= sort keys %hash; decode_entities($x[0]);'
5.014002
Can't inline decode readonly string at -e line 1.

Yves

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

From @Leont

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.

The actual code that kicked this off was this​:

$ booking-perl5.14.2 -MDevel​::Peek -MHTML​::Entities -le'print $]; my
%hash=("A".."D"); @​x= sort keys %hash; decode_entities($x[0]);'
5.014002
Can't inline decode readonly string at -e line 1.

This is a perfect example of one of those reasons why
SV_CHECK_THINKFIRST was already necessary before the recent COW work.

Leon

From @demerphq

On 22 March 2013 18​:01, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.

The actual code that kicked this off was this​:

$ booking-perl5.14.2 -MDevel​::Peek -MHTML​::Entities -le'print $]; my
%hash=("A".."D"); @​x= sort keys %hash; decode_entities($x[0]);'
5.014002
Can't inline decode readonly string at -e line 1.

This is a perfect example of one of those reasons why
SV_CHECK_THINKFIRST was already necessary before the recent COW work.

So all Gisle has to do is call that?

I filed this ticket with the distro​:

https://rt.cpan.org/Ticket/Display.html?id=84144

Yves

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

From @nwc10

On Fri, Mar 22, 2013 at 06​:01​:02PM +0100, Leon Timmermans wrote​:

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.

The actual code that kicked this off was this​:

$ booking-perl5.14.2 -MDevel​::Peek -MHTML​::Entities -le'print $]; my
%hash=("A".."D"); @​x= sort keys %hash; decode_entities($x[0]);'
5.014002
Can't inline decode readonly string at -e line 1.

This is a perfect example of one of those reasons why
SV_CHECK_THINKFIRST was already necessary before the recent COW work.

It actually dates all the way back to 5.8.0, which made hash keys themselves
Copy On Write. So this has been around for over a decade. But before 5.10.0
you only really get to spot it if you use something that doesn't copy the
hash keys, such as foreach() or a function parameter. So​:

$ ~/Sandpit/580g/bin/perl5.8.0 -MHTML​::Entities -le 'my %hash=("A".."D"); foreach (keys %hash) {decode_entities($_); print $_}'
Can't inline decode readonly string at -e line 1.

whereas concatenating an empty string on the key​:

$ ~/Sandpit/580g/bin/perl5.8.0 -MHTML​::Entities -le 'my %hash=("A".."D"); foreach (keys %hash) {$_ .= ""; decode_entities($_); print $_}'
A
C

triggers the copy.

Nicholas Clark

From @demerphq

On 25 March 2013 10​:44, Nicholas Clark <nick@​ccl4.org> wrote​:

On Fri, Mar 22, 2013 at 06​:01​:02PM +0100, Leon Timmermans wrote​:

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.

The actual code that kicked this off was this​:

$ booking-perl5.14.2 -MDevel​::Peek -MHTML​::Entities -le'print $]; my
%hash=("A".."D"); @​x= sort keys %hash; decode_entities($x[0]);'
5.014002
Can't inline decode readonly string at -e line 1.

This is a perfect example of one of those reasons why
SV_CHECK_THINKFIRST was already necessary before the recent COW work.

It actually dates all the way back to 5.8.0, which made hash keys themselves
Copy On Write. So this has been around for over a decade. But before 5.10.0
you only really get to spot it if you use something that doesn't copy the
hash keys, such as foreach() or a function parameter. So​:

$ ~/Sandpit/580g/bin/perl5.8.0 -MHTML​::Entities -le 'my %hash=("A".."D"); foreach (keys %hash) {decode_entities($_); print $_}'
Can't inline decode readonly string at -e line 1.

whereas concatenating an empty string on the key​:

$ ~/Sandpit/580g/bin/perl5.8.0 -MHTML​::Entities -le 'my %hash=("A".."D"); foreach (keys %hash) {$_ .= ""; decode_entities($_); print $_}'
A
C

triggers the copy.

Thanks, yeah, I had figured that out already.

I tried to fix HTML​::Entities but there is a minor issue, see the mail
to p5p about SV_CHECK_THINKFIRST() being not so wonderful.

cheers,
yves

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

From @cpansprout

On Fri Mar 22 10​:18​:45 2013, demerphq wrote​:

On 22 March 2013 18​:01, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.
...
I filed this ticket with the distro​:

https://rt.cpan.org/Ticket/Display.html?id=84144

That ticket is resolved, so I am resolving this one, too. Please say
something if this resolution is premature.

--

Father Chrysostomos

@cpansprout - Status changed from 'open' to 'resolved'

From @demerphq

How is the original problem resolved?

I am reopening this ticket as it has no information on why it was
resolved and what the solution was.

Please dont close tickets without an explanation.

Thanks,
Yves

On 4 June 2013 03​:26, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Fri Mar 22 10​:18​:45 2013, demerphq wrote​:

On 22 March 2013 18​:01, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.
...
I filed this ticket with the distro​:

https://rt.cpan.org/Ticket/Display.html?id=84144

That ticket is resolved, so I am resolving this one, too. Please say
something if this resolution is premature.

--

Father Chrysostomos

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

From @demerphq

The point I refer to is that you made read-only keys no longer have
the read only flag set.

This means that old code that tests for read-onlyness will mindlessly
modify read only data structures, which is an unacceptable failure
mode.

How it affected HTML​::Entities is just a symptom of a dangerous
non-backwards compatible change that cannot persist in 5.20.

So, what did you do to address this issue so that this ticket can be closed?

On 4 June 2013 07​:38, demerphq <demerphq@​gmail.com> wrote​:

How is the original problem resolved?

I am reopening this ticket as it has no information on why it was
resolved and what the solution was.

Please dont close tickets without an explanation.

Thanks,
Yves

On 4 June 2013 03​:26, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Fri Mar 22 10​:18​:45 2013, demerphq wrote​:

On 22 March 2013 18​:01, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Fri, Mar 22, 2013 at 5​:58 PM, demerphq <demerphq@​gmail.com> wrote​:

Ah. I see. So the bug is in HTML​::Entities.
...
I filed this ticket with the distro​:

https://rt.cpan.org/Ticket/Display.html?id=84144

That ticket is resolved, so I am resolving this one, too. Please say
something if this resolution is premature.

--

Father Chrysostomos

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

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

From @cpansprout

On Mon Jun 03 22​:45​:16 2013, demerphq wrote​:

The point I refer to is that you made read-only keys no longer have
the read only flag set.

But that was not the subject of the ticket. :-)

If you want this ticket to encompass that, so be it. Reopening....

--

Father Chrysostomos

@cpansprout - Status changed from 'resolved' to 'open'

From @cpansprout

On Mon Jun 03 22​:45​:16 2013, demerphq wrote​:

The point I refer to is that you made read-only keys no longer have
the read only flag set.

This means that old code that tests for read-onlyness will mindlessly
modify read only data structures, which is an unacceptable failure
mode.

How it affected HTML​::Entities is just a symptom of a dangerous
non-backwards compatible change that cannot persist in 5.20.

I still disagree with you. I am going to try and persuade you to change
you mind. :-)

In my experience, most XS code simply does not modify string buffers
directly, but uses sv_set* and sv_cat*.

Most of the code I have seen that modifies SvPVX in place is old code
that predates the API. And then much of what I have seen was already
buggy anyway.

I also find it unconvincing that the presence of the SvREADONLY flag is
a panacea that allays all such problems with corrupted buffers. I found
more than a dozen bugs, both in core and on CPAN, where the read-only
flag was simply ignored. And it wasn’t the end of the world.

If copy-on-write is to become more prevalent, then under the
READONLY+FAKE system almost every piece of XS code is going to have to
do if (SvREADONLY && !SvFAKE). Believe me, you will see dozens of
tickets on rt.cpan.org about ‘Modification of a read-only value’ under 5.20.

Anyway, the point I am trying to get to is that there are certain
optimisations I would like to implement, involving read-only COWs, that
I cannot do if there is a chance the flags will be reverted back to the
way they used to work.

According to the READONLY+FAKE scheme, it is not possible to have a COW
that is read-only to Perl space. Or, it is not possible to do COW with
read-only scalars.

That means that $x = "hello" will always copy the buffer. It also means
that $x = "h"."ello" will copy the buffer, but disabling the constant
folding ($x = ${\"h"}."ello") allows the buffer not to be copied,
speeding things up.

$ time ./perl -Ilib -e '$x = " "x2**31'

real 0m32.062s
user 0m2.832s
sys 0m3.578s
$ time ./perl -Ilib -e '$x = ${\" "}x2**31'

real 0m2.635s
user 0m1.157s
sys 0m1.409s

Some people lock their objects supposedly for the sake of speed
(avoiding extra copies), but Hash​::Util​::lock_value (i.e.,
Internals​::SvREADONLY) forces the string to be copied if it is COW,
defeating the purpose.

All this can be cleaned up if we allow read-only COWs, which cannot
happen if SvREADONLY means two things.

--

Father Chrysostomos