Gotos out of conditionals.

[p5pRT]

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

Searchable as RT1594$

[p5pRT]

From neild@zorg.hitchhiker.org

I have run across what appears to be a bug involving gotos out of
conditionals. The following code demonstrates the problem.

  # This subroutine triggers the bug.
  sub fn {
  my $s = "foo";
  if ($s eq "foo") {
  goto label;
  } elsif (0) {
  } else {
  label​:
  }
  }
  fn();

  # The following code merely serves to illustrate that something has
  # gone wrong; it fails with a "Modification of a read-only value" if
  # the above function is called, and runs normally otherwise.
  sub ctr { my $i = 0; return sub { $i++ }; }
  my $c = ctr;
  print $c->(), "\n";

The problem is that the context stack is becoming corrupted after the
goto statement. Things tend to get odd after this point -- the code
which initially triggered this problem caused a segfault. (Many thanks
to Purify for helping track down where things were going wrong. :>)

The following is an excerpt from the op tree generated by the above
code. (Specifically, the if/elsif/else statement.)

  {
5 TYPE = nextstate ===> 6
  FLAGS = (VOID)
  LINE = 3
  }
  {
  TYPE = null ===> (0)
  FLAGS = (UNKNOWN,KIDS,PARENS)
  {
9 TYPE = cond_expr ===> 14
  FLAGS = (UNKNOWN,KIDS)
  TRUE ===> 10
  FALSE ===> 15
  {
8 TYPE = seq ===> 9
  FLAGS = (SCALAR,KIDS)
  {
6 TYPE = padsv ===> 7
  TARG = 1
  FLAGS = (SCALAR)
  }
  {
7 TYPE = const ===> 8
  FLAGS = (SCALAR)
  SV = PV("foo")
  }
  }

Here is where things go wrong. The enter op in the following subtree
creates a new context. This context's oldcop is set to curop at the
time the enter op is executed -- specifically, the nextstate op back
at the beginning of this excerpt.

When pp_goto() executes, it walks up the context stack looking for
the label it references. At each level, the search begins at
cx->blk_oldcop->op_sibling. So, the search now will begin at the null
op at the start of this excerpt. This is wrong, however -- the search
should start at the leave op (#13) below. Since this is not done,
pp_goto() ends up thinking the label is in the same context as we are
currently executing in. This means the current context is never
properly unrolled.

It seems to me that there should be a nextstate op immediately
preceding the leave op (#13). In this case, pp_goto() will begin its
search in the correct place, fail to find the label on the first pass,
and correctly unroll the cxstack when it does find the label one level
up in the stack.

  {
13 TYPE = leave ===> 14
  FLAGS = (UNKNOWN,KIDS,PARENS)
  {
10 TYPE = enter ===> 11
  }
  {
11 TYPE = nextstate ===> 12
  FLAGS = (VOID)
  LINE = 4
  }
  {
12 TYPE = goto ===> 13
  FLAGS = (SCALAR)
  }
  }

  {
  TYPE = lineseq ===> (0)
  FLAGS = (UNKNOWN,KIDS)
  {
15 TYPE = nextstate ===> 16
  LINE = 5
  }
  {
18 TYPE = leave ===> 14
  FLAGS = (UNKNOWN,KIDS,PARENS)
  {
16 TYPE = enter ===> 17
  }
  {
17 TYPE = nextstate ===> 18
  FLAGS = (VOID)
  LINE = 7
  LABEL = "label"
  }
  {
  TYPE = null ===> (18)
  }
  }
  }
  }
  }

The following is a patch to perly.y. It appears to correct the op tree
generation. All tests continue to pass with it applied, and the sample
code above now runs correctly. This is, however, my first foray into
Perl's guts, and there may well be implications to this patch which I
am not seeing.

I only modify 'if' conditionals here. I presume the same change
needs to be made for 'unless' ones.

Inline Patch

--- /home/neild/Debugperl/perl5.005_03/perly.y	Sat Mar 27 10:04:11 1999
+++ perly.y	Sat Oct  9 22:36:06 1999
@@ -180,7 +180,8 @@
 cond	:	IF '(' remember mexpr ')' mblock else
 			{ PL_copline = $1;
 			    $$ = block_end($3,
-				   newCONDOP(0, $4, scope($6), $7)); }
+                                  newCONDOP(0, $4,
+                                    newSTATEOP(0, Nullch, scope($6)), $7)); }
 	|	UNLESS '(' remember miexpr ')' mblock else
 			{ PL_copline = $1;
 			    $$ = block_end($3,

The following is the same op tree excerpt as above, after applying this patch\.

Hmm. I only just now noticed that this op's LINE is now 8. (It is 3
in the unmodified Perl above.) I'm not certain why this is, or if it
is correct or not.

  {
5 TYPE = nextstate ===> 6
  FLAGS = (VOID)
  LINE = 8
  }

  {
  TYPE = null ===> (0)
  FLAGS = (UNKNOWN,KIDS,PARENS)
  {
9 TYPE = cond_expr ===> 15
  FLAGS = (UNKNOWN,KIDS)
  TRUE ===> 10
  FALSE ===> 16
  {
8 TYPE = seq ===> 9
  FLAGS = (SCALAR,KIDS)
  {
6 TYPE = padsv ===> 7
  TARG = 1
  FLAGS = (SCALAR)
  }
  {
7 TYPE = const ===> 8
  FLAGS = (SCALAR)
  SV = PV("foo")
  }
  }

Here is the only part of the tree (with the exception of the LINE value
above) which the patch changes. pp_goto() is much happier with this
version.

  {
  TYPE = lineseq ===> (0)
  FLAGS = (UNKNOWN,KIDS)
  {
10 TYPE = nextstate ===> 11
  LINE = 3
  }
  {
14 TYPE = leave ===> 15
  FLAGS = (UNKNOWN,KIDS,PARENS)
  {
11 TYPE = enter ===> 12
  }
  {
12 TYPE = nextstate ===> 13
  FLAGS = (VOID)
  LINE = 4
  }
  {
13 TYPE = goto ===> 14
  FLAGS = (SCALAR)
  }
  }
  }

  {
  TYPE = lineseq ===> (0)
  FLAGS = (UNKNOWN,KIDS)
  {
16 TYPE = nextstate ===> 17
  LINE = 5
  }
  {
19 TYPE = leave ===> 15
  FLAGS = (UNKNOWN,KIDS,PARENS)
  {
17 TYPE = enter ===> 18
  }
  {
18 TYPE = nextstate ===> 19
  FLAGS = (VOID)
  LINE = 7
  LABEL = "label"
  }
  {
  TYPE = null ===> (19)
  }
  }
  }
  }
  }

As I mentioned before, this is my first real foray into Perl's
internals. Comments from anyone who understands what they are doing
would be greatly appreciated.

In particular, I don't fully understand the role of the nextstate op.
It twiddles the stack and updates curcop -- what is the purpose of
curcop, though? pp_goto() makes assumptions about a relation between
nextstate ops and ops which add to the context stack; is this a valid
assumption for it to make?

  - Damien

Perl Info

Site configuration information for perl 5.00503:

Configured by torin at Wed Sep 22 00:18:38 PDT 1999.

Summary of my perl5 (5.0 patchlevel 5 subversion 3) configuration:
  Platform:
    osname=linux, osvers=2.0.36, archname=i386-linux
    uname='linux perv 2.0.36 #2 wed nov 18 03:00:48 pst 1998 i686 unknown '
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef useperlio=undef d_sfio=undef
  Compiler:
    cc='cc', optimize='-O2', gccversion=2.95.1 19990809 (prerelease)
    cppflags='-Dbool=char -DHAS_BOOL -D_REENTRANT -DDEBIAN -I/usr/local/include'
    ccflags ='-Dbool=char -DHAS_BOOL -D_REENTRANT -DDEBIAN -I/usr/local/include'
    stdchar='char', d_stdstdio=undef, usevfork=false
    intsize=4, longsize=4, ptrsize=4, doublesize=8
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    alignbytes=4, usemymalloc=n, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lndbm -lgdbm -ldbm -ldb -ldl -lm -lc -lposix -lcrypt
    libc=, so=so, useshrplib=false, libperl=libperl.a
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic'
    cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl 5.00503:
    /usr/lib/perl5/5.005/i386-linux
    /usr/lib/perl5/5.005
    /usr/local/lib/site_perl/i386-linux
    /usr/local/lib/site_perl
    /usr/lib/perl5
    .


Environment for perl 5.00503:
    HOME=/home/neild
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/opt/mame:/opt/pdev/bin:~/bin:/opt/jdk1.2/bin:/opt/jre1.2/bin:/opt/jikes-1.04:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/games
    PERL_BADLANG (unset)
    SHELL=/bin/bash

[p5pRT]

From @gsar

On 10 Oct 1999 06​:25​:04 -0000, neild@​zorg.hitchhiker.org wrote​:

I have run across what appears to be a bug involving gotos out of
conditionals. The following code demonstrates the problem.

# This subroutine triggers the bug.
sub fn {
my $s = "foo";
if ($s eq "foo") {
goto label;
} elsif (0) {
} else {
label​:
}
}
fn();

# The following code merely serves to illustrate that something has
# gone wrong; it fails with a "Modification of a read-only value" if
# the above function is called, and runs normally otherwise.
sub ctr { my $i = 0; return sub { $i++ }; }
my $c = ctr;
print $c->(), "\n";

The problem is that the context stack is becoming corrupted after the
goto statement. Things tend to get odd after this point -- the code
which initially triggered this problem caused a segfault. (Many thanks
to Purify for helping track down where things were going wrong. :>)
[...]
The following is a patch to perly.y. It appears to correct the op tree
generation. All tests continue to pass with it applied, and the sample
code above now runs correctly. This is, however, my first foray into
Perl's guts, and there may well be implications to this patch which I
am not seeing.

I only modify 'if' conditionals here. I presume the same change
needs to be made for 'unless' ones.

--- /home/neild/Debugperl/perl5.005_03/perly.y Sat Mar 27 10​:04​:11 1999
+++ perly.y Sat Oct 9 22​:36​:06 1999
@​@​ -180,7 +180,8 @​@​
cond : IF '(' remember mexpr ')' mblock else
{ PL_copline = $1;
$$ = block_end($3,
- newCONDOP(0, $4, scope($6), $7)); }
+ newCONDOP(0, $4,
+ newSTATEOP(0, Nullch, scope($6)), $7)); }
| UNLESS '(' remember miexpr ')' mblock else

I put in a different fix for the same problem a few months back. Please
see​:
  http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/1999-07/msg00953.html

Per my reading, it is the extra newSTATEOP() in the ELSIF production
which causes the trouble. If you try my patch above, please ignore
the pp_ctl.c change completely--it was an oops that got rolled back
later in​:
  ftp​://ftp.linux.activestate.com/pub/staff/gsar/APC/5.005_59/diffs/3878.gz

You test case above works fine with what I've got here.

In particular, I don't fully understand the role of the nextstate op.
It twiddles the stack and updates curcop -- what is the purpose of
curcop, though?

pp_nextstate() is responsible for cleaning up any temporaries that
build up at every statement. It also sets PL_curcop to itself,
so that any diagnostics and such may report proper line numbers.

            pp\_goto\(\) makes assumptions about a relation between

nextstate ops and ops which add to the context stack; is this a valid
assumption for it to make?

Yes. OPs that add to the context stack are all blockish constructs
(all of them do PUSHBLOCK() followed by whatever context they need
to maintain). nextstate is merely the op that is involved in
statement transitions *within* those blocks.

Sarathy
gsar@​activestate.com

[p5pRT]

From [Unknown Contact. See original ticket]

On Sun, Oct 10, 1999 at 02​:53​:01AM -0700, Gurusamy Sarathy wrote​:

I put in a different fix for the same problem a few months back. Please
see​:
http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/1999-07/msg00953.html

Per my reading, it is the extra newSTATEOP() in the ELSIF production
which causes the trouble. If you try my patch above, please ignore
the pp_ctl.c change completely--it was an oops that got rolled back
later in​:
ftp​://ftp.linux.activestate.com/pub/staff/gsar/APC/5.005_59/diffs/3878.gz

You test case above works fine with what I've got here.

Your change and mine share different behavior under some circumstances.
Consider​:

  sub fn {
  local $a = 1;
  my $s = "foo";
  if ($s eq "foo") {
  local $a = 2;
  print "a = $a\n";
  goto label;
  } elsif (0) {
  } else {
  label​:
  print "a = $a\n";
  }
  }

With my patch, this will print a = 2, a = 1. With your patch, this
prints a = 2, a = 2. (With an unpatched perl, it prints a = 2, a = 2;
however, if you swap the clauses around you can get it to print a = 1,
a = 2, so I don't think we should use it as a guideline.)

The difference, of course, is that in your patch considers all the clauses
of a conditional to be a single block (yet one with multiple leave/enter op
pairs), while mine treats the clauses as individual blocks.

I don't really have any preference for one behavior over the other. A
quick poll of the Perl programmers available to me seems to indicate
that having each clause be its own block (a = 2, a = 1) is the expected
behavior. Either way, perhaps a test case should be added to formalize
what should be happening?

  - Damien

[p5pRT]

From @gsar

On Sun, 10 Oct 1999 13​:06​:16 PDT, Damien Neil wrote​:

With my patch, this will print a = 2, a = 1. With your patch, this
prints a = 2, a = 2. (With an unpatched perl, it prints a = 2, a = 2;
however, if you swap the clauses around you can get it to print a = 1,
a = 2, so I don't think we should use it as a guideline.)

The difference, of course, is that in your patch considers all the clauses
of a conditional to be a single block (yet one with multiple leave/enter op
pairs), while mine treats the clauses as individual blocks.

I don't really have any preference for one behavior over the other. A
quick poll of the Perl programmers available to me seems to indicate
that having each clause be its own block (a = 2, a = 1) is the expected
behavior.

I don't disagree, but making them true blocks adds two "useless"
pp_nextstate()s to the call chain for the common case. I'd rather
it be fixed by making pp_goto()/dofindlabel() smarter about
leave OPs it encounters than by bloating the op tree.

     Either way\, perhaps a test case should be added to formalize

what should be happening?

I added a single test to t/op/goto.t when I patched it,
but if you can write a more comprehensive set of tests, that
would be wonderful.

Thanks.

Sarathy
gsar@​activestate.com

[p5pRT]

From [Unknown Contact. See original ticket]

On Sun, Oct 10, 1999 at 04​:20​:05PM -0700, Gurusamy Sarathy wrote​:

I don't really have any preference for one behavior over the other. A
quick poll of the Perl programmers available to me seems to indicate
that having each clause be its own block (a = 2, a = 1) is the expected
behavior.

I don't disagree, but making them true blocks adds two "useless"
pp_nextstate()s to the call chain for the common case. I'd rather
it be fixed by making pp_goto()/dofindlabel() smarter about
leave OPs it encounters than by bloating the op tree.

I can understand the desire not to bloat the op tree. Is the overhead
of making the clauses true blocks high enough that doing so is not an
option, or merely undesirable? It does appear to me that this is the
cleanest and most correct solution, but not if it comes at a significant
efficiency cost.

The current problem is that pp_goto()/dofindlabel() don't actually
encounter any leave OPs -- according to the assumptions they make about
the relation between nextstate and leave OPs (specifically, that every
leave be preceded by a nextstate), the start of the block entered in a
conditional clause is at the start of the conditional.

Either pp_goto() needs to be modified to specifically recognize blocks
associated with conditional clauses or the op tree needs to be changed.
The former is going to be a kludge any way we do it; perhaps there is
some flag which could be set on conditional clause blocks to indicate
they need special handling?

     Either way\, perhaps a test case should be added to formalize

what should be happening?

I added a single test to t/op/goto.t when I patched it,
but if you can write a more comprehensive set of tests, that
would be wonderful.

I'll write a test for this once I'm certain which behavior is going to
be declared the correct one. For the record, I'm strongly in favor of
having the different clauses of a conditional be separate blocks. (Which
means the current behavior is wrong, in my opinion.)

  - Damien

[p5pRT]

From @gsar

On Sun, 10 Oct 1999 17​:59​:16 PDT, Damien Neil wrote​:

I can understand the desire not to bloat the op tree. Is the overhead
of making the clauses true blocks high enough that doing so is not an
option, or merely undesirable?

Depends on how much the slowdown/memory bloat is.

Without actually benchmarking it, I'd guess that it would be a
noticeable efficiency hit.

                          It does appear to me that this is the

cleanest and most correct solution, but not if it comes at a significant
efficiency cost.

Doing redundant work is not very "clean". And "cleanest and most
correct" is a state of mind. :-)

The current problem is that pp_goto()/dofindlabel() don't actually
encounter any leave OPs -- according to the assumptions they make about
the relation between nextstate and leave OPs (specifically, that every
leave be preceded by a nextstate), the start of the block entered in a
conditional clause is at the start of the conditional.

Either pp_goto() needs to be modified to specifically recognize blocks
associated with conditional clauses or the op tree needs to be changed.
The former is going to be a kludge any way we do it; perhaps there is
some flag which could be set on conditional clause blocks to indicate
they need special handling?

I'd imagine that all pp_goto() needs to do would be to cook up a
LEAVE_SCOPE() out of the OP_COND_EXPR followed by reentering it
(by adding it to "enterops", perhaps) if the target of the jump is
not a sibling of the OP_GOTO, but is in a sibling optree of the
OP_LEAVE.

Another approach is to add the newSTATEOP()s as in your patch iff
there's a goto within the branches of a conditional. I think this
could be done in the peephole optimizer at compile-time.

Sarathy
gsar@​activestate.com