From @karenetheridge

A while ago I applied some changes to B​::Hooks​::Parser as suggested by ppport.h​:

karenetheridge/B-Hooks-Parser@66a934c

..and then yesterday released as
https://metacpan.org/release/ETHER/B-Hooks-Parser-0.20.

This failed on threaded builds​:
https://rt.cpan.org/Ticket/Display.html?id=129173

09​:43 <%ether> khw​: your changes in B​::Hooks​::Parser looked good, so I
released last night. and then
https://rt.cpan.org/Ticket/Display.html?id=129173
09​:43 < dipsy> [ Bug #129173 for B-Hooks-Parser​: Compilation error
with threaded perls ]
09​:45 <%ether> which looks related to my change
karenetheridge/B-Hooks-Parser@66a934c
which I made in response to ppport.h's suggestions (Tux walked me
through how to
  interpret ppport's suggestions at a Toolchain Summit a
few years ago). so I'm ...
09​:45 <%ether> ... wondering why these suggestions were made if the
new function doesn't exist and the old one does
09​:45 <%ether> I will write up something for the list if this doesn't
jump out as an obvious "aha"
09​:46 < khw> Yes, the aha is
09​:47 < khw> remove the pTHX from the
09​:47 < khw> filter_read
09​:47 < khw> The Perl_ form requires the thread context
09​:48 < khw> but the macro filter_read()
09​:48 < khw> adds it iff necessary
09​:49 <%ether> would it be correct to conclude that Devel​::PPPort made
an error here, with the syntax of the new form it suggested?
09​:50 < khw> If it said to include pTHX_, yes
09​:52 < khw> ether, by removing the 'Perl_'
09​:52 < khw> you get the macro, instead of a hard link basically to
borrow a term
09​:52 < khw> and the macro can evaluate to different things.
09​:52 < khw> That presumably is why they recommend changing to the macro
09​:53 < khw> The function can be swapped out for an improved version.
09​:53 < khw> If you say 'Perl_foo', you're always going to get the old version
09​:53 <%ether> finds the docs for pTHX_ in perlguts
09​:54 < khw> It's just the prototype for passing the thread to the routine
09​:54 <%ether> heh, maybe we should run ppport.h on core now :)
09​:54 < khw> The underscore stands for a comma when it actually means something.
09​:55 < khw> We need a new release of PPPort, indeed
09​:55 < khw> summon atoomic
09​:55 < dipsy> ATOOMIC ATOOMIC ATOOMIC ATOOMIC ATOOMIC ATOOMIC COME TO ME
09​:55 < khw> So on unthreaded builds, pTHX_ evaluates to thin air
09​:56 < khw> On threaded ones, it evaluates to pTHX,
09​:56 < khw> which in turn evaluates to some pointer to the current thread
09​:57 < khw> actually to a declaration for a ptr
09​:58 < khw> aTHX evaluates to the actual pointer.
09​:58 < khw> the a stands for actual; the p stands for prototype
09​:58 < khw> or something like that
10​:54 <%ether> khw​: I did s/pTHX_ int idx/int idx/ and got failures​:
https://travis-ci.org/karenetheridge/B-Hooks-Parser/jobs/519983070
10​:54 <%ether> should that be int *idx?
10​:57 < khw> ether, that's not the problem. My take is you should go
back to the way things were, that is, not take PPPort's suggestion for
this line
10​:58 < khw> and see what happens.
10​:58 < khw> If I had access, the way I'd go about figuring this out
is to make a .i file and look at the C code after the macro got
expanded.
10​:59 < khw> There's something wrong there.
10​:59 < khw> Travis should show it works on blead, right? as that code
should be #ifdef'd out
11​:00 <%ether> yes, blead passes -
https://travis-ci.org/karenetheridge/B-Hooks-Parser
11​:00 < khw> But the easiest is to forgo the macro. It's worked this
long, and on blead and later it is irrelevant

From @khwilliamson

I started to look at this, and can no longer recall the exact suggestion that was wrong, and can't find it in the ticket. Could you furnish that?
--
Karl Williamson

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

From @karenetheridge

On Sun, 21 Apr 2019 17​:49​:34 -0700, khw wrote​:

I started to look at this, and can no longer recall the exact
suggestion that was wrong, and can't find it in the ticket. Could you
furnish that?

The suggestions it made are exactly what I patched in this commit​:
karenetheridge/B-Hooks-Parser@66a934c

Going back one commit further than that is the 0.19 release, and running 'perl ppport.h' at that point should produce the same results. It had version 3.35 of the ppport file at the time, but last week I checked the lastest Devel​::PPPort (3.45) and it still produced the same suggestions.

From @tonycoz

On Sun, 14 Apr 2019 11​:15​:27 -0700, karen@​froods.org wrote​:

A while ago I applied some changes to B​::Hooks​::Parser as suggested by
ppport.h​:

https://github.com/karenetheridge/B-Hooks-
Parser/commit/66a934c02168048cebeb5a95d6ad3bbbe6e8f707

..and then yesterday released as
https://metacpan.org/release/ETHER/B-Hooks-Parser-0.20.

This failed on threaded builds​:
https://rt.cpan.org/Ticket/Display.html?id=129173

I expect the output from ppport.h is intended for callers of the perl API filter_read() rather than for code that tries to redefine it.

Calling the filter_read() macro instead of using Perl_filter_read() is good advice for callers of filter_read(), I don't think it's necessary for it to deal with the hopefully much less common case of code that tries to redefine perl API functions.

Tony

From @khwilliamson

On Tue, 23 Apr 2019 18​:56​:18 -0700, tonyc wrote​:

On Sun, 14 Apr 2019 11​:15​:27 -0700, karen@​froods.org wrote​:

A while ago I applied some changes to B​::Hooks​::Parser as suggested
by
ppport.h​:

https://github.com/karenetheridge/B-Hooks-
Parser/commit/66a934c02168048cebeb5a95d6ad3bbbe6e8f707

..and then yesterday released as
https://metacpan.org/release/ETHER/B-Hooks-Parser-0.20.

This failed on threaded builds​:
https://rt.cpan.org/Ticket/Display.html?id=129173

I expect the output from ppport.h is intended for callers of the perl
API filter_read() rather than for code that tries to redefine it.

Calling the filter_read() macro instead of using Perl_filter_read() is
good advice for callers of filter_read(), I don't think it's necessary
for it to deal with the hopefully much less common case of code that
tries to redefine perl API functions.

Tony

I've studied this more, and I agree with Tony. I'm thinking this ticket should be rejected.

The problem is that this is using stolen core code, which is a situation we want to avoid, and should be quite rare. And I don't think it's worth enhancing ppport to handle this unlikely case. I looked at its code, and it does handle the case of a call to such a function, noting the aTHX_, and including its removal in the suggestion. But it has no current way of coping with a function definition which uses pTHX_ instead. If someone wanted to undertake doing this, I'd be happy to accept their pull request. But there are much more important things to spend one's time on.
--
Karl Williamson

From @khwilliamson

Due to the reasons already given, I'm rejecting this ticket
--
Karl Williamson

@khwilliamson - Status changed from 'open' to 'rejected'