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

Re: 5.17.7 breaks rules of assignment #12744

Open
p5pRT opened this issue Jan 29, 2013 · 41 comments
Open

Re: 5.17.7 breaks rules of assignment #12744

p5pRT opened this issue Jan 29, 2013 · 41 comments

Comments

@p5pRT
Copy link

p5pRT commented Jan 29, 2013

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

Searchable as RT116569$

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @sisyphus

From​: sisyphus1@​optusnet.com.au

In a nutshell, with 5.17.7​:

$buffer = ‘some string’;
$buf = $buffer;

Then call an XSub that writes into $buf (by using sprintf, say).
I then find that $buffer (which should not have changed) has also been
written into, and is the same as $buf.

It's still the same behaviour in 5.17.8.
Is this how it's going to be in 5.18 ?

Again, here's a simple demo of how things have changed between 5.16 and
5.17.7/8​:

####################################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

SV * foo(char * buffer) {
  char * c = "cow";
  sprintf(buffer, "%s", c);
  return newSVpv(buffer, 0);
}

EOC

my $buff = 'dog';
my $buf = $buff;
print "\$buf​: $buf\n";
print "\$buff​: $buff\n";

foo($buf);

print "\$buf​: $buf\n";
print "\$buff​: $buff\n";
####################################

On 5.17.7 and 5.17.8 that outputs

$buf​: dog
$buff​: dog
$buf​: cow
$buff​: cow

On 5.16 and earlier it outputs

$buf​: dog
$buff​: dog
$buf​: cow
$buff​: dog

For some (insane) reason an XS sub that changes the value of $buf, also
changes the value of $buff.
(Yet a perl sub that changes the value of $buf, does *not* change the value
of $buff.)

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @bulk88

On Mon Jan 28 23​:14​:57 2013, sisyphus wrote​:

From​: sisyphus1@​optusnet.com.au

In a nutshell, with 5.17.7​:

$buffer = ‘some string’;
$buf = $buffer;

Then call an XSub that writes into $buf (by using sprintf, say).
I then find that $buffer (which should not have changed) has also been
written into, and is the same as $buf.

It's still the same behaviour in 5.17.8.
Is this how it's going to be in 5.18 ?

Again, here's a simple demo of how things have changed between 5.16 and
5.17.7/8​:

####################################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

SV * foo(char * buffer) {
char * c = "cow";
sprintf(buffer, "%s", c);
return newSVpv(buffer, 0);
}

EOC

For some (insane) reason an XS sub that changes the value of $buf, also
changes the value of $buff.
(Yet a perl sub that changes the value of $buf, does *not* change the
value
of $buff.)

Cheers,
Rob

No SvTHINKFIRST. Almost a buffer overflow too. No sv_grow. Didn't update
SvCUR and POK either.

Note that for a potential fix, const char * and char * are different.

This is COW related.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @sisyphus

-----Original Message-----
From​: bulk88 via RT
Sent​: Tuesday, January 29, 2013 8​:42 PM
To​: sisyphus1@​optusnet.com.au
Subject​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

On Mon Jan 28 23​:14​:57 2013, sisyphus wrote​:

From​: sisyphus1@​optusnet.com.au

In a nutshell, with 5.17.7​:

$buffer = ‘some string’;
$buf = $buffer;

Then call an XSub that writes into $buf (by using sprintf, say).
I then find that $buffer (which should not have changed) has also been
written into, and is the same as $buf.

It's still the same behaviour in 5.17.8.
Is this how it's going to be in 5.18 ?

Again, here's a simple demo of how things have changed between 5.16 and
5.17.7/8​:

####################################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

SV * foo(char * buffer) {
char * c = "cow";
sprintf(buffer, "%s", c);
return newSVpv(buffer, 0);
}

EOC

For some (insane) reason an XS sub that changes the value of $buf, also
changes the value of $buff.
(Yet a perl sub that changes the value of $buf, does *not* change the
value of $buff.)

No SvTHINKFIRST. Almost a buffer overflow too. No sv_grow. Didn't update
SvCUR and POK either.

It overwites the string 'dog' with the string'cow'. Why is any of that
necessary ?

Note that for a potential fix, const char * and char * are different.

This is COW related.

Yeah - but, irrespective of which farmyard animal dreamt it up, is this the
way it's going to behave in 5.18 ?
Or is someone going to step in and restore sanity ?
That's what I'd *really* like to know.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @Leont

On Tue, Jan 29, 2013 at 11​:47 AM, <sisyphus1@​optusnet.com.au> wrote​:

No SvTHINKFIRST. Almost a buffer overflow too. No sv_grow. Didn't update
SvCUR and POK either.

Because we already had COW, we're just using it in more places than
before. Your code is already broken if $buffer is a magical variable,
a hash key, a fake glob or a readonly variable; and possbilby if it's
a reference to.

*Your code is broken today*. This change makes it more obvious, but
that doesn't mean we just broke it.

It overwites the string 'dog' with the string'cow'. Why is any of that
necessary ?

I'm not sure what for it would be more necessary than that.

Or is someone going to step in and restore sanity ?
That's what I'd *really* like to know.

The reality that a lot of code on CPAN is broken in this way is
unfortunate, but THINKFIRST is unlikely to go away.

Leon

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @sisyphus

-----Original Message-----
From​: Leon Timmermans via RT
Sent​: Tuesday, January 29, 2013 10​:29 PM
To​: sisyphus1@​optusnet.com.au
Cc​: bulk88@​hotmail.com
Subject​: Re​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

On Tue, Jan 29, 2013 at 11​:47 AM, <sisyphus1@​optusnet.com.au> wrote​:

No SvTHINKFIRST. Almost a buffer overflow too. No sv_grow. Didn't update
SvCUR and POK either.

Because we already had COW, we're just using it in more places than
before. Your code is already broken if $buffer is a magical variable,
a hash key, a fake glob or a readonly variable; and possbilby if it's
a reference to.

*Your code is broken today*. This change makes it more obvious, but
that doesn't mean we just broke it.

Does this mean that the same code is going to behave in the same way when
5.18 comes along ?
I just want someone to tell me definitively that it won't/will (or might)
change between now and 5.18.

Or should I just wait and see what 5.18 delivers before I do anything ?

The reality that a lot of code on CPAN is broken in this way is
unfortunate, but THINKFIRST is unlikely to go away.

Where is this THINKFIRST stuff documented ? (I know nothing about it -
couldn't find any mention of it in perlapi or perlguts.)

In terms of my own code on CPAN that's broken, it's not such a big deal
(afaik).

I know I have one test script where the problem goes away if I change some
instances of (something like)​:

$copy = $original;
to​:
$copy = "$original";

When I make that change, $original no longer gets ovewrwritten by the XSub
to which $copy is passed.

Still, it would be nice to know how to achieve the same result by modifying
the XSub (which simply passes its arguments along to a 3rd party library
over which I have no control), rather than by making that change to the perl
code.
So - let me ask that question specifically.

Say my XSub looks like​:

SV * foo(char *buffer) {
  /* alter() is a 3rd party C library function
  over which I have no control */
  alter(buffer);
  return newSVpv(buffer, 0);
}

Given that I already know that "buffer" will accommodate the changes that
alter() will inflict, how should I rewrite that XSub to ensure that the perl
scalar passed to foo() is the *only* perl scalar that gets altered ?

My main objection to the current behaviour is on grounds of dwimmery and
expectations.
If I have code that does​:

$orig = 'some string';
$copy = $orig;
foo($copy);

then I find it unacceptable that $orig should be altered, irrespective of
what foo() does ... unless, of course, foo() is coded to explicitly do
something to a perl variable named "$orig" - which is not the case in the
example I provided.
In short, $orig should be unreachable from foo($copy). Altering the copy
should not alter the original. The whole idea of creating the copy is to
have something to mess with, *without* altering the original.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @Leont

On Tue, Jan 29, 2013 at 2​:22 PM, <sisyphus1@​optusnet.com.au> wrote​:

Does this mean that the same code is going to behave in the same way when
5.18 comes along ?
I just want someone to tell me definitively that it won't/will (or might)
change between now and 5.18.

Or should I just wait and see what 5.18 delivers before I do anything ?

I wouldn't expect it to change, but I wouldn't rule it out either.

Where is this THINKFIRST stuff documented ? (I know nothing about it -
couldn't find any mention of it in perlapi or perlguts.)

Probably it isn't. A lot of stuff isn't. Welcome to core.

Still, it would be nice to know how to achieve the same result by modifying
the XSub (which simply passes its arguments along to a 3rd party library
over which I have no control), rather than by making that change to the perl
code.

That would be entirely reasonable.

So - let me ask that question specifically.

Say my XSub looks like​:

SV * foo(char *buffer) {
/* alter() is a 3rd party C library function
over which I have no control */
alter(buffer);
return newSVpv(buffer, 0);
}

Given that I already know that "buffer" will accommodate the changes that
alter() will inflict, how should I rewrite that XSub to ensure that the perl
scalar passed to foo() is the *only* perl scalar that gets altered ?

I don't know how Inline​::C works exactly, but I'm guessing the
typemaps it uses need to be updated; in which case it is perl's
responsibility to do so.

My main objection to the current behaviour is on grounds of dwimmery and
expectations.
If I have code that does​:

$orig = 'some string';
$copy = $orig;
foo($copy);

then I find it unacceptable that $orig should be altered, irrespective of
what foo() does ... unless, of course, foo() is coded to explicitly do
something to a perl variable named "$orig" - which is not the case in the
example I provided.
In short, $orig should be unreachable from foo($copy). Altering the copy
should not alter the original. The whole idea of creating the copy is to
have something to mess with, *without* altering the original.

I agree with that on a high level, but if foo is an XSUB then foo
shares that responsibility with perl to ensure nothing bad happens.
That's the price you pay for mucking around with SV internals.

Leon

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @doy

On Tue, Jan 29, 2013 at 04​:54​:16PM +0100, Leon Timmermans wrote​:

On Tue, Jan 29, 2013 at 2​:22 PM, <sisyphus1@​optusnet.com.au> wrote​:

So - let me ask that question specifically.

Say my XSub looks like​:

SV * foo(char *buffer) {
/* alter() is a 3rd party C library function
over which I have no control */
alter(buffer);
return newSVpv(buffer, 0);
}

Given that I already know that "buffer" will accommodate the changes that
alter() will inflict, how should I rewrite that XSub to ensure that the perl
scalar passed to foo() is the *only* perl scalar that gets altered ?

I don't know how Inline​::C works exactly, but I'm guessing the
typemaps it uses need to be updated; in which case it is perl's
responsibility to do so.

Yes - I think the answer to the underlying question here is that the
typemap for char* in core needs to be updated to check for SvTHINKFIRST.

-doy

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2013

From @tsee

On 01/29/2013 05​:01 PM, Jesse Luehrs wrote​:

On Tue, Jan 29, 2013 at 04​:54​:16PM +0100, Leon Timmermans wrote​:

On Tue, Jan 29, 2013 at 2​:22 PM, <sisyphus1@​optusnet.com.au> wrote​:

So - let me ask that question specifically.

Say my XSub looks like​:

SV * foo(char *buffer) {
/* alter() is a 3rd party C library function
over which I have no control */
alter(buffer);
return newSVpv(buffer, 0);
}

Given that I already know that "buffer" will accommodate the changes that
alter() will inflict, how should I rewrite that XSub to ensure that the perl
scalar passed to foo() is the *only* perl scalar that gets altered ?

I don't know how Inline​::C works exactly, but I'm guessing the
typemaps it uses need to be updated; in which case it is perl's
responsibility to do so.

Yes - I think the answer to the underlying question here is that the
typemap for char* in core needs to be updated to check for SvTHINKFIRST.

Which is of some concern because they can't travel back in time. In
theory, we now have the tools (ExtUtils​::Typemaps and a modern
ExtUtils​::ParseXS 3.X) to do so, but I fear I won't have the time to
figure out exactly how that would work wrt. backwards-compatibility and
superseding core typemaps.

--Steffen

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2013

From @sisyphus

-----Original Message-----
From​: Jesse Luehrs
Sent​: Wednesday, January 30, 2013 3​:01 AM
To​: Leon Timmermans
Cc​: sisyphus1@​optusnet.com.au ; perlbug-followup@​perl.org
Subject​: Re​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

On Tue, Jan 29, 2013 at 04​:54​:16PM +0100, Leon Timmermans wrote​:

On Tue, Jan 29, 2013 at 2​:22 PM, <sisyphus1@​optusnet.com.au> wrote​:

So - let me ask that question specifically.

Say my XSub looks like​:

SV * foo(char *buffer) {
/* alter() is a 3rd party C library function
over which I have no control */
alter(buffer);
return newSVpv(buffer, 0);
}

Given that I already know that "buffer" will accommodate the changes
that
alter() will inflict, how should I rewrite that XSub to ensure that the
perl
scalar passed to foo() is the *only* perl scalar that gets altered ?

I don't know how Inline​::C works exactly, but I'm guessing the
typemaps it uses need to be updated; in which case it is perl's
responsibility to do so.

Yes - I think the answer to the underlying question here is that the
typemap for char* in core needs to be updated to check for SvTHINKFIRST.

Ok - if it's just a problem with the char* typemap, that's not such a big
deal for me.

I'll just avoid passing char* arguments - or, at least, proceed with caution
when a char* does get passed.

Thanks guys - Jesse, Leon, Steffen, bulk88.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2013

From @iabyn

On Tue, Jan 29, 2013 at 04​:54​:16PM +0100, Leon Timmermans wrote​:

Or should I just wait and see what 5.18 delivers before I do anything ?

I wouldn't expect it to change, but I wouldn't rule it out either.

There has been some talk of disabling the COW changes for the 5.18
release, given the amount of breakage (not necessarily its fault) its
causing, and give it more time to mature.

This ticket I think counts as another +1 to disablement.

--
Fire extinguisher (n) a device for holding open fire doors.

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2013

From @Leont

On Wed, Jan 30, 2013 at 8​:24 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

There has been some talk of disabling the COW changes for the 5.18
release, given the amount of breakage (not necessarily its fault) its
causing, and give it more time to mature.

This ticket I think counts as another +1 to disablement.

I interpreted the previous talk as being about removing it from the
regexp engine, not removing it entirely (I assumed the former can be
done without the latter).

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2013

From @sisyphus

-----Original Message-----
From​: Jesse Luehrs
Sent​: Wednesday, January 30, 2013 3​:01 AM
To​: Leon Timmermans
Cc​: sisyphus1@​optusnet.com.au ; perlbug-followup@​perl.org
Subject​: Re​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

Yes - I think the answer to the underlying question here is that the
typemap for char* in core needs to be updated to check for SvTHINKFIRST.

Seems to me that it's not solely the char* typemap that's the issue here.

I think the following Inline​::C demo avoids the char* typemap, yet still
displays the same behaviour.
(At least, removing the char* typemapping from lib/ExtUtils/typemap doesn't
make any dsifference.)

#########################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

void foo (SV * x) {
  char *new = "new";
  char *str = SvPV_nolen(x);
  sprintf(str, "%s", "new");
}

EOC

my $orig = "orig";
my $copy = $orig;

foo($copy);
print "\$orig = $orig\n\$copy = $copy\n";
#########################

On 5.16, this script outputs​:

$orig = orig
$copy = new

On 5.17.8 it outputs​:

$orig = new
$copy = new

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2013

From @doy

On Tue, Feb 05, 2013 at 05​:28​:16PM +1100, sisyphus1@​optusnet.com.au wrote​:

-----Original Message----- From​: Jesse Luehrs
Sent​: Wednesday, January 30, 2013 3​:01 AM
To​: Leon Timmermans
Cc​: sisyphus1@​optusnet.com.au ; perlbug-followup@​perl.org
Subject​: Re​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

Yes - I think the answer to the underlying question here is that the
typemap for char* in core needs to be updated to check for SvTHINKFIRST.

Seems to me that it's not solely the char* typemap that's the issue here.

I think the following Inline​::C demo avoids the char* typemap, yet
still displays the same behaviour.
(At least, removing the char* typemapping from lib/ExtUtils/typemap
doesn't make any dsifference.)

#########################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

void foo (SV * x) {
char *new = "new";
char *str = SvPV_nolen(x);
sprintf(str, "%s", "new");
}

EOC

my $orig = "orig";
my $copy = $orig;

foo($copy);
print "\$orig = $orig\n\$copy = $copy\n";
#########################

On 5.16, this script outputs​:

$orig = orig
$copy = new

On 5.17.8 it outputs​:

$orig = new
$copy = new

This part is not a bug. Using the SV* typemap will bypass any of the
things we've been discussing, and the change you're seeing here is an
intentional one. You'd see the same behavior in previous versions of
perl if you passed a variable to foo() that used copy-on-write, it's
just that that used to happen in a lot fewer cases. The buffer returned
by SvPV was never intended to be a writable buffer - for that, you need
to use SvTHINKFIRST and sv_force_normal_flags in order to make sure it's
a real independent copy.

-doy

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2013

From @sisyphus

-----Original Message-----
From​: Jesse Luehrs
Sent​: Tuesday, February 05, 2013 5​:33 PM
To​: sisyphus1@​optusnet.com.au
Cc​: Leon Timmermans ; perlbug-followup@​perl.org
Subject​: Re​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

#########################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

void foo (SV * x) {
char *new = "new";
char *str = SvPV_nolen(x);
sprintf(str, "%s", "new");
}

EOC

my $orig = "orig";
my $copy = $orig;

foo($copy);
print "\$orig = $orig\n\$copy = $copy\n";
#########################

On 5.16, this script outputs​:

$orig = orig
$copy = new

On 5.17.8 it outputs​:

$orig = new
$copy = new

This part is not a bug. Using the SV* typemap will bypass any of the
things we've been discussing, and the change you're seeing here is an
intentional one. You'd see the same behavior in previous versions of
perl if you passed a variable to foo() that used copy-on-write, it's
just that that used to happen in a lot fewer cases. The buffer returned
by SvPV was never intended to be a writable buffer - for that, you need
to use SvTHINKFIRST and sv_force_normal_flags in order to make sure it's
a real independent copy.

Ok ... it might be starting to sink in.

I've found that if I rewrite foo() in the above script as​:

void foo (SV * x) {
  sv_setpv(x, "new");
}

then, for both 5.16 and 5.17.8, $copy gets changed to 'new' but $orig
remains unchanged.
Is that a safe and recommended way of ensuring that only the copy gets
altered when IsCOW is set ?

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2013

From @doy

On Tue, Feb 05, 2013 at 07​:48​:20PM +1100, sisyphus1@​optusnet.com.au wrote​:

-----Original Message----- From​: Jesse Luehrs
Sent​: Tuesday, February 05, 2013 5​:33 PM
To​: sisyphus1@​optusnet.com.au
Cc​: Leon Timmermans ; perlbug-followup@​perl.org
Subject​: Re​: [perl #116569] Re​: 5.17.7 breaks rules of assignment

#########################
use strict;
use warnings;

#use Inline C => Config =>
# BUILD_NOISY => 1;

use Inline C => <<'EOC';

void foo (SV * x) {
char *new = "new";
char *str = SvPV_nolen(x);
sprintf(str, "%s", "new");
}

EOC

my $orig = "orig";
my $copy = $orig;

foo($copy);
print "\$orig = $orig\n\$copy = $copy\n";
#########################

On 5.16, this script outputs​:

$orig = orig
$copy = new

On 5.17.8 it outputs​:

$orig = new
$copy = new

This part is not a bug. Using the SV* typemap will bypass any of the
things we've been discussing, and the change you're seeing here is an
intentional one. You'd see the same behavior in previous versions of
perl if you passed a variable to foo() that used copy-on-write, it's
just that that used to happen in a lot fewer cases. The buffer returned
by SvPV was never intended to be a writable buffer - for that, you need
to use SvTHINKFIRST and sv_force_normal_flags in order to make sure it's
a real independent copy.

Ok ... it might be starting to sink in.

I've found that if I rewrite foo() in the above script as​:

void foo (SV * x) {
sv_setpv(x, "new");
}

then, for both 5.16 and 5.17.8, $copy gets changed to 'new' but
$orig remains unchanged.
Is that a safe and recommended way of ensuring that only the copy
gets altered when IsCOW is set ?

Yes, if there is no reason that you actually need to use the existing
memory buffer, then sv_setpv is almost certainly preferable.

-doy

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2013

From @iabyn

On Wed, Jan 30, 2013 at 08​:56​:52PM +0100, Leon Timmermans wrote​:

On Wed, Jan 30, 2013 at 8​:24 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

There has been some talk of disabling the COW changes for the 5.18
release, given the amount of breakage (not necessarily its fault) its
causing, and give it more time to mature.

This ticket I think counts as another +1 to disablement.

I interpreted the previous talk as being about removing it from the
regexp engine, not removing it entirely

Quite possibly.

(I assumed the former can be done without the latter).

Yeah I think so, with PERL_SAWAMPERSAND.

However, later on in this thread we seem to be telling XS authors that
their code is broken unless they are using the non-API SvTHINKFIRST macro,
which as it happens, isn't described in any of the xs*.pod or perlguts.pod
files.

If this is actually the case, then I think it would be wrong to release
5.18 with it enabled.

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with -DPERL_COW).
Then in perldelta we say that's its an experimental feature, that is
likely to be enabled by default in 5.20, and that XS authors in particular
should try it out and pay attention to SvTHINKFIRST etc yadayada.

Then as soon as 5.18 is released, we enable in blead and work on fixing
any bugs.

--
"You're so sadly neglected, and often ignored.
A poor second to Belgium, When going abroad."
  -- Monty Python, "Finland"

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2013

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2013-02-05T09​:53​:23]

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with -DPERL_COW).
Then in perldelta we say that's its an experimental feature, that is
likely to be enabled by default in 5.20, and that XS authors in particular
should try it out and pay attention to SvTHINKFIRST etc yadayada.

I am in favor of this.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2013

From @Leont

On Tue, Feb 5, 2013 at 3​:53 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

However, later on in this thread we seem to be telling XS authors that
their code is broken unless they are using the non-API SvTHINKFIRST macro,
which as it happens, isn't described in any of the xs*.pod or perlguts.pod
files.

If this is actually the case, then I think it would be wrong to release
5.18 with it enabled.

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with -DPERL_COW).
Then in perldelta we say that's its an experimental feature, that is
likely to be enabled by default in 5.20, and that XS authors in particular
should try it out and pay attention to SvTHINKFIRST etc yadayada.

Then as soon as 5.18 is released, we enable in blead and work on fixing
any bugs.

Sounds reasonable to me

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2013

From @khwilliamson

On 02/05/2013 07​:45 PM, Leon Timmermans wrote​:

On Tue, Feb 5, 2013 at 3​:53 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

However, later on in this thread we seem to be telling XS authors that
their code is broken unless they are using the non-API SvTHINKFIRST macro,
which as it happens, isn't described in any of the xs*.pod or perlguts.pod
files.

If this is actually the case, then I think it would be wrong to release
5.18 with it enabled.

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with -DPERL_COW).
Then in perldelta we say that's its an experimental feature, that is
likely to be enabled by default in 5.20, and that XS authors in particular
should try it out and pay attention to SvTHINKFIRST etc yadayada.

Then as soon as 5.18 is released, we enable in blead and work on fixing
any bugs.

Sounds reasonable to me

Leon

+1

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2013

From @sisyphus

On 02/05/2013 07​:45 PM, Leon Timmermans wrote​:

On Tue, Feb 5, 2013 at 3​:53 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

However, later on in this thread we seem to be telling XS authors that
their code is broken unless they are using the non-API SvTHINKFIRST
macro,
which as it happens, isn't described in any of the xs*.pod or
perlguts.pod
files.

If this is actually the case, then I think it would be wrong to release
5.18 with it enabled.

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with -DPERL_COW).
Then in perldelta we say that's its an experimental feature, that is
likely to be enabled by default in 5.20, and that XS authors in
particular
should try it out and pay attention to SvTHINKFIRST etc yadayada.

Then as soon as 5.18 is released, we enable in blead and work on fixing
any bugs.

Sounds reasonable to me

I think that's a better approach, too.

So .... if an XS author then wants to try out his module with COW enabled,
but his perl was built with -DPERL_NO_COW, I take that he first needs to
obtain a perl that was built with -DPERL_COW.
Or is COW-enablement something that can be turned on from within the XS
author's code ?

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2013

From @demerphq

On 6 February 2013 00​:54, Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote​:

* Dave Mitchell <davem@​iabyn.com> [2013-02-05T09​:53​:23]

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with -DPERL_COW).
Then in perldelta we say that's its an experimental feature, that is
likely to be enabled by default in 5.20, and that XS authors in particular
should try it out and pay attention to SvTHINKFIRST etc yadayada.

I am in favor of this.

Note we need a proper documentation of SvTHINKFIRST and related macros.

Also, I wonder if this bites us anywhere in the core? Since
SvTHINKFIRST isnt documented, I know I could have easily omitted it.
(OTOH, I dont think ive ever coded stuff that modifes an arbitrary SV,
but ive coded stuff that modifies a brand new clean SV).

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

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2013

From @Leont

On Wed, Feb 6, 2013 at 7​:12 AM, demerphq <demerphq@​gmail.com> wrote​:

Note we need a proper documentation of SvTHINKFIRST and related macros.

Also, I wonder if this bites us anywhere in the core? Since
SvTHINKFIRST isnt documented, I know I could have easily omitted it.
(OTOH, I dont think ive ever coded stuff that modifes an arbitrary SV,
but ive coded stuff that modifies a brand new clean SV).

I think the usual functions to manipulate SV's with (sv_set[ps]v,
sv_cat[ps]v, …), already take it into account (though checking them
all may be a good idea). I did see Father C adding SvIsCOW checks in
some places that I suspect deserve SvTHINKFIRST checks, but I'm not
certain I'm correct.

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2013

From @iabyn

On Wed, Feb 06, 2013 at 03​:07​:13PM +1100, sisyphus1@​optusnet.com.au wrote​:

So .... if an XS author then wants to try out his module with COW
enabled, but his perl was built with -DPERL_NO_COW, I take that he
first needs to obtain a perl that was built with -DPERL_COW.
Or is COW-enablement something that can be turned on from within the
XS author's code ?

A vanilla 5.18 build wont have COW enabled.
5.18 with -DPERL_COW used at perl build time will have COW.
5.18 will ignore -DPERL_NO_COW as this is the default anyway; only
  5.17.7 and 5.17.8 recognise that switch.

