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

Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94518

Closed
arhadthedev opened this issue Jul 2, 2022 · 2 comments
Closed

Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94518

arhadthedev opened this issue Jul 2, 2022 · 2 comments
Labels
3.12 bugs and security fixes topic-argument-clinic type-feature A feature request or enhancement

Comments

@arhadthedev
Copy link
Member

arhadthedev commented Jul 2, 2022

Currently the function is parsed with the following behemoth:

if (!PyArg_ParseTuple(
            args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec",
            &process_args, &executable_list,
            &close_fds, &PyTuple_Type, &py_fds_to_keep,
            &cwd_obj, &env_list,
            &p2cread, &p2cwrite, &c2pread, &c2pwrite,
            &errread, &errwrite, &errpipe_read, &errpipe_write,
            &restore_signals, &call_setsid, &pgid_to_set,
            &gid_object, &groups_list, &uid_object, &child_umask,
            &preexec_fn, &allow_vfork))
        return NULL;

Conversion will:

  • hide this from a realm of manual and error-prone labor into a precise and checked world of automation
  • allow to use faster methods like METH_FASTCALL+_PyArg_CheckPositional.

Linked PRs

@gpshead
Copy link
Member

gpshead commented Jul 3, 2022

Indeed, one reason I've left it a behemoth in the past is that it only has 2-3 call-sites and gained parameters slowly over time. Cleaning this monster up will be good.

One minor concern with argument clinic on this is the C stack space use increase due to the intermediate function calling the implementation with everything as C arguments on the stack. BUT... there is some other long overdue _posixsubprocess refactoring to be done that could alleviate stack use elsewhere (not to be done as part of this issue) in 3.12 that could make up for this change by reducing a similar amount of C stack use. (the internal C call to the child fork exec function which is also a giant messy pile of C args)

Also, the argument clinic generated code is effectively a tail call to the _impl. Ideally compilers see that in optimized builds and effectively inline/merge the two functions instead of wasting time separating them with a calling convention sucking up stack. (No guarantees on that)

If we went an alternate route and constructed a structure with all of the values instead of a giant list of arguments (a namedtuple could make it an easy transition), the C stack overhead from argument clinic would be gone. But so would the nice value type checking boilerplate that argument clinic handles for us. Unless we created our own internal to _posixsubprocess type instead of namedtuple and used argument clinic on its construction and data fill-in APIs instead. That idea could just be overcomplicated. I'll ponder it.

In the meantime, thanks for the PR. I'll get to a review on that soon.

@gpshead gpshead added the 3.12 bugs and security fixes label Jul 3, 2022
gpshead pushed a commit that referenced this issue Jan 14, 2023
…erved values (#94687)

Have _posixsubprocess.c stop using boolean flags to say if gid and uid values were supplied and action is required.  Such an implicit "either initialized or look somewhere else" confused both the reader (another mental connection to constantly track between functions) and a compiler (warnings on potentially uninitialized variables being passed). Instead, we can utilize a special group/user id as a flag value -1 defined by POSIX but used nowhere else. Namely:

gid: call_setgid = False → gid = -1

uid: call_setuid = False → uid = -1

groups: call_setgroups = False → groups = NULL (obtained with (groups_list != Py_None) ? groups : NULL)

This PR is required for #94519.
gpshead pushed a commit that referenced this issue Jan 26, 2023
* Rename `group*` to `extra_group*` to avoid confusion
* Rename `num_groups` into `extra_group_size`
* Rename `groups_list` to `extra_groups_packed`
mdboom pushed a commit to mdboom/cpython that referenced this issue Jan 31, 2023
…ython#101054)

* Rename `group*` to `extra_group*` to avoid confusion
* Rename `num_groups` into `extra_group_size`
* Rename `groups_list` to `extra_groups_packed`
gpshead added a commit that referenced this issue Apr 24, 2023
…linic (#94519)

Convert fork_exec to pre-inlined-argparser Argument Clinic

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpshead
Copy link
Member

gpshead commented Apr 24, 2023

thanks! :)

@gpshead gpshead closed this as completed Apr 24, 2023
carljm added a commit to carljm/cpython that referenced this issue Apr 24, 2023
* main: (53 commits)
  pythongh-102498 Clean up unused variables and imports in the email module  (python#102482)
  pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244)
  pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365)
  pythongh-94300: Update datetime.strptime documentation (python#95318)
  pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778)
  pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456)
  pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775)
  Revert "Add tests for empty range equality (python#103751)" (python#103770)
  pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519)
  pythonGH-65022: Fix description of copyreg.pickle function (python#102656)
  pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324)
  pythongh-91687: modernize dataclass example typing (python#103773)
  pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747)
  pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769)
  pythongh-87452: Improve the Popen.returncode docs
  Removed unnecessary escaping of asterisks (python#103714)
  pythonGH-102973: Slim down Fedora packages in the dev container (python#103283)
  pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095)
  Add tests for empty range equality (python#103751)
  pythongh-103712: Increase the length of the type name in AttributeError messages (python#103713)
  ...
carljm added a commit to carljm/cpython that referenced this issue Apr 24, 2023
* superopt: (82 commits)
  pythongh-101517: fix line number propagation in code generated for except* (python#103550)
  pythongh-103780: Use patch instead of mock in asyncio unix events test (python#103782)
  pythongh-102498 Clean up unused variables and imports in the email module  (python#102482)
  pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244)
  pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365)
  pythongh-94300: Update datetime.strptime documentation (python#95318)
  pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778)
  pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456)
  pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775)
  Revert "Add tests for empty range equality (python#103751)" (python#103770)
  pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519)
  pythonGH-65022: Fix description of copyreg.pickle function (python#102656)
  pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324)
  pythongh-91687: modernize dataclass example typing (python#103773)
  pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747)
  pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769)
  pythongh-87452: Improve the Popen.returncode docs
  Removed unnecessary escaping of asterisks (python#103714)
  pythonGH-102973: Slim down Fedora packages in the dev container (python#103283)
  pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes topic-argument-clinic type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants