GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-91048: Refactor _testexternalinspection and add Windows support by pablogsal · Pull Request #132852 · python/cpython

Conversation

@pablogsal

@pablogsal pablogsal commented Apr 23, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Signed-off-by: Pablo Galindo [email protected]

  • Issue: Async Call-Stack Reconstruction #91048

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@pablogsal
pablogsal requested review from a team and erlend-aasland as code owners April 23, 2025 19:11
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 23, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Apr 23, 2025
Async Call-Stack Reconstruction #91048
Closed
@pablogsal pablogsal added the skip news label Apr 23, 2025
@pablogsal
pablogsal force-pushed the gh-91048-2 branch 3 times, most recently from 66b7014 to b04fbc5 Compare April 23, 2025 19:28
@pablogsal
pablogsal requested a review from ambv April 23, 2025 19:31
@pablogsal
pablogsal force-pushed the gh-91048-2 branch 2 times, most recently from 46bf7d7 to e13c5a6 Compare April 23, 2025 19:35
zooba
zooba reviewed Apr 23, 2025
View reviewed changes
Comment thread Include/internal/pycore_remote_debug.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal
pablogsal force-pushed the gh-91048-2 branch 2 times, most recently from 5ab86ad to 4ffe070 Compare April 23, 2025 20:15
zooba
zooba reviewed Apr 23, 2025
View reviewed changes
Comment thread Include/internal/pycore_remote_debug.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal
pablogsal force-pushed the gh-91048-2 branch from 2370931 to fe6a0cb Compare April 23, 2025 20:40
pablogsal
pablogsal commented Apr 23, 2025
View reviewed changes
Comment thread Lib/test/test_external_inspection.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 23, 2025

Copy link
Copy Markdown
Member Author

Man, getting windows to work was NOT easy

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal
pablogsal force-pushed the gh-91048-2 branch 3 times, most recently from 47efe9c to fe42a7f Compare April 24, 2025 01:16
@pablogsal pablogsal mentioned this pull request Apr 25, 2025
GH-114809: Add support for macOS multi-arch builds with the JIT enabled #131751
Merged
pablogsal and others added 8 commits April 25, 2025 01:59
@pablogsal
pythongh-91048: Refactor _testexternalinspection and add Windows support
4b40cb2
Signed-off-by: Pablo Galindo <[email protected]>
@pablogsal
remove write function
fb069ed
Signed-off-by: Pablo Galindo <[email protected]>
@pablogsal
Move everything to a heder file
33598e8
@pablogsal
windows fix
f6e124b
@pablogsal
windows fix
bcd4acb
@pablogsal
windows fix
1f61b55
@pablogsal
fix lint
0fc495f
@pablogsal
Fix test
e4e4e89
@pablogsal
pablogsal force-pushed the gh-91048-2 branch from fe42a7f to e4e4e89 Compare April 25, 2025 01:01
pablogsal added 2 commits April 25, 2025 02:10
@pablogsal
fix macos
97beb5d
@pablogsal
Move header file
22fdc33
@pablogsal
pablogsal merged commit e8cf3a1 into python:main Apr 25, 2025
@pablogsal
pablogsal deleted the gh-91048-2 branch April 25, 2025 13:12
@bedevere-app bedevere-app Bot removed the awaiting core review label Apr 25, 2025
@vstinner

vstinner commented Apr 25, 2025

Copy link
Copy Markdown
Member

This change broke FreeBSD buildbot:

--- Modules/_testexternalinspection.o ---
In file included from ./Modules/_testexternalinspection.c:20:
./Modules/../Python/remote_debug.h:695:15: error: call to undeclared function 'search_map_for_section'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
  695 |     address = search_map_for_section(handle, "PyRuntime", "libpython");
      |               ^
./Modules/_testexternalinspection.c:62:15: error: call to undeclared function 'search_map_for_section'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
   62 |     address = search_map_for_section(handle, "AsyncioDebug", "_asyncio.cpython");
      |               ^
2 errors generated.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 25, 2025

Copy link
Copy Markdown
Member

This change broke FreeBSD buildbot

