regex qr/(?[ [B] | ! ( [^B] ) ])/ fails to compile on Solaris x64 debugging optimized builds #15159

p5pRT · 2016-02-02T23:48:34Z

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

Searchable as RT127455$

p5pRT · 2016-02-02T23:48:35Z

From @tonycoz

When blead@42d92f4a6420c9e925e441055e97be24645d0a52 is built with Solaris Studio 12.3 for 64bitall and -DDEBUGGING, re/regex_sets.t fails to compile, choking on the regular expression:

qr/(?[ [B] | ! ( [^B] ) ])/

as follows:

tony@nereid:~/dev/perl/git/perl$ LD_LIBRARY_PATH=/home/tony/dev/perl/git/perl ./perl -e 'qr/(?[ [B] | ! ( [^B] ) ])/'
The regex_sets feature is experimental in regex; marked by <-- HERE in m/(?[ <-- HERE [B] | ! ( [^B] ) ])/ at -e line 1.
Incomplete expression within '(?[ ])' in regex; marked by <-- HERE in m/(?[ [B] | ! ( [^B] ) <-- HERE ])/ at -e line 1.

Building with -Doptimize=-O0\ -g produces a segmentation fault during the build process:

LD_LIBRARY_PATH=/home/tony/dev/perl/git/perl ./miniperl -Ilib lib/unicore/mktables -C lib/unicore -P pod -maketest -makelist -p
Processing PropertyAliases.txt
Finishing property setup
Processing PropValueAliases.txt
Processing extracted/DGeneralCategory.txt
Processing extracted/DCombiningClass.txt
Processing extracted/DNumType.txt
Processing extracted/DEastAsianWidth.txt
Processing extracted/DLineBreak.txt
Processing extracted/DBidiClass.txt
Processing extracted/DDecompositionType.txt
Processing extracted/DBinaryProperties.txt
Processing extracted/DNumValues.txt
Processing extracted/DJoinGroup.txt
Processing extracted/DJoinType.txt
Processing Jamo.txt
Processing UnicodeData.txt
Processing ArabicShaping.txt
Processing Blocks.txt
Processing PropList.txt
Processing SpecialCasing.txt
Processing LineBreak.txt
Processing EastAsianWidth.txt
Processing CompositionExclusions.txt
Processing BidiMirroring.txt
Processing CaseFolding.txt
Processing DCoreProperties.txt
Processing Scripts.txt
Processing DNormalizationProps.txt
Processing DAge.txt
Processing HangulSyllableType.txt
Processing auxiliary/WordBreakProperty.txt
Processing auxiliary/GraphemeBreakProperty.txt
Processing auxiliary/GCBTest.txt
Processing auxiliary/SBTest.txt
Processing auxiliary/WBTest.txt
Processing auxiliary/SentenceBreakProperty.txt
Processing NamedSequences.txt
Processing NameAliases.txt
Processing auxiliary/LBTest.txt
Processing ScriptExtensions.txt
Processing IndicSyllabicCategory.txt
Processing BidiBrackets.txt
Processing IndicPositionalCategory.txt
Finishing processing Unicode properties
Compiling Perl properties
Creating Perl synonyms
Writing tables
Making pod file
Making test script
Updating 'mktables.lst'
gmake: *** [uni.data] Segmentation Fault (core dumped)

Backtrace:

Updating 'mktables.lst'

