Skip Menu |
Report information
Id: 131522
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: mauke- <l.mai [at] web.de>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.26.0
Fixed In: 5.26.1



From: l.mai [...] web.de
Date: Tue, 06 Jun 2017 22:52:39 +0200
Subject: Spurious "Assuming NOT a POSIX class" warning
To: perlbug [...] perl.org
This is a bug report for perl from l.mai@web.de, generated with the help of perlbug 1.40 running under perl 5.26.0. ----------------------------------------------------------------- [Please describe your issue here] This is actually a regression. Bug #127581 was reported against 5.23.8 and reportedly fixed in 5.24.0, but it's back again in 5.26.0: $ perl -we 'qr/[][[:alpha:]\@\\^_?]/' Assuming NOT a POSIX class since it doesn't start with a '[' in regex; marked by <-- HERE in m/[][[:alpha:]\@\\ <-- HERE ^_?]/ at -e line 1. Assuming NOT a POSIX class since the '^' must come after the colon in regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e line 1. Assuming NOT a POSIX class since there must be a starting ':' in regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e line 1. This should not throw any warnings. NB: When this was patched, a regression test was added to t/re/reg_mesg.t. But this test is broken for 3 reasons: - It was added to @death, not @warning, so it actually tests that the pattern dies. - The pattern does die because of a typo: The final / is missing from the match operator. - The regex in the test doesn't actually trigger this problem. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.26.0: Configured by mauke at Tue May 30 23:06:36 CEST 2017. Summary of my perl5 (revision 5 version 26 subversion 0) configuration: Platform: osname=linux osvers=4.10.11-1-arch archname=i686-linux uname='linux simplicio 4.10.11-1-arch #1 smp preempt tue apr 18 09:00:04 cest 2017 i686 gnulinux ' config_args='' hint=recommended useposix=true d_sigaction=define useithreads=undef usemultiplicity=undef use64bitint=undef use64bitall=undef uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='cc' ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64' optimize='-O2 -march=native' cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='7.1.1 20170516' gccosandvers='' intsize=4 longsize=4 ptrsize=4 doublesize=8 byteorder=1234 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=12 longdblkind=3 ivtype='long' ivsize=4 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=4 prototype=define Linker and Libraries: ld='cc' ldflags ='-fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/i686-pc-linux-gnu/7.1.1/include-fixed /usr/lib /lib libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.25.so so=so useshrplib=false libperl=libperl.a gnulibc_version='2.25' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl,-E' cccdlflags='-fPIC' lddlflags='-shared -O2 -march=native -L/usr/local/lib -fstack-protector-strong' --- @INC for perl 5.26.0: /home/mauke/usr/lib/perl5/site_perl/5.26.0/i686-linux /home/mauke/usr/lib/perl5/site_perl/5.26.0 /home/mauke/usr/lib/perl5/5.26.0/i686-linux /home/mauke/usr/lib/perl5/5.26.0 --- Environment for perl 5.26.0: HOME=/home/mauke LANG=en_US.UTF-8 LANGUAGE=en_US LC_COLLATE=C LC_MONETARY=de_DE.UTF-8 LC_TIME=de_DE.UTF-8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/mauke/perl5/perlbrew/bin:/home/mauke/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl PERLBREW_BASHRC_VERSION=0.73 PERLBREW_HOME=/home/mauke/.perlbrew PERLBREW_ROOT=/home/mauke/perl5/perlbrew PERL_BADLANG (unset) PERL_UNICODE=SAL SHELL=/bin/bash
RT-Send-CC: perl5-porters [...] perl.org
On Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote: Show quoted text
> > This is actually a regression. Bug #127581 was reported against 5.23.8 > and > reportedly fixed in 5.24.0, but it's back again in 5.26.0: > > $ perl -we 'qr/[][[:alpha:]\@\\^_?]/' > Assuming NOT a POSIX class since it doesn't start with a '[' in regex; > marked by <-- HERE in m/[][[:alpha:]\@\\ <-- HERE ^_?]/ at -e line 1. > Assuming NOT a POSIX class since the '^' must come after the colon in > regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e > line 1. > Assuming NOT a POSIX class since there must be a starting ':' in > regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e > line 1. > > This should not throw any warnings.
I bisected it to commit 7eec73eb790f7c4982edfc28c17c011e8a072490 Author: Yves Orton <demerphq@gmail.com> Date: Fri Jun 10 12:20:20 2016 +0200 move warning text to RExC_state (via RExC_warn_text) This way we reuse the same AV each time, and avoid various refcount bookkeeping issues, all at a relatively modest cost (IMO)
CC: perl5-porters [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Date: Tue, 13 Jun 2017 12:16:07 -0600
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
To: perlbug-followup [...] perl.org, Yves Orton <yves.orton [...] booking.com>
Download (untitled) / with headers
text/plain 1.2k
On 6/6/2017 4:20 PM, l.mai@web.de via RT wrote: Show quoted text
> On Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote:
>> >> This is actually a regression. Bug #127581 was reported against 5.23.8 >> and >> reportedly fixed in 5.24.0, but it's back again in 5.26.0: >> >> $ perl -we 'qr/[][[:alpha:]\@\\^_?]/' >> Assuming NOT a POSIX class since it doesn't start with a '[' in regex; >> marked by <-- HERE in m/[][[:alpha:]\@\\ <-- HERE ^_?]/ at -e line 1. >> Assuming NOT a POSIX class since the '^' must come after the colon in >> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e >> line 1. >> Assuming NOT a POSIX class since there must be a starting ':' in >> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e >> line 1. >> >> This should not throw any warnings.
> > I bisected it to commit 7eec73eb790f7c4982edfc28c17c011e8a072490 > Author: Yves Orton <demerphq@gmail.com> > Date: Fri Jun 10 12:20:20 2016 +0200 > > move warning text to RExC_state (via RExC_warn_text) > > This way we reuse the same AV each time, and avoid various refcount bookkeeping issues, all at a relatively modest cost (IMO) >
Yves, What was the motivation of this commit. I don't understand from your message, and though I have some ideas, I'd like to hear yours.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Tue, 13 Jun 2017 11:16:40 -0700, public@khwilliamson.com wrote: Show quoted text
> On 6/6/2017 4:20 PM, l.mai@web.de via RT wrote:
> > On Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote:
> >> > >> This is actually a regression. Bug #127581 was reported against > >> 5.23.8 > >> and > >> reportedly fixed in 5.24.0, but it's back again in 5.26.0: > >> > >> $ perl -we 'qr/[][[:alpha:]\@\\^_?]/' > >> Assuming NOT a POSIX class since it doesn't start with a '[' in > >> regex; > >> marked by <-- HERE in m/[][[:alpha:]\@\\ <-- HERE ^_?]/ at -e line > >> 1. > >> Assuming NOT a POSIX class since the '^' must come after the colon > >> in > >> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e > >> line 1. > >> Assuming NOT a POSIX class since there must be a starting ':' in > >> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e > >> line 1. > >> > >> This should not throw any warnings.
> > > > I bisected it to commit 7eec73eb790f7c4982edfc28c17c011e8a072490 > > Author: Yves Orton <demerphq@gmail.com> > > Date: Fri Jun 10 12:20:20 2016 +0200 > > > > move warning text to RExC_state (via RExC_warn_text) > > > > This way we reuse the same AV each time, and avoid various refcount > > bookkeeping issues, all at a relatively modest cost (IMO) > >
> > Yves, > > What was the motivation of this commit. I don't understand from your > message, and though I have some ideas, I'd like to hear yours.
I seem to remember that the purpose was to avoid having to free the via the savestack AV in various code paths, which was done before to prevent fatal warnings from causing a memory leak. The commit was supposed to accomplish the same thing in a different way that was easier to maintain, with a negligible memory trade-off. -- Father Chrysostomos
Date: Wed, 14 Jun 2017 11:44:13 -0600
From: Karl Williamson <public [...] khwilliamson.com>
CC: perl5-porters [...] perl.org
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
To: perlbug-followup [...] perl.org
On 06/13/2017 04:47 PM, Father Chrysostomos via RT wrote: Show quoted text
> On Tue, 13 Jun 2017 11:16:40 -0700, public@khwilliamson.com wrote:
>> On 6/6/2017 4:20 PM, l.mai@web.de via RT wrote:
>>> On Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote:
>>>> >>>> This is actually a regression. Bug #127581 was reported against >>>> 5.23.8 >>>> and >>>> reportedly fixed in 5.24.0, but it's back again in 5.26.0: >>>> >>>> $ perl -we 'qr/[][[:alpha:]\@\\^_?]/' >>>> Assuming NOT a POSIX class since it doesn't start with a '[' in >>>> regex; >>>> marked by <-- HERE in m/[][[:alpha:]\@\\ <-- HERE ^_?]/ at -e line >>>> 1. >>>> Assuming NOT a POSIX class since the '^' must come after the colon >>>> in >>>> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e >>>> line 1. >>>> Assuming NOT a POSIX class since there must be a starting ':' in >>>> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e >>>> line 1. >>>> >>>> This should not throw any warnings.
>>> >>> I bisected it to commit 7eec73eb790f7c4982edfc28c17c011e8a072490 >>> Author: Yves Orton <demerphq@gmail.com> >>> Date: Fri Jun 10 12:20:20 2016 +0200 >>> >>> move warning text to RExC_state (via RExC_warn_text) >>> >>> This way we reuse the same AV each time, and avoid various refcount >>> bookkeeping issues, all at a relatively modest cost (IMO) >>>
>> >> Yves, >> >> What was the motivation of this commit. I don't understand from your >> message, and though I have some ideas, I'd like to hear yours.
> > I seem to remember that the purpose was to avoid having to free the via the savestack AV in various code paths, which was done before to prevent fatal warnings from causing a memory leak. The commit was supposed to accomplish the same thing in a different way that was easier to maintain, with a negligible memory trade-off. >
I don't know anything about the savestack. The code this commit changed set the AV to mortal upon creation. I believe that puts things on a temps stack. And I don't know what you mean by "various code paths". Please explain.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On Wed, 14 Jun 2017 10:44:37 -0700, public@khwilliamson.com wrote: Show quoted text
> On 06/13/2017 04:47 PM, Father Chrysostomos via RT wrote:
> > > > I seem to remember that the purpose was to avoid having to free the > > via the savestack AV in various code paths, which was done before to > > prevent fatal warnings from causing a memory leak. The commit was > > supposed to accomplish the same thing in a different way that was > > easier to maintain, with a negligible memory trade-off. > >
> > I don't know anything about the savestack.
Sorry for taking so long to respond. I meant the temps stack, not the savestack (though the same thing can be done with the savestack via SAVEFREESV). Show quoted text
> The code this commit > changed > set the AV to mortal upon creation. I believe that puts things on a > temps stack. And I don't know what you mean by "various code paths". > Please explain.
It seems I misremember and confused it with another commit (probably accb4364d92e26). Again, apologies for the confusion. The details for the commits leading to *this* bug (ee072c8989 and 7eec73eb790f) can be found in ticket #128313. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.6k
On Sat, 17 Jun 2017 07:28:38 -0700, sprout wrote: Show quoted text
> On Wed, 14 Jun 2017 10:44:37 -0700, public@khwilliamson.com wrote:
> > On 06/13/2017 04:47 PM, Father Chrysostomos via RT wrote:
> > > > > > I seem to remember that the purpose was to avoid having to free the > > > via the savestack AV in various code paths, which was done before > > > to > > > prevent fatal warnings from causing a memory leak. The commit was > > > supposed to accomplish the same thing in a different way that was > > > easier to maintain, with a negligible memory trade-off. > > >
> > > > I don't know anything about the savestack.
> > Sorry for taking so long to respond. > > I meant the temps stack, not the savestack (though the same thing can > be done with the savestack via SAVEFREESV). >
> > The code this commit > > changed > > set the AV to mortal upon creation. I believe that puts things on a > > temps stack. And I don't know what you mean by "various code paths". > > Please explain.
> > It seems I misremember and confused it with another commit (probably > accb4364d92e26). Again, apologies for the confusion. > > The details for the commits leading to *this* bug (ee072c8989 and > 7eec73eb790f) can be found in ticket #128313.
I don’t see anything wrong with the code in 7eec73eb790f (which moved a C auto, warn_text, into the RExC_state_t struct, to avoid the possibility of too many mortal AVs), but I do not fully understand the scope of the new location. I suspect some internal temporary warning state is being allowed to last too long. I.e., RExC_warn_text is not being cleared at the right point. But I am only guessing. -- Father Chrysostomos
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
From: demerphq <demerphq [...] gmail.com>
Date: Sun, 18 Jun 2017 00:58:04 +0200
Download (untitled) / with headers
text/plain 1.3k
I missed this mail. Next time please add me to cc explicitly. I will investigate and see if I can fix. Thanks for the bisect! 

On 6 Jun 2017 18:20, "l.mai@web.de via RT" <perlbug-followup@perl.org> wrote:
Show quoted text
On Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote:
>
> This is actually a regression. Bug #127581 was reported against 5.23.8
> and
> reportedly fixed in 5.24.0, but it's back again in 5.26.0:
>
> $ perl -we 'qr/[][[:alpha:]\@\\^_?]/'
> Assuming NOT a POSIX class since it doesn't start with a '[' in regex;
> marked by <-- HERE in m/[][[:alpha:]\@\\ <-- HERE ^_?]/ at -e line 1.
> Assuming NOT a POSIX class since the '^' must come after the colon in
> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e
> line 1.
> Assuming NOT a POSIX class since there must be a starting ':' in
> regex; marked by <-- HERE in m/[][[:alpha:]\@\\^ <-- HERE _?]/ at -e
> line 1.
>
> This should not throw any warnings.

I bisected it to commit 7eec73eb790f7c4982edfc28c17c011e8a072490
Author: Yves Orton <demerphq@gmail.com>
Date:   Fri Jun 10 12:20:20 2016 +0200

    move warning text to RExC_state (via RExC_warn_text)

    This way we reuse the same AV each time, and avoid various refcount bookkeeping issues, all at a relatively modest cost (IMO)


---
via perlbug:  queue: perl5 status: new
https://rt.perl.org/Ticket/Display.html?id=131522

To: perlbug-followup [...] perl.org
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
From: Karl Williamson <public [...] khwilliamson.com>
Date: Sun, 18 Jun 2017 10:18:16 -0600
CC: perl5-porters [...] perl.org, Yves Orton <yves.orton [...] booking.com>
Download (untitled) / with headers
text/plain 2.8k
On 06/17/2017 08:28 AM, Father Chrysostomos via RT wrote: Show quoted text
> On Wed, 14 Jun 2017 10:44:37 -0700, public@khwilliamson.com wrote:
>> On 06/13/2017 04:47 PM, Father Chrysostomos via RT wrote:
>>> >>> I seem to remember that the purpose was to avoid having to free the >>> via the savestack AV in various code paths, which was done before to >>> prevent fatal warnings from causing a memory leak. The commit was >>> supposed to accomplish the same thing in a different way that was >>> easier to maintain, with a negligible memory trade-off. >>>
>> >> I don't know anything about the savestack.
> > Sorry for taking so long to respond. > > I meant the temps stack, not the savestack (though the same thing can be done with the savestack via SAVEFREESV). >
>> The code this commit >> changed >> set the AV to mortal upon creation. I believe that puts things on a >> temps stack. And I don't know what you mean by "various code paths". >> Please explain.
> > It seems I misremember and confused it with another commit (probably accb4364d92e26). Again, apologies for the confusion. > > The details for the commits leading to *this* bug (ee072c8989 and 7eec73eb790f) can be found in ticket #128313. >
Hmm. I was involved in that discussion, but forgot all about it. Now I've studied the situation more closely. The commit breaks the algorithm. The algorithm is to create tentative warnings, which may get discarded before being output, based on information later in the parse. It used to be that each time the function that parses potential posix classes was called, it returned a pointer to a new set of warnings. The caller was responsible for dealing with the pointer, and deciding to output the messages or not. With the change, currently, how to deal with the pointer gets moved into the function, where it doesn't really logically belong. And the pointer ends up always pointing to the same place, which might be undiscarded messages. More precisely, the current code has the function upon entry check if there were previous messages, and if so discards them. Then the function may generate new messages, but there is nowhere those get discarded if inappropriate. Thus the decision to discard needs to come from outside the function. (The current implementation also assumes that only one set of messages is active at any time. While currently true, that again should be a decision made outside the function.) I have several possible simple fixes; I was just trying to really grok what problem(s) that commit was trying to solve, in order to decide which way to go. The commit message said leak. But I couldn't understand and still don't, how there could be a leak with things declared mortal. What I can understand is minimizing the number of things that are mortal, as that can lead to excessive memory usage. And prior to that commit there was a lot of mortalizing that could be avoided.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On Sun, 18 Jun 2017 09:18:50 -0700, public@khwilliamson.com wrote: Show quoted text
> I have several possible simple fixes; I was just trying to really grok > what problem(s) that commit was trying to solve, in order to decide > which way to go. The commit message said leak. But I couldn't > understand and still don't, how there could be a leak with things > declared mortal.
The commit message for ee072c8989 says leak, and it did fix a leak my making the AV mortal earlier in a function that could exit early. The commit message for 7eec73eb790f (the problematic) commit does not say leak. It is the one can came up with a different approach (which is faulty, as you explained), in order to avoid using the temps stack. As Dan Collins pointed out, ee072c8989 did not actually increase the number of mortals, but changed when things were made mortal. So simply reverting 7eec73eb790f may be the solution. Show quoted text
> What I can understand is minimizing the number of > things that are mortal, as that can lead to excessive memory usage. > And > prior to that commit there was a lot of mortalizing that could be > avoided.
-- Father Chrysostomos
CC: Perl5 Porteros <perl5-porters [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
Date: Sun, 18 Jun 2017 20:43:20 +0200
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Download (untitled) / with headers
text/plain 1.6k
On 18 June 2017 at 19:32, Father Chrysostomos via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Sun, 18 Jun 2017 09:18:50 -0700, public@khwilliamson.com wrote:
>> I have several possible simple fixes; I was just trying to really grok >> what problem(s) that commit was trying to solve, in order to decide >> which way to go. The commit message said leak. But I couldn't >> understand and still don't, how there could be a leak with things >> declared mortal.
> > The commit message for ee072c8989 says leak, and it did fix a leak my making the AV mortal earlier in a function that could exit early. > > The commit message for 7eec73eb790f (the problematic) commit does not say leak. It is the one can came up with a different approach (which is faulty, as you explained), in order to avoid using the temps stack.
I guess it depends on your definition of faulty. Show quoted text
> As Dan Collins pointed out, ee072c8989 did not actually increase the number of mortals, but changed when things were made mortal.
IIRC still problematically. Show quoted text
> So simply reverting 7eec73eb790f may be the solution.
I don't think that is the right thing to do. 7eec73eb makes sure we reuse the AV over and over, and not create a new one each time we try to parse a posix charclass. Show quoted text
>> What I can understand is minimizing the number of >> things that are mortal, as that can lead to excessive memory usage. >> And >> prior to that commit there was a lot of mortalizing that could be >> avoided.
I have a patch to fix this already, I just haven't had time to push it. I am preparing to leave my parents and fly back to Europe. I will try to push it later today. It is not complex. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
Date: Sun, 18 Jun 2017 23:47:56 +0200
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
Download (untitled) / with headers
text/plain 2.1k
On 18 June 2017 at 20:43, demerphq <demerphq@gmail.com> wrote: Show quoted text
> On 18 June 2017 at 19:32, Father Chrysostomos via RT > <perlbug-followup@perl.org> wrote:
>> On Sun, 18 Jun 2017 09:18:50 -0700, public@khwilliamson.com wrote:
>>> I have several possible simple fixes; I was just trying to really grok >>> what problem(s) that commit was trying to solve, in order to decide >>> which way to go. The commit message said leak. But I couldn't >>> understand and still don't, how there could be a leak with things >>> declared mortal.
>> >> The commit message for ee072c8989 says leak, and it did fix a leak my making the AV mortal earlier in a function that could exit early. >> >> The commit message for 7eec73eb790f (the problematic) commit does not say leak. It is the one can came up with a different approach (which is faulty, as you explained), in order to avoid using the temps stack.
> > I guess it depends on your definition of faulty. >
>> As Dan Collins pointed out, ee072c8989 did not actually increase the number of mortals, but changed when things were made mortal.
> > IIRC still problematically. >
>> So simply reverting 7eec73eb790f may be the solution.
> > I don't think that is the right thing to do. 7eec73eb makes sure we > reuse the AV over and over, and not create a new one each time we try > to parse a posix charclass. >
>>> What I can understand is minimizing the number of >>> things that are mortal, as that can lead to excessive memory usage. >>> And >>> prior to that commit there was a lot of mortalizing that could be >>> avoided.
> > I have a patch to fix this already, I just haven't had time to push > it. I am preparing to leave my parents and fly back to Europe. I will > try to push it later today. It is not complex.
Fixed with: commit d730a80128abafff1e47e2506c23a8c1a06cfef4 Author: Yves Orton <demerphq@gmail.com> Date: Sun Jun 18 23:44:07 2017 +0200 add test for [perl #131522] and fix test for (related) [perl #127581] commit bab0f8e933b383b6bef406d79c2da340bbcded33 Author: Yves Orton <demerphq@gmail.com> Date: Sun Jun 18 20:45:30 2017 +0200 Resolve Perl #131522: Spurious "Assuming NOT a POSIX class" warning yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Date: Fri, 23 Jun 2017 11:49:57 +0200
From: demerphq <demerphq [...] gmail.com>
CC: Perl RT Bug Tracker <perlbug-followup [...] perl.org>, Perl5 Porteros <perl5-porters [...] perl.org>, Yves Orton <yves.orton [...] booking.com>
Subject: Re: [perl #131522] Spurious "Assuming NOT a POSIX class" warning
To: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 3.9k
On 18 June 2017 at 18:18, Karl Williamson <public@khwilliamson.com> wrote: Show quoted text
> On 06/17/2017 08:28 AM, Father Chrysostomos via RT wrote:
>> >> On Wed, 14 Jun 2017 10:44:37 -0700, public@khwilliamson.com wrote:
>>> >>> On 06/13/2017 04:47 PM, Father Chrysostomos via RT wrote:
>>>> >>>> >>>> I seem to remember that the purpose was to avoid having to free the >>>> via the savestack AV in various code paths, which was done before to >>>> prevent fatal warnings from causing a memory leak. The commit was >>>> supposed to accomplish the same thing in a different way that was >>>> easier to maintain, with a negligible memory trade-off. >>>>
>>> >>> I don't know anything about the savestack.
>> >> >> Sorry for taking so long to respond. >> >> I meant the temps stack, not the savestack (though the same thing can be >> done with the savestack via SAVEFREESV). >>
>>> The code this commit >>> changed >>> set the AV to mortal upon creation. I believe that puts things on a >>> temps stack. And I don't know what you mean by "various code paths". >>> Please explain.
>> >> >> It seems I misremember and confused it with another commit (probably >> accb4364d92e26). Again, apologies for the confusion. >> >> The details for the commits leading to *this* bug (ee072c8989 and >> 7eec73eb790f) can be found in ticket #128313. >>
> > Hmm. I was involved in that discussion, but forgot all about it. Now I've > studied the situation more closely. > > The commit breaks the algorithm. The algorithm is to create tentative > warnings, which may get discarded before being output, based on information > later in the parse. It used to be that each time the function that parses > potential posix classes was called, it returned a pointer to a new set of > warnings. The caller was responsible for dealing with the pointer, and > deciding to output the messages or not. With the change, currently, how to > deal with the pointer gets moved into the function, where it doesn't really > logically belong. And the pointer ends up always pointing to the same > place, which might be undiscarded messages. More precisely, the current > code has the function upon entry check if there were previous messages, and > if so discards them. Then the function may generate new messages, but there > is nowhere those get discarded if inappropriate. Thus the decision to > discard needs to come from outside the function. (The current > implementation also assumes that only one set of messages is active at any > time. While currently true, that again should be a decision made outside > the function.) > > I have several possible simple fixes; I was just trying to really grok what > problem(s) that commit was trying to solve, in order to decide which way to > go. The commit message said leak. But I couldn't understand and still > don't, how there could be a leak with things declared mortal. What I can > understand is minimizing the number of things that are mortal, as that can > lead to excessive memory usage. And prior to that commit there was a lot of > mortalizing that could be avoided.
This thread has gotten a bit fragmented. To recap from FC: Show quoted text
> The commit message for ee072c8989 says leak, and it did fix a leak my making the AV mortal earlier in a function that could exit early. > > The commit message for 7eec73eb790f (the problematic) commit does not say leak. It is the one can came up with a different approach (which is faulty, as you explained), in order to avoid using the temps stack.
The problematic point of of 7eec73eb790f was that I misunderstood the implication of some of the early exit pathways in the heuristics. The patch bab0f8e933b383b6bef406d79c2da340bbcded33 fixes this and makes the exit pathways clear about what they do. IIRC prior to 7eec7 we would create a new AV each parse pass per POSIX style char class. After 7eec7 and combined bab0f8e933b383b6bef406d79c2da340bbcded33 we allocate at most 1 AV. cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Now fixed in 5.26.1.


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

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org