I wrote #132945 to fix the build on FreeBSD.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Apr 25, 2025
gh-131798: JIT: Narrow the return type of _CALL_LEN to int #132940
Merged
Commit to trigger tail call and JIT CI #132953
Closed
@Fidget-Spinner

Fidget-Spinner commented Apr 25, 2025

Copy link
Copy Markdown
Member

Seems like this change broke JIT CI on main for Windows #132953

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

Seems like this change broke JIT CI on main for Windows #132953

How? This should not affect the JIT in any way

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zooba

zooba commented Apr 25, 2025

Copy link
Copy Markdown
Member
======================================================================
ERROR: test_async_gather_remote_stack_trace (test.test_external_inspection.TestGetStackTrace.test_async_gather_remote_stack_trace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_external_inspection.py", line 314, in test_async_gather_remote_stack_trace
    stack_trace = get_async_stack_trace(p.pid)
RuntimeError: Failed to read asyncio debug offsets

All look like that

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Seems like this change broke JIT CI on main for Windows #132953

How? This should not affect the JIT in any way

Yeah I'm confused as well. See what Steve said above about the tests failing though. I wonder if the JIT build lays out stuff differently in memory? No clue.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author
======================================================================
ERROR: test_async_gather_remote_stack_trace (test.test_external_inspection.TestGetStackTrace.test_async_gather_remote_stack_trace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_external_inspection.py", line 314, in test_async_gather_remote_stack_trace
    stack_trace = get_async_stack_trace(p.pid)
RuntimeError: Failed to read asyncio debug offsets

All look like that

No, that looks like whatever the JIT build is doing is breaking this feature, which is different. This PR makes that test run on windows and is failing, ergo is not that this broke the JIT but the other way around.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

I am looking into it

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 25, 2025

Copy link
Copy Markdown
Member

No, that looks like whatever the JIT build is doing is breaking this feature, which is different. This PR makes that test run on windows and is failing, ergo is not that this broke the JIT but the other way around

Yeah I believe the JIT broke the test internal inspection, but the JIT CI was passing before this, so if I'm being pedantic, the PR did add tests that broke the JIT CI :).

Pedantics aside, @chris-eibl is looking into it, and I'm looking too.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

Pedantics aside, @chris-eibl is looking into it, and I'm looking too.

Hold on I am looking into this I think I know what's going on

chris-eibl and Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@chris-eibl

chris-eibl commented Apr 25, 2025

Copy link
Copy Markdown
Member

But JIT must do something different on Windows, because Linux is green?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

But JIT must do something different on Windows, because Linux is green?

Windows is also green in the normal CI. My bet is that it has to be with release/vs not release and not related to the JIT

Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 25, 2025

Copy link
Copy Markdown
Member

But JIT must do something different on Windows, because Linux is green?

Windows is also green in the normal CI. My bet is that it has to be with release/vs not release and not related to the JIT

I think you're right. On the commit, Azure pipelines (which uses release builds for Windows) fails, but not the normal debug builds.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@chris-eibl

chris-eibl commented Apr 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Can confirm on a local Windows release 64bit build, that the tests fail:

======================================================================
ERROR: test_asyncgen_remote_stack_trace (test.test_external_inspection.TestGetStackTrace.test_asyncgen_remote_stack_trace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "e:\cpython_chris\Lib\test\test_external_inspection.py", line 246, in test_asyncgen_remote_stack_trace
    stack_trace = get_async_stack_trace(p.pid)
RuntimeError: Failed to read asyncio debug offsets
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Can confirm on a local Windows release 64bit build, that the tests fail:

I know what the problem is, I am working on a fix

Fidget-Spinner reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

As a takeaway: it would be good to get at least one release CI run on all PRs :)

Fidget-Spinner and chris-eibl reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

#132963

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@chris-eibl

chris-eibl commented Apr 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Yeah,

Dump of file amd64\_asyncio.pyd

File Type: DLL

  Summary

        2000 .data
        1000 .pdata
        6000 .rdata
        1000 .reloc
        1000 .rsrc
        8000 .text

dumpbin shows no AsyncioD section.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba left review comments
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland
@ambv ambv Awaiting requested review from ambv

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@pablogsal @vstinner @Fidget-Spinner @zooba @chris-eibl @ambv
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.