B::CC uses same names for lexical variables and temporary values

[Heinz Knutzen]

This is a follow up to my previous test case fail2.pl.

fail3.pl
=====
sub test {
my $tmp5 = 1;
my $x = 2;
if ($x != 3) { 4; }
}
=====

This fails with:
$ cc_harness -o fail3 fail3.c
...
fail3.c: In function ‘pp_sub_test’:
fail3.c:437: error: redeclaration of ‘d_tmp5’ with no linkage
fail3.c:437: note: previous declaration of ‘d_tmp5’ was here

B::CC translates some lexical variables of Perl to lexical variables of C.
The name of the variable is reused from Perl.
But additional variables are generated which hold temporary values with
names like "tmp$i".
The code in "sub load_pad" of B::CC doesn't check for any name clashes.
Hence we get errors with duplicate names like here and in the other test
case "fail2.pl".

--
Heinz Knutzen

[Reini Urban]

2010/8/16 Heinz Knutzen <heinz....@gmx.de>:

Thanks for the great analysis.
This seems to be easier to fix than I tought.

As a sidenote:
Do you think it is possible to add googlecode issues for future bugs
by yourself?
I'm not sure if you need a google account for this.

http://code.google.com/p/perl-compiler/issues/list

I added most of your reports to the issues and started adding them as
new testcase labeled t/issuenn.t so that we see when it is actually
fixed when regressions might appear.
Actually adding issues alone would be enough.
If you have a google account I'll happily add you as "Project
contributor". (to add and resolve issues)

The thing is that adding new general fails to TESTS will make the
testsuite MUCH slower, seperate issues.t are faster to compile and
test then the whole C+CC -O0..4 enchilada.
--
Reini Urban
http://phpwiki.org/           http://murbreak.at/

[Heinz Knutzen]

I would propose this patch, which fixes issue 35.

--- /usr/local/lib/perl/5.10.1/B/CC.pm 2010-07-23 23:55:39.000000000
+0200
+++ B/CC.pm 2010-08-17 13:32:08.000000000 +0200
@@ -604,7 +604,7 @@
my $namesv = $namelist[$ix];
my $type = T_UNKNOWN;
my $flags = 0;
- my $name = "tmp$ix";
+ my $name = "tmp";
my $class = class($namesv);
if ( !defined($namesv) || $class eq "SPECIAL" ) {
# temporaries have &PL_sv_undef instead of a PVNV for a name
@@ -628,8 +628,9 @@
$flags |= REGISTER if $3;
}
}
+ $name = "${ix}_$name";
$pad[$ix] =
- B::Stackobj::Padsv->new( $type, $flags, $ix, "i_$name", "d_$name" );
+ B::Stackobj::Padsv->new( $type, $flags, $ix, "i$name", "d$name" );

debug sprintf( "PL_curpad[$ix] = %s\n", $pad[$ix]->peek ) if
$debug{pad};
}

There seems to be some non-determinism in B::CC.
When applying this patch, the offending variables "x" and "tmp5" aren't
declared any longer in the resulting C code.

I had to change the test programs slightly to get them back.

fail2.pl
=====
sub test {
{ my $x = 1; my $y = $x + 1;}

my $x = 2;
if ($x != 3) { 4; }
}
=====

fail3.pl
=====
sub test {
my $tmp5 = 1;

my $x = $tmp5 + 1;

if ($x != 3) { 4; }
}
=====

> As a sidenote:
> Do you think it is possible to add googlecode issues for future bugs
> by yourself?
>

For joining this mailing list I needed a google account already.
I will add future issues to googlecode myself.

> If you have a google account I'll happily add you as "Project
> contributor". (to add and resolve issues)
>

What does it mean to resolve issues as contributor?
Will I be able to change the code?
--
Heinz Knutzen

[Reini Urban]

2010/8/17 Heinz Knutzen <heinz....@gmx.de>:

> I would propose this patch, which fixes issue 35.
>
> --- /usr/local/lib/perl/5.10.1/B/CC.pm    2010-07-23 23:55:39.000000000
> +0200
> +++ B/CC.pm    2010-08-17 13:32:08.000000000 +0200
> @@ -604,7 +604,7 @@
>     my $namesv = $namelist[$ix];
>     my $type   = T_UNKNOWN;
>     my $flags  = 0;
> -    my $name   = "tmp$ix";
> +    my $name   = "tmp";
>     my $class  = class($namesv);
>     if ( !defined($namesv) || $class eq "SPECIAL" ) {
>       # temporaries have &PL_sv_undef instead of a PVNV for a name
> @@ -628,8 +628,9 @@
>         $flags |= REGISTER if $3;
>       }
>     }
> +    $name = "${ix}_$name";
>     $pad[$ix] =
> -      B::Stackobj::Padsv->new( $type, $flags, $ix, "i_$name", "d_$name" );
> +      B::Stackobj::Padsv->new( $type, $flags, $ix, "i$name", "d$name" );

This does not smell right since the new names should
start with i_ or d_ for consistency.

--- lib/B/CC.pm (revision 521)
+++ lib/B/CC.pm (working copy)
@@ -605,7 +605,7 @@

my $namesv = $namelist[$ix];
my $type = T_UNKNOWN;
my $flags = 0;
- my $name = "tmp$ix";
+ my $name = "tmp";
my $class = class($namesv);
if ( !defined($namesv) || $class eq "SPECIAL" ) {
# temporaries have &PL_sv_undef instead of a PVNV for a name

@@ -629,6 +629,7 @@

$flags |= REGISTER if $3;
}
}

+ $name = "$name$ix";
$pad[$ix] =

B::Stackobj::Padsv->new( $type, $flags, $ix, "i_$name", "d_$name" );

d_tmp5 sounds better than d5_tmp

> There seems to be some non-determinism in B::CC.
> When applying this patch, the offending variables "x" and "tmp5" aren't
> declared any longer in the resulting C code.

I get d_x2 and d_tmp5 which looks good to me, without the changed test.

> I had to change the test programs slightly to get them back.
>
> fail2.pl
> =====
> sub test {
>    { my $x = 1; my $y = $x + 1;}
>    my $x = 2;
>    if ($x != 3) { 4; }
> }
> =====
>
> fail3.pl
> =====
> sub test {
>    my $tmp5 = 1;
>    my $x = $tmp5 + 1;
>    if ($x != 3) { 4; }
> }
> =====
>>
>> As a sidenote:
>> Do you think it is possible to add googlecode issues for future bugs
>> by yourself?
>>
>
> For joining this mailing list I needed a google account already.
> I will add future issues to googlecode myself.

Great! Thanks.
I didn't know that you need a google account for joining the mailing
list at all.
This sounds like a blocker for future contributors.

>> If you have a google account I'll happily add you as "Project
>> contributor". (to add and resolve issues)

> What does it mean to resolve issues as contributor?
> Will I be able to change the code?

If you come up with more good patches I'll give you svn write access also.
contributer has issue permissions, not svn I believe.
> --
> Heinz Knutzen
--
Reini Urban

[Heinz Knutzen]

> This does not smell right since the new names should start with i_ or
> d_ for consistency.
>

> d_tmp5 sounds better than d5_tmp

Only adding a number as suffix doesn't solve the problem.
If some name itselfs ends with a number we could get duplicates again.
Example:
x11 -> x111
x -> x2
...
y -> y10
x1 -> x111

Because of this problem I proposed to add the number after the starting "d" or "i".
But if you don't like that we should add it as suffix, but separated with "_":
$name = "$name_$ix";
--
Heinz Knutzen

[Heinz Knutzen]

Am I supposed to add a comment like this to issue 35?
Should it be sent to the mailing list as well?

> --
> Unsubscribe via mail to perl-compile...@googlegroups.com
> Options: http://groups.google.com/group/perl-compiler?hl=en

[Reini Urban]

2010/8/17 Heinz Knutzen <heinz....@gmx.de>:

> Am I supposed to add a comment like this to issue 35?
> Should it be sent to the mailing list as well?

Into the issue is better.

If it's of general interest to the list.

>> > This does not smell right since the new names should start with i_ or
>> > d_ for consistency.
>> >
>> > d_tmp5 sounds better than d5_tmp
>>
>> Only adding a number as suffix doesn't solve the problem.
>> If some name itselfs ends with a number we could get duplicates again.
>> Example:
>> x11 -> x111
>> x -> x2
>> ...
>> y -> y10
>> x1 -> x111
>>
>> Because of this problem I proposed to add the number after the starting
>> "d" or "i".

Hmm, okay then.

>> But if you don't like that we should add it as suffix, but separated with
>> "_":
>> $name = "$name_$ix";

I see.
tmp names are not affected because they have no name, but named lexicals are.
So we'll also have to add a test case also where have a named pad
with such a nameclash.

What is your google account name btw?
--
Reini