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
Comments
From @sisyphus
It's still the same behaviour in 5.17.8. Again, here's a simple demo of how things have changed between 5.16 and #################################### #use Inline C => Config => use Inline C => <<'EOC'; SV * foo(char * buffer) { EOC my $buff = 'dog'; foo($buf); print "\$buf: $buf\n"; On 5.17.7 and 5.17.8 that outputs $buf: dog On 5.16 and earlier it outputs $buf: dog For some (insane) reason an XS sub that changes the value of $buf, also Cheers, |
From @bulk88On Mon Jan 28 23:14:57 2013, sisyphus wrote:
No SvTHINKFIRST. Almost a buffer overflow too. No sv_grow. Didn't update Note that for a potential fix, const char * and char * are different. This is COW related. -- |
The RT System itself - Status changed from 'new' to 'open' |
From @sisyphus
It overwites the string 'dog' with the string'cow'. Why is any of that
Yeah - but, irrespective of which farmyard animal dreamt it up, is this the Cheers, |
From @LeontOn Tue, Jan 29, 2013 at 11:47 AM, <sisyphus1@optusnet.com.au> wrote:
Because we already had COW, we're just using it in more places than *Your code is broken today*. This change makes it more obvious, but
I'm not sure what for it would be more necessary than that.
The reality that a lot of code on CPAN is broken in this way is Leon |
From @sisyphus-----Original Message-----
Does this mean that the same code is going to behave in the same way when Or should I just wait and see what 5.18 delivers before I do anything ?
Where is this THINKFIRST stuff documented ? (I know nothing about it - In terms of my own code on CPAN that's broken, it's not such a big deal I know I have one test script where the problem goes away if I change some $copy = $original; When I make that change, $original no longer gets ovewrwritten by the XSub Still, it would be nice to know how to achieve the same result by modifying Say my XSub looks like: SV * foo(char *buffer) { Given that I already know that "buffer" will accommodate the changes that My main objection to the current behaviour is on grounds of dwimmery and $orig = 'some string'; then I find it unacceptable that $orig should be altered, irrespective of Cheers, |
From @LeontOn Tue, Jan 29, 2013 at 2:22 PM, <sisyphus1@optusnet.com.au> wrote:
I wouldn't expect it to change, but I wouldn't rule it out either.
Probably it isn't. A lot of stuff isn't. Welcome to core.
That would be entirely reasonable.
I don't know how Inline::C works exactly, but I'm guessing the
I agree with that on a high level, but if foo is an XSUB then foo Leon |
From @doyOn Tue, Jan 29, 2013 at 04:54:16PM +0100, Leon Timmermans wrote:
Yes - I think the answer to the underlying question here is that the -doy |
From @tseeOn 01/29/2013 05:01 PM, Jesse Luehrs wrote:
Which is of some concern because they can't travel back in time. In --Steffen |
From @sisyphus-----Original Message-----
Ok - if it's just a problem with the char* typemap, that's not such a big I'll just avoid passing char* arguments - or, at least, proceed with caution Thanks guys - Jesse, Leon, Steffen, bulk88. Cheers, |
From @iabynOn Tue, Jan 29, 2013 at 04:54:16PM +0100, Leon Timmermans wrote:
There has been some talk of disabling the COW changes for the 5.18 This ticket I think counts as another +1 to disablement. -- |
From @LeontOn Wed, Jan 30, 2013 at 8:24 PM, Dave Mitchell <davem@iabyn.com> wrote:
I interpreted the previous talk as being about removing it from the Leon |
From @sisyphus-----Original Message-----
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 ######################### #use Inline C => Config => use Inline C => <<'EOC'; void foo (SV * x) { EOC my $orig = "orig"; foo($copy); On 5.16, this script outputs: $orig = orig On 5.17.8 it outputs: $orig = new Cheers, |
From @doyOn Tue, Feb 05, 2013 at 05:28:16PM +1100, sisyphus1@optusnet.com.au wrote:
This part is not a bug. Using the SV* typemap will bypass any of the -doy |
From @sisyphus-----Original Message-----
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) { then, for both 5.16 and 5.17.8, $copy gets changed to 'new' but $orig Cheers, |
From @doyOn Tue, Feb 05, 2013 at 07:48:20PM +1100, sisyphus1@optusnet.com.au wrote:
Yes, if there is no reason that you actually need to use the existing -doy |
From @iabynOn Wed, Jan 30, 2013 at 08:56:52PM +0100, Leon Timmermans wrote:
Quite possibly.
Yeah I think so, with PERL_SAWAMPERSAND. However, later on in this thread we seem to be telling XS authors that If this is actually the case, then I think it would be wrong to release I propose that in the next week or two, we turn it off by default, Then as soon as 5.18 is released, we enable in blead and work on fixing -- |
From @rjbs* Dave Mitchell <davem@iabyn.com> [2013-02-05T09:53:23]
I am in favor of this. -- |
From @LeontOn Tue, Feb 5, 2013 at 3:53 PM, Dave Mitchell <davem@iabyn.com> wrote:
Sounds reasonable to me Leon |
From @khwilliamsonOn 02/05/2013 07:45 PM, Leon Timmermans wrote:
+1 |
From @sisyphusOn 02/05/2013 07:45 PM, Leon Timmermans wrote:
I think that's a better approach, too. So .... if an XS author then wants to try out his module with COW enabled, Cheers, |
From @demerphqOn 6 February 2013 00:54, Ricardo Signes <perl.p5p@rjbs.manxome.org> wrote:
Note we need a proper documentation of SvTHINKFIRST and related macros. Also, I wonder if this bites us anywhere in the core? Since Cheers, |
From @LeontOn Wed, Feb 6, 2013 at 7:12 AM, demerphq <demerphq@gmail.com> wrote:
I think the usual functions to manipulate SV's with (sv_set[ps]v, Leon |
From @iabynOn Wed, Feb 06, 2013 at 03:07:13PM +1100, sisyphus1@optusnet.com.au wrote:
A vanilla 5.18 build wont have COW enabled. In all cases it only affects how perl is built, and can't be enabled or -- |
From @sisyphus-----Original Message-----
Thanks for confirming, Dave. (That's as I expected.) I envisage that a fair proportion of cpan authors rely on using the version I, for one, will be using COW-enabled. Are COW-enabled and COW-disabled Looking forward to seeing what 5.17.9 brings !! Cheers, |
From @steve-m-haydemerphq wrote on 2013-02-06:
SvTHINKFIRST is documented, but only in perlintern.pod (from the source Adding an 'A' to the flags in the "=for apidoc" line in sv.h moves the Inline Patchdiff --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) &=
/* A quick flag check to see whether an sv should be passed to |
From @bulk88On Fri Feb 08 06:01:48 2013, Steve.Hay@verosoftware.com wrote:
A guide on how to make a SV be undef safely, and a guide on how to -- |
From @janduboisOn Fri, Feb 8, 2013 at 8:21 AM, bulk88 via RT <perlbug-followup@perl.org> wrote:
I think this should be safe everywhere: sv_setsv(sv, NULL); /* same as sv_setsv(sv, &PL_sv_undef) */ Cheers, |
From @bulk88On Fri Feb 08 10:49:41 2013, jdb wrote:
That works, but its a call, not a macro, with no chance to avoid the SvPREPSET(sv, SVt_IV); /* upgrade and thinkfirst and offset off if or SvPREPSETPV(sv, newbufferlen); /* unlike SvGROW this will undo COW always */ -- |
From @iabynOn Thu, Feb 07, 2013 at 11:51:23AM +0000, Dave Mitchell wrote:
I've now committed 'disable COW by default' as 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. -- |
From @sisyphus-----Original Message-----
Seems fine here - on Windows 7 (64-bit) I built both cow-enabled and
Using MinGW, I just specified -DPERL_NEW_COPY_ON_WRITE at the "EXTRACFLAGS"
I look forward to seeing some documentation on this over the months leading Thanks for the update Dave. Cheers, |
From @iabyn
... and broke most builds as a result. Now fixed with commit 6b89892 ensure PL_sawampersand is exported. Affected files ... Differences ... Inline Patchdiff --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. |
From @ikegamiOn Tue, Feb 5, 2013 at 1:28 AM, <sisyphus1@optusnet.com.au> wrote:
What happens if you have it use SvPV_force? "You want force if you are going to update the SvPVX directly." |
From @sisyphusFrom: Eric Brine
Yes, if I use SVPV_force() I get the output I was expecting: $orig = orig With both SvPV() and SvPV_nolen() I get the output that I was *not* $orig = new Cheers, |
From @jkeenanMy 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 * 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. |
From @sisyphus-----Original Message-----
This ticket was created (by me) when COW was enabled by default in 5.17. I have no objection to the closing of this ticket ... and I think that would Cheers, |
From @jkeenanOn Sun May 19 17:55:32 2013, sisyphus wrote:
Thanks for getting back so quickly, Rob. I'll take this ticket for the purpose of closing it in 7 days. If Thank you very much. |
From @iabynOn Mon, May 20, 2013 at 10:54:07AM +1000, sisyphus1@optusnet.com.au wrote:
The plan (or at least my plan) is to re-enable COW by default shortly -- |
From @bulk88On Mon May 20 04:01:31 2013, davem wrote:
+1 -- |
From @iabynOn Mon, May 20, 2013 at 11:24:27AM +0100, Dave Mitchell wrote:
And now re-enabled in blead with 13b0f67. -- |
Migrated from rt.perl.org#116569 (status was 'open')
Searchable as RT116569$
The text was updated successfully, but these errors were encountered: