Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

charset.t and subst.t fail on Solaris under -Duse64bitall #15240

Closed
p5pRT opened this issue Mar 19, 2016 · 16 comments
Closed

charset.t and subst.t fail on Solaris under -Duse64bitall #15240

p5pRT opened this issue Mar 19, 2016 · 16 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 19, 2016

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

Searchable as RT127746$

@p5pRT
Copy link
Author

p5pRT commented Mar 19, 2016

From @khwilliamson

This is a recently introduced bug, in the last month or so. It is failing some locale-related tests. When I run a sample test by hand from the command line, it works. Under DEBUGGING, it works, as well as use re Debug.

I'm having trouble bisecting on that platform
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 22, 2016

From @khwilliamson

There are also some other failures now, under threads

../t/re/subst.t.............................................FAILED
  247-248
../t/re/subst_wamp.t........................................FAILED
  247-248
../t/re/substT.t............................................FAILED
  247-248

--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 23, 2016

From @khwilliamson

Tony Cook bisected the charset to

commit ba6840f
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sun Mar 6 13​:56​:44 2016 +0100

  fix Perl #126182, out of memory due to infinite pattern recursion

I'll look to see if that is also the source of the subst.t problems.
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 23, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Mar 24, 2016

From @khwilliamson

On Wed Mar 23 15​:52​:48 2016, khw wrote​:

Tony Cook bisected the charset to

commit ba6840f
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sun Mar 6 13​:56​:44 2016 +0100

 fix Perl \#126182\, out of memory due to infinite pattern recursion

I'll look to see if that is also the source of the subst.t problems.

And it is the source for subst.t
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Apr 1, 2016

From @iabyn

On Thu, Mar 24, 2016 at 04​:56​:44PM -0700, Karl Williamson via RT wrote​:

On Wed Mar 23 15​:52​:48 2016, khw wrote​:

Tony Cook bisected the charset to

commit ba6840f
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sun Mar 6 13​:56​:44 2016 +0100

 fix Perl \#126182\, out of memory due to infinite pattern recursion

I'll look to see if that is also the source of the subst.t problems.

And it is the source for subst.t

Are any of the usual suspects (Karl, Yves, Tony) already actively looking
at this? If not I'll have a look now.

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

@p5pRT
Copy link
Author

p5pRT commented Apr 1, 2016

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2016-04-01T10​:36​:54]

And it is the source for subst.t

Are any of the usual suspects (Karl, Yves, Tony) already actively looking
at this? If not I'll have a look now.

I emailed Yves just this morning to ask him to have a look.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Apr 3, 2016

From @demerphq

On 1 April 2016 at 16​:36, Dave Mitchell <davem@​iabyn.com> wrote​:

On Thu, Mar 24, 2016 at 04​:56​:44PM -0700, Karl Williamson via RT wrote​:

On Wed Mar 23 15​:52​:48 2016, khw wrote​:

Tony Cook bisected the charset to

commit ba6840f
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sun Mar 6 13​:56​:44 2016 +0100

 fix Perl \#126182\, out of memory due to infinite pattern recursion

I'll look to see if that is also the source of the subst.t problems.

And it is the source for subst.t

Are any of the usual suspects (Karl, Yves, Tony) already actively looking
at this? If not I'll have a look now.

I don't have access to Solaris, and I cannot replicate on my box. I
tried using valgrind as well. So if you have a Solaris box, and/or you
want to check out the code then please do. I will note that there were
several related follow up patches to the one found in the bisect
check. I did various changes to the regex engine in that patch
sequence but I think the relevant patches are as follows​:

commit 595de76
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Mon Mar 14 20​:03​:22 2016 +0100

  fix perl #127705, incorrect restoration of state during EVAL/GOSUB

  We were not restoring the cur_curlyx property properly during our funky
  stack traversal when we really need to. This was a longstanding bug in this
  code which my patches appear to have "tickled", although I am not sure why.

commit d1c49ad
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Mon Mar 14 19​:58​:33 2016 +0100

  add consistency with other union members

  In most cases the curlyx member is the first thing after the yes state
  member, but eval was reversed. While debugging perl #127705 I switched
  them to see what would happen, which changed the bug, and ultimately
  revealed the cause of the problem. So I am going to leave them in the
  "consistent" order.

commit 401a802
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sun Mar 13 11​:15​:30 2016 +0100

  [perl #126182] rework pattern GOSUB infinite recursion detection

  In ba6840f I tried to fix
  [perl #126182] which is a bug about us failing to detect regex
  left recursion in some cases.

  There were two problems with that patch, both pointed out by
  Zefram. The first is that I made left recursion a match fail,
  instead of throwing an exception, this makes left-recursion match
  sometimes, but at least sometimes in what is arguably the wrong
  way. Zefram was able to convince me that dying is better than
  matching incorrectly.

  The second patch was that it ignored some subtleties in how
  the backtracking stack works, which affected how the patch
  restored the recurse_locinput[] data which is used to track
  what position a GOSUB was entered from. This meant that in
  various cases it would not be restored correctly, and we would
  still infinite recurse. I believe that it works correctly now.

  Thanks for Zefram for the feedback on the original patch.

Cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Apr 3, 2016

From @iabyn

On Sun, Apr 03, 2016 at 10​:55​:10AM +0200, demerphq wrote​:

On 1 April 2016 at 16​:36, Dave Mitchell <davem@​iabyn.com> wrote​:

On Thu, Mar 24, 2016 at 04​:56​:44PM -0700, Karl Williamson via RT wrote​:

On Wed Mar 23 15​:52​:48 2016, khw wrote​:

Tony Cook bisected the charset to

commit ba6840f
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sun Mar 6 13​:56​:44 2016 +0100

 fix Perl \#126182\, out of memory due to infinite pattern recursion

I'll look to see if that is also the source of the subst.t problems.

And it is the source for subst.t

Are any of the usual suspects (Karl, Yves, Tony) already actively looking
at this? If not I'll have a look now.

I don't have access to Solaris, and I cannot replicate on my box.

I can replicate it on Solaris.

It can be reduced to this code​:

  use POSIX;
  POSIX​::setlocale(&POSIX​::LC_ALL, 'de_DE.UTF-8')
  or die "Couldn't change locale";

  my $a = "A";
  utf8​::upgrade($a);
  print "MATCH\n" if $a =~ qr/.\b$/l;

Solaris fails to match.

Putting debugging printf's in S_regmatch() around the line​:

  while (scan != NULL) {

which converts it to​:

  if (PL_debug)
  PerlIO_printf(Perl_debug_log,
  "at while top​: PL_charclass[n]=0x%x\n", PL_charclass[(U8) ('\n')]);

  if (PL_debug)
  PerlIO_printf(Perl_debug_log,
  "at while top #2​: PL_charclass[n]=0x%x\n", PL_charclass[(U8) ('\n')]);

  while (scan != NULL) {
  if (PL_debug)
  PerlIO_printf(Perl_debug_log,
  "at while top inside​: PL_charclass[n]=0x%x\n", PL_charclass[(U8) ('\n')]);

shows that when first entering the loop while executing the qr/.\b$/l,
I see this output​:

  at while top​: PL_charclass[n]=0x90e400
  at while top #2​: PL_charclass[n]=0x90e400
  at while top inside​: PL_charclass[n]=0xffff80ff

i.e. PL_charclass appears to get corrupted when entering the while loop.
I suspect a compiler bug. I'll look further tomorrow.

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

@p5pRT
Copy link
Author

p5pRT commented Apr 4, 2016

From @khwilliamson

On 04/03/2016 03​:01 PM, Dave Mitchell via RT wrote​:

i.e. PL_charclass appears to get corrupted when entering the while loop.
I suspect a compiler bug. I'll look further tomorrow.

PL_charclass is supposed to be const

@p5pRT
Copy link
Author

p5pRT commented Apr 4, 2016

From @iabyn

On Sun, Apr 03, 2016 at 11​:04​:30PM -0600, Karl Williamson wrote​:

On 04/03/2016 03​:01 PM, Dave Mitchell via RT wrote​:

i.e. PL_charclass appears to get corrupted when entering the while loop.
I suspect a compiler bug. I'll look further tomorrow.

PL_charclass is supposed to be const

Provisionally fixed by the commit shown below - but we'll have to see what
the smokes do.

I'm also slightly uncomfortable with putting lines like the following
directly in regexec.c​: I have a vague feeling that this sort of thing
aught to be in hints/solaris_2.sh instead​:

+#if (defined(__SUNPRO_C) && (__SUNPRO_C == 0x5120) && defined(__x86_64) && defined(USE_64_BIT_ALL))

so I'm open to suggestions on how to improve this commit.

commit c5d7841
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Mon Apr 4 13​:27​:35 2016 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Apr 4 14​:32​:51 2016 +0100

  S_regmatch()​: work around Solaris optimiser bug
 
  Recently these test scripts started failing on Solaris i386 with
  -Duse64bitall​:
 
  re/charset.t
  re/subst.t
  re/substT.t
  re/subst_wamp.t
 
  The first failure is due to a Solaris Studio optimiser bug; this commit
  works around that failure. It may or may not also fix the other failures
  (all tests pass now on the development box I was using, but I didn't
  confirm whether the other tests failed before the fix).
 
  The basic problem is that within the main while loop in S_regmatch(),
  the expression PL_charclass['\n'] returns the wrong value.
 
  Looking at the disassembled code, it appears that looking up this value
  just before the while loop correctly does it by getting the address of the
  global array PL_charclass, adding 40 to it (ord('\n')*sizeof(U32)), then
  dereffing it.
 
  Within the loop however, it just reads the value from a local var. The
  optimiser is under the misapprehension that this value has previously been
  read in and assigned to a local var, but it certainly hasn't been at the
  point where the while loop is first entered.
 
  This commit works round the problem on builds with -Duse64bitall and the
  bad version of Solaris Studio compiler (12.3), by making a copy of
  PL_charclass's address.
 
  This is a bit of hack. For one thing, I don't know whether other versions
  of the compiler also need this workaround.
 
  The failures were seen on a 32-bit system with -Duse64bitall​: this created
  a 64-bit executable (i.e. with 64-bit pointers) with the -m64 compiler
  flag. I would speculate that the bug in the compiler relates to to
  this particular circumstance (i.e. the bug might not appear for a build
  done on a 64-bit OS host with or without -Duse64bitall).

Affected files ...
  M regexec.c

Differences ...

Inline Patch
diff --git a/regexec.c b/regexec.c
index 0c549df..9acb5a0 100644
--- a/regexec.c
+++ b/regexec.c
@@ -5232,6 +5232,13 @@ S_regmatch(pTHX_ regmatch_info *reginfo, char *startpos, regnode *prog)
     bool is_utf8_pat = reginfo->is_utf8_pat;
     bool match = FALSE;
 
+/* Solaris Studio 12.3 messes up fetching PL_charclass['\n'] */
+#if (defined(__SUNPRO_C) && (__SUNPRO_C == 0x5120) && defined(__x86_64) && defined(USE_64_BIT_ALL))
+#  define SOLARIS_BAD_OPTIMIZER
+    const U32 *pl_charclass_dup = PL_charclass;
+#  define PL_charclass pl_charclass_dup
+#endif
+
 #ifdef DEBUGGING
     GET_RE_DEBUG_FLAGS_DECL;
 #endif
@@ -8316,6 +8323,9 @@ NULL
             /* NOTREACHED */
 	}
     }
+#ifdef SOLARIS_BAD_OPTIMIZER
+#  undef pl_charclass_dup
+#endif
 
     /*
     * We get here only if there's trouble -- normally "case END" is



-- 

Art is anything that has a label (especially if the label is "untitled 1")

@p5pRT
Copy link
Author

p5pRT commented Apr 4, 2016

From @iabyn

On Mon, Apr 04, 2016 at 02​:39​:00PM +0100, Dave Mitchell wrote​:

On Sun, Apr 03, 2016 at 11​:04​:30PM -0600, Karl Williamson wrote​:

On 04/03/2016 03​:01 PM, Dave Mitchell via RT wrote​:

i.e. PL_charclass appears to get corrupted when entering the while loop.
I suspect a compiler bug. I'll look further tomorrow.

PL_charclass is supposed to be const

Provisionally fixed by the commit shown below - but we'll have to see what
the smokes do.

... plus follow-up​:

commit 1994952
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Mon Apr 4 14​:51​:40 2016 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Apr 4 14​:51​:40 2016 +0100

  Follow-up to previous Solaris/S_regmatch commit
 
  PL_charclass is the thing being temporarily #defined to something else, so
  it's the thing that needs #undeffing at the end, not pl_charclass_dup.
 
  Doesn't make any difference at the moment, but would if at some point
  a function following S_regmatch() started making use of PL_charclass.

Affected files ...
  M regexec.c

Differences ...

Inline Patch
diff --git a/regexec.c b/regexec.c
index 9acb5a0..cdaa95c 100644
--- a/regexec.c
+++ b/regexec.c
@@ -8324,7 +8324,7 @@ NULL
 	}
     }
 #ifdef SOLARIS_BAD_OPTIMIZER
-#  undef pl_charclass_dup
+#  undef PL_charclass
 #endif
 
     /*


-- 

Technology is dominated by two types of people​: those who understand what
they do not manage, and those who manage what they do not understand.

@p5pRT
Copy link
Author

p5pRT commented Apr 7, 2016

From @khwilliamson

These patches fix the problem on our Solaris smokers
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Apr 7, 2016

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

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

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 p5pRT closed this as completed May 13, 2016
@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant