Skip Menu |
 

This queue is for tickets about the Audio-FindChunks CPAN distribution.

Report information
The Basics
Id: 95272
Status: open
Priority: 0/
Queue: Audio-FindChunks

People
Owner: Nobody in particular
Requestors: SREZIC [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: 2.00
Fixed in: (no value)



Subject: Test fails with bleadperl
Download (untitled) / with headers
text/plain 273b
On most systems with perl 5.19.x the test suite is failing. See http://matrix.cpantesters.org/?dist=Audio-FindChunks for an overview. Maybe it's related to the malloc wrapper which is by default enabled for bleadperl, but this is just a gut feeling. Regards, Slaven
FTR, FindChunks did get a BBC, namely http://rt.perl.org/rt3/Ticket/Display.html?id=115910
CC: Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Fri, 9 May 2014 20:59:53 -0700
To: Andreas Koenig via RT <bug-Audio-FindChunks [...] rt.cpan.org>
From: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
Download (untitled) / with headers
text/plain 859b
[Cc'ed to p5p, I hope] On Fri, May 09, 2014 at 03:17:18PM -0400, Andreas Koenig via RT wrote: Show quoted text
> Queue: Audio-FindChunks > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=95272 > > > FTR, FindChunks did get a BBC, namely http://rt.perl.org/rt3/Ticket/Display.html?id=115910
Do not know what is a BBC… Is there a document summarizing what one is NOT supposed to do with the new COW scheme? The things possible with older Perls, but not with newer builds? (Based on the experience of “stitching” modules not working with newer Perls…) BTW, with the edit of such a breaking power, I would make it a flag to allocate **all** the PVs for SVs “unmanaged by Perl” (with SvLen == 0). Then a module (e.g., in PERL5OPTS) might have set this flag to 1 in CHECK time, and one could run the usual test suite in this environment… Ilya
CC: Andreas Koenig via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Sat, 10 May 2014 20:30:51 +0200
From: Andreas Koenig <andreas.koenig.7os6VVqR [...] franz.ak.mind.de>
Download (untitled) / with headers
text/plain 505b
Ilya Zakharevich <nospam-abuse@ilyaz.org> writes: Show quoted text
> Do not know what is a BBC…
Hey, Ilya, this nowadays quite indispensable[0] acronym stands for "Bleadperl Breaks CPAN", which is usually manifesting in a bugreport and the subject thereof contains the words 'Bleadperl' and 'breaks'. There's of course also the verb to BBC which denotes the act of writing a BBC. -- andreas [0] we hardly ever use it, it's just in the weeks before a new perl release that it sometimes slips into written documents.
CC: Andreas Koenig via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Sat, 10 May 2014 20:47:23 +0200
To: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 829b
On Sat, May 10, 2014 at 5:59 AM, Ilya Zakharevich <nospam-abuse@ilyaz.org>wrote: Show quoted text
> Is there a document summarizing what one is NOT supposed to do with > the new COW scheme? The things possible with older Perls, but not > with newer builds? (Based on the experience of “stitching” modules > not working with newer Perls…) >
The new perlguts has a section on this, but it I'm not sure it's advertised elsewhere. sv_force_normal and sv_force_normal_flags are your friends. Show quoted text
> BTW, with the edit of such a breaking power, I would make it a flag to > allocate **all** the PVs for SVs “unmanaged by Perl” (with SvLen == > 0). Then a module (e.g., in PERL5OPTS) might have set this flag to 1 > in CHECK time, and one could run the usual test suite in this > environment…
I'm not quite sure that you mean here. Leon
CC: Andreas Koenig via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Sat, 10 May 2014 12:39:16 -0700
To: Leon Timmermans <fawaka [...] gmail.com>
From: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
Download (untitled) / with headers
text/plain 2.1k
On Sat, May 10, 2014 at 08:47:23PM +0200, Leon Timmermans wrote: Show quoted text
> The new perlguts has a section on this, but it I'm not sure it's advertised > elsewhere. sv_force_normal and sv_force_normal_flags are your friends.
Aha, it is now easily findable via search.cpan.org, great! Will have a look at it… Show quoted text
> > BTW, with the edit of such a breaking power, I would make it a flag to > > allocate **all** the PVs for SVs “unmanaged by Perl” (with SvLen == > > 0). Then a module (e.g., in PERL5OPTS) might have set this flag to 1 > > in CHECK time, and one could run the usual test suite in this > > environment…
Show quoted text
> I'm not quite sure that you mean here.
Yeah, must be more explicit. I assume there is still a way to import a “foreign” memory region into a Perl string; years ago you would do it by setting SvLen to 0. This was a signal to Perl guts so that they should not freely relocate the region. The opposite of this is how Perl deals with the strings for which Perl allocates memory itself: one is free to realloc() it etc. Consider this as “a default” Perl strategy of allocation. I propose to make this strategy “a real default”, i.e., a choice of many (as opposed to a choice of one, as it is now — or at least was long ago ;-). Given that a certain flag is set, Perl would allocate new PVs for SVs using the “foreign memory” model (instead of “I can reallocate it” model). I do not see it useful in the world at large; however, it may be useful for testing purposes: • Suppose that Perl test suite would grant PERL5OPTS (I suspect that currently it disables PERL5OPTS); presume that it is PERLTEST5OPTS. • Suppose that there is a module which would raise the flag above at the CHECK{} time (why so late? just in case; I suspect a few “standard” SVPVs may need to be allocated with the default policy); call it Devel::ForeignMemory. • then env PERLTEST5OPTS=-MDevel::ForeignMemory=:set make test would test how Perl core is able to deal with the new allocation policy. I expect that some test would break even without COW enabled. But the “new” failures correlated with COW would indicate possible problems with COW. Hope this helps, Ilya
CC: Andreas Koenig via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Sun, 11 May 2014 21:18:51 +0200
To: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 2.4k
On Sat, May 10, 2014 at 9:39 PM, Ilya Zakharevich <nospam-abuse@ilyaz.org>wrote: Show quoted text
> > > BTW, with the edit of such a breaking power, I would make it a flag to > > > allocate **all** the PVs for SVs “unmanaged by Perl” (with SvLen == > > > 0). Then a module (e.g., in PERL5OPTS) might have set this flag to 1 > > > in CHECK time, and one could run the usual test suite in this > > > environment…
>
> > I'm not quite sure that you mean here.
> > Yeah, must be more explicit. I assume there is still a way to import > a “foreign” memory region into a Perl string; years ago you would do > it by setting SvLen to 0. This was a signal to Perl guts so that they > should not freely relocate the region. > > The opposite of this is how Perl deals with the strings for which Perl > allocates memory itself: one is free to realloc() it etc. Consider > this as “a default” Perl strategy of allocation. > > I propose to make this strategy “a real default”, i.e., a choice of > many (as opposed to a choice of one, as it is now — or at least was > long ago ;-). Given that a certain flag is set, Perl would allocate > new PVs for SVs using the “foreign memory” model (instead of “I can > reallocate it” model). > > I do not see it useful in the world at large; however, it may be > useful for testing purposes: > • Suppose that Perl test suite would grant PERL5OPTS (I suspect that > currently it disables PERL5OPTS); presume that it is > PERLTEST5OPTS. > • Suppose that there is a module which would raise the flag above at > the CHECK{} time (why so late? just in case; I suspect a few > “standard” SVPVs may need to be allocated with the default > policy); call it Devel::ForeignMemory. > • then > env PERLTEST5OPTS=-MDevel::ForeignMemory=:set make test > would test how Perl core is able to deal with the new allocation > policy. > > I expect that some test would break even without COW enabled. But the > “new” failures correlated with COW would indicate possible problems > with COW. >
I suspect the PERL_DEBUG_READONLY_COW flag mentioned in perlguts is close enough what you want, though with a completely different implementation: It uses mmap + mprotect to turn logical violations of COW into segfaults. It could probably even be made to fix things up for you in a SEGV handler, but that would be insane by most people's standards ;-) (and too slow to be usable anyway) Leon
Download (untitled) / with headers
text/plain 995b
On Fri May 02 08:01:53 2014, SREZIC wrote: Show quoted text
> On most systems with perl 5.19.x the test suite is failing. See > http://matrix.cpantesters.org/?dist=Audio-FindChunks for an overview. > > Maybe it's related to the malloc wrapper which is by default enabled > for bleadperl, but this is just a gut feeling.
The problem is that the code writes over its arguments without ensuring they're not COWed, so for example: above_thres_in_window => [sub { my $a = shift; my @r = $a->[0]; # Reserve space int_sum_window($a->[0], $r[0], shift, shift); \@r}, 'above_thres', 'chunks', 'above_thres_window'] copies $a->[0] into $r[0], which with COW means that both $a->[0] and $r[0] share the same buffer, and when int_sum_window() writes to $r[0]'s buffer, it ends up writing over $a->[0] as well (and any other SVs that share the same buffer. I've attached a patch that ensures that each output SV is not using a shared buffer, though it makes the XS a lot more verbose unfortunately. Tony
Subject: FindChunks.diff
Download FindChunks.diff
text/x-diff 2.6k
diff -ru Audio-FindChunks-2.00-orig/FindChunks.xs Audio-FindChunks-2.00/FindChunks.xs --- Audio-FindChunks-2.00-orig/FindChunks.xs 2008-04-08 19:58:00.000000000 +1000 +++ Audio-FindChunks-2.00/FindChunks.xs 2014-05-14 11:36:37.000000000 +1000 @@ -15,30 +15,55 @@ PROTOTYPES: ENABLE long -bool_find_runs(input, output, cnt, out_cnt) +bool_find_runs(input, output_sv, cnt, out_cnt) int * input - array_run_t * output + SV * output_sv long cnt long out_cnt - + PREINIT: + array_run_t * output = (array_run_t *)SvPV_force_nolen(output_sv); + CODE: + RETVAL = bool_find_runs(input, output, cnt, out_cnt); + OUTPUT: + RETVAL + output_sv + void -double_find_above(input, output, cnt, threshold) +double_find_above(input, output_sv, cnt, threshold) double * input - int * output + SV * output_sv long cnt double threshold + PREINIT: + int * output = (int *)SvPV_force_nolen(output_sv); + CODE: + double_find_above(input, output, cnt, threshold); + OUTPUT: + output_sv void -double_median3(rmsarray, medarray, total_blocks) +double_median3(rmsarray, medarray_sv, total_blocks) double * rmsarray - double * medarray + SV * medarray_sv long total_blocks + PREINIT: + double * medarray = (double *)SvPV_force_nolen(medarray_sv); + CODE: + double_median3(rmsarray, medarray, total_blocks); + OUTPUT: + medarray_sv void -double_sort(input, output, cnt) +double_sort(input, output_sv, cnt) double * input - double * output + SV * output_sv long cnt + PREINIT: + double * output = (double *)SvPV_force_nolen(output_sv); + CODE: + double_sort(input, output, cnt); + OUTPUT: + output_sv double double_sum(input, off, cnt) @@ -47,25 +72,43 @@ long cnt void -int_find_above(input, output, cnt, threshold) +int_find_above(input, output_sv, cnt, threshold) int * input - int * output + SV * output_sv long cnt int threshold + PREINIT: + int * output = (int *)SvPV_force_nolen(output_sv); + CODE: + int_find_above(input, output, cnt, threshold); + OUTPUT: + output_sv void -int_sum_window(input, output, cnt, window_size) +int_sum_window(input, output_sv, cnt, window_size) int * input - int * output + SV * output_sv long cnt int window_size + PREINIT: + int * output = (int *)SvPV_force_nolen(output_sv); + CODE: + int_sum_window(input, output, cnt, window_size); + OUTPUT: + output_sv void -le_short_sample_stats(buf, stride, samples, stat) +le_short_sample_stats(buf, stride, samples, stat_sv) void_char * buf int stride long samples - array_stats_t * stat + SV * stat_sv + PREINIT: + array_stats_t * stat = (array_stats_t *)SvPV_force_nolen(stat_sv); + CODE: + le_short_sample_stats(buf, stride, samples, stat); + OUTPUT: + stat_sv int _s_size()
CC: Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Thu, 29 May 2014 00:29:10 -0700
To: TONYC via RT <bug-Audio-FindChunks [...] rt.cpan.org>
From: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
Download (untitled) / with headers
text/plain 2.1k
On Tue, May 13, 2014 at 09:44:06PM -0400, TONYC via RT wrote: Show quoted text
> The problem is that the code writes over its arguments without ensuring they're not COWed, so for example: > > above_thres_in_window => [sub { my $a = shift; my @r = $a->[0]; # Reserve space > int_sum_window($a->[0], $r[0], shift, shift); > \@r}, 'above_thres', 'chunks', 'above_thres_window'] > > copies $a->[0] into $r[0], which with COW means that both $a->[0] and $r[0] share the same buffer, and when int_sum_window() writes to $r[0]'s buffer, it ends up writing over $a->[0] as well (and any other SVs that share the same buffer.
Note that int_sum_window() does not access SV in any way. The buffer is given to it by typemap’s entry for int * which is assigned to T_PV (in the ./typemap file of the distribution). Do you say that T_PV is now restricted for read-only access only? If so, where is it documented? And what is a reason for such an incompatible change? And which typemap entry should one use for R/W access? I’m pretty sure that many CPAN modules use T_PV for R/W (and for every one where it is detected by the test suite, there are 20 where it is not)… Show quoted text
> I've attached a patch that ensures that each output SV is not using a shared buffer, though it makes the XS a lot more verbose unfortunately. > > Tony
Show quoted text
> diff -ru Audio-FindChunks-2.00-orig/FindChunks.xs Audio-FindChunks-2.00/FindChunks.xs > --- Audio-FindChunks-2.00-orig/FindChunks.xs 2008-04-08 19:58:00.000000000 +1000 > +++ Audio-FindChunks-2.00/FindChunks.xs 2014-05-14 11:36:37.000000000 +1000 > @@ -15,30 +15,55 @@ > PROTOTYPES: ENABLE > > long > -bool_find_runs(input, output, cnt, out_cnt) > +bool_find_runs(input, output_sv, cnt, out_cnt) > int * input > - array_run_t * output > + SV * output_sv > long cnt > long out_cnt > - > + PREINIT: > + array_run_t * output = (array_run_t *)SvPV_force_nolen(output_sv); > + CODE: > + RETVAL = bool_find_runs(input, output, cnt, out_cnt); > + OUTPUT: > + RETVAL > + output_sv > +
I see no reason why this should be done in XS. It should have been done in the typemap — preferably, the Perl’s typemap, which knows the idiosyncrasies of the current version of Perl. Thanks, Ilya
CC: TONYC via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Fri, 30 May 2014 10:23:03 +1000
To: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
From: Tony Cook <tony [...] develop-help.com>
Download (untitled) / with headers
text/plain 2.8k
On Thu, May 29, 2014 at 12:29:10AM -0700, Ilya Zakharevich wrote: Show quoted text
> On Tue, May 13, 2014 at 09:44:06PM -0400, TONYC via RT wrote:
> > The problem is that the code writes over its arguments without ensuring they're not COWed, so for example: > > > > above_thres_in_window => [sub { my $a = shift; my @r = $a->[0]; # Reserve space > > int_sum_window($a->[0], $r[0], shift, shift); > > \@r}, 'above_thres', 'chunks', 'above_thres_window'] > > > > copies $a->[0] into $r[0], which with COW means that both $a->[0] and $r[0] share the same buffer, and when int_sum_window() writes to $r[0]'s buffer, it ends up writing over $a->[0] as well (and any other SVs that share the same buffer.
> > Note that int_sum_window() does not access SV in any way. The buffer > is given to it by typemap’s entry for > int * > which is assigned to T_PV (in the ./typemap file of the distribution). > > Do you say that T_PV is now restricted for read-only access only? If > so, where is it documented? And what is a reason for such an > incompatible change? And which typemap entry should one use for R/W > access? > > I’m pretty sure that many CPAN modules use T_PV for R/W (and for every > one where it is detected by the test suite, there are 20 where it is > not)…
Using T_PV for write access has never been safe, consider what happens if the caller supplies a reference, or undef, or a "" overloaded value. If T_PV were modified to allowing writing to the value, it would need to use SvPV_force(), which modifies non-PV parameters, changing the behaviour of any code that uses T_PV parameters for read-only. Show quoted text
> > I've attached a patch that ensures that each output SV is not using a shared buffer, though it makes the XS a lot more verbose unfortunately. > > > > Tony
>
> > diff -ru Audio-FindChunks-2.00-orig/FindChunks.xs Audio-FindChunks-2.00/FindChunks.xs > > --- Audio-FindChunks-2.00-orig/FindChunks.xs 2008-04-08 19:58:00.000000000 +1000 > > +++ Audio-FindChunks-2.00/FindChunks.xs 2014-05-14 11:36:37.000000000 +1000 > > @@ -15,30 +15,55 @@ > > PROTOTYPES: ENABLE > > > > long > > -bool_find_runs(input, output, cnt, out_cnt) > > +bool_find_runs(input, output_sv, cnt, out_cnt) > > int * input > > - array_run_t * output > > + SV * output_sv > > long cnt > > long out_cnt > > - > > + PREINIT: > > + array_run_t * output = (array_run_t *)SvPV_force_nolen(output_sv); > > + CODE: > > + RETVAL = bool_find_runs(input, output, cnt, out_cnt); > > + OUTPUT: > > + RETVAL > > + output_sv > > +
> > I see no reason why this should be done in XS. It should have been > done in the typemap — preferably, the Perl’s typemap, which knows the > idiosyncrasies of the current version of Perl.
It could be done with a new typemap entry, and it was discussed, but to be safe the typemap entry would need to know the output size, which would mean a custom typemap. Tony
CC: TONYC via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Fri, 30 May 2014 06:17:59 -0700
To: Tony Cook <tony [...] develop-help.com>
From: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
Download (untitled) / with headers
text/plain 3.5k
On Fri, May 30, 2014 at 10:23:03AM +1000, Tony Cook wrote: Show quoted text
> > Note that int_sum_window() does not access SV in any way. The buffer > > is given to it by typemap’s entry for > > int * > > which is assigned to T_PV (in the ./typemap file of the distribution). > > > > Do you say that T_PV is now restricted for read-only access only? If > > so, where is it documented? And what is a reason for such an > > incompatible change? And which typemap entry should one use for R/W > > access? > > > > I’m pretty sure that many CPAN modules use T_PV for R/W (and for every > > one where it is detected by the test suite, there are 20 where it is > > not)…
> > Using T_PV for write access has never been safe, consider what happens > if the caller supplies a reference, or undef, or a "" overloaded value.
I do not see any problem there, except “So do not do this then”. There IS a value in code which may be used only in controlled environment. Show quoted text
> If T_PV were modified to allowing writing to the value,
^^ There is no “if” here. T_PV DOES allow writing to the value — or at least WAS allowing it before 5.19 (provided PV is there). For backward compatibility, it is very important that it continues to do so. Show quoted text
> it would need to use SvPV_force(), which modifies non-PV parameters, > changing the behaviour of any code that uses T_PV parameters for > read-only.
This is wrong on so many counts, that I do not even know where to start… First of all, you write that it cannot use SvPV_force() without breaking backward compatibility. So why do you even mention SvPV_force()? AFAIU, __ALL__ the T_PV code should do is to disable COW state if it is present. This way, the damage done by upgrading to 5.19 is fixed. ----------------------------- BTW, I think there must be a way to make it easy for an XSUB to specify that it uses T_PV in read-only mode. Something like #define T_PV_IS_READONLY 1 near the start of XSUB. Then T_PV-expansion could check for this, and generate code WITHOUT unlinking COW. This way: • extension may use T_PV, so be backward-compatible and future-proof; • extension may use benefits of COW. Show quoted text
> > > I've attached a patch that ensures that each output SV is not using a shared buffer, though it makes the XS a lot more verbose unfortunately. > > > > > > Tony
> >
> > > diff -ru Audio-FindChunks-2.00-orig/FindChunks.xs Audio-FindChunks-2.00/FindChunks.xs > > > --- Audio-FindChunks-2.00-orig/FindChunks.xs 2008-04-08 19:58:00.000000000 +1000 > > > +++ Audio-FindChunks-2.00/FindChunks.xs 2014-05-14 11:36:37.000000000 +1000 > > > @@ -15,30 +15,55 @@ > > > PROTOTYPES: ENABLE > > > > > > long > > > -bool_find_runs(input, output, cnt, out_cnt) > > > +bool_find_runs(input, output_sv, cnt, out_cnt) > > > int * input > > > - array_run_t * output > > > + SV * output_sv > > > long cnt > > > long out_cnt > > > - > > > + PREINIT: > > > + array_run_t * output = (array_run_t *)SvPV_force_nolen(output_sv); > > > + CODE: > > > + RETVAL = bool_find_runs(input, output, cnt, out_cnt); > > > + OUTPUT: > > > + RETVAL > > > + output_sv > > > +
> > > > I see no reason why this should be done in XS. It should have been > > done in the typemap — preferably, the Perl’s typemap, which knows the > > idiosyncrasies of the current version of Perl.
> > It could be done with a new typemap entry, and it was discussed, but > to be safe the typemap entry would need to know the output size, which > would mean a custom typemap.
It DID NOT need to know the output size before. So it should not need it now… Yours, Ilya
CC: Tony Cook <tony [...] develop-help.com>, TONYC via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>, Ricardo Signes <rjbs [...] manxome.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Sat, 31 May 2014 11:50:17 +0200
To: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
From: demerphq <demerphq [...] gmail.com>
Download (untitled) / with headers
text/plain 4.4k
On Friday, 30 May 2014, Ilya Zakharevich <nospam-abuse@ilyaz.org> wrote: Show quoted text
> On Fri, May 30, 2014 at 10:23:03AM +1000, Tony Cook wrote:
>> > Note that int_sum_window() does not access SV in any way. The buffer >> > is given to it by typemap’s entry for >> > int * >> > which is assigned to T_PV (in the ./typemap file of the distribution). >> > >> > Do you say that T_PV is now restricted for read-only access only? If >> > so, where is it documented? And what is a reason for such an >> > incompatible change? And which typemap entry should one use for R/W >> > access? >> > >> > I’m pretty sure that many CPAN modules use T_PV for R/W (and for every >> > one where it is detected by the test suite, there are 20 where it is >> > not)…
>> >> Using T_PV for write access has never been safe, consider what happens >> if the caller supplies a reference, or undef, or a "" overloaded value.
> > I do not see any problem there, except > “So do not do this then”. > There IS a value in code which may be used only in controlled environment. >
>> If T_PV were modified to allowing writing to the value,
> ^^ > > There is no “if” here. T_PV DOES allow writing to the value — or at > least WAS allowing it before 5.19 (provided PV is there). For backward > compatibility, it is very important that it continues to do so. >
>> it would need to use SvPV_force(), which modifies non-PV parameters, >> changing the behaviour of any code that uses T_PV parameters for >> read-only.
> > This is wrong on so many counts, that I do not even know where to start… > First of all, you write that it cannot use SvPV_force() without > breaking backward compatibility. So why do you even mention SvPV_force()? > > AFAIU, __ALL__ the T_PV code should do is to disable COW state if it > is present. This way, the damage done by upgrading to 5.19 is fixed.
I think ilya is right here. We broke back compat and instead of admitting it we hide behind unconvincing language lawyering to try to pretend we didnt. This is exactly why people complain we don't care about backward compatibility. Show quoted text
> ----------------------------- > > BTW, I think there must be a way to make it easy for an XSUB to > specify that it uses T_PV in read-only mode. Something like > > #define T_PV_IS_READONLY 1 > > near the start of XSUB. Then T_PV-expansion could check for this, > and generate code WITHOUT unlinking COW. > > This way: > • extension may use T_PV, so be backward-compatible and future-proof; > • extension may use benefits of COW. >
>> > > I've attached a patch that ensures that each output SV is not using
a shared buffer, though it makes the XS a lot more verbose unfortunately. Show quoted text
>> > > >> > > Tony
>> >
>> > > diff -ru Audio-FindChunks-2.00-orig/FindChunks.xs
Audio-FindChunks-2.00/FindChunks.xs Show quoted text
>> > > --- Audio-FindChunks-2.00-orig/FindChunks.xs 2008-04-08
19:58:00.000000000 +1000 Show quoted text
>> > > +++ Audio-FindChunks-2.00/FindChunks.xs 2014-05-14
11:36:37.000000000 +1000 Show quoted text
>> > > @@ -15,30 +15,55 @@ >> > > PROTOTYPES: ENABLE >> > > >> > > long >> > > -bool_find_runs(input, output, cnt, out_cnt) >> > > +bool_find_runs(input, output_sv, cnt, out_cnt) >> > > int * input >> > > - array_run_t * output >> > > + SV * output_sv >> > > long cnt >> > > long out_cnt >> > > - >> > > + PREINIT: >> > > + array_run_t * output = (array_run_t
*)SvPV_force_nolen(output_sv); Show quoted text
>> > > + CODE: >> > > + RETVAL = bool_find_runs(input, output, cnt, out_cnt); >> > > + OUTPUT: >> > > + RETVAL >> > > + output_sv >> > > +
>> > >> > I see no reason why this should be done in XS. It should have been >> > done in the typemap — preferably, the Perl’s typemap, which knows the >> > idiosyncrasies of the current version of Perl.
>> >> It could be done with a new typemap entry, and it was discussed, but >> to be safe the typemap entry would need to know the output size, which >> would mean a custom typemap.
> > It DID NOT need to know the output size before. So it should not need > it now…
Again i think ilya is right. We broke back compat and we did it in a dangerous way (changing the meaning of the RO flag) and then we blamed xs authors for "doing it wrong in the first place" while completely ignoring there was no documented wrong or right for them to follow. Specifically I challenge anyone arguing this to find a single doc that says that svpv args to xs are readonly. Dont waste too much time onit as it doesnt exist. Show quoted text
> Yours, > Ilya >
-- perl -Mre=debug -e "/just|another|perl|hacker/"
CC: Tony Cook <tony [...] develop-help.com>, TONYC via RT <bug-Audio-FindChunks [...] rt.cpan.org>, Mailing list Perl5 <perl5-porters [...] perl.org>, Ricardo Signes <rjbs [...] manxome.org>
Subject: Re: [rt.cpan.org #95272] Test fails with bleadperl
Date: Sun, 1 Jun 2014 13:52:10 -0700
To: demerphq <demerphq [...] gmail.com>
From: Ilya Zakharevich <nospam-abuse [...] ilyaz.org>
Download (untitled) / with headers
text/plain 4.6k
On Sat, May 31, 2014 at 11:50:17AM +0200, demerphq wrote: Show quoted text
> >> > I see no reason why this should be done in XS. It should have been > >> > done in the typemap — preferably, the Perl’s typemap, which knows the > >> > idiosyncrasies of the current version of Perl.
> >> > >> It could be done with a new typemap entry, and it was discussed, but > >> to be safe the typemap entry would need to know the output size, which > >> would mean a custom typemap.
> > > > It DID NOT need to know the output size before. So it should not need > > it now…
> > Again i think ilya is right. We broke back compat and we did it in a > dangerous way (changing the meaning of the RO flag) and then we blamed xs > authors for "doing it wrong in the first place" while completely ignoring > there was no documented wrong or right for them to follow. Specifically I > challenge anyone arguing this to find a single doc that says that svpv args > to xs are readonly. Dont waste too much time onit as it doesnt exist.
I want to make a couple of remarks. • First, I do not want to be as legalistic as that. Perl is (uniquely) gifted to have a wonderfully useful documentation — so useful, that one can utilize it standalone, without side communication channels (Usenet, books, googling, etc). However, one should not forget that most other software packages are not up to this standard — so people ARE used to be needing side communication channels. And on such channels, my usage of T_PV ***would*** be frowned upon. So in some ways, I was more or less consciously going against the flow by literally following “what is possible to do” (and maybe it is my usage of these non-features which FORCED them not being documented as non-supported!). In short: the fact that the documentation does not prohibit this usage should be taken with a certain grain of salt. • Why go against the flow? I believe that it IS crucial to have quick-and-dirty ways to program XSUBs “the C way”. Use PVs as windows-into-computer-memory etc. On one hand, it is important as a (semi-)automatic way to import C libraries. On the other hand, it is also important standalone: for Audio::FindChunks, I did not start with a C library; it is a complete Perl+XSUB rewrite of (an undocumented) C code. Doing this “in a protective, Perl-core-like way” would not only increase the development time dramatically; it would also be completely useless since the chance that these XSUBs are going to used by anything but the companion Perl subs is close to nil. And I should not explain why moving the “protect from misuses” logic from XSUB into Perl decreases complexity a lot. Writing XSUBs in a Perl-core-way, when it is impossible to crash the interpreter no matter what you through at these XSUBs ***has*** its advantages. My point is that these advantages are not universal, and in many (most?) situations such level of “correctness” is excessive. • After the patch by TonyC arrived, for me it would be much easier to patch the distribution than to continue this discussion. I had chosen to continue since I think the current p5p approach to the problem is very wrong. I was very lucky with Audio::FindChunks so that in a few hours, I could write a test suite which tests the functionality more or less completely. For a lot of my modules, the test suite is an empty shell: it basically checks that the module loads, and nothing else. And the reason for this is that writing a test suite is out of question: the mode of usage and amount of dependencies on the environment make the task of automating the testing unfeasible. I would not be surprised if the ratio of well-auto-tested/not-auto-tested-at-all modules on CPAN were 1/20. Taking into account that the failures resulting from COW-not-being-triggered are ABSOLUTELY impossible to debug (unless you know in advance that they are due to COW), this would put the developers into a quite an ugly corner. In addition, on CPAN the failures may be happening on remote machines only, the people observing the failures may have communication problems etc.: this hurdles make it very hard to debug even the stuff which would be fixed immediately when it happens locally. In short: COW-failures + CPAN could make any developer suicidal. One should cut every corner trying to bend in direction of simplifying life of CPAN developers in this regard. Thanks for your patience, Ilya


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.