Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upv5.25.5-184-ga5540cf breaks texinfo #15696
Comments
From @jkeenan[Transferring to RT a report of blead breaking a downstream program ##### Like every week, I updated the perl5-devel FreeBSD port (this week, from Today, I discovered that texinfo could not be built with the new Perl The error I get in texinfo is: Modification of a read-only value attempted at That line being: } elsif ($text =~ s/^(([^\s\p{InFullwidth}]|[\x{202f}\x{00a0}])+)//) { When that fails, $text contains " Texinfo documentation system". Adding use diagnostics to that file, I get [pastebin.com link has It turns out that the problem is \p{InFullwidth} (comes from [For reference, Texinfo::Convert::Line is part of the GNU texinfo Mat: Could you please provide the output of ./perl -lib -V from the Thank you very much. [perl -V info omitted because this is not being sent from the |
From @khwilliamsonAs a BBC ticket, I've marked this as a blocker for 5.26 |
|
The RT System itself - Status changed from 'new' to 'open' |
From mat@cpan.orgContent of the pastebin: Modification of a read-only value attempted at ../tp/Texinfo/Convert/Line.pm sub mod { $_[0] = 1 } Another way is to assign to a substr() that's off the end of the string. Yet another way is to assign to a foreach loop VAR when VAR $x = 1; Uncaught exception from user code: |
From mat@cpan.orgroot@11amd64-ports-perl5-devel:~ # perl -lib -V Platform: Characteristics of this binary (from libperl): Compile-time options: HAS_TIMES MULTIPLICITY PERLIO_LAYERS PERL_COPY_ON_WRITE PERL_DONT_CREATE_GVSV PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_IMPLICIT_CONTEXT PERL_MALLOC_WRAP PERL_OP_PARENT PERL_PRESERVE_IVUV PERL_USE_DEVEL USE_64_BIT_ALL USE_64_BIT_INT USE_ITHREADS USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_PERLIO USE_PERL_ATOF USE_REENTRANT_API Built under freebsd @INC: /usr/local/lib/perl5/site_perl/mach/5.25 /usr/local/lib/perl5/site_perl /usr/local/lib/perl5/5.25/mach /usr/local/lib/perl5/5.25 . |
From @jkeenanOn Wed Nov 02 20:33:30 2016, jkeen@verizon.net wrote:
Today I downloaded this tarball and attempted to configure/build/test it following the instructions in its INSTALL.generic file. I experienced a problem as early as './configure'. As that shell script ran, at one point I got output like this: ##### (Full typescript available upon request.) texinfo's ./configure helpfully writes a 'config.log' file. grepping that file, I observe repeated instances of the following: ##### Preliminary inferences: 1. texinfo's problems are not limited to FreeBSD. 2. texinfo's problems are not limited to that introduced at the commit point in 5.25 reported by mat. The program's ./configure appears to have problems on perl-5.24.0. 3. But since once I ran 'make' I had what appeared to be a valid 'Makefile' -- and since I know little about texinfo itself -- I'm not sure of the real importance of (1) and (2) or its relation to the blead breakage reported by mat. Thank you very much. -- |
From @jkeenanOn Thu Nov 03 11:20:48 2016, jkeenan wrote:
I subsequently installed blead: ##### ... and then, before calling texinfo's './Configure', I said: ##### I got similar errors in config.log, but, once again, they didn't seem to prevent me from getting a valid 'Makefile'. I then called 'make check' and logged the output. texinfo's 'make check' runs quite a few shell scripts. ##### After some digging, I came up with this ack: ##### tp/tests/formatting/out_parser/simple_with_menu_docbook_info/simple_with_menu.2 tp/tests/formatting/out_parser/cond_info_no-ifhtml_no-ifinfo_no-iftex/cond.2 tp/tests/formatting/out_parser/direntry_dircategory_info_split/direntry_dircategory.2 tp/tests/formatting/out_parser/split_nocopying_split_dev_null/split_nocopying.2 tp/tests/formatting/out_parser/cond_info_ifhtml_ifinfo_iftex/cond.2 tp/tests/formatting/out_parser/split_nocopying_split/split_nocopying.2 tp/tests/formatting/out_parser/split_nocopying/split_nocopying.2 tp/tests/formatting/out_parser/documentlanguage_set_option_info/documentlanguage_set.2 tp/tests/formatting/out_parser/cond_info/cond.2 This is the same kind of failure reported by mat, but reported from a different location. Here is the relevant part of texinfo's tp/Texinfo/Convert/ParagraphNonXS.pm ##### my $protect_spaces_flag = $paragraph->{'protect_spaces'}; ##### below is line 327 ##### I don't see any attempt to modify a read-only value at that line -- but, again, I know little about texinfo. Here's the code (excerpted) from tp/Texinfo/Convert/Line.pm that is relevant to mat's original report. ##### In each case there's some heavy-duty regex action going on. Perhaps we've recently introduced some regex changes that are intefering with texinfo's assumptions. Any ideas? Thank you very much. |
From @jkeenanOn Thu Nov 03 12:28:06 2016, jkeenan wrote:
I can now reproduce panics at both the location reported by mat and the location I noted above. ##### my @segments = split $text =~ s/^(([^\s\p{InFullwidth}]|[\x{202f}\x{00a0}])+)//; Running each against blead: ##### This is perl 5, version 25, subversion 7 (v5.25.7 (v5.25.6-172-g3e8592f)) built for x86_64-linux FBOW, I cannot find "InFullwidth" in the Perl 5 core distribution. ##### Hope that helps. -- |
From @khwilliamsonOn 11/03/2016 02:00 PM, James E Keenan via RT wrote:
That's because it is a subroutine in Unicode::EastAsianWidth |
From @jkeenanOn Thu Nov 03 13:00:51 2016, jkeenan wrote:
[snip]
A DDG search suggests that 'InFullwidth' is a property settable in these CPAN distributions, neither of which is part of the Perl 5 core distribution. ##### And an 'ack' on the texinfo source code quickly shows that it has a dependency on Unicode::EastAsianWidth: ##### $ ack -l 'Unicode(::|-|\/)(Property::XS)' | wc -l If, once I have built blead, installed Unicode::EastAsianWidth and then required that module in my two little scripts above, the panics go away. ##### $ ./bin/perl /home/jkeenan/learn/perl/p5p/texinfo/as_in_line.pl mat: Could you explore what happens when you try to configure/make/make check texinfo in an environment where Unicode::EastAsianWidth is already installed against what texinfo's ./configure believes to be $PERL? Thank you very much. -- |
From mat@cpan.org
Mmmm, Unicode::EastAsianWidth is bundled with the texinfo FreeBSD uses, it comes from: ftp://ftp.stack.nl/pub/users/johans/texinfo/20160425/texinfo-6.1.tar.xz (I don't quite understand why it is not the official tarball, from what I gather, there are some datafiles that are updated) I will try to see what happens if I have Unicode::EastAsianWidth present when building texinfo tomorrow. |
From @jkeenanOn Thu, 03 Nov 2016 23:06:13 GMT, mat@cpan.org wrote:
mat, thanks for looking into this. texinfo is a *massive* distribution -- over 4200 files in just one of the top-level directories -- and its test suite is not easy to understand. I found that I had to insert 'print' statements for debugging, run 'make check', and then grep the distro for instances of the "Modification ..." failures we've been investigating. I can't reproduce those failures/crashes in simple Perl programs where Unicode::EastAsianWidth is clearly 'use'd. So the challenge will be to get a reproducible failure that can be run outside of 'make check'. The only thing I'm fairly certain of now is that this is not FreeBSD-specific. Since texinfo is maintained upstream of you by GNU, I feel we should alert the GNU maintainers about this problem. Thank you very much. -- |
From mat@cpan.orgLe Thu, 03 Nov 2016 13:52:10 -0700, jkeenan a écrit :
Friday, RT was down, so I could not comment, but installing Unicode::EastAsianWidth does not help. |
From @jkeenanOn Mon, 07 Nov 2016 10:21:33 GMT, mat@cpan.org wrote:
Agreed. In the hope that others may help with this debugging effort, I am attaching several files: * 130010_debugging_procedures.pod, which has step-by-step instructions as to how to reproduce the failures in the texinfo test suite. * install_branch_for_testing.sh: Shell script (adapted from rafl and khw) for installing a perl built at the failing commit. * dollar-tree.txt and dollar-converter.txt: Referenced in 130010_debugging_procedures.pod The question is: What is it about this the pattern: ##### ... that (a) as of commit C<a5540cf> but not previously; and (b) in the context of this test suite but not in isolation, perceives something to be a read-only value not subject to modification? Thank you very much. -- |
From @jkeenanx $tree 0 HASH(0x3208208) |
From @jkeenanOn Mon, 07 Nov 2016 13:47:55 GMT, jkeenan wrote:
My next brainstorm: Add "use re 'debug';" to sub add_text() in tp/Texinfo/Convert/ParagraphNonXS.pm. When I did so and ran the debugging program found in one of my previous attachments, I got this output: ##### Since I have never previously used the regex debugger, I have no idea if there are any clues to a solution in that output. Thoughts? Thank you very much. -- |
From @jkeenanOn Mon, 07 Nov 2016 16:29:40 GMT, jkeenan wrote:
Compiling perl at what has been identified as the last good commit, and then running the test program through the debugger, I got much better output. It's quite long, so I'm posting it here: https://gist.github.com/jkeenan/184d2aaf914e4aa0410fe2ea1f36da91 Thank you very much. -- |
From @khwilliamsonTop posting. This smells like something to try valgrind on. Try make test.valgrind On 11/07/2016 05:59 PM, James E Keenan via RT wrote:
|
From @khwilliamsonOn 11/07/2016 08:34 PM, Karl Williamson wrote:
You can use TEST_JOBS= to make it go faster if you have multiple cores
|
From @demerphqOn 7 November 2016 at 17:29, James E Keenan via RT
It looks like the pattern [^\s\p{InFullwidth}] causes this. Yves -- |
From @jkeenanOn Mon, 07 Nov 2016 16:59:37 GMT, jkeenan wrote:
Yet another gist (actually, excerpts): https://gist.github.com/jkeenan/faad48a7d3dfe0c40eab07684388edfb At the first "bad" commit, I build perl with -DDEBUGGING. I reduced the invocation of the Perl script from within the texinfo test suite that I had been using to the minimum number of command-line switches that would still generate the panic. I got the output in the gist. I got similar output when I used the Perl debugger and stepped through to the failure point and then, unlike previously I called 's' *into* the 'my @segments' line. But, whatever debugging procedure I use, I always seem to end up with output like this: ##### ... with no insight into what the read-only value is and where the 'modification' is being attempted. Thank you very much. -- |
From @khwilliamsonOn 11/08/2016 02:12 AM, James E Keenan via RT wrote:
My valgrind suggestion didn't work. Try this patch to try to force a |
From @jkeenanOn Tue, 08 Nov 2016 01:12:25 GMT, jkeenan wrote:
One thing I forgot to mention earlier. If you step through the program with the Perl debugger and, when you come to this critical line: ##### -- |
From @jkeenanOn Tue, 08 Nov 2016 01:21:54 GMT, khw wrote:
Patch apparently not attached. -- |
From @jkeenanOn Mon, 07 Nov 2016 22:14:43 GMT, demerphq wrote:
Very likely. When, at an earlier stage of debugging, I removed the two parts of the pattern that contained '\p{InFullwidth}', the panic disappeared. However, that pattern was not panicking up until commit a5540cf. And, even with a5540cf and commits thereafter (e.g., HEAD of blead), I can write a very simple perl program that has that pattern and not get a panic. It's something about the interaction of commit a5540cf, that pattern, and the code in texinfo that is the problem. Thanks for taking a look at this. -- |
From @khwilliamsonOn 11/08/2016 04:37 AM, James E Keenan via RT wrote:
Sorry. but here it is. |
From @khwilliamson0001-Add-an-assert-to-try-to-get-a-stack-trace.patchFrom b76ea3d255eb3b4b8a89dd36ce3c0f1ce56f2efe Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Tue, 8 Nov 2016 02:19:41 +0100
Subject: [PATCH] Add an assert to try to get a stack trace
---
util.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/util.c b/util.c
index a69ddad..f906c8b 100644
--- a/util.c
+++ b/util.c
@@ -1877,6 +1877,7 @@ paths reduces CPU cache pressure.
void
Perl_croak_no_modify(void)
{
+ assert(0);
Perl_croak_nocontext( "%s", PL_no_modify);
}
--
2.9.3
|
From @jkeenanOn Tue, 08 Nov 2016 07:38:12 GMT, khw wrote:
Output is large; posted here: But I'm not sure that told us anything new. Here's the tail of the output: ##### Thank you very much. -- |
From @jkeenanOn Tue, 08 Nov 2016 12:20:37 GMT, jkeenan wrote:
For reference, I pushed a branch: jkeenan/a5540cf-amended -- |
From @khwilliamsonOn 11/08/2016 01:20 PM, James E Keenan via RT wrote:
It looks like the man page of gdb says you can say gdb /path/to/perl /path/to/core and then use the bt command to print the stack. If this doesn't work, someone on irc can advise you #####
|
From @jkeenanOn Tue, 08 Nov 2016 03:44:25 GMT, jkeenan wrote:
I am attaching a program, 'pseudo_east_asian_width.pl', which may be useful in resolving this problem. Once we solve the problem, we have to be able to write tests that demonstrate that solution and fail in the event of regressions. To write such a test in the core distribution we won't be able to say: ##### ... but we will need to simulate enough of that module's functionality to write a realistic test. In the attachment, 'package gamma' (the name is purely provisional) simulates much of Unicode::EastAsianWidth. 'package main' exercises gamma's 'InPseudoFullwidth' user-defined regex property in a similar (I think) to what Unicode::EastAsianWidth does. If I go into the two parts of the texinfo library where mat and I have identified failures -- Texinfo::Convert::Line and Texinfo::Convert::ParagraphNonXS -- and replace calls to Unicode::EastAsianWidth with calls to gamma and then call the texi2any.pl program with the perl from the "bad" commit point, then I can reproduce the panic condition. I haven't yet been able to generate the panic condition outside the texi2any.pl program, though. Thank you very much. -- |
From @jkeenanOn Tue, 08 Nov 2016 22:16:57 GMT, khw wrote:
I followed those instructions but, once again, notwithstanding the "core dumped" message, I could not locate any such core dump file. I issued a 'find' with a '-cmin 5' and came up with nothing. I know the kind of file you were expecting because in our FreeBSD-11.0 lib/locale.t problems (remember them?) I get 't/perl.core' all the time.
-- |
From @demerphqOn 7 Nov 2016 17:29, "James E Keenan via RT" <perlbug-followup@perl.org>
Have you tried putting a breakpoint or assert on the code that issues the If its a macro that you arent sure where it gets called, put an assert that Once you have a stack-trace we will be interested in whatever happens in or Once you have that you should be able to add a sv_dump() call to the right BTW, from what i can tell from the re 'debug' output, the error comes from [^\s\p{InFullwidth}] which is the third item in that alternation. So it seems like the issue is Anyway, I have not managed to find a copy of InFullWidth to look at, nor Another thing I would do is twiddle that charclass. If you change it to [^ \t\n\r\p{InFullWidth}] does the bug go away? What happens if you remove all the \p{InFullWidth} from that pattern? No doubt you are way beyond any of the advice here, but i thought i would Yves -- |
From @khwilliamsonOn 11/09/2016 09:54 AM, demerphq wrote:
User-defined \p properties, which InFullWidth is, that aren't available
|
From @jkeenanOn Wed, 09 Nov 2016 09:13:47 GMT, khw wrote:
Once we're into the world of gdb and backtrace, we're at a point where people more fluent in those techniques than me should take over. To facilitate this, I have placed a copy of the texinfo-6.1 source code on github.com: https://github.com/jkeenan/texinfo-p5p In a local clone of this repository you can create branches with debugging code to your heart's content. For suggestions as to how to proceed, see https://github.com/jkeenan/texinfo-p5p/blob/master/130010_debugging_procedure.pod. Thank you very much. -- |
From @hvdsI built and installed blead@392582f8 with './Configure -des -Dcc=gcc -Dprefix=/opt/blead-d0 -Doptimize='-g -O0' -DDEBUGGING -Dusedevel -Uversiononly'. On attempting the texinfo build, the first point it gives the error can be simplified to this command: After a quick grep for the error message (found in perl.h) and then for PL_no_modify, I rebuilt and installed the perl with this patch: On reinvoking the above command, I get a core dump with the backtrace below. I'll try to cut down the repro case, but the backtrace might allow others to make some progress in the meantime. Hugo (gdb) where |
From @demerphqOn 9 November 2016 at 17:22, Hugo van der Sanden via RT
Thanks. I did the same, and added an sv_dump to the caller of For some reason we are trying to process an INVLIST object, which Karl, is this enough for you to dig in? BTW, you dont need an installed perl to replicate this, I was able to TEXINFO_DEV_SOURCE=1 top_srcdir=".." top_builddir=".." gdb --args Yves Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". -- |
From @demerphqOn 9 November 2016 at 18:27, demerphq <demerphq@gmail.com> wrote:
the code responsible is this: sv_usepvn_flags(dest, /* This flag is documented to cause a copy to be avoided */ dest is at this point an INVLIST with the readonly flag set. Karl, is this enough for you to figure it out? Yves -- |
From @demerphqOn 9 November 2016 at 10:12, Karl Williamson <khw@cpan.org> wrote:
I dont get it. If InFullWidth is not loaded, how can people use it? I would just make that a fatal exception.
I don't think the regex engine should call things like this twice. It Anyway, this is just my opinion, but IMO there is little difference Feel free to point out the flaw in my reasoning. cheers, -- |
From @hvdsI was able to reduce it further to a standalone case: % cat lib/Unicode/EastAsianWidth.pm our @EXPORT = qw(InFullwidth); sub InFullwidth { 1; Note that if the module is C<use>d before the package declaration, the assertion is not hit. Hugo |
From @demerphqOn 9 November 2016 at 20:17, Hugo van der Sanden via RT
Oooh. ++ Even further: ./perl -Ilib -e 'A::xx(); package A; sub InFullwidth{ return "\n" } If I replace the \s with its logical equivalent there is no assert: ./perl -Ilib -e 'A::xx(); package A { sub InFullwidth{ return "\n" } and if I replace it with \w there is also no assert, but with \s, \h, I have been poking around a bit more, and the place the invlist gets It looks to me like it gets compiled once, then for some reason we try Yves |
From @khwilliamsonOn 11/09/2016 09:21 PM, demerphq wrote:
I have enough info to debug now. User-defined properties are implemented by the user defining |
From @maukeAm 09.11.2016 um 21:25 schrieb Karl Williamson:
That, by itself, isn't a very convincing rationale. Attributes are also -- |
From @khwilliamsonOn 11/09/2016 09:29 PM, Lukas Mai wrote:
This is the way it has already worked. Changing it would break this |
From @khwilliamsonOn 11/09/2016 09:32 PM, Karl Williamson wrote:
s/already/always/ |
From @jkeenanOn Wed, 09 Nov 2016 16:22:58 GMT, hv wrote:
Another backtrace: https://gist.github.com/jkeenan/6dbf40aa5d4301511b287961d8d8f7db -- |
From @jkeenanOn Wed, 09 Nov 2016 20:22:07 GMT, demerphq wrote:
khw created a new branch: origin/smoke-me/khw-kid51 and in commit 0d87c6c provided a fix for this problem. In that same branch I adapted Yves' test above and pushed it in commit 1f3d9c6. (I haven't written too many regex tests in core, so if it's not in the right format, please feel free to adjust.) Thank you very much. -- |
|
@khwilliamson - Status changed from 'open' to 'pending release' |
From @jkeenanOn Thu, 10 Nov 2016 02:04:08 GMT, jkeenan wrote:
Karl, on Nov 14 you marked this ticket as "Pending Release," but there's nothing in the ticket indicating when blead was patched. Does the attached diff demonstrate that blead has been fixed? Thank you very much. -- |
From @jkeenan130010.diffdiff --git a/t/re/pat_advanced.t b/t/re/pat_advanced.t
index 5eb2cc5..08f4f53 100644
--- a/t/re/pat_advanced.t
+++ b/t/re/pat_advanced.t
@@ -2433,6 +2433,15 @@ EOF
'No segfault [perl #126886]');
}
+ {
+ # [perl 130010] Downstream application texinfo started to report panics
+ # as of commit a5540cf.
+
+ runperl( prog => 'A::xx(); package A; sub InFullwidth{ return qq|\n| } sub xx { split /[^\s\p{InFullwidth}]/, q|x| }' );
+ ok(! $?, "User-defined pattern did not cause panic [perl 130010]");
+ }
+
+
# !!! NOTE that tests that aren't at all likely to crash perl should go
# a ways above, above these last ones. There's a comment there that, like
# this comment, contains the word 'NOTE'
diff --git a/utf8.c b/utf8.c
index 0a4466f..4dbefe5 100644
--- a/utf8.c
+++ b/utf8.c
@@ -3398,6 +3398,7 @@ Perl__core_swash_init(pTHX_ const char* pkg, const char* name, SV *listsv, I32 m
/* Add the passed-in inversion list, which invalidates the one
* already stored in the swash */
invlist_in_swash_is_valid = FALSE;
+ SvREADONLY_off(swash_invlist); /* Turned on again below */
_invlist_union(invlist, swash_invlist, &swash_invlist);
}
else {
|
From @iabynOn Fri, Mar 17, 2017 at 05:18:47PM -0700, James E Keenan via RT wrote:
Well, Karl applied the suggested fix and test as v5.25.6-208-geee4c92 and -- |
From @iabynOn Mon, Mar 20, 2017 at 01:18:24PM +0000, Dave Mitchell wrote:
so I've removed it from the blockers list -- |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#130010 (status was 'resolved')
Searchable as RT130010$