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

Blead Breaks CPAN: Class::Std #16436

Closed
p5pRT opened this issue Feb 24, 2018 · 46 comments
Closed

Blead Breaks CPAN: Class::Std #16436

p5pRT opened this issue Feb 24, 2018 · 46 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 24, 2018

Migrated from rt.perl.org#132902 (status was 'resolved')

Searchable as RT132902$

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From carlos@carlosguevara.com

It looks like blead broke Class​::Std. The install now hangs with CPU
utilization at 100% at this point​:
#####
# Testing Class​::Std 0.013
t/00.load.t ............ ok
t/access.t ............. ok
Deep recursion on anonymous subroutine at
/home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line
572.
Deep recursion on subroutine "Carp​::croak" at
/home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line
290.
Deep recursion on subroutine "Carp​::shortmess" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 196.
Deep recursion on subroutine "Carp​::shortmess_heavy" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 193.
Deep recursion on subroutine "Carp​::ret_summary" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 589.
Deep recursion on subroutine "Carp​::caller_info" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 543.
Deep recursion on subroutine "Carp​::format_arg" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 276.
Deep recursion on anonymous subroutine at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 344.
#####

Completed install after killing the hung process​:
http​://www.cpantesters.org/cpan/report/c9ae98e0-18ca-11e8-9afc-de9a541938d7

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From @demerphq

On 24 Feb 2018 09​:46, "Carlos Guevara" <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by Carlos Guevara
# Please include the string​: [perl #132902]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132902 >

It looks like blead broke Class​::Std. The install now hangs with CPU
utilization at 100% at this point​:
#####
# Testing Class​::Std 0.013
t/00.load.t ............ ok
t/access.t ............. ok
Deep recursion on anonymous subroutine at
/home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line
572.
Deep recursion on subroutine "Carp​::croak" at
/home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line
290.
Deep recursion on subroutine "Carp​::shortmess" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 196.
Deep recursion on subroutine "Carp​::shortmess_heavy" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 193.
Deep recursion on subroutine "Carp​::ret_summary" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 589.
Deep recursion on subroutine "Carp​::caller_info" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 543.
Deep recursion on subroutine "Carp​::format_arg" at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 276.
Deep recursion on anonymous subroutine at
/home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 344.
#####

Completed install after killing the hung process​:
http​://www.cpantesters.org/cpan/report/c9ae98e0-18ca-11e8-9afc-de9a541938d7

Thanks, this looks like fallout from a fix I pushed yesterday. I will
investigate and fix.

Yves

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From zefram@fysh.org

demerphq wrote​:

Thanks, this looks like fallout from a fix I pushed yesterday.

Confirmed​: it bisects to commit c99363a
"fix Perl #132828 - dont use overload to bypass overloads". The relevant
part of Class​::Std is overriding UNIVERSAL​::can.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From @demerphq

On 24 February 2018 at 03​:44, Zefram <zefram@​fysh.org> wrote​:

demerphq wrote​:

Thanks, this looks like fallout from a fix I pushed yesterday.

Confirmed​: it bisects to commit c99363a
"fix Perl #132828 - dont use overload to bypass overloads". The relevant
part of Class​::Std is overriding UNIVERSAL​::can.

Which means its Class​::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From @demerphq

On 24 February 2018 at 03​:46, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:44, Zefram <zefram@​fysh.org> wrote​:

demerphq wrote​:

Thanks, this looks like fallout from a fix I pushed yesterday.

Confirmed​: it bisects to commit c99363a
"fix Perl #132828 - dont use overload to bypass overloads". The relevant
part of Class​::Std is overriding UNIVERSAL​::can.

Which means its Class​::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class​::Std to fix this.

chorny/Class-Std#2

I also will push a patch to fix this in Carp.pm as well.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From @demerphq

On 24 February 2018 at 12​:32, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:46, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:44, Zefram <zefram@​fysh.org> wrote​:

demerphq wrote​:

Thanks, this looks like fallout from a fix I pushed yesterday.

Confirmed​: it bisects to commit c99363a
"fix Perl #132828 - dont use overload to bypass overloads". The relevant
part of Class​::Std is overriding UNIVERSAL​::can.

Which means its Class​::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class​::Std to fix this.

chorny/Class-Std#2

I also will push a patch to fix this in Carp.pm as well.

Fixed in 17157c4

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From @cpansprout

On Sat, 24 Feb 2018 04​:16​:48 -0800, demerphq wrote​:

On 24 February 2018 at 12​:32, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:46, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:44, Zefram <zefram@​fysh.org> wrote​:

demerphq wrote​:

Thanks, this looks like fallout from a fix I pushed yesterday.

Confirmed​: it bisects to commit
c99363a
"fix Perl #132828 - dont use overload to bypass overloads". The
relevant
part of Class​::Std is overriding UNIVERSAL​::can.

Which means its Class​::Std's fault for not implementing can
properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class​::Std to fix this.

chorny/Class-Std#2

I also will push a patch to fix this in Carp.pm as well.

Fixed in 17157c4

I don’t like the current state of the code in Carp.pm, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns, before I forget them, and later if I have time I might come up with a patch​:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc), since older ones used (), not ((. And I think even current perl allows XS modules to register overloading via just (), without ((. I need to check.

‘can’ is really not the right thing to use. Overriding ‘can’ makes sense in the presence of AUTOLOAD, but since overloading bypassing AUTOLOAD, it should be bypassing ‘can‘ as well. This is why overload.pm implements its own ‘can’. Perhaps overload.pm should be loaded unconditionally when Carp is loaded (it is more lightweight than Carp, after all). Then theoretically one could use overload​::Overloaded, but unfortunately old versions have the same ‘can’ bug that will cause the recursion. Maybe calling overload​::mycan directly is the solution.

This is a real can of worms.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2018

From @demerphq

On 25 Feb 2018 02​:01, "Father Chrysostomos via RT" <
perlbug-followup@​perl.org> wrote​:

On Sat, 24 Feb 2018 04​:16​:48 -0800, demerphq wrote​:

On 24 February 2018 at 12​:32, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:46, demerphq <demerphq@​gmail.com> wrote​:

On 24 February 2018 at 03​:44, Zefram <zefram@​fysh.org> wrote​:

demerphq wrote​:

Thanks, this looks like fallout from a fix I pushed yesterday.

Confirmed​: it bisects to commit
c99363a
"fix Perl #132828 - dont use overload to bypass overloads". The
relevant
part of Class​::Std is overriding UNIVERSAL​::can.

Which means its Class​::Std's fault for not implementing can
properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class​::Std to fix this.

chorny/Class-Std#2

I also will push a patch to fix this in Carp.pm as well.

Fixed in 17157c4

I don’t like the current state of the code in Carp.pm, but I can’t say I
have a better suggestion off the top of my head. I have not thought it
through thoroughly yet. Let me just outline my concerns, before I forget
them, and later if I have time I might come up with a patch​:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc), since older ones used (),
not ((. And I think even current perl allows XS modules to register
overloading via just (), without ((. I need to check.

We can add a check for () as well.

‘can’ is really not the right thing to use.

Can you explain why? Since overloads are inherited it seems there is no
choice.

  Overriding ‘can’ makes sense in the presence of AUTOLOAD, but since
overloading bypassing AUTOLOAD, it should be bypassing ‘can‘ as well. This
is why overload.pm implements its own ‘can’. Perhaps overload.pm should be
loaded unconditionally when Carp is loaded (it is more lightweight than
Carp, after all). Then theoretically one could use overload​::Overloaded,
but unfortunately old versions have the same ‘can’ bug that will cause the
recursion. Maybe calling overload​::mycan directly is the solution.

If Carp unilaterally loads overload we don't need to use can at all. The
only reason we use it is to avoid loading overload when we don't need to.

I already suggested that this policy was counter productive and the only
reason I didn't change was that you expressed an opinion that we should be
able to keep it.

If you no longer care about loading overload then the patch becomes trivial.

This is a real can of worms.

A real mycan of worms?

Yves

--

Father Chrysostomos


via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132902

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @cpansprout

On Sat, 24 Feb 2018 15​:51​:03 -0800, demerphq wrote​:

On 25 Feb 2018 02​:01, "Father Chrysostomos via RT" <
perlbug-followup@​perl.org> wrote​:

I don’t like the current state of the code in Carp.pm, but I can’t say I
have a better suggestion off the top of my head. I have not thought it
through thoroughly yet. Let me just outline my concerns, before I forget
them, and later if I have time I might come up with a patch​:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc), since older ones used (),
not ((. And I think even current perl allows XS modules to register
overloading via just (), without ((. I need to check.

We can add a check for () as well.

‘can’ is really not the right thing to use.

Can you explain why? Since overloads are inherited it seems there is no
choice.

The text that follows was meant to be the explanation. can and AUTOLOAD go together. Since overloading does not use AUTOLOAD, using can to detect overloading can be problematic.

Overriding ‘can’ makes sense in the presence of AUTOLOAD, but since
overloading bypassing AUTOLOAD, it should be bypassing ‘can‘ as well. This
is why overload.pm implements its own ‘can’. Perhaps overload.pm should be
loaded unconditionally when Carp is loaded (it is more lightweight than
Carp, after all). Then theoretically one could use overload​::Overloaded,
but unfortunately old versions have the same ‘can’ bug that will cause the
recursion.

Not just recursion. Look at the test I added in e6bb0a4.

Maybe calling overload​::mycan directly is the solution.

If Carp unilaterally loads overload we don't need to use can at all. The
only reason we use it is to avoid loading overload when we don't need to.

I already suggested that this policy was counter productive and the only
reason I didn't change was that you expressed an opinion that we should be
able to keep it.

If you no longer care about loading overload then the patch becomes trivial.

Well, since Carp is used *everywhere*, I believe it should remain as lightweight as possible. Now that I’ve had a chance to look at the code in more detail, I see that it only occurs for stack traces. I am loath to force another module to be loaded, considering that many quick scripts never use stack traces. But loading overload.pm on demand in that case is itself problematic, since Carp is often called when things are in an inconsistent state. You shouldn’t risk dying when trying to load something in order to report an error. (Yes, this actually happened. That’s why Carp​::Heavy was eliminated.)

That said, I’m still on the fence about it. I know Zefram cares about this sort of thing, too, so I would ask him which is the best approach​:

• Go ahead and load overload.pm unconditionally. It’s already much smaller than Carp.

• Copy & paste mycan into Carp. It’s just seven lines or so (but we need two versions depending on the perl version).

This is a real can of worms.

A real mycan of worms?

Yves

s/v//r is my answer. :-)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @demerphq

On 25 February 2018 at 02​:40, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 24 Feb 2018 15​:51​:03 -0800, demerphq wrote​:

On 25 Feb 2018 02​:01, "Father Chrysostomos via RT" <
perlbug-followup@​perl.org> wrote​:

I don’t like the current state of the code in Carp.pm, but I can’t say I
have a better suggestion off the top of my head. I have not thought it
through thoroughly yet. Let me just outline my concerns, before I forget
them, and later if I have time I might come up with a patch​:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc), since older ones used (),
not ((. And I think even current perl allows XS modules to register
overloading via just (), without ((. I need to check.

We can add a check for () as well.

‘can’ is really not the right thing to use.

Can you explain why? Since overloads are inherited it seems there is no
choice.

The text that follows was meant to be the explanation. can and AUTOLOAD go together. Since overloading does not use AUTOLOAD, using can to detect overloading can be problematic.

Yeah I read that text, but I am still not seeing the connection the way you do.

So, it makes sense to override can if you use AUTOLOAD.

And overload's dont trigger AUTOLOAD. That is clear.

But, it doesnt seem to obvious to me that that means that we shouldnt
use can() for finding inheritable methods. Overload is *designed* so
this works. That is why the methods are registered the way they are.
(At least that is what the comments in overload indicate to me.)

It seems like you are saying something like​: "Because the main reason
to override can() is to work properly with AUTOLOAD, people often
implement their can overrides without accounting for overload subs,
and thus most of them are buggy, thus we shouldn't use can()".

Which doesn't seem logical to me, even if perhaps it is practical.

Why not simply say "We should document that can() overrides need to be
robust to overload calls"? I mean, i would consider any can() that
doesn't handle overload subs buggy, why don't you? It seems a strange
basis to argue we shouldn't use the one thing we have available to use
(Zefram mentions there is internal code that exactly what we want but
it is not exposed).

To me that there are buggy can() implementations out there is a
problem for those buggy can() implementations, and maybe an indication
we need better docs on this, not an argument to avoid can() for this
type of purpose.

Overriding ‘can’ makes sense in the presence of AUTOLOAD, but since
overloading bypassing AUTOLOAD, it should be bypassing ‘can‘ as well. This
is why overload.pm implements its own ‘can’. Perhaps overload.pm should be
loaded unconditionally when Carp is loaded (it is more lightweight than
Carp, after all). Then theoretically one could use overload​::Overloaded,
but unfortunately old versions have the same ‘can’ bug that will cause the
recursion.

Not just recursion. Look at the test I added in e6bb0a4.

Thanks, i will look.

Maybe calling overload​::mycan directly is the solution.

If Carp unilaterally loads overload we don't need to use can at all. The
only reason we use it is to avoid loading overload when we don't need to.

I already suggested that this policy was counter productive and the only
reason I didn't change was that you expressed an opinion that we should be
able to keep it.

If you no longer care about loading overload then the patch becomes trivial.

Well, since Carp is used *everywhere*, I believe it should remain as lightweight as possible.

Overload is widely used as well. I also am not that sympathetic to the
lightweight argument, i don't see a single module used for this kind
of purpose as a problem, especially when you consider how much code we
have in Carp to deal with these types of issues.

Now that I’ve had a chance to look at the code in more detail, I see that it only occurs for stack traces. I am loath to force another module to be loaded, considering that many quick scripts never use stack traces.

I bet you couldn't even see the extra module load in normal benchmarks
of any kind of non-trivial script.

But loading overload.pm on demand in that case is itself problematic, since Carp is often called when things are in an inconsistent state. You shouldn’t risk dying when trying to load something in order to report an error. (Yes, this actually happened. That’s why Carp​::Heavy was eliminated.)

I totally agree. No argument there at all. See
02c84d7 for an example of me patching
for this objective. ;-)

That said, I’m still on the fence about it. I know Zefram cares about this sort of thing, too, so I would ask him which is the best approach​:

• Go ahead and load overload.pm unconditionally. It’s already much smaller than Carp.

• Copy & paste mycan into Carp. It’s just seven lines or so (but we need two versions depending on the perl version).

Isnt mycan slow compared to the internals version? Maybe in modern
perls we should expose the internals function Zefram mentioned​:

(05​:57​:51) Zefram​: annoyingly, the exact thing you want is available
as a Perl op, method_named, but you can't get at that op in isolation
through Perl source code

That way BOTH carp and overload could use it in newer perls.

I don't really want to see Carp slowed down massively just because we
haven't documented how people should implement UNIVERSAL​::can
overrides properly. That seems just wrong.

This is a real can of worms.

A real mycan of worms?

Yves

s/v//r is my answer. :-)

Heh.

BTW, going forward there is another solution, expose unoverloaded
stringification via Internals​:: or perhaps scalar​::

It wont help older builds, but in the long run it would avoid needing
overload at all.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @demerphq

On 25 February 2018 at 02​:40, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 24 Feb 2018 15​:51​:03 -0800, demerphq wrote​:

On 25 Feb 2018 02​:01, "Father Chrysostomos via RT" <
perlbug-followup@​perl.org> wrote​:

I don’t like the current state of the code in Carp.pm, but I can’t say I
have a better suggestion off the top of my head. I have not thought it
through thoroughly yet. Let me just outline my concerns, before I forget
them, and later if I have time I might come up with a patch​:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc), since older ones used (),
not ((. And I think even current perl allows XS modules to register
overloading via just (), without ((. I need to check.

Fixed in 687fa12

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @cpansprout

On Sat, 24 Feb 2018 18​:42​:22 -0800, demerphq wrote​:

On 25 February 2018 at 02​:40, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 24 Feb 2018 15​:51​:03 -0800, demerphq wrote​:
The text that follows was meant to be the explanation. can and
AUTOLOAD go together. Since overloading does not use AUTOLOAD, using
can to detect overloading can be problematic.

Yeah I read that text, but I am still not seeing the connection the
way you do.

So, it makes sense to override can if you use AUTOLOAD.

And overload's dont trigger AUTOLOAD. That is clear.

But, it doesnt seem to obvious to me that that means that we shouldnt
use can() for finding inheritable methods. Overload is *designed* so
this works. That is why the methods are registered the way they are.
(At least that is what the comments in overload indicate to me.)

It seems like you are saying something like​: "Because the main reason
to override can() is to work properly with AUTOLOAD, people often
implement their can overrides without accounting for overload subs,
and thus most of them are buggy, thus we shouldn't use can()".

Which doesn't seem logical to me, even if perhaps it is practical.

Why not simply say "We should document that can() overrides need to be
robust to overload calls"? I mean, i would consider any can() that
doesn't handle overload subs buggy, why don't you? It seems a strange
basis to argue we shouldn't use the one thing we have available to use
(Zefram mentions there is internal code that exactly what we want but
it is not exposed).

To me that there are buggy can() implementations out there is a
problem for those buggy can() implementations, and maybe an indication
we need better docs on this, not an argument to avoid can() for this
type of purpose.

The way I see it, the fact that overloading uses weird-named subs is just an artefact of the implementation. Overloading can be inherited, but it does not consist of method calls (though they may trigger methods), as shown by the fact that AUTOLOAD is not respected, and by the fact that the fallback value (a scalar) is also inherited. In fact, overloading originally did not share internal code with method calls at all; that was just the most convenient way to get inheritance to work.

I don’t think we should blur this distinction and call ‘can’, because it puts the burden on too many programmers to ensure that their ‘can’ methods behave a way that ‘can’ was not designed for to begin with.

I do *not* consider what you call ‘buggy can() implementations’ to be buggy.

Not just recursion. Look at the test I added in e6bb0a4.

Thanks, i will look.

Based on your explanation above, it has a ‘buggy can() implementation’.

Well, since Carp is used *everywhere*, I believe it should remain as
lightweight as possible.

Overload is widely used as well. I also am not that sympathetic to the
lightweight argument, i don't see a single module used for this kind
of purpose as a problem, especially when you consider how much code we
have in Carp to deal with these types of issues.

I’m not entirely opposed to loading overload.pm unconditionally. I just want it to be justified, and not just convenient because it saves two lines of code. In this instance, it is justifiable.

I totally agree. No argument there at all. See
02c84d7 for an example of me patching
for this objective. ;-)

That said, I’m still on the fence about it. I know Zefram cares
about this sort of thing, too, so I would ask him which is the best
approach​:

• Go ahead and load overload.pm unconditionally. It’s already much
smaller than Carp.

• Copy & paste mycan into Carp. It’s just seven lines or so (but we
need two versions depending on the perl version).

Isnt mycan slow compared to the internals version? Maybe in modern
perls we should expose the internals function Zefram mentioned​:

(05​:57​:51) Zefram​: annoyingly, the exact thing you want is available
as a Perl op, method_named, but you can't get at that op in isolation
through Perl source code

It already exists. It is called UNIVERSAL​::can (which we can use, since we control it; even though it was designed for methods, it happens to work for this case). Unfortunately, there are (buggy) CPAN modules that diddle with it. Fortunately, Carp already has (partial) code to detect whether that has happened.

(Note​: There is a comment in overload.pm about ‘real’ can() leaving stubs. I think we need to get to the bottom of that before trying UNIVERSAL​::can. Leaving a "()" stub behind may break things, since that’s where the fallback value is stored.)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @cpansprout

On Sat, 24 Feb 2018 22​:24​:01 -0800, sprout wrote​:

On Sat, 24 Feb 2018 18​:42​:22 -0800, demerphq wrote​:

(05​:57​:51) Zefram​: annoyingly, the exact thing you want is available
as a Perl op, method_named, but you can't get at that op in isolation
through Perl source code

It already exists. It is called UNIVERSAL​::can (which we can use,
since we control it; even though it was designed for methods, it
happens to work for this case). Unfortunately, there are (buggy) CPAN
modules that diddle with it. Fortunately, Carp already has (partial)
code to detect whether that has happened.

(Note​: There is a comment in overload.pm about ‘real’ can() leaving
stubs. I think we need to get to the bottom of that before trying
UNIVERSAL​::can. Leaving a "()" stub behind may break things, since
that’s where the fallback value is stored.)

Never mind that note. Since overload​::Overloaded used to call ->can, it obviously doesn’t matter for this case.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From carlos@carlosguevara.com

This is a bug report for perl from Carlos Guevara <carlos@​carlosguevara.com>,
generated with the help of perlbug 1.41 running under perl 5.27.9.


It looks like blead broke Test​::MockObject​:
http​://www.cpantesters.org/cpan/report/cf360f98-19e0-11e8-809c-69696b55ae40



Flags​:
  category=core
  severity=low


Site configuration information for perl 5.27.9​:

Configured by root at Sun Feb 25 19​:21​:55 UTC 2018.

Summary of my perl5 (revision 5 version 27 subversion 9) configuration​:
  Commit id​: 33e5a35
  Platform​:
  osname=linux
  osvers=3.2.0-5-amd64
  archname=x86_64-linux
  uname='linux cjg-wheezy 3.2.0-5-amd64 #1 smp debian 3.2.96-3
x86_64 gnulinux '
  config_args='-des -Dprefix=/bin/perl-blead
-Dscriptdir=
/bin/perl-blead/bin -Dusedevel -Duse64bitall'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-D_FORTIFY_SOURCE=2'
  optimize='-O2'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include'
  ccversion=''
  gccversion='4.7.2'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  ivtype='long'
  ivsize=8
  nvtype='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='cc'
  ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib
/usr/lib/gcc/x86_64-linux-gnu/4.7/include-fixed
/usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu
/lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
  libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.13.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.13'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'


@​INC for perl 5.27.9​:
  /home/cpan/bin/perl-blead/lib/site_perl/5.27.9/x86_64-linux
  /home/cpan/bin/perl-blead/lib/site_perl/5.27.9
  /home/cpan/bin/perl-blead/lib/5.27.9/x86_64-linux
  /home/cpan/bin/perl-blead/lib/5.27.9


Environment for perl 5.27.9​:
  HOME=/home/cpan
  LANG (unset)
  LANGUAGE (unset)
  LC_ALL=C
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/home/cpan/bin/perl-blead/bin​:/home/cpan/bin​:/home/cpan/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/local/games​:/usr/games
  PERL_BADLANG (unset)
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @dur-randir

On Sun, 25 Feb 2018 11​:39​:39 -0800, carlos@​carlosguevara.com wrote​:

It looks like blead broke Test​::MockObject​:
http​://www.cpantesters.org/cpan/report/cf360f98-19e0-11e8-809c-
69696b55ae40

Bisect points to the same commit c99363a as in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132902

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From zefram@fysh.org

Carlos Guevara wrote​:

It looks like blead broke Test​::MockObject​:

It's down to Carp. Bisects to commit
c99363a "fix Perl #132828 - dont use
overload to bypass overloads".

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2018

From @cpansprout

On Sat, 24 Feb 2018 18​:42​:22 -0800, demerphq wrote​:

Isnt mycan slow compared to the internals version? Maybe in modern
perls we should expose the internals function Zefram mentioned​:

(05​:57​:51) Zefram​: annoyingly, the exact thing you want is available
as a Perl op, method_named, but you can't get at that op in isolation
through Perl source code

That way BOTH carp and overload could use it in newer perls.

I’ve just submitted a patch against the UNIVERSAL​::can module, making exactly that point​: https://rt.cpan.org/Ticket/Display.html?id=124585

(As I mentioned in another message, UNIVERSAL​::can is that exposure of the internal function.)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @demerphq

On 25 February 2018 at 21​:28, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 24 Feb 2018 18​:42​:22 -0800, demerphq wrote​:

Isnt mycan slow compared to the internals version? Maybe in modern
perls we should expose the internals function Zefram mentioned​:

(05​:57​:51) Zefram​: annoyingly, the exact thing you want is available
as a Perl op, method_named, but you can't get at that op in isolation
through Perl source code

That way BOTH carp and overload could use it in newer perls.

I’ve just submitted a patch against the UNIVERSAL​::can module, making exactly that point​: https://rt.cpan.org/Ticket/Display.html?id=124585

(As I mentioned in another message, UNIVERSAL​::can is that exposure of the internal function.)

I am really struggling to understand what you think is right.

So first off, there are *two* UNIVERSAL​::can()'s involved, the *real*
one, in Perl. And the fake one, in the CPAN module UNIVERSAL-can.

That module thinks that is "buggy" to call UNIVERSAL​::can() as a
function, which I definitely do not agree with, and is in fact what I
think Carp should do for this check.

In one of the other posts in this thread, you argue that can()
implementations should not be aware of overload subs, but now you
think the right thing to do is patch a /non-core module/ to be aware
of them.

You also have said you think that UNIVERSAL​::can() should not be used
for things like $obj->can("((") because overload subs do not respect
AUTOLOAD. (I don't agree with that either.)

I do not see how it is right that a patch to a module not in core is
required to fix behavior that is in core.

It seems to me that we need an exposure of UNIVERSAL​::can() that does
NOT live in the UNIVERSAL namespace and which cannot be overriden by a
module, and that in future Perls we should use that in overload and in
Carp.

I am right now quite baffled.

Honestly at this point I think the right thing is to make Carp load
overload unilaterally and make Carp use that, and move on to more
interesting things.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

On Sun, 25 Feb 2018 12​:19​:07 -0800, zefram@​fysh.org wrote​:

Carlos Guevara wrote​:

It looks like blead broke Test​::MockObject​:

It's down to Carp. Bisects to commit
c99363a "fix Perl #132828 - dont use
overload to bypass overloads".

Fixed in 4efd247.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

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

@p5pRT p5pRT closed this as completed Feb 26, 2018
@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

On Sat, 24 Feb 2018 22​:24​:01 -0800, sprout wrote​:

It already exists. It is called UNIVERSAL​::can (which we can use,
since we control it; even though it was designed for methods, it
happens to work for this case). Unfortunately, there are (buggy) CPAN
modules that diddle with it. Fortunately, Carp already has (partial)
code to detect whether that has happened.

I’ve done this in commit 4efd247, because it came up in another ticket. The use of ->can did cause CPAN breakage.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

These two tickets should not have been merged. They are related, but they are two different issues, one caused by the fix for the other.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From [Unknown Contact. See original ticket]

These two tickets should not have been merged. They are related, but they are two different issues, one caused by the fix for the other.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @demerphq

On 26 February 2018 at 03​:22, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 24 Feb 2018 22​:24​:01 -0800, sprout wrote​:

It already exists. It is called UNIVERSAL​::can (which we can use,
since we control it; even though it was designed for methods, it
happens to work for this case). Unfortunately, there are (buggy) CPAN
modules that diddle with it. Fortunately, Carp already has (partial)
code to detect whether that has happened.

I’ve done this in commit 4efd247, because it came up in another ticket. The use of ->can did cause CPAN breakage.

Personally i find this very irritating that you just push patches
related to this without properly engaging the feedback I provided.

On the same basis of unilateral imposition of ones own views without
debate, why shouldn't i just push a patch that makes Carp load
overload.pm? It would be simpler, faster, and would avoid any need to
call can() at all.

/me irritated that you aren't engaging properly.

cheers,
Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

On Sun, 25 Feb 2018 16​:14​:44 -0800, demerphq wrote​:

I am really struggling to understand what you think is right.

Well, I can be enigmatic. :-)

So first off, there are *two* UNIVERSAL​::can()'s involved, the *real*
one, in Perl. And the fake one, in the CPAN module UNIVERSAL-can.

That module thinks that is "buggy" to call UNIVERSAL​::can() as a
function, which I definitely do not agree with, and is in fact what I
think Carp should do for this check.

I agree with you here.

In one of the other posts in this thread, you argue that can()
implementations should not be aware of overload subs, but now you
think the right thing to do is patch a /non-core module/ to be aware
of them.

Regular modules’ ‘can’ implementations should not have to care about them. Modules that meddle with UNIVERSAL​::can need to bend over backwards to make sure they are not breaking things. With great power (overriding UNIVERSAL) comes great responsibility.

You also have said you think that UNIVERSAL​::can() should not be used

I don’t think $some_object->can should be used. UNIVERSAL​::can($object, ...) should be fine.

I think this way, not because I think that a method designed to look for methods is a good fit for overloading, but because in this particular instance (core’s UNIVERSAL​::can) it happens to do exactly the right thing. And, in being a stable interface, we can depend on it.

for things like $obj->can("((") because overload subs do not respect
AUTOLOAD. (I don't agree with that either.)

I do not see how it is right that a patch to a module not in core is
required to fix behavior that is in core.

If merely loading some module (and a not-so-unpopular module) will break the core, and it didn’t in the previous Perl version, then what should be someone else’s problem (a broken override) becomes *our* problem.

You may disagree with that, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.

It seems to me that we need an exposure of UNIVERSAL​::can() that does
NOT live in the UNIVERSAL namespace and which cannot be overriden by a
module, and that in future Perls we should use that in overload and in
Carp.

We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.

I am right now quite baffled.

Funny thing, I used the number 0xbaff1ed_bee in one of the tests.

Honestly at this point I think the right thing is to make Carp load
overload unilaterally and make Carp use that, and move on to more
interesting things.

I fixed this a different way before I read your message. I, too, intend to move on to more interesting things, after I’ve checked one other thing that may be broken in Carp.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @demerphq

On 26 February 2018 at 05​:02, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 16​:14​:44 -0800, demerphq wrote​:

I am really struggling to understand what you think is right.

Well, I can be enigmatic. :-)

Indeed. :-) Perhaps a bit too close to the machine version though. ;-)

So first off, there are *two* UNIVERSAL​::can()'s involved, the *real*
one, in Perl. And the fake one, in the CPAN module UNIVERSAL-can.

That module thinks that is "buggy" to call UNIVERSAL​::can() as a
function, which I definitely do not agree with, and is in fact what I
think Carp should do for this check.

I agree with you here.

\o/

In one of the other posts in this thread, you argue that can()
implementations should not be aware of overload subs, but now you
think the right thing to do is patch a /non-core module/ to be aware
of them.

Regular modules’ ‘can’ implementations should not have to care about them. Modules that meddle with UNIVERSAL​::can need to bend over backwards to make sure they are not breaking things. With great power (overriding UNIVERSAL) comes great responsibility.

I definitely agree with the latter two points. I am not entirely
convinced about the first one, but I can live your interpretation.

You also have said you think that UNIVERSAL​::can() should not be used

I don’t think $some_object->can should be used. UNIVERSAL​::can($object, ...) should be fine.

I think this way, not because I think that a method designed to look for methods is a good fit for overloading, but because in this particular instance (core’s UNIVERSAL​::can) it happens to do exactly the right thing. And, in being a stable interface, we can depend on it.

I agree with this.

for things like $obj->can("((") because overload subs do not respect
AUTOLOAD. (I don't agree with that either.)

I do not see how it is right that a patch to a module not in core is
required to fix behavior that is in core.

If merely loading some module (and a not-so-unpopular module) will break the core, and it didn’t in the previous Perl version, then what should be someone else’s problem (a broken override) becomes *our* problem.

You may disagree with that, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.

I'm willing to bend over backwards as well, or I wouldn't have tried
to fix this in the first place. :-)

[ And really, that paragraph seems pretty close to suggesting I am
acting in bad-faith here, which I am most certainly not.]

The place I disagree is *not* about avoiding breakage, but rather
about the right way to do so. And even there, I would not actually say
I disagree with you, I just am not convinced yet and feel the subject
merits more debate than we have given.

In particular I feel that we are going through huge contortions and
complexity simply to avoid loading overload.pm, and imposing
performance penalties on stack serialization to do so.

It feels like to me we have possibly already pushed past the point
where our efforts to avoid loading overload.pm are more expensive than
just loading overload.pm.

So for instance, if I was able to show that adding a "use overload;"
to the Carp had negligble or positive load time consequences would you
concur that we should remove this complexity? If not, what would
convince you?

It seems to me that we need an exposure of UNIVERSAL​::can() that does
NOT live in the UNIVERSAL namespace and which cannot be overriden by a
module, and that in future Perls we should use that in overload and in
Carp.

We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.

I think anything that overrides an Internals​:: (or equivalent)
function gets to keep both pieces. And because said logic would not
reside in UNIVERSAL, it would not affect all the normal uses of can()
that we both agree should work.

I am right now quite baffled.

Funny thing, I used the number 0xbaff1ed_bee in one of the tests.

:-) [ Interestingly some time back I was told by an academic that not
all language groups appreciate Irony the way English speakers do. ]

Honestly at this point I think the right thing is to make Carp load
overload unilaterally and make Carp use that, and move on to more
interesting things.

I fixed this a different way before I read your message.

But that is my point, this patch sequence has had too many patches and
too little discussion. Maybe a bit more deliberation would improve the
quality of our work.

I, too, intend to move on to more interesting things, after I’ve checked one other thing that may be broken in Carp.

Please lets resolve this discussion before you move on.

I appreciate your work and opinion, and I apologize if my attempts to
fix Carp have lead you to do more work, but at the same time, I note
that if we had just dropped the policy of eschewing "use overload;"
like I suggested in the first place /none/ of these patches or bug
reports would have happened.

So I feel like we really aught to address that point and come to a
consensus before we move on.

Which is why I cc'ed Sawyer on my last post, and why I am cc'ing him here.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

On Sun, 25 Feb 2018 20​:28​:02 -0800, demerphq wrote​:

On 26 February 2018 at 05​:02, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 16​:14​:44 -0800, demerphq wrote​:
If merely loading some module (and a not-so-unpopular module) will
break the core, and it didn’t in the previous Perl version, then what
should be someone else’s problem (a broken override) becomes *our*
problem.

You may disagree with that, but that’s fine. I’m willing to bend
over backwards to get things working. You don’t have to.

I'm willing to bend over backwards as well, or I wouldn't have tried
to fix this in the first place. :-)

[ And really, that paragraph seems pretty close to suggesting I am
acting in bad-faith here, which I am most certainly not.]

Sorry if it came across that way. I know that in the past (for example, when it came to hash randomization), you did not think it was p5p’s responsibility to patch all the broken modules, whereas I thought we should patch as many as we could. In order words, I’m trying to concede to your view, pointing out that the burden falls upon those who think differently.

The place I disagree is *not* about avoiding breakage, but rather
about the right way to do so. And even there, I would not actually say
I disagree with you, I just am not convinced yet and feel the subject
merits more debate than we have given.

In particular I feel that we are going through huge contortions and
complexity simply to avoid loading overload.pm, and imposing
performance penalties on stack serialization to do so.

It feels like to me we have possibly already pushed past the point
where our efforts to avoid loading overload.pm are more expensive than
just loading overload.pm.

So for instance, if I was able to show that adding a "use overload;"
to the Carp had negligble or positive load time consequences would you
concur that we should remove this complexity? If not, what would
convince you?

If you look at my latest patch, namely, 4efd247, you will see that it actually does load overload.pm on startup, iff UNIVERSAL​::can is already loaded. Otherwise it just uses Perl’s UNIVERSAL​::can, which suffices.

It seems to me that we need an exposure of UNIVERSAL​::can() that
does
NOT live in the UNIVERSAL namespace and which cannot be overriden by
a
module, and that in future Perls we should use that in overload and
in
Carp.

We can’t stop buggy modules from overriding things. But we can patch
the one buggy module that currently does it to ‘can’.

I think anything that overrides an Internals​:: (or equivalent)
function gets to keep both pieces.

I think the same is true of UNIVERSAL (but we already have existing modules to cope with).

And because said logic would not
reside in UNIVERSAL, it would not affect all the normal uses of can()
that we both agree should work.

To use the new function would add more conditions to Carp. The logic I have already added​:

+BEGIN {
+ *_mycan = _univ_mod_loaded('can')
+ ? do { require "overload.pm"; _fetch_sub overload => 'mycan' }
+ : \&UNIVERSAL​::can
+}

will still have to stay. So I see no need to proceed that way.

Honestly at this point I think the right thing is to make Carp load
overload unilaterally and make Carp use that, and move on to more
interesting things.

I fixed this a different way before I read your message.

But that is my point, this patch sequence has had too many patches and
too little discussion. Maybe a bit more deliberation would improve the
quality of our work.

I, too, intend to move on to more interesting things, after I’ve
checked one other thing that may be broken in Carp.

Please lets resolve this discussion before you move on.

I appreciate your work and opinion, and I apologize if my attempts to
fix Carp have lead you to do more work,

Oh, not at all. I actually really enjoy this kind of bug fixing, the digital counterpart to tightrope walking. Thank you for the opportunity.

but at the same time, I note
that if we had just dropped the policy of eschewing "use overload;"
like I suggested in the first place /none/ of these patches or bug
reports would have happened.

Bug #132910, which was erroneously merged into this ticket, would have happened, but we would not have noticed, because it would only have happened with a new Carp on perl 5.14 or earlier.

So I feel like we really aught to address that point and come to a
consensus before we move on.

I think you are proposing that we load overload.pm up front and just use overload​::StrVal without checking for overloading. Am I right?

When it comes to recent versions of overload.pm (1.18+; perl 5.16), that will just work.

Version 1.01–1.17 (perl 5.8.1 to 5.14) load Scalar​::Util at run time. *That* is a serious problem. (In fact, Carp currently loading overload.pm at run time is problematic, the ‘other thing that may be broken’ that I mentioned above.)

Carp is sometimes called from $SIG{__DIE__} handlers, which may trigger due to a syntax error. After a syntax error, BEGIN blocks won’t run. (This actually happened. See commit 018c7c8.) I have written a test that fails in current blead because of this.

That means​:
- For perl 5.16 onwards, we need to load overload.pm up front.
- For perl 5.8.1 to 5.14, we need to load Scalar​::Util up front as well, even though we might not use it.

Alternatively, for perl 5.10.1 to 5.14, we can copy the more recent overload​::StrVal into Carp. It consists of​: sub { no overloading; "$_[0]" }. You can’t get much faster than that. In which case we might as well use it also in current blead and avoid extra conditions. We don’t need to load overload.pm at all in perl 5.10.1+.

Did you know that the Romanian word for carp is crap (the fish, not the verb)?

As for the stash vivification test, I know why Zefram avoided vivifying utf8​::. It broke things on ancient perl versions. That makes sense. But overload? I think the reason is that Carp is so ubiquitous that you don’t want it leaving droppings lying around that it might not even use, every time anything loads it. But loading overloading.pm vivifies the overload stash. I think we have to live with it.

Now, for perl 5.8.1 to 5.10.0, we have (1) an overload​::StrVal that loads Scalar​::Util at run time, and (2) an overload​::Overloaded which for Carp is unusable, since it calls ->can.

Either we go ahead and load Scalar​::Util when loading Carp--which I don’t like, but I can be persuaded--, or we avoid overload​::StrVal and do it the hard way, writing a pure-Perl StrVal.

For Perl 5.8.0 (yes, I think we should support 5.8.0 still), overload​::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it, which may well cause real problems. Use of overload​::StrVal isn’t all that common, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs!

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

On Sun, 25 Feb 2018 23​:50​:45 -0800, sprout wrote​:

Now, for perl 5.8.1 to 5.10.0, we have (1) an overload​::StrVal that
loads Scalar​::Util at run time, and (2) an overload​::Overloaded which
for Carp is unusable, since it calls ->can.

Either we go ahead and load Scalar​::Util when loading Carp--which I
don’t like, but I can be persuaded--, or we avoid overload​::StrVal and
do it the hard way, writing a pure-Perl StrVal.

For Perl 5.8.0 (yes, I think we should support 5.8.0 still),
overload​::StrVal has the worst implementation yet. It unconditionally
blesses any ref passed to it, which may well cause real problems. Use
of overload​::StrVal isn’t all that common, compared to Carp. Carp
will make it common and start blessing people’s references left and
right when generating a stack trace. Avoid this old StrVal at all
costs!

To make things worse, the old pure-Perl Scalar​::Util​::refaddr also unconditionally blesses references. It looks almost the same as the old StrVal, so it was probably copied, pasted and tweaked. That means if we were to use overload​::StrVal from 5.8.1 to 5.10.0 (which uses refaddr) we would have to make sure the XS Scalar​::Util was loaded. That’s getting too complicated.

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

All the more necessary, considering what I just discovered about refaddr.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From zefram@fysh.org

Ticket [perl #132910] (BBC Test​::MockObject) has in RT been merged into
[perl #132902) (BBC Class​::Std), which is marked resolved. That does
not accurately reflect the status of the issues. Although the two BBCs
arise from the same core commit, they work by different mechanisms, and
have been fixed by different commits (#132902 by 17157c4 and #132910
by 4efd247). The tickets should be separate.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @cpansprout

On Sun, 25 Feb 2018 23​:50​:45 -0800, sprout wrote​:

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2018

From @jkeenan

On 02/26/2018 03​:03 AM, Zefram wrote​:

Ticket [perl #132910] (BBC Test​::MockObject) has in RT been merged into
[perl #132902) (BBC Class​::Std), which is marked resolved. That does
not accurately reflect the status of the issues. Although the two BBCs
arise from the same core commit, they work by different mechanisms, and
have been fixed by different commits (#132902 by 17157c4 and #132910
by 4efd247). The tickets should be separate.

-zefram

I'm sorry for misunderstanding the problem. Is there any corrective
action we can take?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2018

From @demerphq

On 26 February 2018 at 08​:50, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 20​:28​:02 -0800, demerphq wrote​:

On 26 February 2018 at 05​:02, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 16​:14​:44 -0800, demerphq wrote​:
If merely loading some module (and a not-so-unpopular module) will
break the core, and it didn’t in the previous Perl version, then what
should be someone else’s problem (a broken override) becomes *our*
problem.

You may disagree with that, but that’s fine. I’m willing to bend
over backwards to get things working. You don’t have to.

I'm willing to bend over backwards as well, or I wouldn't have tried
to fix this in the first place. :-)

[ And really, that paragraph seems pretty close to suggesting I am
acting in bad-faith here, which I am most certainly not.]

Sorry if it came across that way. I know that in the past (for example, when it came to hash randomization), you did not think it was p5p’s responsibility to patch all the broken modules, whereas I thought we should patch as many as we could. In order words, I’m trying to concede to your view, pointing out that the burden falls upon those who think differently.

For the record, I don't feel that that accurately portrays my views. I
recall helping patch at least a half dozen modules or so, although
beyond CGI and Perl itself I don't recall which.

There were a lot of people claiming back then that their code was not
buggy, and that the hash change "broke their code". I was very much
opposed to this view, as we had documented that key order was NOT
defined. Despite that many people assumed that despite the order being
maintained they were entitled to expect that the behavior was
repeatable. But that isn't what undefined means.

Many of the modules involved were "baking in" specific DD dumps for
testing purposes, and I it would not surprise me if I pushed back on
people claiming it was my/our responsibility to fix their tests.

The big issue here and then was that if people write buggy code that a
Perl change tickles, then it is not solely our responsibility to fix.
We are part of a community, and the users of Perl need to play their
role too.

The place I disagree is *not* about avoiding breakage, but rather
about the right way to do so. And even there, I would not actually say
I disagree with you, I just am not convinced yet and feel the subject
merits more debate than we have given.

In particular I feel that we are going through huge contortions and
complexity simply to avoid loading overload.pm, and imposing
performance penalties on stack serialization to do so.

It feels like to me we have possibly already pushed past the point
where our efforts to avoid loading overload.pm are more expensive than
just loading overload.pm.

So for instance, if I was able to show that adding a "use overload;"
to the Carp had negligble or positive load time consequences would you
concur that we should remove this complexity? If not, what would
convince you?

If you look at my latest patch, namely, 4efd247, you will see that it actually does load overload.pm on startup, iff UNIVERSAL​::can is already loaded. Otherwise it just uses Perl’s UNIVERSAL​::can, which suffices.

Ok.

It seems to me that we need an exposure of UNIVERSAL​::can() that
does
NOT live in the UNIVERSAL namespace and which cannot be overriden by
a
module, and that in future Perls we should use that in overload and
in
Carp.

We can’t stop buggy modules from overriding things. But we can patch
the one buggy module that currently does it to ‘can’.

I think anything that overrides an Internals​:: (or equivalent)
function gets to keep both pieces.

I think the same is true of UNIVERSAL (but we already have existing modules to cope with).

Personally due to the nature of UNIVERSAL being the base class for all
classes, I think this is less clear.

And because said logic would not
reside in UNIVERSAL, it would not affect all the normal uses of can()
that we both agree should work.

To use the new function would add more conditions to Carp. The logic I have already added​:

+BEGIN {
+ *_mycan = _univ_mod_loaded('can')
+ ? do { require "overload.pm"; _fetch_sub overload => 'mycan' }
+ : \&UNIVERSAL​::can
+}

will still have to stay. So I see no need to proceed that way.

Why? If we simply replace that with;

use overload ();

then we do not need to call UNIVERSAL​::can() at all.

Honestly at this point I think the right thing is to make Carp load
overload unilaterally and make Carp use that, and move on to more
interesting things.

I fixed this a different way before I read your message.

But that is my point, this patch sequence has had too many patches and
too little discussion. Maybe a bit more deliberation would improve the
quality of our work.

I, too, intend to move on to more interesting things, after I’ve
checked one other thing that may be broken in Carp.

Please lets resolve this discussion before you move on.

I appreciate your work and opinion, and I apologize if my attempts to
fix Carp have lead you to do more work,

Oh, not at all. I actually really enjoy this kind of bug fixing, the digital counterpart to tightrope walking. Thank you for the opportunity.

Heh, ok. Ill take that at face value. :-)

but at the same time, I note
that if we had just dropped the policy of eschewing "use overload;"
like I suggested in the first place /none/ of these patches or bug
reports would have happened.

Bug #132910, which was erroneously merged into this ticket, would have happened, but we would not have noticed, because it would only have happened with a new Carp on perl 5.14 or earlier.

I dont follow. If we simply did

use overload ();

then we could replace this logic​:

  # overload uses the presence of a special
  # "method" named "((" or "()" to signal
  # it is in effect. This test seeks to see if it has been set up.
  if (_mycan($pack, "((") || _mycan($pack, "()")) {
  # Argument is blessed into a class with overloading, and
  # so might have an overloaded stringification. We don't
  # want to risk getting the overloaded stringification,
  # so we need to use overload​::StrVal() below. But it's
  # possible that the overload module hasn't been loaded​:
  # overload methods can be installed without it. So load
  # the module here. The bareword form of require is here
  # eschewed to avoid this compile-time effect of vivifying
  # the target module's stash.
  require "overload.pm";
  }
  my $sub = _fetch_sub(overload => 'StrVal');
  return $sub ? &$sub($arg) : "$arg";

with a simple​:

return overload​::StrVal($arg);

So I feel like we really aught to address that point and come to a
consensus before we move on.

I think you are proposing that we load overload.pm up front and just use overload​::StrVal without checking for overloading. Am I right?

Yep.

When it comes to recent versions of overload.pm (1.18+; perl 5.16), that will just work.

Version 1.01–1.17 (perl 5.8.1 to 5.14) load Scalar​::Util at run time. *That* is a serious problem. (In fact, Carp currently loading overload.pm at run time is problematic, the ‘other thing that may be broken’ that I mentioned above.)

Ok, I see.

Carp is sometimes called from $SIG{__DIE__} handlers, which may trigger due to a syntax error. After a syntax error, BEGIN blocks won’t run. (This actually happened. See commit 018c7c8.) I have written a test that fails in current blead because of this.

That means​:
- For perl 5.16 onwards, we need to load overload.pm up front.
- For perl 5.8.1 to 5.14, we need to load Scalar​::Util up front as well, even though we might not use it.

I dont see any of this as an issue.

Alternatively, for perl 5.10.1 to 5.14, we can copy the more recent overload​::StrVal into Carp. It consists of​: sub { no overloading; "$_[0]" }. You can’t get much faster than that. In which case we might as well use it also in current blead and avoid extra conditions. We don’t need to load overload.pm at all in perl 5.10.1+.

Doesnt this also suffer the problem you mentioned of loading code at run time?

Did you know that the Romanian word for carp is crap (the fish, not the verb)?

No. How appropriate.

As for the stash vivification test, I know why Zefram avoided vivifying utf8​::. It broke things on ancient perl versions. That makes sense. But overload? I think the reason is that Carp is so ubiquitous that you don’t want it leaving droppings lying around that it might not even use, every time anything loads it. But loading overloading.pm vivifies the overload stash. I think we have to live with it.

Now, for perl 5.8.1 to 5.10.0, we have (1) an overload​::StrVal that loads Scalar​::Util at run time, and (2) an overload​::Overloaded which for Carp is unusable, since it calls ->can.

Either we go ahead and load Scalar​::Util when loading Carp--which I don’t like, but I can be persuaded--, or we avoid overload​::StrVal and do it the hard way, writing a pure-Perl StrVal.

For Perl 5.8.0 (yes, I think we should support 5.8.0 still), overload​::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it, which may well cause real problems. Use of overload​::StrVal isn’t all that common, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs!

Would some of this be solved by moving overload.pm to dist and
allowing it to be released on cpan like Carp is?

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

Ok, well, if the patches you have pushed work then I am fine to drop
this. I will just say I find some of the reasons for this stuff to be
excessive pandering to backwards compat. But i guess there isnt much
we can do about that.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2018

From @demerphq

On 26 February 2018 at 10​:33, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 23​:50​:45 -0800, sprout wrote​:

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

It is definitely cleaner than what we have now.

My only complaint is I hate this​:

qw 'UNIVERSAL isa'

:-)

Anyway, looks much saner to me than what we do now.

cheers,
Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2018

From @cpansprout

On Mon, 26 Feb 2018 23​:23​:40 -0800, demerphq wrote​:

On 26 February 2018 at 08​:50, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 20​:28​:02 -0800, demerphq wrote​:

On 26 February 2018 at 05​:02, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 16​:14​:44 -0800, demerphq wrote​:
Bug #132910, which was erroneously merged into this ticket, would
have happened, but we would not have noticed, because it would only
have happened with a new Carp on perl 5.14 or earlier.

I dont follow. If we simply did

use overload ();

then we could replace this logic​:

# overload uses the presence of a special
# "method" named "((" or "()" to signal
# it is in effect. This test seeks to see if it has been set up.
if (_mycan($pack, "((") || _mycan($pack, "()")) {
# Argument is blessed into a class with overloading, and
# so might have an overloaded stringification. We don't
# want to risk getting the overloaded stringification,
# so we need to use overload​::StrVal() below. But it's
# possible that the overload module hasn't been loaded​:
# overload methods can be installed without it. So load
# the module here. The bareword form of require is here
# eschewed to avoid this compile-time effect of vivifying
# the target module's stash.
require "overload.pm";
}
my $sub = _fetch_sub(overload => 'StrVal');
return $sub ? &$sub($arg) : "$arg";

with a simple​:

return overload​::StrVal($arg);

You’re right.

Alternatively, for perl 5.10.1 to 5.14, we can copy the more recent
overload​::StrVal into Carp. It consists of​: sub { no overloading;
"$_[0]" }. You can’t get much faster than that. In which case we
might as well use it also in current blead and avoid extra
conditions. We don’t need to load overload.pm at all in perl
5.10.1+.

Doesnt this also suffer the problem you mentioned of loading code at
run time?

Not if we load overloading.pm up front, which my patch does. I think it’s unavoidable.

For Perl 5.8.0 (yes, I think we should support 5.8.0 still),
overload​::StrVal has the worst implementation yet. It
unconditionally blesses any ref passed to it, which may well cause
real problems. Use of overload​::StrVal isn’t all that common,
compared to Carp. Carp will make it common and start blessing
people’s references left and right when generating a stack trace.
Avoid this old StrVal at all costs!

Would some of this be solved by moving overload.pm to dist and
allowing it to be released on cpan like Carp is?

Please, no. That would be a nightmare. We would end up playing this same compatibility game with overload.pm.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2018

From @cpansprout

On Mon, 26 Feb 2018 23​:51​:01 -0800, demerphq wrote​:

On 26 February 2018 at 10​:33, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 23​:50​:45 -0800, sprout wrote​:

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

It is definitely cleaner than what we have now.

My only complaint is I hate this​:

qw 'UNIVERSAL isa'

:-)

Would you prefer qw FUNIVERSAL isaF? It has FUN in it. :-)

Anyway, looks much saner to me than what we do now.

Thank you. I’ve pushed it as 5c8d107 (with qw/.../).

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2018

From @bulk88

On Tue, 27 Feb 2018 09​:16​:44 -0800, sprout wrote​:

On Mon, 26 Feb 2018 23​:51​:01 -0800, demerphq wrote​:

On 26 February 2018 at 10​:33, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sun, 25 Feb 2018 23​:50​:45 -0800, sprout wrote​:

It looks as though the simplest approach to all this is​:
- For 5.10.1+, use overloading.pm.
- For 5.10.0-, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

It is definitely cleaner than what we have now.

My only complaint is I hate this​:

qw 'UNIVERSAL isa'

:-)

Would you prefer qw FUNIVERSAL isaF? It has FUN in it. :-)

Anyway, looks much saner to me than what we do now.

Thank you. I’ve pushed it as 5c8d107 (with qw/.../).

Commit 5c8d fails badly on Windows for me.


C​:\p525\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err.t
& cd ..\win32
../dist/Carp/t/stack_after_err.t ..
1..4
# Failed test 'Carp does not try to load modules on demand for overloaded args
'
not ok 1 - Carp does not try to load modules on demand for overloaded args# at
t/stack_after_err.t line 22.

# ''
# doesn't match '(?^s​:Looks lark.*o=ARRAY.* CODE)'
not ok 2 - StrVal fallback in the presence of UNIVERSAL​::isa
# Failed test 'StrVal fallback in the presence of UNIVERSAL​::isa'
# at t/stack_after_err.t line 69.
# ''
# doesn't match '(?^s​:Looks lark.*o=ARRAY.* CODE)'
not ok 3 - StrVal fallback in the presence of UNIVERSAL​::can# Failed test 'Str
Val fallback in the presence of UNIVERSAL​::can'

# at t/stack_after_err.t line 69.
# ''
# doesn't match '(?^s​:Looks lark.*o=ARRAY.* CODE)'
not ok 4 - StrVal fallback in the presence of UNIVERSAL​::can/isa# Failed test
'StrVal fallback in the presence of UNIVERSAL​::can/isa'

# at t/stack_after_err.t line 69.
# ''
# doesn't match '(?^s​:Looks lark.*o=ARRAY.* CODE)'
# Looks like you failed 4 tests of 4.
Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/4 subtests

Test Summary Report


../dist/Carp/t/stack_after_err.t (Wstat​: 1024 Tests​: 4 Failed​: 4)
  Failed tests​: 1-4
  Non-zero exit status​: 4
Files=1, Tests=4, 1 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU)
Result​: FAIL

C​:\p525\src\win32>


--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2018

From @bulk88

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process


Command line​: C​:\p525\src\t\perl.exe -e
  use Carp;
  sub foom {
  Carp​::confess("Looks lark we got a error​: $_[0]")
  }
  BEGIN {
  *{"o​::()"} = sub {};
  *{'o​::(""'} = sub {"hay"};
  $o​::OVERLOAD{dummy}++; # perls before 5.18 need this
  *{"CODE​::()"} = sub {};
  $SIG{__DIE__} = sub { foom (@​_, bless([], o), sub {}) }
  }
  $a +
 


but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2018

From @bulk88

On Tue, 27 Feb 2018 20​:21​:24 -0800, bulk88 wrote​:

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process

If Win32's open3 sees a " char anywhere in the command line, it processes/splits/dont have time to research more, the command line differently.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2018

From @cpansprout

On Tue, 27 Feb 2018 20​:21​:24 -0800, bulk88 wrote​:

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process
------------------------
Command line​: C​:\p525\src\t\perl.exe -e
use Carp;
sub foom {
Carp​::confess("Looks lark we got a error​: $_[0]")
}
BEGIN {
*{"o​::()"} = sub {};
*{'o​::(""'} = sub {"hay"};
$o​::OVERLOAD{dummy}++; # perls before 5.18 need this
*{"CODE​::()"} = sub {};
$SIG{__DIE__} = sub { foom (@​_, bless([], o), sub {}) }
}
$a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the
command line foe -e's arg.

I suspect IPC​::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2018

From @demerphq

On 28 February 2018 at 06​:41, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Tue, 27 Feb 2018 20​:21​:24 -0800, bulk88 wrote​:

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process
------------------------
Command line​: C​:\p525\src\t\perl.exe -e
use Carp;
sub foom {
Carp​::confess("Looks lark we got a error​: $_[0]")
}
BEGIN {
*{"o​::()"} = sub {};
*{'o​::(""'} = sub {"hay"};
$o​::OVERLOAD{dummy}++; # perls before 5.18 need this
*{"CODE​::()"} = sub {};
$SIG{__DIE__} = sub { foom (@​_, bless([], o), sub {}) }
}
$a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the
command line foe -e's arg.

I suspect IPC​::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed​:

commit 01d4cc0
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Feb 28 16​:02​:17 2018 +0100

  rework Carp/t/stack_after_err.t to not use perl -e

cheers,
Yves

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

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2018

From @cpansprout

On Wed, 28 Feb 2018 07​:03​:52 -0800, demerphq wrote​:

On 28 February 2018 at 06​:41, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

Would it be feasible to feed the program to stdin and drop -e?

I just pushed​:

commit 01d4cc0
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Feb 28 16​:02​:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

cheers,
Yves

Thank you.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Mar 1, 2018

From @demerphq

On 28 February 2018 at 16​:03, demerphq <demerphq@​gmail.com> wrote​:

On 28 February 2018 at 06​:41, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Tue, 27 Feb 2018 20​:21​:24 -0800, bulk88 wrote​:

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process
------------------------
Command line​: C​:\p525\src\t\perl.exe -e
use Carp;
sub foom {
Carp​::confess("Looks lark we got a error​: $_[0]")
}
BEGIN {
*{"o​::()"} = sub {};
*{'o​::(""'} = sub {"hay"};
$o​::OVERLOAD{dummy}++; # perls before 5.18 need this
*{"CODE​::()"} = sub {};
$SIG{__DIE__} = sub { foom (@​_, bless([], o), sub {}) }
}
$a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the
command line foe -e's arg.

I suspect IPC​::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed​:

commit 01d4cc0
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Feb 28 16​:02​:17 2018 +0100

rework Carp/t/stack\_after\_err\.t to not use perl \-e

Bulk88 can you confirm this patch fixed the win32 build issue?

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Mar 1, 2018

From @bulk88

On Thu, 01 Mar 2018 00​:22​:19 -0800, demerphq wrote​:

On 28 February 2018 at 16​:03, demerphq <demerphq@​gmail.com> wrote​:

On 28 February 2018 at 06​:41, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Tue, 27 Feb 2018 20​:21​:24 -0800, bulk88 wrote​:

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process
------------------------
Command line​: C​:\p525\src\t\perl.exe -e
use Carp;
sub foom {
Carp​::confess("Looks lark we got a error​: $_[0]")
}
BEGIN {
*{"o​::()"} = sub {};
*{'o​::(""'} = sub {"hay"};
$o​::OVERLOAD{dummy}++; # perls before 5.18 need this
*{"CODE​::()"} = sub {};
$SIG{__DIE__} = sub { foom (@​_, bless([], o), sub {})
}
}
$a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the
command line foe -e's arg.

I suspect IPC​::Open3 (or something it calls) is adding the quotation
marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed​:

commit 01d4cc0
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Feb 28 16​:02​:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

Bulk88 can you confirm this patch fixed the win32 build issue?

Yves

works for me


C​:\perl521\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err.
t & cd ..\win32
../dist/Carp/t/stack_after_err.t ..
1..4
ok 1 - Carp does not try to load modules on demand for overloaded args
ok 2 - StrVal fallback in the presence of UNIVERSAL​::isa
ok 3 - StrVal fallback in the presence of UNIVERSAL​::can
ok 4 - StrVal fallback in the presence of UNIVERSAL​::can/isa
ok
All tests successful.
Files=1, Tests=4, 0 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU)
Result​: PASS

C​:\perl521\src\win32>


--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 1, 2018

From @demerphq

On 1 March 2018 at 09​:47, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

On Thu, 01 Mar 2018 00​:22​:19 -0800, demerphq wrote​:

On 28 February 2018 at 16​:03, demerphq <demerphq@​gmail.com> wrote​:

On 28 February 2018 at 06​:41, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Tue, 27 Feb 2018 20​:21​:24 -0800, bulk88 wrote​:

On Tue, 27 Feb 2018 20​:04​:14 -0800, bulk88 wrote​:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process
------------------------
Command line​: C​:\p525\src\t\perl.exe -e
use Carp;
sub foom {
Carp​::confess("Looks lark we got a error​: $_[0]")
}
BEGIN {
*{"o​::()"} = sub {};
*{'o​::(""'} = sub {"hay"};
$o​::OVERLOAD{dummy}++; # perls before 5.18 need this
*{"CODE​::()"} = sub {};
$SIG{__DIE__} = sub { foom (@​_, bless([], o), sub {})
}
}
$a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the
command line foe -e's arg.

I suspect IPC​::Open3 (or something it calls) is adding the quotation
marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed​:

commit 01d4cc0
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Feb 28 16​:02​:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

Bulk88 can you confirm this patch fixed the win32 build issue?

Yves

works for me
-------------------------------------------
C​:\perl521\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err.
t & cd ..\win32
../dist/Carp/t/stack_after_err.t ..
1..4
ok 1 - Carp does not try to load modules on demand for overloaded args
ok 2 - StrVal fallback in the presence of UNIVERSAL​::isa
ok 3 - StrVal fallback in the presence of UNIVERSAL​::can
ok 4 - StrVal fallback in the presence of UNIVERSAL​::can/isa
ok
All tests successful.
Files=1, Tests=4, 0 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU)
Result​: PASS

Great. Thanks for the report!

cheers,
Yves

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

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

1 participant