[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

Just noting that while I've closed this issue as fixed, I think we still have 
quite a bit of work to do to get our handling of these pre-init config settings 
to a place we're genuinely happy with (or as happy as backwards compatibility 
constraints permit).

However, the next step in that process is likely to involve at least myself, 
Victor & Eric getting together in person to discuss it at PyCon US (as I'm 
pretty sure we'll all be there this year), and deciding on how we'd like PEP 
432 to look for Python 3.8.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Change by Nick Coghlan :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

Harmut, thanks for the status update!

Victor, the problem is that both sys.warnoptions and sys._xoptions get read by 
Py_Initialize(), so setting them afterwards is essentially pointless - while 
you do still change the contents of the sys attributes, doing so doesn't have 
any further effect.

In 3.8, we'll hopefully make an updated version of PEP 432 public, and hence be 
able to deprecate calling them prior to Py_InitializeCore, and either advise 
folks to use the the new config structs instead, or else adjust the way that 
_PySys_BeginInit and _PySys_EndInit work so that the relevant sys module 
attributes get created in Py_InitializeCore rather than in 
Py_InitializeMainInterpreter.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Hartmut Goebel]

Hartmut Goebel  added the comment:

I can confirm that c6d94c37f4fd863c73fbfbcc918fd23b458b5301 makes the 
PyInstaller test-case, which war the origin of this bug-report, pass.

Thanks for fixing!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[STINNER Victor]

STINNER Victor  added the comment:

Would it be possible to not officially allow calling PySys_AddWarnOption() 
before Py_Initialize()? Fix the bug, but don't promote doing that. I just 
suggest to revert:

Doc/c-api/init.rst:

+  * :c:func:`PySys_AddWarnOption`
+  * :c:func:`PySys_AddXOption`
+  * :c:func:`PySys_ResetWarnOptions`

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:


New changeset bc77eff8b96be4f035e665ab35c1d06e22f46491 by Nick Coghlan in 
branch 'master':
bpo-33042: Fix pre-initialization sys module configuration (GH-6157)
https://github.com/python/cpython/commit/bc77eff8b96be4f035e665ab35c1d06e22f46491


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

While I've merged an initial fix for this (so it should be working again in 
3.7.0b3), I'm not going to close the issue until folks have had a chance to 
review and comment on the linked list based approach I've taken.

--
versions: +Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[miss-islington]

miss-islington  added the comment:


New changeset c6d94c37f4fd863c73fbfbcc918fd23b458b5301 by Miss Islington (bot) 
in branch '3.7':
bpo-33042: Fix pre-initialization sys module configuration (GH-6157)
https://github.com/python/cpython/commit/c6d94c37f4fd863c73fbfbcc918fd23b458b5301


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[miss-islington]

Change by miss-islington :


--
pull_requests: +5971

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

