Modules used only in anonymous subs (including Try::Tiny) not saved when module in child namespace previously used

[GoogleCodeExporter]

Problem exists in ff8f39b (master) and release branches.

To reproduce:

1. Declare package Foo with new subroutine returning blessed reference, and 
another subroutine to perform an action on the blessed reference

2. Declare package Foo::Bar with a non-op subroutine baz to call later

3. Call Foo::Bar::baz()

4. Call Foo->new in an anonymous CODE ref, and save the result to a scalar

5. Call $foo->method

Please see the attached test case for a more explicit example of how to 
reproduce this problem.  Under plain Perl, the call to $foo->method succeeds, 
but when compiled with perlcc, package Foo is not saved:

-- snip --
Can't locate object method "method" via package "Foo" at test.pl line 26.
-- snip --

An analysis of this issue is as follows, with reference to this specific test 
case:

1. B::GV::savecv() is called for each method in Foo, but it never gets to save 
Foo to %dumped_package because nothing had previously put it into 
%include_package.

2. mark_package() is called later to put Foo into %include_package, however.

3. method_named() is also called later and decides once again to ensure Foo is 
put into %include_package.

4. inc_cleanup() is called a bit later, and calls delete_unsaved_hashINC() to 
remove Foo from %curINC because Foo was not placed into %dumped_package earlier.

In order to better understand what is NOT happening in this case, one must 
study the code more closely, and follow along what happens when Foo->new is 
called outside of an anonymous subroutine.

1. method_named() is called for each operation performed under Foo, and for 
those where it finds a method defined in Foo itself, it will ensure Foo is 
placed into %include_package early on.

2. B::GV::savecv() is called a bit later for each method in Foo.  Since 
method_named() was called previously for Foo code, B::GV::savecv() is able to 
put Foo into %dumped_packages.

3. When inc_cleanup() is called much later, delete_unsaved_hashINC() is never 
called because the previous sequence of events happened earlier.

-fwalkall, in theory, should help us work around the issue, but in actual 
practice it does not.  The same sequence of events occur when using it as in 
the first example, starting with B::GV::savecv() considering saving the package 
to %dumped_package, though that does not happen.

Workaround: When a package is being erroneously excluded when used directly by 
bareword name inside an anonymous subroutine, call it outside an anonymous 
subroutine first, in some way.

Original issue reported on code.google.com by erin.schoenhals on 17 Jun 2014 at 8:32

Attachments:

• foo.pl

[GoogleCodeExporter]

Great analysis! We really need to improve our should_save() logic, looking at 
%INC, and not if it's later used.
We cannot reliable catch such method calls, and the risc is too high missing 
those. Only internally used packages might be skipped, never external packages.

Original comment by reini.urban on 17 Jun 2014 at 9:50

• Changed state: Started
• Added labels: Priority-High, Effort-Medium
• Removed labels: Priority-Medium

[GoogleCodeExporter]

I've attached a patch which resolves the issue in both the original, isolated 
test case provided above, as well as an issue affecting cPanel production 
systems.

Rationale:

Given the example packages Foo and Foo::Bar as illustrated in the body of my 
original post, when calling should_save() when passing Foo, should_save() 
should not prevent Foo from being stored in %dumped_package simply because it 
is in a parent namespace to one of the modules it is considering optimizing 
out, in this case Foo::Bar.

Ordinarily, Foo would have already been stored in %dumped_package, but its 
first usage (as illustrated in foo.pl) was evaluated in a deferred way inside 
an anonymous subroutine, leaving should_save() the first chance for Foo to 
reached %dumped_package.  This patch ensures Foo is stored in %dumped_package 
by preventing Foo from being optimized out simply because Foo::Bar exists in 
%include_package; when evaluating Foo::Bar, if Foo exists in %include_package 
already, then Foo should be kept in %dumped_package.

This patch is ideal because:

1. It works regardless of how the package was introduced into Perl; either 
inline or from an external file

2. If a program references code in Foo::Bar, which in turn use()s Foo, but no 
code in Foo is ever touched, then Foo::Bar will remain, but Foo will still be 
optimized out.

Original comment by erin.schoenhals on 19 Jun 2014 at 10:12

Attachments:

• 348.patch

[GoogleCodeExporter]

Erin, don't want to come up with a testcase (t/issue348.t) and a git patch 
also? foo.pl misses Foo::new still

Original comment by reini.urban on 23 Jun 2014 at 1:56

[GoogleCodeExporter]

I uploaded the incorrect test file when reporting my analysis.  The correct 
file is below.  The main difference is that I forgot to declare the package Foo 
at all, instead declaring a package called Cat.  Changing the sample provides a 
better illustration of the issue and the corresponding fix.

Original comment by erin.schoenhals on 30 Jun 2014 at 8:56

Attachments:

• foo.pl

[GoogleCodeExporter]

348.patch in my earlier comment appears to function 100% of the time, when 
compiling the newly-uploaded test case above.

Original comment by erin.schoenhals on 30 Jun 2014 at 9:06

[GoogleCodeExporter]

Original comment by todd.e.rinaldo on 1 Jul 2014 at 6:46

• Changed title: Modules used only in anonymous subs (including Try::Tiny) not saved when module in child namespace previously used

[GoogleCodeExporter]

A pretty good version if dumping all previously missed modules is now in the 
branch `walkall`

Original comment by reini.urban on 2 Jul 2014 at 2:25

[GoogleCodeExporter]

Fails currently (walkall 2ca2bbf) only for threaded.

Considering Foo
...
And because Foo is not in %INC we don't dump it. Which is wrong.

Original comment by reini.urban on 3 Jul 2014 at 3:32

[GoogleCodeExporter]

Fixed in master with commit 0e09a80d18fd9487231f224dc78d54206d7ab418
Author: Reini Urban <rurban@cpanel.net>
Date:   Mon Jun 23 11:03:48 2014 -0600

    C 1.47_03: revised walkall, dump_rest at inc_cleanup

Original comment by reini.urban on 3 Jul 2014 at 8:38

• Changed state: Fixed