Program received signal SIGSEGV, Segmentation fault.
0x000000000072a89e in Perl_sv_vcatpvfn_flags (sv=0x211b360,
pat=0x9841a0 "%s %s %2d %02d:%02d:%02d %ld", patlen=28,
args=0xffff80ffbffff370, svargs=0x0, svmax=0, maybe_tainted=0x0, flags=0)
at sv.c:11912
11912 default: iv = va_arg(*args, int); break;
Current language: auto; currently minimal
(gdb) bt
#0 0x000000000072a89e in Perl_sv_vcatpvfn_flags (sv=0x211b360,
pat=0x9841a0 "%s %s %2d %02d:%02d:%02d %ld", patlen=28,
args=0xffff80ffbffff370, svargs=0x0, svmax=0, maybe_tainted=0x0, flags=0)
at sv.c:11912
#1 0x000000000072561d in Perl_sv_vsetpvfn (sv=0x211b360,
pat=0x9841a0 "%s %s %2d %02d:%02d:%02d %ld", patlen=28,
args=0xffff80ffbffff370, svargs=0x0, svmax=0, maybe_tainted=0x0)
at sv.c:10760
#2 0x0000000000724ebb in Perl_sv_vsetpvf_mg (sv=0x211b360,
pat=0x9841a0 "%s %s %2d %02d:%02d:%02d %ld", args=0xffff80ffbffff370)
at sv.c:10611
#3 0x0000000000724d9d in Perl_sv_setpvf_mg (sv=0x211b360,
pat=0x9841a0 "%s %s %2d %02d:%02d:%02d %ld") at sv.c:10592
#4 0x00000000007e87e5 in Perl_pp_gmtime () at pp_sys.c:4774
#5 0x000000000063644d in Perl_runops_debug () at dump.c:2224
#6 0x00000000004608bf in S_run_body (oldscope=1) at perl.c:2466
#7 0x000000000045fe4c in perl_run (my_perl=0x9c4dd0) at perl.c:2389
#8 0x00000000004cb6f6 in main (argc=10, argv=0xffff80ffbffffb78,
env=0xffff80ffbffffbd0) at miniperlmain.c:122

Summary of my perl5 (revision 5 version 23 subversion 8) configuration:
Commit id: 42d92f4
Platform:
osname=solaris, osvers=2.11, archname=i86pc-solaris-64
uname='sunos nereid 5.11 11.1 i86pc i386 i86pc '
config_args='-des -Dusedevel -Duse64bitall -DDEBUGGING -Dcc=/opt/solarisstudio12.3/bin/cc'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
use64bitint=define, use64bitall=define, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='/opt/solarisstudio12.3/bin/cc', ccflags ='-m64 -DDEBUGGING -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DPERL_USE_SAFE_PUTENV',
optimize='-O -g',
cppflags='-m64 -DDEBUGGING'
ccversion='Sun C 5.12 SunOS_i386 2011/11/16', gccversion='', 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='/opt/solarisstudio12.3/bin/cc', ldflags =' -m64 -L/opt/solarisstudio12.3/prod/lib/amd64 -L/lib/64 -L/usr/gnu/lib '
libpth=/opt/solarisstudio12.3/prod/lib/amd64 /lib/64 /usr/gnu/lib /usr/lib /usr/ccs/lib
libs=-lpthread -lsocket -lnsl -lgdbm -ldb -ldl -lm -lc
perllibs=-lpthread -lsocket -lnsl -ldl -lm -lc
libc=/lib/libc.so, so=so, useshrplib=true, libperl=libperl.so
gnulibc_version=''
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags=' -R /opt/lib/perl5/5.23.8/i86pc-solaris-64/CORE'
cccdlflags='-KPIC', lddlflags=' -G -m64 -L/opt/solarisstudio12.3/prod/lib/amd64 -L/lib/64 -L/usr/gnu/lib'

Characteristics of this binary (from libperl):
Compile-time options: DEBUGGING HAS_TIMES PERLIO_LAYERS PERL_COPY_ON_WRITE
PERL_DONT_CREATE_GVSV
PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_MALLOC_WRAP
PERL_PRESERVE_IVUV PERL_USE_DEVEL
PERL_USE_SAFE_PUTENV USE_64_BIT_ALL USE_64_BIT_INT
USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE
USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME
USE_PERLIO USE_PERL_ATOF
Built under solaris
Compiled at Feb 3 2016 10:17:16
@INC:
lib
/opt/lib/perl5/site_perl/5.23.8/i86pc-solaris-64
/opt/lib/perl5/site_perl/5.23.8
/opt/lib/perl5/5.23.8/i86pc-solaris-64
/opt/lib/perl5/5.23.8
.

p5pRT · 2016-02-23T22:25:16Z

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

p5pRT · 2016-03-11T05:06:57Z

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

p5pRT · 2016-03-11T05:08:43Z

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

p5pRT · 2016-03-12T10:26:49Z

From @iabyn

On Tue, Feb 02, 2016 at 03:48:35PM -0800, Tony Cook wrote:

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

When blead@42d92f4a6420c9e925e441055e97be24645d0a52 is built with Solaris Studio 12.3 for 64bitall and -DDEBUGGING, re/regex_sets.t fails to compile, choking on the regular expression:

qr/(?[ [B] | ! ( [^B] ) ])/

as follows:

tony@nereid:~/dev/perl/git/perl$ LD_LIBRARY_PATH=/home/tony/dev/perl/git/perl ./perl -e 'qr/(?[ [B] | ! ( [^B] ) ])/'
The regex_sets feature is experimental in regex; marked by <-- HERE in m/(?[ <-- HERE [B] | ! ( [^B] ) ])/ at -e line 1.
Incomplete expression within '(?[ ])' in regex; marked by <-- HERE in m/(?[ [B] | ! ( [^B] ) <-- HERE ])/ at -e line 1.

This is hopefully fixed by:

commit d9cb841
Author: David Mitchell <davem@iabyn.com>
AuthorDate: Sat Mar 12 09:39:41 2016 +0000
Commit: David Mitchell <davem@iabyn.com>
CommitDate: Sat Mar 12 10:22:51 2016 +0000

regex sets: fix Solaris optimiser bug

[perl #127455]

On Solaris with -DDEBUGGING, re/regex_sets.t was failing to compile.

This appears to be due to an optimiser bug.

The code in question looked like:

handle_operand:
top_index = av_tindex_nomg(stack);
if (top_index - fence >= 0) {
...
}

printf()ing the value of fence after the av_tindex_nomg() showed that its
value was corrupted (compared with its expected value based on a different
platform with the same debugging print). However, putting a another printf
prior to the av_tindex_nomg() call not only displayed the correct value,
but caused the later printf() to also display the correct value. It seems
that merely accessing fence prior to av_tindex_nomg() avoids the
corruption.

Simplifying the av_tindex_nomg(), the bad behaviour could be reduced to:

if (!stack) { __assert( "" , "", 1); }

Putting a printf after this gave a corrupted fence; a printf before
made everything work.

So this workaround commit just makes sure that fence is accessed prior to
calling av_tindex_nomg().

Affected files ...
M regcomp.c

Differences ...

Inline Patch

diff --git a/regcomp.c b/regcomp.c
index ffee749..e1dc3c8 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15122,7 +15122,22 @@ redo_curchar:
              * stack, we have to check if it is a !.  But first, the code above
              * may have altered the stack in the time since we earlier set
              * 'top_index'.  */
-            top_index = av_tindex_nomg(stack);
+
+            {
+                /* Work round an optimiser bug in Solaris Studio 12.3:
+                 * for some reason, the presence of the __assert() in
+                 * av_tindex_nomg() causes the value of fence to get
+                 * corrupted, even though the assert is never called. So
+                 * save the value then restore afterwards.
+                 * Note that in fact merely accessing the value of fence
+                 * prior to the statement containing the assert is enough
+                 * to make the bug go away.
+                 */
+                IV f = fence;
+                top_index = av_tindex_nomg(stack);
+                fence = f;
+            }
+
             if (top_index - fence >= 0) {
                 /* If the top entry on the stack is an operator, it had better
                  * be a '!', otherwise the entry below the top operand should


--

Indomitable in retreat, invincible in advance, insufferable in victory
-- Churchill on Montgomery

p5pRT · 2016-03-12T16:48:09Z

From @khwilliamson

It's unfortunate, given that there's so few of us coding this project,
that there was duplication of effort here. But I guess it happens
infrequently enough that it's tolerable.

Tony Cook (mostly him) and I also had been tracking this down, and
communicating on irc about it. I had discovered that adding a DEBUG_r
statement (that outputs 'fence') in the right place fixes the bug, and
Tony had discovered that declaring 'fence' to be volatile also fixes it.

I don't know if one approach is better than the others.

Also, av_tindex_nomg() is a new construct that I added based on Tony's
observation that these internal AV's did not have length magic, so that
calling straight av_tindex() does useless work in checking for that
magic. I had also hoped that that would change things enough so the bug
would go away without us having to do anything else. But it didn't.
Since then, Tony has pointed out that the '_nomg' suffix could mislead
people into thinking this was parallel to the various sv_foo_nomg()
functions, so they might try to do

SvGETMAGIC( (SV *) av); IV x = av_tindex_nomg(av);

and that doesn't work as they might expect it to. My current thinking
on this is to say in the SvGETMAGIC documentation that it only works on
SV's, and not on aggregate types. Since you need to cast the AV* to a
SV* to get the code to compile, that might be sufficient.
(av_tindex_nomg() also is not part of the public API.) Another option
is to rename it to be something that indicates you can't use get magic
on it, but we can't think of anything short and sweet. Ideas welcome.

On 03/12/2016 03:26 AM, Dave Mitchell wrote:

On Tue, Feb 02, 2016 at 03:48:35PM -0800, Tony Cook wrote:

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

When blead@42d92f4a6420c9e925e441055e97be24645d0a52 is built with Solaris Studio 12.3 for 64bitall and -DDEBUGGING, re/regex_sets.t fails to compile, choking on the regular expression:

qr/(?[ [B] | ! ( [^B] ) ])/

as follows:

tony@nereid:~/dev/perl/git/perl$ LD_LIBRARY_PATH=/home/tony/dev/perl/git/perl ./perl -e 'qr/(?[ [B] | ! ( [^B] ) ])/'
The regex_sets feature is experimental in regex; marked by <-- HERE in m/(?[ <-- HERE [B] | ! ( [^B] ) ])/ at -e line 1.
Incomplete expression within '(?[ ])' in regex; marked by <-- HERE in m/(?[ [B] | ! ( [^B] ) <-- HERE ])/ at -e line 1.

This is hopefully fixed by:

commit d9cb841
Author: David Mitchell <davem@iabyn.com>
AuthorDate: Sat Mar 12 09:39:41 2016 +0000
Commit: David Mitchell <davem@iabyn.com>
CommitDate: Sat Mar 12 10:22:51 2016 +0000
 regex sets&#8203;: fix Solaris optimiser bug

 \[perl \#127455\]

 On Solaris with \-DDEBUGGING\, re/regex\_sets\.t was failing to compile\.

 This appears to be due to an optimiser bug\.

 The code in question looked like&#8203;:

   handle\_operand&#8203;:
     top\_index = av\_tindex\_nomg$stack$;
     if $top\_index \- fence >= 0$ \{
         \.\.\.
     \}

 printfing the value of fence after the av\_tindex\_nomg showed that its
 value was corrupted $compared with its expected value based on a different
 platform with the same debugging print$\. However\, putting a another printf
 prior to the av\_tindex\_nomg call not only displayed the correct value\,
 but caused the later printf to also display the correct value\. It seems
 that merely accessing fence prior to av\_tindex\_nomg avoids the
 corruption\.

 Simplifying the av\_tindex\_nomg\, the bad behaviour could be reduced to&#8203;:

     if $\!stack$ \{ \_\_assert$ "" \, ""\, 1$; \}

 Putting a printf after this gave a corrupted fence; a printf before
 made everything work\.

 So this workaround commit just makes sure that fence is accessed prior to
 calling av\_tindex\_nomg\.
Affected files ...
M regcomp.c

Differences ...

diff --git a/regcomp.c b/regcomp.c
index ffee749..e1dc3c8 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15122,7 +15122,22 @@ redo_curchar:
* stack, we have to check if it is a !. But first, the code above
* may have altered the stack in the time since we earlier set
* 'top_index'. */
- top_index = av_tindex_nomg(stack);
+
+ {
+ /* Work round an optimiser bug in Solaris Studio 12.3:
+ * for some reason, the presence of the __assert() in
+ * av_tindex_nomg() causes the value of fence to get
+ * corrupted, even though the assert is never called. So
+ * save the value then restore afterwards.
+ * Note that in fact merely accessing the value of fence
+ * prior to the statement containing the assert is enough
+ * to make the bug go away.
+ */
+ IV f = fence;
+ top_index = av_tindex_nomg(stack);
+ fence = f;
+ }
+
if (top_index - fence >= 0) {
/* If the top entry on the stack is an operator, it had better
* be a '!', otherwise the entry below the top operand should

p5pRT · 2016-03-14T09:00:32Z

From @iabyn

On Sat, Mar 12, 2016 at 09:47:20AM -0700, Karl Williamson wrote:

It's unfortunate, given that there's so few of us coding this project, that
there was duplication of effort here. But I guess it happens infrequently
enough that it's tolerable.

Tony Cook (mostly him) and I also had been tracking this down, and
communicating on irc about it. I had discovered that adding a DEBUG_r
statement (that outputs 'fence') in the right place fixes the bug, and Tony
had discovered that declaring 'fence' to be volatile also fixes it.

I don't know if one approach is better than the others.

Well, my one's
a) already in blead
b) should be optimised away, so won't affect any code
so I think we might as well go with mine.

Also, av_tindex_nomg() is a new construct that I added based on Tony's
observation that these internal AV's did not have length magic, so that
calling straight av_tindex() does useless work in checking for that magic.
I had also hoped that that would change things enough so the bug would go
away without us having to do anything else. But it didn't. Since then, Tony
has pointed out that the '_nomg' suffix could mislead people into thinking
this was parallel to the various sv_foo_nomg() functions, so they might try
to do
SvGETMAGIC$ \(SV \*$ av\); IV x = av\_tindex\_nomg$av$;
and that doesn't work as they might expect it to. My current thinking on
this is to say in the SvGETMAGIC documentation that it only works on SV's,
and not on aggregate types. Since you need to cast the AV* to a SV* to get
the code to compile, that might be sufficient. (av_tindex_nomg() also is not
part of the public API.) Another option is to rename it to be something
that indicates you can't use get magic on it, but we can't think of anything
short and sweet. Ideas welcome.

Is it really necessary for av_tindex_nomg(av) to assert that av is an AV?
And if not, why not just use AvFILLp(av) directly?

AS for SvGETMAGIC documentation, I'd say leave it alone. There's nothing
technically wrong with doing SvGETMAGIC() on an aggregate type: if it has
get magic, SvGETMAGIC() will call mg_get().

--
Diplomacy is telling someone to go to hell in such a way that they'll
look forward to the trip

p5pRT · 2016-03-18T00:32:08Z

From @khwilliamson

On 03/14/2016 02:59 AM, Dave Mitchell wrote:

On Sat, Mar 12, 2016 at 09:47:20AM -0700, Karl Williamson wrote:

It's unfortunate, given that there's so few of us coding this project, that
there was duplication of effort here. But I guess it happens infrequently
enough that it's tolerable.

Tony Cook (mostly him) and I also had been tracking this down, and
communicating on irc about it. I had discovered that adding a DEBUG_r
statement (that outputs 'fence') in the right place fixes the bug, and Tony
had discovered that declaring 'fence' to be volatile also fixes it.

I don't know if one approach is better than the others.

Well, my one's
a) already in blead
b) should be optimised away, so won't affect any code
so I think we might as well go with mine.

That's fine with me. But I notice that there are still problems on the
Tony's smoker.

Also, av_tindex_nomg() is a new construct that I added based on Tony's
observation that these internal AV's did not have length magic, so that
calling straight av_tindex() does useless work in checking for that magic.
I had also hoped that that would change things enough so the bug would go
away without us having to do anything else. But it didn't. Since then, Tony
has pointed out that the '_nomg' suffix could mislead people into thinking
this was parallel to the various sv_foo_nomg() functions, so they might try
to do
 SvGETMAGIC$ \(SV \*$ av\); IV x = av\_tindex\_nomg$av$;
and that doesn't work as they might expect it to. My current thinking on
this is to say in the SvGETMAGIC documentation that it only works on SV's,
and not on aggregate types. Since you need to cast the AV* to a SV* to get
the code to compile, that might be sufficient. (av_tindex_nomg() also is not
part of the public API.) Another option is to rename it to be something
that indicates you can't use get magic on it, but we can't think of anything
short and sweet. Ideas welcome.
Is it really necessary for av_tindex_nomg(av) to assert that av is an AV?
And if not, why not just use AvFILLp(av) directly?

Is it necessary really to use any assert()? It is here just a guard
against a programming mistake, and could be omitted.

I don't want to use AvFILLp() because it is obscure. I had no idea it
existed. It is not documented, and not mnemonic (at least to me, and my
guess to many other people as well). I asked on irc what the 'p' in the
name signified. No one knew, but there was speculation from looking at
the code that it stood for 'plain'. I had presumed it meant 'private',
with the analogy of many macros that are documented, such as SvNIOKp().
And regcomp.c has no business dealing with private fields of what
should be an encapsulated underlying layer. So I want to come up with a
mnemonic macro that preserves encapsulation. Another name that comes to
me is av_tindex_no_len_mg().

AS for SvGETMAGIC documentation, I'd say leave it alone. There's nothing
technically wrong with doing SvGETMAGIC() on an aggregate type: if it has
get magic, SvGETMAGIC() will call mg_get().

The code for av_tindex() indicates that SvGETMAGIC() does not work on
length magic for AV's. I don't know much about magic in perl, so I
presumed there was no other kind. I see that was wrong now.

p5pRT · 2016-03-18T23:11:25Z

From @iabyn

On Thu, Mar 17, 2016 at 06:30:38PM -0600, Karl Williamson wrote:

On 03/14/2016 02:59 AM, Dave Mitchell wrote:

On Sat, Mar 12, 2016 at 09:47:20AM -0700, Karl Williamson wrote:

It's unfortunate, given that there's so few of us coding this project, that
there was duplication of effort here. But I guess it happens infrequently
enough that it's tolerable.

Tony Cook (mostly him) and I also had been tracking this down, and
communicating on irc about it. I had discovered that adding a DEBUG_r
statement (that outputs 'fence') in the right place fixes the bug, and Tony
had discovered that declaring 'fence' to be volatile also fixes it.

I don't know if one approach is better than the others.

Well, my one's
a) already in blead
b) should be optimised away, so won't affect any code
so I think we might as well go with mine.

That's fine with me. But I notice that there are still problems on the
Tony's smoker.

Bother. I've just pushed 34f817b, which
uses Tony's suggested volatile approach instead.

Is it really necessary for av_tindex_nomg(av) to assert that av is an AV?
And if not, why not just use AvFILLp(av) directly?

Is it necessary really to use any assert()? It is here just a guard against
a programming mistake, and could be omitted.

There's a bit of continuum, and that assert seems to be at the less
necessary end. But (modulo compiler bugs) I guess it can't hurt.

I don't want to use AvFILLp() because it is obscure. I had no idea it
existed. It is not documented, and not mnemonic (at least to me, and my
guess to many other people as well). I asked on irc what the 'p' in the
name signified. No one knew, but there was speculation from looking at the
code that it stood for 'plain'. I had presumed it meant 'private', with the
analogy of many macros that are documented, such as SvNIOKp(). And
regcomp.c has no business dealing with private fields of what should be an
encapsulated underlying layer. So I want to come up with a mnemonic macro
that preserves encapsulation. Another name that comes to me is
av_tindex_no_len_mg().

Well, I personally find the names av_tindex*() just as obscure as
AvFILL*(), and just create confusion by having multiple names for the same
thing. I've you want to create a short-cut macro then please call it
av_top_index-something rather than av_tindex-something.

I think the 'p' suffix is well-established as a private or short-cut
version of another macro, e.g. GvIOp() verses GvIO(), but that only really
works with upper-case names.

How about av_top_index_x()? The _x is fairly non-mnemonic, but at least
isn't misleading like _nomg is.

Finally, if you are to have an access function/macro that explicitly only
works on objects without RMG, then it should definitely include an assert
to that effect. That seems much more important to me than asserting that
it's an AV.

AS for SvGETMAGIC documentation, I'd say leave it alone. There's nothing
technically wrong with doing SvGETMAGIC() on an aggregate type: if it has
get magic, SvGETMAGIC() will call mg_get().

The code for av_tindex() indicates that SvGETMAGIC() does not work on length
magic for AV's. I don't know much about magic in perl, so I presumed there
was no other kind. I see that was wrong now.

Well, what I said earlier may have been a bit too strong; its certainly
possible for an AV to acquire get magic, but I'm not sure that calling
SvGETMAGIC() on a AV would ever do anything useful.

--
More than any other time in history, mankind faces a crossroads. One path
leads to despair and utter hopelessness. The other, to total extinction.
Let us pray we have the wisdom to choose correctly.
-- Woody Allen

p5pRT · 2016-03-22T18:27:52Z

From @khwilliamson

That commit did fix this problem. There are other failures, covered in #127746
--
Karl Williamson

p5pRT · 2016-03-22T18:27:54Z

@khwilliamson - Status changed from 'open' to 'pending release'

p5pRT · 2016-05-13T04:04:42Z

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.

With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

p5pRT · 2016-05-13T04:04:43Z

@khwilliamson - Status changed from 'pending release' to 'resolved'

p5pRT · 2017-02-12T05:14:32Z

From @khwilliamson

On Fri, 18 Mar 2016 16:11:25 -0700, davem wrote:

On Thu, Mar 17, 2016 at 06:30:38PM -0600, Karl Williamson wrote:

On 03/14/2016 02:59 AM, Dave Mitchell wrote:

On Sat, Mar 12, 2016 at 09:47:20AM -0700, Karl Williamson wrote:
[snip]
regcomp.c has no business dealing with private fields of what should
be an
encapsulated underlying layer. So I want to come up with a mnemonic
macro
that preserves encapsulation. Another name that comes to me is
av_tindex_no_len_mg().

Well, I personally find the names av_tindex*() just as obscure as
AvFILL*(), and just create confusion by having multiple names for the
same
thing. I've you want to create a short-cut macro then please call it
av_top_index-something rather than av_tindex-something.

We actually went through this name process earlier starting with an RFC by me

http://www.nntp.perl.org/group/perl.perl5.porters/2012/10/msg194085.html

In that thread, you said you recalled AvFILL sounded strange when you first saw it, but you had come to know what it meant. I talked about AvFILLp, and how it didn't make much sense to me. In the meantime, I had forgotten completely that I had ever heard of AvFILLp. Anyway, that thread concluded with av_top_index() and av_tindex() being new synonyms with not dissent.

I think the 'p' suffix is well-established as a private or short-cut
version of another macro, e.g. GvIOp() verses GvIO(), but that only
really
works with upper-case names.

How about av_top_index_x()? The _x is fairly non-mnemonic, but at
least
isn't misleading like _nomg is.

In commit 9506e94, I changed the names to either
av_top_index_skip_len_mg
av_tindex_skip_len_mg

based on looking through the source and finding instances of where it uses 'skip_foo_mg'. I think that these are no longer misleading.

[snip]

--
Karl Williamson

p5pRT closed this as completed May 13, 2016

p5pRT added the Severity Low label Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

regex qr/(?[ [B] | ! ( [^B] ) ])/ fails to compile on Solaris x64 debugging optimized builds #15159

regex qr/(?[ [B] | ! ( [^B] ) ])/ fails to compile on Solaris x64 debugging optimized builds #15159

p5pRT commented Feb 2, 2016

p5pRT commented Feb 2, 2016

p5pRT commented Feb 23, 2016

p5pRT commented Mar 11, 2016

p5pRT commented Mar 11, 2016

p5pRT commented Mar 12, 2016

p5pRT commented Mar 12, 2016

p5pRT commented Mar 14, 2016

p5pRT commented Mar 18, 2016

p5pRT commented Mar 18, 2016

p5pRT commented Mar 22, 2016

p5pRT commented Mar 22, 2016

p5pRT commented May 13, 2016

p5pRT commented May 13, 2016

p5pRT commented Feb 12, 2017

regex qr/(?[ [B] | ! ( [^B] ) ])/ fails to compile on Solaris x64 debugging optimized builds #15159

regex qr/(?[ [B] | ! ( [^B] ) ])/ fails to compile on Solaris x64 debugging optimized builds #15159

Comments

p5pRT commented Feb 2, 2016

p5pRT commented Feb 2, 2016

From @tonycoz

p5pRT commented Feb 23, 2016

p5pRT commented Mar 11, 2016

p5pRT commented Mar 11, 2016

p5pRT commented Mar 12, 2016

From @iabyn

p5pRT commented Mar 12, 2016

From @khwilliamson

p5pRT commented Mar 14, 2016

From @iabyn

p5pRT commented Mar 18, 2016

From @khwilliamson

p5pRT commented Mar 18, 2016

From @iabyn

p5pRT commented Mar 22, 2016

From @khwilliamson

p5pRT commented Mar 22, 2016

p5pRT commented May 13, 2016

From @khwilliamson

p5pRT commented May 13, 2016

p5pRT commented Feb 12, 2017

From @khwilliamson