In all cases it only affects how perl is built, and can't be enabled or
disabled later​: XS authors will have to specially build a COW-enabled perl
in order to test their code.

--
"There's something wrong with our bloody ships today, Chatfield."
  -- Admiral Beatty at the Battle of Jutland, 31st May 1916.

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2013

From @sisyphus

-----Original Message-----
From​: Dave Mitchell

On Wed, Feb 06, 2013 at 03​:07​:13PM +1100, sisyphus1@​optusnet.com.au wrote​:

So .... if an XS author then wants to try out his module with COW
enabled, but his perl was built with -DPERL_NO_COW, I take that he
first needs to obtain a perl that was built with -DPERL_COW.
Or is COW-enablement something that can be turned on from within the
XS author's code ?

A vanilla 5.18 build wont have COW enabled.
5.18 with -DPERL_COW used at perl build time will have COW.
5.18 will ignore -DPERL_NO_COW as this is the default anyway; only
5.17.7 and 5.17.8 recognise that switch.

In all cases it only affects how perl is built, and can't be enabled or
disabled later​: XS authors will have to specially build a COW-enabled perl
in order to test their code.

Thanks for confirming, Dave. (That's as I expected.)

I envisage that a fair proportion of cpan authors rely on using the version
of perl that's provided by their perl package manager - and that it's up to
those perl package managers as to whether the perl-5.18 that they provide is
COW-enabled or COW-disabled. (I would expect that most managers would go
with the default "COW-disabled".)

I, for one, will be using COW-enabled. Are COW-enabled and COW-disabled
binary-compatible ? (No matter ... I'll soon suck it and see ;-)

Looking forward to seeing what 5.17.9 brings !!

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2013

From @steve-m-hay

demerphq wrote on 2013-02-06​:

On 6 February 2013 00​:54, Ricardo Signes <perl.p5p@​rjbs.manxome.org>
wrote​:

* Dave Mitchell <davem@​iabyn.com> [2013-02-05T09​:53​:23]

I propose that in the next week or two, we turn it off by default,
but make it enable-able (i.e. replace -DPERL_NO_COW with
-DPERL_COW).
Then in perldelta we say that's its an experimental feature, that
is likely to be enabled by default in 5.20, and that XS authors in
particular should try it out and pay attention to SvTHINKFIRST etc
yadayada.

I am in favor of this.

Note we need a proper documentation of SvTHINKFIRST and related
macros.

Also, I wonder if this bites us anywhere in the core? Since
SvTHINKFIRST isnt documented, I know I could have easily omitted it.
(OTOH, I dont think ive ever coded stuff that modifes an arbitrary SV,
but ive coded stuff that modifies a brand new clean SV).

SvTHINKFIRST is documented, but only in perlintern.pod (from the source
in sv.h), meaning (according to that POD file itself) "not marked as
part of the Perl API. In other words, B<they are not for use in
extensions>!".

Adding an 'A' to the flags in the "=for apidoc" line in sv.h moves the
documentation from perlintern.pod into perlapi.pod. Presumably this
should be done, but I don't know if the documentation of it is up to
date or what, if anything, else needs similarly moving.

Inline Patch
diff --git a/sv.h b/sv.h
index 54d606b..afe837c 100644
--- a/sv.h
+++ b/sv.h
@@ -992,7 +992,7 @@ the scalar's value cannot change unless written to.
 #define SvPCS_IMPORTED_off(sv) (SvFLAGS(sv) &=
~(SVf_ROK|SVprv_PCS_IMPORTED))

/*
-=for apidoc m|U32|SvTHINKFIRST|SV *sv
+=for apidoc Am|U32|SvTHINKFIRST|SV *sv

A quick flag check to see whether an sv should be passed to
sv_force_normal
to be "downgraded" before SvIVX or SvPVX can be modified directly.

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2013

From @bulk88

On Fri Feb 08 06​:01​:48 2013, Steve.Hay@​verosoftware.com wrote​:

SvTHINKFIRST is documented, but only in perlintern.pod (from the source
in sv.h), meaning (according to that POD file itself) "not marked as
part of the Perl API. In other words, B<they are not for use in
extensions>!".

Adding an 'A' to the flags in the "=for apidoc" line in sv.h moves the
documentation from perlintern.pod into perlapi.pod. Presumably this
should be done, but I don't know if the documentation of it is up to
date or what, if anything, else needs similarly moving.

diff --git a/sv.h b/sv.h
index 54d606b..afe837c 100644
--- a/sv.h
+++ b/sv.h
@​@​ -992,7 +992,7 @​@​ the scalar's value cannot change unless written to.
#define SvPCS_IMPORTED_off(sv) (SvFLAGS(sv) &=
~(SVf_ROK|SVprv_PCS_IMPORTED))

/*
-=for apidoc m|U32|SvTHINKFIRST|SV *sv
+=for apidoc Am|U32|SvTHINKFIRST|SV *sv

A quick flag check to see whether an sv should be passed to
sv_force_normal
to be "downgraded" before SvIVX or SvPVX can be modified directly.

A guide on how to make a SV be undef safely, and a guide on how to
directly write into existing scalars needs to be created. SvOK_off isn't
safe or public. sv_clear deletes the SV body. sv_replace wont take an
immortal.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2013

From @jandubois

On Fri, Feb 8, 2013 at 8​:21 AM, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

A guide on how to make a SV be undef safely, and a guide on how to
directly write into existing scalars needs to be created. SvOK_off isn't
safe or public. sv_clear deletes the SV body. sv_replace wont take an
immortal.

I think this should be safe everywhere​:

  sv_setsv(sv, NULL); /* same as sv_setsv(sv, &PL_sv_undef) */

Cheers,
-Jan

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2013

From @bulk88

On Fri Feb 08 10​:49​:41 2013, jdb wrote​:

I think this should be safe everywhere​:

sv\_setsv\(sv\, NULL\);  /\* same as sv\_setsv\(sv\, &PL\_sv\_undef\) \*/

Cheers,
-Jan

That works, but its a call, not a macro, with no chance to avoid the
call. Yes this is a feature request. Something like

SvPREPSET(sv, SVt_IV); /* upgrade and thinkfirst and offset off if
necessary */

or

SvPREPSETPV(sv, newbufferlen); /* unlike SvGROW this will undo COW always */

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 2, 2013

From @iabyn

On Thu, Feb 07, 2013 at 11​:51​:23AM +0000, Dave Mitchell wrote​:

On Wed, Feb 06, 2013 at 03​:07​:13PM +1100, sisyphus1@​optusnet.com.au wrote​:

So .... if an XS author then wants to try out his module with COW
enabled, but his perl was built with -DPERL_NO_COW, I take that he
first needs to obtain a perl that was built with -DPERL_COW.
Or is COW-enablement something that can be turned on from within the
XS author's code ?

A vanilla 5.18 build wont have COW enabled.
5.18 with -DPERL_COW used at perl build time will have COW.
5.18 will ignore -DPERL_NO_COW as this is the default anyway; only
5.17.7 and 5.17.8 recognise that switch.

In all cases it only affects how perl is built, and can't be enabled or
disabled later​: XS authors will have to specially build a COW-enabled perl
in order to test their code.

I've now committed 'disable COW by default' as
9f351b4

It's as I described, except that to build a COW-enabled perl, you'll need

  Configure -Accflags=-DPERL_NEW_COPY_ON_WRITE

I also haven't done anything with the XS docs about how to be COW-safe.

--
O Unicef Clearasil!
Gibberish and Drivel!
  -- "Bored of the Rings"

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From @sisyphus

-----Original Message-----
From​: Dave Mitchell

On Thu, Feb 07, 2013 at 11​:51​:23AM +0000, Dave Mitchell wrote​:

On Wed, Feb 06, 2013 at 03​:07​:13PM +1100, sisyphus1@​optusnet.com.au
wrote​:

Or is COW-enablement something that can be turned on from within the
XS author's code ?

A vanilla 5.18 build wont have COW enabled.
5.18 with -DPERL_COW used at perl build time will have COW.
5.18 will ignore -DPERL_NO_COW as this is the default anyway; only
5.17.7 and 5.17.8 recognise that switch.

In all cases it only affects how perl is built, and can't be enabled or
disabled later​: XS authors will have to specially build a COW-enabled
perl
in order to test their code.

I've now committed 'disable COW by default' as
9f351b4

Seems fine here - on Windows 7 (64-bit) I built both cow-enabled and
cow-disabled versions using current git.
No issues.

It's as I described, except that to build a COW-enabled perl, you'll need

Configure \-Accflags=\-DPERL\_NEW\_COPY\_ON\_WRITE

Using MinGW, I just specified -DPERL_NEW_COPY_ON_WRITE at the "EXTRACFLAGS"
entry in win32/makefile.mk.
That seems to get the job done.

I also haven't done anything with the XS docs about how to be COW-safe.

I look forward to seeing some documentation on this over the months leading
up to 5.20.

Thanks for the update Dave.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From @iabyn

I've now committed 'disable COW by default' as
9f351b4

... and broke most builds as a result.

Now fixed with

commit 6b89892
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Sun Mar 3 18​:30​:40 2013 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Sun Mar 3 18​:37​:16 2013 +0000

  ensure PL_sawampersand is exported.
 
  Commit 1a904fc disabled
  PL_sawampersand by default, since it was redundant with COW enabled.
  However the mechanism to optionally re-enable it (PERL_SAWAMPERSAND)
  didn't actually export the var, due to a typo.
 
  With the new default of disabling COW but enabling PL_sawampersand,
  this broke perl on builds where exports matters (and broke linux in
  non-threaded builds, where porting/globvar.t detects this).

Affected files ...
 
  M makedef.pl

Differences ...

Inline Patch
diff --git a/makedef.pl b/makedef.pl
index eefbbe4..bcfed24 100644
--- a/makedef.pl
+++ b/makedef.pl
@@ -279,7 +279,7 @@ unless ($define{'PERL_OLD_COPY_ON_WRITE'}
     ++$skip{Perl_sv_setsv_cow};
 }
 
-unless ($define{PERL_SAW_AMPERSAND}) {
+unless ($define{PERL_SAWAMPERSAND}) {
     ++$skip{PL_sawampersand};
 }
 


-- 

I before E. Except when it isn't.

@p5pRT
Copy link
Author

p5pRT commented Mar 4, 2013

From @rurban

On Sun, Mar 3, 2013 at 12​:40 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

I've now committed 'disable COW by default' as
9f351b4

Thanks. Now only XS docs are missing.
--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

@p5pRT
Copy link
Author

p5pRT commented Mar 4, 2013

From @ikegami

On Tue, Feb 5, 2013 at 1​:28 AM, <sisyphus1@​optusnet.com.au> wrote​:

I think the following Inline​::C demo avoids the char* typemap, yet still
displays the same behaviour.
(At least, removing the char* typemapping from lib/ExtUtils/typemap
doesn't make any dsifference.)

What happens if you have it use SvPV_force?

"You want force if you are going to update the SvPVX directly."

@p5pRT
Copy link
Author

p5pRT commented Mar 5, 2013

From @sisyphus

From​: Eric Brine

On Tue, Feb 5, 2013 at 1​:28 AM, <sisyphus1@​optusnet.com.au> wrote​:
I think the following Inline​::C demo avoids the char* typemap, yet still
displays the same behaviour.
(At least, removing the char* typemapping from lib/ExtUtils/typemap
doesn't make any dsifference.)

What happens if you have it use SvPV_force?

"You want force if you are going to update the SvPVX directly."

Yes, if I use SVPV_force() I get the output I was expecting​:

$orig = orig
$copy = new

With both SvPV() and SvPV_nolen() I get the output that I was *not*
expecting​:

$orig = new
$copy = new

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented May 20, 2013

From @jkeenan

My impression is that the discussion in this RT went all over the place.

Now that Perl 5.18.0 is out, could those who contributed to this ticket
review the discussion with an eye to​:

* Creating a new ticket for any well-scoped problem that appears with 5.18.

* Indicating whether we can close *this* ticket.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented May 20, 2013

From @sisyphus

-----Original Message-----
From​: James E Keenan via RT

Now that Perl 5.18.0 is out, could those who contributed to this ticket
review the discussion with an eye to​:

* Creating a new ticket for any well-scoped problem that appears with
5.18.

* Indicating whether we can close *this* ticket.

This ticket was created (by me) when COW was enabled by default in 5.17.
Shortly after, COW was disabled by default in 5.17 (and 5.18) - so the
ticket then became irrelevant wrt default builds of both 5.17 and 5.18.
However, perl 5.18 *can* be built COW-enabled if the build process is
configured accordingly.

I have no objection to the closing of this ticket ... and I think that would
be a reasonable thing to do (though I'm not entirely sure :-)

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented May 20, 2013

From @jkeenan

On Sun May 19 17​:55​:32 2013, sisyphus wrote​:

-----Original Message-----
From​: James E Keenan via RT

Now that Perl 5.18.0 is out, could those who contributed to this
ticket
review the discussion with an eye to​:

* Creating a new ticket for any well-scoped problem that appears
with
5.18.

* Indicating whether we can close *this* ticket.

This ticket was created (by me) when COW was enabled by default in
5.17.
Shortly after, COW was disabled by default in 5.17 (and 5.18) - so the
ticket then became irrelevant wrt default builds of both 5.17 and
5.18.
However, perl 5.18 *can* be built COW-enabled if the build process is
configured accordingly.

I have no objection to the closing of this ticket ... and I think that
would
be a reasonable thing to do (though I'm not entirely sure :-)

Cheers,
Rob

Thanks for getting back so quickly, Rob.

I'll take this ticket for the purpose of closing it in 7 days. If
anyone wants to keep it open, they can provide a reason and Take it from me.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented May 20, 2013

From @iabyn

On Mon, May 20, 2013 at 10​:54​:07AM +1000, sisyphus1@​optusnet.com.au wrote​:

-----Original Message----- From​: James E Keenan via RT

Now that Perl 5.18.0 is out, could those who contributed to this ticket
review the discussion with an eye to​:

* Creating a new ticket for any well-scoped problem that appears
with 5.18.

* Indicating whether we can close *this* ticket.

This ticket was created (by me) when COW was enabled by default in 5.17.
Shortly after, COW was disabled by default in 5.17 (and 5.18) - so
the ticket then became irrelevant wrt default builds of both 5.17
and 5.18. However, perl 5.18 *can* be built COW-enabled if the build
process is configured accordingly.

The plan (or at least my plan) is to re-enable COW by default shortly
after 5.19.0 is released (thus giving us a year to iron out issues and
allow CPAN authors to fix things up). Thus this ticket should be left
open.

--
The Enterprise is captured by a vastly superior alien intelligence which
does not put them on trial.
  -- Things That Never Happen in "Star Trek" #10

@p5pRT
Copy link
Author

p5pRT commented May 21, 2013

From @bulk88

On Mon May 20 04​:01​:31 2013, davem wrote​:

The plan (or at least my plan) is to re-enable COW by default shortly
after 5.19.0 is released (thus giving us a year to iron out issues and
allow CPAN authors to fix things up). Thus this ticket should be left
open.

+1

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 26, 2013

From @iabyn

On Mon, May 20, 2013 at 11​:24​:27AM +0100, Dave Mitchell wrote​:

The plan (or at least my plan) is to re-enable COW by default shortly
after 5.19.0 is released (thus giving us a year to iron out issues and
allow CPAN authors to fix things up). Thus this ticket should be left
open.

And now re-enabled in blead with 13b0f67.

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

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

No branches or pull requests

2 participants