Nicholas Clark's fix for the IO::getline problem #17343
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Extend the tests for <> and the open pragma to verify that the behaviour changes with/without the open pragma.
…rl#14816. Re-implement getline() and getlines() as XS code. The underlying problem that we're trying to solve here is making getline() and getlines() in IO::Handle respect the open pragma. That bug was first addressed in Sept 2011 by commit 986a805: Make IO::Handle::getline(s) respect the open pragma However, that fix introduced a more subtle bug, hence this reworking. Including the entirety of the rest of that commit message because it explains both the bug the previous approach: See <https://rt.cpan.org/Ticket/Display.html?id=66474>. Also, this came up in <https://rt.perl.org/rt3/Ticket/Display.html?id=92728>. The <> operator, when reading from the magic ARGV handle, automatic- ally opens the next file. Layers set by the lexical open pragma are applied, if they are in scope at the point where <> is used. This works almost all the time, because the common convention is: use open ":utf8"; while(<>) { ... } IO::Handle’s getline and getlines methods are Perl subroutines that call <> themselves. But that happens within the scope of IO/Handle.pm, so the caller’s I/O layer settings are ignored. That means that these two expressions are not equivalent within in a ‘use open’ scope: <> *ARGV->getline The latter will open the next file with no layers applied. This commit solves that by putting PL_check hooks in place in IO::Handle before compiling the getline and getlines subroutines. Those hooks cause every state op (nextstate, or dbstate under the debugger) to have a custom pp function that saves the previous value of PL_curcop, calls the default pp function, and then restores PL_curcop. That means that getline and getlines run with the caller’s compile- time hints. Another way to see it is that getline and getlines’s own lexical hints are never activated. (A state op carries all the lexical pragmata. Every statement has one. When any op executes, it’s ‘pp’ function is called. pp_nextstate and pp_dbstate both set PL_curcop to the op itself. Any code that checks hints looks at PL_curcop, which contains the current run-time hints.) The problem with this approach is that the (current) design and implementation of PL_check hooks is actually not threadsafe. There's one array (as a global), which is used by all interpreters in the process. But as the code added to IO.xs demonstrates, realistically it needs to be possible to change the hook just for this interpreter. GH Perl#14816 has a fix for that bug for blead. However, it will be tricky (to impossible) to backport to earlier perl versions. Hence it's also worthwhile to change IO.xs to use a different approach to solve the original bug. As described above, the bug is fixed by having the readline OP (that implements getline() and getlines()) see the caller's lexical state, not their "own". Unlike Perl subroutines, XS subroutines don't have any lexical hints of their own. getline() and getlines() are very simple, mostly parameter checking, ending with a one line that maps to a single core OP, whose values are directly returned. Hence "all" we need to do re-implement the Perl code as XS. This might look easy, but turns out to be trickier than expected. There isn't any API to be called for the OP in question, pp_readline(). The body of the OP inspects interpreter state, it directly calls pp_rv2gv() which also inspects state, and then it tail calls Perl_do_readline(), which inspects state. The easiest approach seems to be to set up enough state, and then call pp_readline() directly. This leaves us very tightly coupled to the internals, but so do all other approaches to try to tackle this bug. The current implementation of PL_check (and possibly other arrays) still needs to be addressed.
|
This was merged in 7a992cc |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
as described in #16554
This fixes that problem, but not the general problem that PL_check is thread-unsafe
This was smoke-med and seems to have been forgotten after that.