Skip to content

gh-132314: fix stack array init warning #132376

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dura0ok
Copy link
Contributor

@dura0ok dura0ok commented Apr 10, 2025

@dura0ok dura0ok requested a review from markshannon as a code owner April 10, 2025 19:57
@python-cla-bot
Copy link

python-cla-bot bot commented Apr 10, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Apr 10, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@dura0ok
Copy link
Contributor Author

dura0ok commented Apr 10, 2025

There was no warning with the configuration without options, but I reproduced it with the --enable-optimization --with-lto options. The mistake is very simple.

Python/ceval.c Outdated
@@ -1814,7 +1814,7 @@ _PyEvalFramePushAndInit_Ex(PyThreadState *tstate, _PyStackRef func,
PyObject *kwnames = NULL;
_PyStackRef *newargs;
PyObject *const *object_array = NULL;
_PyStackRef stack_array[8];
_PyStackRef stack_array[8] = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, the compiler warning is a false-positive.

If the stack_array is used

cpython/Python/ceval.c

Lines 1832 to 1834 in ad3bbe8

if (nargs <= 8) {
newargs = stack_array;
}

it is initialized

cpython/Python/ceval.c

Lines 1844 to 1846 in ad3bbe8

for (Py_ssize_t i = 0; i < nargs; i++) {
newargs[i] = PyStackRef_FromPyObjectNew(PyTuple_GET_ITEM(callargs, i));
}

Having a small array on the stack to avoid the PyMem_Malloc is a common pattern, e.g. further down

_PyStackRef stack_array[8];

or like mentioned in the issue #132314
PyObject *small_stack[_PY_FASTCALL_SMALL_STACK];

They all follow the same pattern, and if the compiler is not smart enough to detect it, it will issue a warning.

IMHO, initializing those stack variables using = {} is a waste of CPU cycles, and we should silence the warning using a diagnostic pragma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think hiding warnings using pragma it is not good idea, because we have other compilers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but not all of them emit the warning, so we only have to silence those that do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and we can selectively decide which compilers need to be silenced as well so I think for a falsey positive like that it's better to silence the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Check please new version. @chris-eibl @picnixz

Add macros to disable and re-enable compiler-specific warnings about
possibly uninitialized variables (`-Wmaybe-uninitialized` for GCC,
`-Wuninitialized` for Clang, and warning C4700 for MSVC). Use these
macros in `_PyEvalFramePushAndInit_Ex()` to suppress false positives
on stack-allocated arrays like `stack_array`.

This also addresses warnings such as the one in `pycore_call.h` when
using `small_stack[_PY_FASTCALL_SMALL_STACK]` that may be flagged by
GCC's `-Wmaybe-uninitialized`.
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.

4 participants