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

gh-93696: Locate frozen module source with __file__ #93697

Merged
merged 14 commits into from
Oct 25, 2022

Conversation

SnoopJ
Copy link
Contributor

@SnoopJ SnoopJ commented Jun 10, 2022

See #93696, this patch allows pdb to locate the source for frozen stdlib modules. With the patch applied, the output of pdb is as expected:

$ git describe && ./python -V
v3.11.0b1-358-gc91afc1c0a
Python 3.12.0a0
$ ./python -m pdb repro.py 
> /home/snoopjedi/repos/cpython/repro.py(2)<module>()
-> import importlib._bootstrap
(Pdb) n
> /home/snoopjedi/repos/cpython/repro.py(5)<module>()
-> importlib._bootstrap._resolve_name("os", ".", 1)
(Pdb) s
--Call--
> <frozen importlib._bootstrap>(1037)_resolve_name()
(Pdb) l
1032        def __exit__(self, exc_type, exc_value, exc_traceback):
1033            """Release the import lock regardless of any raised exceptions."""
1034            _imp.release_lock()
1035 
1036 
1037 -> def _resolve_name(name, package, level):
1038        """Resolve a relative module name to an absolute one."""
1039        bits = package.rsplit('.', level - 1)
1040        if len(bits) < level:
1041            raise ImportError('attempted relative import beyond top-level package')
1042        base = bits[0]

I am not particularly familiar with the frozen module system so I'm not sure if there are edge cases where this will produce undesirable behavior, but thought that I would send it upstream just in case 😅

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Jun 10, 2022

Edit: I have now added a test to this PR, see follow-up comment

I did not include an additional test with this PR because I don't think there's a good way to generate a dummy frozen module for use in the test and it seems inappropriate to test against one of the stdlib modules. All the same, here's what the test would look like for the current version of importlib._bootstrap, in case there's some clever testing trick I missed out here.

click to see patch
diff --git a/Lib/test/test_pdb.py b/Lib/test/test_pdb.py
index 0141b739c2..894f3e9099 100644
--- a/Lib/test/test_pdb.py
+++ b/Lib/test/test_pdb.py
@@ -532,6 +532,40 @@ def test_list_commands():
     (Pdb) continue
     """
 
+def test_frozen_list():
+    '''Test the list command on frozen stdlib modules
+
+    >>> def test_function():
+    ...     import importlib._bootstrap
+    ...     import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
+    ...     importlib._bootstrap._resolve_name("os", ".", 1)
+
+    >>> with PdbTestInput([  # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE
+    ...     'step',
+    ...     'list',
+    ...     'continue',
+    ... ]):
+    ...     test_function()
+    > <doctest test.test_pdb.test_frozen_list[0]>(4)test_function()
+    -> importlib._bootstrap._resolve_name("os", ".", 1)
+    (Pdb) step
+    --Call--
+    > <frozen importlib._bootstrap>(1037)_resolve_name()
+    (Pdb) list
+    1032            def __exit__(self, exc_type, exc_value, exc_traceback):
+    1033                """Release the import lock regardless of any raised exceptions."""
+    1034                _imp.release_lock()
+    1035
+    1036
+    1037 ->     def _resolve_name(name, package, level):
+    1038            """Resolve a relative module name to an absolute one."""
+    1039            bits = package.rsplit('.', level - 1)
+    1040            if len(bits) < level:
+    1041                raise ImportError('attempted relative import beyond top-level package')
+    1042            base = bits[0]
+    (Pdb) continue
+    '''
+
 def test_pdb_whatis_command():
     """Test the whatis command

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Jun 12, 2022

Updated with a test that creates a module that suitably fakes the behavior of functions inside of frozen modules, in the sense that co_filename on the contained function starts with '<frozen', and the __file__ attribute of the module will point to the "real" source file that pdb should use for resolving list

@kumaraditya303 kumaraditya303 self-requested a review July 16, 2022 06:52
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jul 17, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

SnoopJ and others added 2 commits July 17, 2022 14:38
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Sep 29, 2022

@ericsnowcurrently does this patch need any changes?

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Oct 24, 2022

@JelleZijlstra generously offered to have a look-see at this during the 3.11 release stream, dropping this here to ping him :)

list
quit
"""
with open('gh93696.py', 'w') as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little weird that we're writing into the CWD, but that appears to be what the other tests in this file are doing too.

Lib/pdb.py Outdated
# this workaround can be removed with the closure of gh-89815
if filename.startswith("<frozen"):
if "__file__" in self.curframe.f_globals:
filename = self.curframe.f_globals["__file__"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if __file__ is not a string? It looks like get_file_breaks will crash in that case. We should probably just ignore __file__ in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's a good point. I would think it's a small edge, but it's easy to be more cautious about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's important for a debugger to be resilient in the face of unusual program state.

@JelleZijlstra JelleZijlstra merged commit d91de28 into python:main Oct 25, 2022
@miss-islington
Copy link
Contributor

Thanks @SnoopJ for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 25, 2022
…3697)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
(cherry picked from commit d91de28)

Co-authored-by: James Gerity <snoopjedi@gmail.com>
@bedevere-bot
Copy link

GH-98653 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 25, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 25, 2022
…3697)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
(cherry picked from commit d91de28)

Co-authored-by: James Gerity <snoopjedi@gmail.com>
@bedevere-bot
Copy link

GH-98654 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 25, 2022
@SnoopJ SnoopJ deleted the gh-93696-pdb-find-frozen-source branch October 25, 2022 13:23
miss-islington added a commit that referenced this pull request Oct 25, 2022
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
(cherry picked from commit d91de28)

Co-authored-by: James Gerity <snoopjedi@gmail.com>
miss-islington added a commit that referenced this pull request Oct 25, 2022
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
(cherry picked from commit d91de28)

Co-authored-by: James Gerity <snoopjedi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-importlib type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants