Skip to content

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

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

Merged
merged 10 commits into from
Apr 25, 2025

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 23, 2025

Signed-off-by: Pablo Galindo pablogsal@gmail.com

@pablogsal pablogsal requested review from a team and erlend-aasland as code owners April 23, 2025 19:11
@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
@pablogsal pablogsal force-pushed the gh-91048-2 branch 2 times, most recently from 5ab86ad to 4ffe070 Compare April 23, 2025 20:15
@pablogsal
Copy link
Member Author

Man, getting windows to work was NOT easy

@pablogsal pablogsal merged commit e8cf3a1 into python:main Apr 25, 2025
41 checks passed
@pablogsal pablogsal deleted the gh-91048-2 branch April 25, 2025 13:12
@vstinner
Copy link
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
Member

This change broke FreeBSD buildbot

I wrote #132945 to fix the build on FreeBSD.

@Fidget-Spinner
Copy link
Member

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

@pablogsal
Copy link
Member Author

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
Member

zooba commented Apr 25, 2025

======================================================================
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
Copy link
Member

Fidget-Spinner commented Apr 25, 2025

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
Copy link
Member Author

pablogsal commented Apr 25, 2025

======================================================================
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
Member Author

I am looking into it

@Fidget-Spinner
Copy link
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
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
Copy link
Member

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

@pablogsal
Copy link
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
Copy link
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
Copy link
Member

chris-eibl commented Apr 25, 2025

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
Copy link
Member Author

pablogsal commented Apr 25, 2025

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
Member Author

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

@pablogsal
Copy link
Member Author

#132963

@chris-eibl
Copy link
Member

chris-eibl commented Apr 25, 2025

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants