Skip to content

gh-91048: Refactor _testexternalinspection and add Windows support - #132852

Merged
pablogsal merged 10 commits into
python:mainfrom
pablogsal:gh-91048-2
Apr 25, 2025
Merged

gh-91048: Refactor _testexternalinspection and add Windows support#132852
pablogsal merged 10 commits into
python:mainfrom
pablogsal:gh-91048-2

Conversation

@pablogsal

@pablogsalpablogsal commented Apr 23, 2025

Copy link
Copy Markdown
Member

@pablogsal
pablogsal requested review from a team and erlend-aasland as code ownersApril 23, 2025 19:11
@pablogsal
pablogsalforce-pushed the gh-91048-2 branch 3 times, most recently from 66b7014 to b04fbc5CompareApril 23, 2025 19:28
@pablogsal
pablogsal requested a review from ambvApril 23, 2025 19:31
@pablogsal
pablogsalforce-pushed the gh-91048-2 branch 2 times, most recently from 46bf7d7 to e13c5a6CompareApril 23, 2025 19:35
Comment threadInclude/internal/pycore_remote_debug.h Outdated
@pablogsal
pablogsalforce-pushed the gh-91048-2 branch 2 times, most recently from 5ab86ad to 4ffe070CompareApril 23, 2025 20:15
Comment threadInclude/internal/pycore_remote_debug.h Outdated
Comment threadLib/test/test_external_inspection.py
@pablogsal

Copy link
Copy Markdown
MemberAuthor

Man, getting windows to work was NOT easy

@pablogsal
pablogsal merged commit e8cf3a1 into python:mainApr 25, 2025
@pablogsal
pablogsal deleted the gh-91048-2 branch April 25, 2025 13:12
@vstinner

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.

@vstinner

Copy link
Copy Markdown
Member

This change broke FreeBSD buildbot

I wrote #132945 to fix the build on FreeBSD.

@Fidget-Spinner

Copy link
Copy Markdown
Member

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

@pablogsal

Copy link
Copy Markdown
MemberAuthor

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

How? This should not affect the JIT in any way

@zooba

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

@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

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.

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
MemberAuthor
======================================================================
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.

@pablogsal

Copy link
Copy Markdown
MemberAuthor

I am looking into it

@Fidget-Spinner

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.

@pablogsal

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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

@pablogsal

Copy link
Copy Markdown
MemberAuthor

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

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.

@chris-eibl

chris-eibl commented Apr 25, 2025

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

@pablogsal

pablogsal commented Apr 25, 2025

Copy link
Copy Markdown
MemberAuthor

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

@pablogsal

Copy link
Copy Markdown
MemberAuthor

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

@pablogsal

Copy link
Copy Markdown
MemberAuthor

#132963

@chris-eibl

chris-eibl commented Apr 25, 2025

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.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@pablogsal@vstinner@Fidget-Spinner@zooba@chris-eibl@ambv