I added the new test case, and found to my surprise that it didn't fail, even 
in debug mode (where there aren't any default filters).

The point I had missed was that even though warnoptions can be NULL in 
CoreConfig, _Py_MainInterpreterConfig_Read ensures that it's never NULL for the 
main interpreter config (which is what _PySys_EndInit reads), which means that 
neither we nor any embedding application can currently trigger the code path in 
get_warnoptions() that lazily creates the warnings list without adding the 
reference back into the config settings.

We'll need to fix that before we make the new configuration API public, but for 
now I'm just going to note it in the source code as a "PEP432 TODO".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

PR has been updated to be mostly complete (just pending docs changes now), but 
I think I've found a potential issue with the interaction between the way I've 
currently implemented it and the way _Py_InitializeCore and 
_Py_InitializeMainInterpreter work.

Specifically, sys.warnoptions and sys._xoptions don't get created until 
_PySys_EndInit, so that's where I added the code to read the pre-initialization 
linked lists and move those values into sys.warnoptions and sys._xoptions. The 
current test is just checking that those sys attribute have the expected 
contents - it isn't checking that the consequences of those settings have 
correctly propagated to the warnings filter list.

For the default filters add by `_PyWarnings_Init` at the end of 
`_Py_InitializeCore`, I think that's fine - we're going to want the embedding 
application's filters to be add after the default filter list anyway.

However, the code in `_PyInitialize_MainInterpreter` to actually import the 
warnings module (which then reads `sys.warnoptions`) is guarded by a check for 
a non-empty config->warnoptions, and that's not going to trip in the case where 
get_warnoptions() has created a new sys.warnoptions list, and 
config->warnoptions is still NULL.

Rather than changing the preinit sys module code to be config-aware, I'm 
thinking that what I'd like to do is:

1. Update the new test case to also check that the most recent 3 entries in the 
warnings filter list match what we expect
2. Assuming that fails (as I expect it will), change the guard in 
_Py_InitializeMainInterpreter to check PySys_HasWarnOptions (which will 
correctly handle the case where config->warnoptions is NULL, but entries have 
been added to sys.warnoptions by some other mechanism, like PySys_AddWarnOption)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

Thinking about PySys_AddWarnOptionUnicode a little further, I'm not sure we 
actually need to change anything in the implementation there - I think we can 
just make it clearer in the docs that *only* PySys_AddWarnOption is supported 
prior to Py_Initialize for the embedding use case - the unicode version of the 
API currently only makes sense in CPython, where we can call it after 
_Py_InitializeCore.

It will make sense for embedding applications in 3.8 once we make 
Py_InitializeCore a public API.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

The linked PR has the draft test case for this - it goes beyond the ones I used 
to find the cause of the problem by actually checking that sys.warnoptions and 
sys._xoptions have been populated as expected.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Change by Nick Coghlan :


--
pull_requests: +5914
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

This shouldn't affect the public ABI, so I don't think it's a blocker for the 
ABI freeze, but it's a regression that effectively makes PySys_AddWarnOption 
(and related APIs) unusable, so we should definitely fix it before shipping 3.7.

For now, I've marked the bug as critical, and I'll be aiming to have it fixed 
before 3.7.0b3 at the end of the month.

For PySys_AddWarnOption and PySys_ResetWarnOptions, my current plan is to 
adjust them to maintain a singly-linked list of "wchar *" pointers when called 
prior to _Py_InitializeCore (i.e. when "PyThreadState_GET() == NULL"), while 
using the existing 3.7.0b2 path when the thread state is available. 
_Py_InitializeCore will then take care of cleaning up the memory allocations 
while converting them to Python unicode entries in a Python list object.

_PySys_AddXOption would work similarly (just with a separate list).

For PySys_AddWarnOptionUnicode, I think we need to make it call Py_FatalError 
for 3.7 when called prior to Py_Initialize, with a recommendation to use 
PySys_AddWarnOption instead. In 3.8, that API will become useful again, once 
the public Py_InitializeCore API provides a supported multi-stage startup 
process that allows unicode objects to be created safely before 
Py_InitializeMainInterpreter (or whatever we end up calling that API) reads 
sys.warnoptions and configures the warning subsystem accordingly.

--
priority: normal -> critical

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[pmpp]

Change by pmpp :


--
nosy: +pmpp

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Ned Deily]

Ned Deily  added the comment:

Nick, what's your assessment of this issue's priority?  Is it a release blocker 
for 3.7.0b3 (proposed ABI freeze)?  For 3.7.0rc1 (code freeze)?  None of the 
above?

--
nosy: +ned.deily

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Jonathan Springer]

Change by Jonathan Springer :


--
nosy: +springermac

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

Patched test_capi results for 2ebc5ce42a8a9e047e790aefbf9a94811569b2b6 (the 
global state consolidation commit): both pre-initialization tests fail

Patched test_capi results for bab21faded31c70b142776b9a6075a4cda055d7f (the 
immediately preceding commit): both pre-initialization tests pass

Ding, ding, ding, we have a winner :)

Rather than reverting the options-related aspects of that change though, I 
think a nicer way to handle it will be to add a "pre-init fallback" path to 
those functions that stores the raw wchar_t data, and postpones converting 
those values to Py_Unicode objects until we need them in _PyInitialize_Core.

I'm not sure what to do about PySys_AddWarnOptionUnicode though: I think that's 
just outright incoherent as an API, since you need to call it before 
Py_Initialize for it to do anything useful, but we don't support calling any of 
the Unicode creation APIs until *after* Py_Initialize.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

While I haven't definitely narrowed it down yet, I suspect it isn't your 
changes that are the problem: since the reported crash relates to attempting to 
access a not-yet-created thread state, `warnoptions` and `_xoptions` likely 
need to become C level globals again for 3.7 (along with anything needed to 
create list and Unicode objects).

We can then transfer them from there into Eric's new runtime state objects in 
_PyRuntime_Initialize (or potentially _Py_InitializeCore).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[STINNER Victor]

STINNER Victor  added the comment:

I suggest to revert my changes on PySys_AddWarnOption*() and "just" re-use the 
code from Python 3.6.

Previously, I made the code more complex since I had to expose a private 
function to add manually warning options. But later, the code changed again, so 
the private function is gone.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

After reworking the patch to backport the pre-initialization embedding tests to 
3.7.0a1, they *both* failed, as that was before Victor fixed Py_DecodeLocale to 
work prior to initialization again.

As a result, I tried 
https://github.com/python/cpython/commit/43605e6bfa8d49612df4a38460d063d6ba781906
 in November, after the initial merge of the PEP 432 working branch, but just 
before the start of Victor's follow on refactoring to really take advantage of 
the new internal structures: double failure there as well.

https://github.com/python/cpython/commit/f5ea83f4864232fecc042ff0d1c2401807b19280
 is the next point I checked (just before Eric's September changes to global 
state management), and it looks like we have a new chief suspect, as applying 
the attached "earlier-tree" version of the patch at this point in the history 
gives two passing test cases :)

Next I'll work out a git bisect run based on those two start and end commits 
and the "earlier-tree" patch, and attempt to narrow the failure down to a 
specific commit (probably tomorrow).


(Even without that bisect run though, the warnoptions changes in 
https://github.com/python/cpython/commit/2ebc5ce42a8a9e047e790aefbf9a94811569b2b6#diff-f38879f4833a6b6847e556b9a07bf4ed
 are now looking *very* suspicious)

--
Added file: 
https://bugs.python.org/file47485/bpo-33042-earlier-tree-test-case.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

https://github.com/python/cpython/commit/b4d1e1f7c1af6ae33f0e371576c8bcafedb099db
 (the first attempted fix for bpo-20891) is the last commit where the draft 
test case patch applies cleanly.

That still segfaults, so I'll jump all the way back to 3.7.0a1, rework the 
patch to apply there, and see if pre-init use of these APIs were still working 
back then.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

Adding a bit of context from my prior email discussion with Hartmut: CPython 
actually reads sys.warnoptions at the end of Py_Initialize (its the last thing 
we do before the function returns).

It also reads sys._xoptions during startup, since that's one way of enabling 
settings like dev mode and UTF-8 mode.

That means that even though PySys_AddWarnOption and PySys_AddXOption weren't 
documented as being safe to call before Py_Initialize, they likely *won't* have 
the desired effect if an embedding application defers calling them until later, 
and their absence from the list of "safe to call before Py_Initialize" 
functions was itself a documentation bug.

For 3.8, I'd hoped that the resolution might be as simple as a hard requirement 
on embedding applications to call PyRuntime_Initialize() before doing 
*anything* with the C API, but including the "internal/pystate.h" header and 
adding a call to _PyRuntime_Initialize() wasn't enough to keep the draft test 
case in my patch from segfaulting :(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[STINNER Victor]

STINNER Victor  added the comment:

Python documentation was enhanced in Python 3.7 to explicitly list all 
functions safe to call *before* Py_Initialize():

https://docs.python.org/dev/c-api/init.html#before-python-initialization

PySys_AddWarnOption() is not part of the list. While it's not in the list, I'm 
kind of unhappy that we broke your use case: it wasn't my intent. Because I 
broken your use case with this change part of the big bpo-32030 refactoring:

commit f7e5b56c37eb859e225e886c79c5d742c567ee95
Author: Victor Stinner 
Date:   Wed Nov 15 15:48:08 2017 -0800

bpo-32030: Split Py_Main() into subfunctions (#4399)

IHMO the regression is that PySys_AddWarnOption() now calls 
_PySys_GetObjectId(): in Python 3.6, it wasn't the case.

Python 3.6 code:
---
void
PySys_AddWarnOptionUnicode(PyObject *unicode)
{
if (warnoptions == NULL || !PyList_Check(warnoptions)) {
Py_XDECREF(warnoptions);
warnoptions = PyList_New(0);
if (warnoptions == NULL)
return;
}
PyList_Append(warnoptions, unicode);
}
---

Again, it's a bad idea to use the Python API before Py_Initialize(): you likely 
have to build a Unicode string and you use a list, whereas these two object 
types are not properly initialized...

The PEP 432 and bpo-32030 prepared Python to have a much better API in Python 
3.8 for embedding Python. You will be able to use a wchar_t* string to pass 
warning options.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Change by Nick Coghlan :


--
assignee:  -> ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Nick Coghlan]

Nick Coghlan  added the comment:

I don't think we made any start-up changes that were specific to 
PySys_AddWarnOption, so my suspicion is that the crash is going to be related 
to a change in the constraints on either the unicode object creation or the 
list append operation.

The attached patch adds a new test case (I also cleaned up several other 
details related to test_embed execution in order to more easily see where the 
segfault happens on failure).

Unfortunately, it isn't yet suitable for use with `git bisect`, as I checked 
3.7.0a3 (the earliest tag where the patch applied cleanly), and that commit 
also segfaults. So setting up for git bisect testing (as per the hot-fix 
example in https://git-scm.com/docs/git-bisect#_examples ) will require:

* checking the commit where "test_embed.py" was extracted from "test_capi.py" 
and seeing whether or not that segfaults
* if it *doesn't* segfault, git bisect between there and v3.7.0a3
* if it *does* segfault, make a revised test patch that applies cleanly to 
early versions, and then go back through the 3.7.0 pre-releases to find one 
that works

--
keywords: +patch
nosy: +ncoghlan
Added file: https://bugs.python.org/file47477/bpo-33042-test-case.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Ned Deily]

Change by Ned Deily :


--
nosy: +eric.snow, vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33042] New 3.7 startup sequence crashes PyInstaller

[Hartmut Goebel]

New submission from Hartmut Goebel :

PyInstaller is a tool for freezing Python applications into stand-alone 
packages, much like py2exe. py2app, and bbfreeze. PyInstaller is providing 
*one* bootloader for all versions of Python supported (2.7, 3.4-3.6).

In PyInstaller the startup sequence is implemented in
pyi_pylib_start_python() in bootloader/src/pyi_pythonlib.c. The workflow 
roughly is:

- SetProgramName
- SetPythonHome
- Py_SetPath
- Setting runtime options
  - some flags using the global variables
  - PySys_AddWarnOption -> crash
- Py_Initialize
- PySys_SetPath

The crash occurs due to tstate (thread state) not being initialized when
calling PySys_AddWarnOption.

--
components: Interpreter Core
messages: 313546
nosy: htgoebel
priority: normal
severity: normal
status: open
title: New 3.7 startup sequence crashes PyInstaller
type: behavior
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com