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

test_embed failures after switching configure options due to missing Makefile dependencies #114505

Closed
smontanaro opened this issue Jan 23, 2024 · 9 comments · Fixed by #114513
Closed
Assignees
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error

Comments

@smontanaro
Copy link
Contributor

smontanaro commented Jan 23, 2024

Bug report

Bug description:

test_embed started failing for me recently on free threaded builds on both M1 Mac and Ubuntu (Intel). git bisect narrowed it down to a suspect commit. @colesbury this is probably for you.

Unfortunately, on my Mac I get this commit as the culprit:

% git bisect bad
5f1997896d9c3ecf92e9863177c452b468a6a2c8 is the first bad commit
commit 5f1997896d9c3ecf92e9863177c452b468a6a2c8
Author: Sam Gross <colesbury@gmail.com>
Date:   Tue Jan 23 13:05:15 2024 -0500

    gh-112984: Fix link error on free-threaded Windows build (GH-114455)
    
    The test_peg_generator test tried to link the python313_d.lib library,
    which failed because the library is now named python313t_d.lib. The
    underlying problem is that the "compiler" attribute was not set when
    we call get_libraries() from distutils.

 Tools/peg_generator/pegen/build.py | 3 +++
 1 file changed, 3 insertions(+)

while in my Linux environment it says this:

% git bisect bad
b331381485c1965d1c88b7aee7ae9604aca05758 is the first bad commit
commit b331381485c1965d1c88b7aee7ae9604aca05758
Author: Sam Gross <colesbury@gmail.com>
Date:   Tue Jan 16 16:42:15 2024 -0500

    gh-112529: Track if debug allocator is used as underlying allocator (#113747)
    
    * gh-112529: Track if debug allocator is used as underlying allocator
    
    The GC implementation for free-threaded builds will need to accurately
    detect if the debug allocator is used because it affects the offset of
    the Python object from the beginning of the memory allocation. The
    current implementation of `_PyMem_DebugEnabled` only considers if the
    debug allocator is the outer-most allocator; it doesn't handle the case
    of "hooks" like tracemalloc being used on top of the debug allocator.
    
    This change enables more accurate detection of the debug allocator by
    tracking when debug hooks are enabled.
    
    * Simplify _PyMem_DebugEnabled

 Include/internal/pycore_pymem.h        |  3 +++
 Include/internal/pycore_pymem_init.h   |  2 ++
 Include/internal/pycore_runtime_init.h |  1 +
 Objects/obmalloc.c                     | 21 +++++++++++++++------
 4 files changed, 21 insertions(+), 6 deletions(-)

(I backed up farther on my Linux box to find a good commit.) 16 January was a long time ago. My inclination is that the later commit is the culprit.

I then tried with the GIL enabled on main. Still an error. Hmmm... I went through the bisect process again on my Mac and landed on this as the putative culprit commit:

% git bisect good
441affc9e7f419ef0b68f734505fa2f79fe653c7 is the first bad commit
commit 441affc9e7f419ef0b68f734505fa2f79fe653c7
Author: Sam Gross <colesbury@gmail.com>
Date:   Tue Jan 23 13:08:23 2024 -0500

    gh-111964: Implement stop-the-world pauses (gh-112471)
    
    The `--disable-gil` builds occasionally need to pause all but one thread.  Some
    examples include:
    
    * Cyclic garbage collection, where this is often called a "stop the world event"
    * Before calling `fork()`, to ensure a consistent state for internal data structures
    * During interpreter shutdown, to ensure that daemon threads aren't accessing Python objects
    
    This adds the following functions to implement global and per-interpreter pauses:
    
    * `_PyEval_StopTheWorldAll()` and `_PyEval_StartTheWorldAll()` (for the global runtime)
    * `_PyEval_StopTheWorld()` and `_PyEval_StartTheWorld()` (per-interpreter)
    
    (The function names may change.)
    
    These functions are no-ops outside of the `--disable-gil` build.

 Include/cpython/pystate.h              |   2 +-
 Include/internal/pycore_ceval.h        |   1 +
 Include/internal/pycore_interp.h       |  17 +++
 Include/internal/pycore_llist.h        |   3 +-
 Include/internal/pycore_pystate.h      |  51 +++++--
 Include/internal/pycore_runtime.h      |   7 +
 Include/internal/pycore_runtime_init.h |   3 +
 Include/pymacro.h                      |   3 +
 Python/ceval_gil.c                     |   9 ++
 Python/pystate.c                       | 269 +++++++++++++++++++++++++++++++--
 10 files changed, 336 insertions(+), 29 deletions(-)

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux, macOS

Linked PRs

@smontanaro smontanaro added the type-bug An unexpected behavior, bug, or error label Jan 23, 2024
@smontanaro smontanaro changed the title test_embed failing in free threaded build test_embed failing Jan 23, 2024
@colesbury colesbury self-assigned this Jan 23, 2024
@smontanaro
Copy link
Contributor Author

I should add that I originally tried this with a --disable-gil build. I later reproduced it on my Mac with the GIL enabled.

@colesbury
Copy link
Contributor

Hi Skip - I tried running test_embed on an M1 Mac (built from d22c066) and haven't seen any errors either with --disable-gil or with the default build.

What is the error you are seeing? Have you tried running make clean?

@smontanaro
Copy link
Contributor Author

Hrm... I guess it's a make dependency problem. After a make clean I'm not able to reproduce it either. Sorry for the noise.

FWIW, here's the error I was seeing:

======================================================================
FAIL: test_init_isolated_config (test.test_embed.InitConfigTests.test_init_isolated_config)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/skip/src/python/cpython/Lib/test/test_embed.py", line 1082, in test_init_isolated_config
    self.check_all_configs("test_init_isolated_config", api=API_ISOLATED)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_embed.py", line 800, in check_all_configs
    out, err = self.run_embedded_interpreter(testname,
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
                                             env=env, cwd=cwd)
                                             ^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_embed.py", line 119, in run_embedded_interpreter
    self.assertEqual(p.returncode, returncode,
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
                     "bad returncode %d, stderr is %r" %
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     (p.returncode, err))
                     ^^^^^^^^^^^^^^^^^^^^
AssertionError: -6 != 0 : bad returncode -6, stderr is 'Assertion failed: (rt_preconfig->isolated == 1), function check_preinit_isolated_config, file _testembed.c, line 1065.\n'

======================================================================
FAIL: test_preinit_isolated_config (test.test_embed.InitConfigTests.test_preinit_isolated_config)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/skip/src/python/cpython/Lib/test/test_embed.py", line 1079, in test_preinit_isolated_config
    self.check_all_configs("test_preinit_isolated_config", api=API_ISOLATED)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_embed.py", line 800, in check_all_configs
    out, err = self.run_embedded_interpreter(testname,
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
                                             env=env, cwd=cwd)
                                             ^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_embed.py", line 119, in run_embedded_interpreter
    self.assertEqual(p.returncode, returncode,
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
                     "bad returncode %d, stderr is %r" %
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     (p.returncode, err))
                     ^^^^^^^^^^^^^^^^^^^^
AssertionError: -6 != 0 : bad returncode -6, stderr is 'Assertion failed: (rt_preconfig->isolated == 1), function check_preinit_isolated_config, file _testembed.c, line 1054.\n'

----------------------------------------------------------------------
Ran 69 tests in 3.743s

FAILED (failures=2, skipped=5)
test test_embed failed

== Tests result: FAILURE ==

10 slowest tests:
- test_embed: 3.8 sec

1 test failed:
    test_embed

0:00:03 load avg: 20.85 Re-running 1 failed tests in verbose mode in subprocesses
0:00:03 load avg: 20.85 Run 1 test in parallel using 1 worker process
0:00:03 load avg: 20.85 [1/1/1] test_embed failed (2 failures)
...

That -6 != 0 error was seen on both Linux and MacOS.

@erlend-aasland
Copy link
Contributor

Have you tried running make clean?

I would recommend git clean -fdx between each build when bisecting; trust only a completely clean build.

@colesbury
Copy link
Contributor

Yeah, I think we don't have all the proper dependencies in the Makefile for _testembed.o:

cpython/Makefile.pre.in

Lines 1401 to 1402 in afe8f37

Programs/_testembed.o: $(srcdir)/Programs/_testembed.c Programs/test_frozenmain.h
$(CC) -c $(PY_CORE_CFLAGS) -o $@ $(srcdir)/Programs/_testembed.c

We are also missing a bunch of header dependencies in the definition of PYTHON_HEADERS.

@smontanaro
Copy link
Contributor Author

Can you change the title to reflect this deficiency and assign it to me? I'll see if I can improve that aspect of the make environment.

@colesbury colesbury changed the title test_embed failing test_embed failures after switching configure options due to missing Makefile dependencies Jan 23, 2024
@colesbury
Copy link
Contributor

Thanks Skip - I updated the title. I think you should be able to do that too -- there's an "Edit" button next to the tile and "New Issue" on this page.

@colesbury colesbury assigned smontanaro and unassigned colesbury Jan 23, 2024
@erlend-aasland
Copy link
Contributor

Can you change the title to reflect this deficiency and assign it to me? I'll see if I can improve that aspect of the make environment.

That would be a very welcome improvement :)

@erlend-aasland erlend-aasland added build The build process and cross-build and removed topic-free-threading labels Jan 23, 2024
@smontanaro
Copy link
Contributor Author

I guess the bigger issue was how you assigned it to me. I didn't see an obvious way to assign it to myself.

@erlend-aasland erlend-aasland linked a pull request Feb 7, 2024 that will close this issue
erlend-aasland pushed a commit that referenced this issue Feb 7, 2024
Also move PYTHON_HEADERS up and make _testembed.o depend on it.
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
Also move PYTHON_HEADERS up and make _testembed.o depend on it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants