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

vstinner · 2017-11-15T01:30:59Z

Don't use "Python runtime" anymore to parse command line options or
to get environment variables: pymain_init() is now a strict
separation.
Use an error message rather than "crashing" directly with
Py_FatalError(). Limit the number of calls to Py_FatalError(). It
prepares the code to handle errors more nicely later.
Warnings options (-W, PYTHONWARNINGS) and "XOptions" (-X) are now
only added to the sys module once Python core is properly
initialized.
_PyMain is now the well identified owner of some important strings
like: warnings options, XOptions, and the "program name". The
program name string is now properly freed at exit.
pymain_free() is now responsible to free the "command" string.
Rename most methods in Modules/main.c to use a "pymain_" prefix to
avoid conflits and ease debug.
Replace _Py_CommandLineDetails_INIT with memset(0)
Reorder a lot of code to fix the initialization ordering. For
example, initializing standard streams now comes before parsing
PYTHONWARNINGS.
pymain_init() now uses the cmdline structure rather than relying on
Py_xxx global configuration variables. For example, replace
Py_UnbufferedStdioFlag with cmdline.use_unbuffered_io.
Py_Main() now handles errors when adding warnings options and
XOptions.
Add _PyMem_GetDefaultRawAllocator() private function.

https://bugs.python.org/issue32030

vstinner · 2017-11-15T01:42:59Z

Hum, test_capi fails on Windows.

vstinner · 2017-11-15T02:12:12Z

test_capi failed on the "gcc C" job of Travis CI:

0:01:20 load avg: 44.72 [ 46/404] test_capi
*** Error in `/home/travis/build/python/cpython/venv/bin/python': munmap_chunk(): invalid pointer: 0x00007f0f5c5e90a0 ***
*** Error in `/home/travis/build/python/cpython/venv/bin/python': munmap_chunk(): invalid pointer: 0x00007f26c992e0a0 ***
*** Error in `/home/travis/build/python/cpython/venv/bin/python': munmap_chunk(): invalid pointer: 0x00007fab599500a0 ***
test test_capi failed -- multiple errors occurred; run in verbose mode for details

ncoghlan

Nice! This looks really good to me - just a couple of minor suggestions inline.

ncoghlan · 2017-11-15T02:26:38Z

Include/pylifecycle.h

@@ -21,17 +21,17 @@ PyAPI_FUNC(int) Py_SetStandardStreamEncoding(const char *encoding,
                                             const char *errors);

 /* PEP 432 Multi-phase initialization API (Private while provisional!) */
-PyAPI_FUNC(void) _Py_InitializeCore(const _PyCoreConfig *);
+PyAPI_FUNC(const char*) _Py_InitializeCore(const _PyCoreConfig *);


I'm wondering if a typedef char _Py_InitErrorMessage; might make these API signatures a bit clearer:

PyAPI_FUNC(const _Py_InitErrorMessage*) _Py_InitializeCore(const _PyCoreConfig *);

While I currently guessed "Oh, that will be an error message" in the context of the patch, I'm not sure I would have without that extra context.

Using a typedef also leaves the door open to upgrading to a struct later if we find a reason for doing that.

I didn't see the need for a structure, but after converting more code to avoid calling Py_FatalError(), I found two places where we exit Python but don't want to exit with abort():

if (Py_ReadHashSeed(seed_text, &use_hash_seed, &hash_seed) < 0) { Py_FatalError("PYTHONHASHSEED must be \"random\" or an integer " "in range [0; 4294967295]"); }

and

m = PyImport_ImportModule("site"); if (m == NULL) { fprintf(stderr, "Failed to import the site module\n"); PyErr_Print(); Py_Finalize(); exit(1); }

These errors are more on the user side, they are not "bugs" of Python itself.

So I implemented a new _PyInitError "type" (technically a structure) to indicate if it's an "user error" or another error. Moreover, I added the name of the function where the error was raised, to the error message.

ncoghlan · 2017-11-15T02:32:20Z

Modules/main.c

@@ -35,9 +35,11 @@
 extern "C" {
 #endif

+extern void _PyMem_GetDefaultRawAllocator(PyMemAllocatorEx *alloc);


When I used this technique for the locale coercion APIs, I ended up causing a build failure on Cygwin: https://bugs.python.org/issue31877

Assuming that's a general concern, PyAPI_FUNC will be needed here as well.

ncoghlan · 2017-11-15T02:57:01Z

One general comment: while I didn't spot any particular instances of this going wrong, it's important to keep in mind that the various global flags (Py_IsolatedFlag, etc) are part of the embedding configuration API. This means that even as we add the more explicit configuration structs and use those to pass settings from Py_Main into the CPython runtime, we still need to account for other applications that are passing in the settings via the global variables instead (and either keep that model working, or else provide explicit porting guidance).

vstinner · 2017-11-15T10:57:06Z

The PR compiles correctly on macOS and the following tests passed:

macbook:master haypo$ PYTHONMALLOC=pymalloc ./python.exe -m test -j0 test_capi test_cmd_line test_cmd_line_script
Run tests in parallel using 6 child processes
0:00:08 load avg: 2.30 [1/3] test_cmd_line passed
0:00:11 load avg: 2.35 [2/3] test_cmd_line_script passed
0:00:17 load avg: 2.49 [3/3] test_capi passed
All 3 tests OK.

Total duration: 17 sec
Tests result: SUCCESS

* Don't use "Python runtime" anymore to parse command line options or to get environment variables: pymain_init() is now a strict separation. * Use an error message rather than "crashing" directly with Py_FatalError(). Limit the number of calls to Py_FatalError(). It prepares the code to handle errors more nicely later. * Warnings options (-W, PYTHONWARNINGS) and "XOptions" (-X) are now only added to the sys module once Python core is properly initialized. * _PyMain is now the well identified owner of some important strings like: warnings options, XOptions, and the "program name". The program name string is now properly freed at exit. pymain_free() is now responsible to free the "command" string. * Rename most methods in Modules/main.c to use a "pymain_" prefix to avoid conflits and ease debug. * Replace _Py_CommandLineDetails_INIT with memset(0) * Reorder a lot of code to fix the initialization ordering. For example, initializing standard streams now comes before parsing PYTHONWARNINGS. * Py_Main() now handles errors when adding warnings options and XOptions. * Add _PyMem_GetDefaultRawAllocator() private function. * Cleanup _PyMem_Initialize(): remove useless global constants: move them into _PyMem_Initialize(). * Call _PyRuntime_Initialize() as soon as possible: _PyRuntime_Initialize() now returns an error message on failure. * Add _PyInitError structure and following macros: * _Py_INIT_OK() * _Py_INIT_ERR(msg) * _Py_INIT_USER_ERR(msg): "user" error, don't abort() in that case * _Py_INIT_FAILED(err)

vstinner · 2017-11-15T17:17:58Z

One general comment: while I didn't spot any particular instances of this going wrong, it's important to keep in mind that the various global flags (Py_IsolatedFlag, etc) are part of the embedding configuration API. (...)

Ok, I changed my code to set Py_xxx flags as soon as possible, and use Py_xxx flags rather than using pymain.

vstinner · 2017-11-15T17:19:38Z

Hum, test_capi fails on Windows.

and

test_capi failed on the "gcc C" job of Travis CI: (...

Oh, I expect PyMem_RawMalloc() to be usable directly, but this changed recently: _PyRuntime_Initialize() must now be called before the first call to PyMem_RawMalloc(). I fixed that as well.

Call _PyRuntime_Initialize() and copy core config before calling _PyMem_SetupAllocators().

Programs/python.c now uses internals/pystate.h which requires Py_BUILD_CORE.

zooba · 2017-11-15T22:29:26Z

Haven't looked closely, but this looks like overall goodness!

If you want one extra case to keep in mind, see how I implemented my spython.c for my PEP 551 implementation. There's obviously no compatibility requirement, but I'd want to be able to keep doing the same things after this change.

vstinner · 2017-11-16T02:55:13Z

@zooba: "Haven't looked closely, but this looks like overall goodness!"

Oh thanks. With @ncoghlan and you who know the topic, I'm now confident enough to merge my change (I already merged it ;-)).

"If you want one extra case to keep in mind, see how I implemented my spython.c for my PEP 551 implementation. There's obviously no compatibility requirement, but I'd want to be able to keep doing the same things after this change."

My change shouldn't break this code. @ncoghlan: would you mind to double-check?

In the first version of this change, I overrided Py_xxx flags from the command lines, ignoring their previous value. @ncoghlan warned me about Py_xxx flags set when Python is embedded. So I fixed my code.

* 'master' of https://github.com/python/cpython: (550 commits) bpo-31867: Remove duplicates in default mimetypes. (python#4388) tokenizer: Remove unused tabs options (python#4422) bpo-31691: Specify where to find build instructions for the Windows installer (python#4426) Fix typo in atexit documentation. (pythonGH-4419) bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). (python#4110) bpo-32043: New "developer mode": "-X dev" option (python#4413) bpo-30349: Raise FutureWarning for nested sets and set operations (python#1553) bpo-32037: Use the INT opcode for 32-bit integers in protocol 0 pickles. (python#4407) bpo-30143: 2to3 now generates a code that uses abstract collection classes (python#1262) bpo-32030: Enhance Py_Main() (python#4412) bpo-32030: Split Py_Main() into subfunctions (python#4399) bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable python#4409 bpo-32025: Add time.thread_time() (python#4410) bpo-32018: Fix inspect.signature repr to follow PEP 8 (python#4408) bpo-30399: Get rid of trailing comma in the repr of BaseException. (python#1650) bpo-30950: Convert round() to Argument Clinic. (python#2740) bpo-32011: Revert "Issue python#15480: Remove the deprecated and unused TYPE_INT64 code from marshal." (python#4381) bpo-32023: Disallow genexprs without parenthesis in class definitions. (python#4400) bpo-31949: Fixed several issues in printing tracebacks (PyTraceBack_Print()). (python#4289) bpo-32032: Test both implementations of module-level pickle API. (python#4401) ...

vstinner requested a review from a team November 15, 2017 01:30

the-knights-who-say-ni added the CLA signed label Nov 15, 2017

bedevere-bot added the awaiting merge label Nov 15, 2017

vstinner added the skip news label Nov 15, 2017

vstinner requested review from ncoghlan and ericsnowcurrently November 15, 2017 01:41

ncoghlan reviewed Nov 15, 2017

View reviewed changes

vstinner added 2 commits November 15, 2017 18:42

Fix _Py_InitializeCore()

02282e5

Call _PyRuntime_Initialize() and copy core config before calling _PyMem_SetupAllocators().

python.vcxproj: define Py_BUILD_CORE

10a6e78

Programs/python.c now uses internals/pystate.h which requires Py_BUILD_CORE.

vstinner requested a review from a team as a code owner November 15, 2017 21:22

vstinner merged commit f7e5b56 into python:master Nov 15, 2017

bedevere-bot removed the awaiting merge label Nov 15, 2017

vstinner deleted the cmdline branch November 15, 2017 23:48

vstinner changed the title ~~[WIP] bpo-32030: Split Py_Main() into subfunctions~~ bpo-32030: Split Py_Main() into subfunctions Nov 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

vstinner commented Nov 15, 2017 •

edited by bedevere-bot

Loading

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

ncoghlan left a comment

Uh oh!

ncoghlan Nov 15, 2017

Uh oh!

vstinner Nov 15, 2017

Uh oh!

vstinner Nov 15, 2017

Uh oh!

ncoghlan Nov 15, 2017

Uh oh!

vstinner Nov 15, 2017

Uh oh!

ncoghlan commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

zooba commented Nov 15, 2017

Uh oh!

vstinner commented Nov 16, 2017

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

vstinner commented Nov 15, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

ncoghlan Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

ncoghlan Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

ncoghlan commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

vstinner commented Nov 15, 2017

Uh oh!

zooba commented Nov 15, 2017

Uh oh!

vstinner commented Nov 16, 2017

Uh oh!

Uh oh!

vstinner commented Nov 15, 2017 •

edited by bedevere-bot

Loading