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

gh-96821: Add config option --with-strict-overflow #96823

Merged
merged 38 commits into from
Mar 4, 2023
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
8863e74
gh-96821: Mark modules that need -fno-strict-overflow
matthiasgoergens Sep 14, 2022
0598ea9
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 14, 2022
50cbb61
Drop fwrapv
matthiasgoergens Sep 14, 2022
f4f3b83
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 16, 2022
8015da0
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 16, 2022
d2a2c82
Foo
matthiasgoergens Sep 17, 2022
25b15bf
fix
matthiasgoergens Sep 17, 2022
86d01d6
Even I can't remember the f
matthiasgoergens Sep 17, 2022
ca2d023
Merge remote-tracking branch 'matthias/strict_overflow' into strict_o…
matthiasgoergens Sep 17, 2022
43baf35
Merge remote-tracking branch 'origin/main' into strict_overflow
matthiasgoergens Sep 17, 2022
2b6ade9
Fix typo
matthiasgoergens Sep 18, 2022
9d402a8
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 18, 2022
e5cff7b
News
matthiasgoergens Sep 18, 2022
2118727
Merge remote-tracking branch 'matthias/strict_overflow' into strict_o…
matthiasgoergens Sep 18, 2022
a2bb1e9
Update Misc/NEWS.d/next/Build/2022-09-14-10-38-15.gh-issue-96821.Zk2a…
matthiasgoergens Sep 19, 2022
89ba064
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 19, 2022
8d50ae4
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 20, 2022
71ca2a5
Merge remote-tracking branch 'origin/main' into strict_overflow
matthiasgoergens Sep 21, 2022
8538fe6
Fix comments and quoting
matthiasgoergens Sep 21, 2022
26956f3
Fix News
matthiasgoergens Sep 21, 2022
88a2c76
_testcapi is strict-overflow clean now
matthiasgoergens Sep 21, 2022
4f95dba
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 26, 2022
506d77e
Merge branch 'main' into strict_overflow
matthiasgoergens Sep 27, 2022
d45d6ae
Update configure.ac
matthiasgoergens Oct 3, 2022
f6d3113
Update configure.ac
matthiasgoergens Oct 3, 2022
d6423c7
Update configure.ac
matthiasgoergens Oct 3, 2022
0f2f931
Merge branch 'main' into strict_overflow
matthiasgoergens Oct 3, 2022
3b4046f
_struct is already strict-overflow safe
matthiasgoergens Oct 3, 2022
3995eb6
Merge remote-tracking branch 'origin/main' into strict_overflow
matthiasgoergens Oct 11, 2022
2714d61
We fixed audioop, so remove it from list of exceptions
matthiasgoergens Oct 11, 2022
6236fb9
Document our --with-strict-overflow
matthiasgoergens Oct 11, 2022
ca6e6ba
Update Doc/using/configure.rst
matthiasgoergens Oct 12, 2022
fa14763
Update Misc/NEWS.d/next/Build/2022-09-14-10-38-15.gh-issue-96821.Zk2a…
matthiasgoergens Oct 12, 2022
6b09fa7
Merge branch 'main' into strict_overflow
matthiasgoergens Oct 12, 2022
9f34477
improve help message
hauntsaninja Jan 20, 2023
7116f84
Merge branch 'main' into strict_overflow
erlend-aasland Jan 20, 2023
e526f6e
fix cache, add warning
hauntsaninja Jan 23, 2023
db665ab
use warn, not error
hauntsaninja Jan 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/using/configure.rst
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,11 @@ also be used to improve performance.

Enable C-level code profiling with ``gprof`` (disabled by default).

.. cmdoption:: --with-strict-overflow

Add ``-fstrict-overflow`` to the C compiler flags (by default we add
``-fno-strict-overflow`` instead).


.. _debug-build:

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Explicitly mark C extension modules that need defined signed integer overflow,
and add a configure option :option:`--with-strict-overflow`.
71 changes: 62 additions & 9 deletions configure

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 35 additions & 9 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -2082,6 +2082,35 @@ case $CC in
fi
esac

dnl Historically, some of our code assumed that signed integer overflow
dnl is defined behaviour via twos-complement.
dnl Set STRICT_OVERFLOW_CFLAGS and NO_STRICT_OVERFLOW_CFLAGS depending on compiler support.
dnl Pass the latter to modules that depend on such behaviour.
_SAVE_VAR([CFLAGS])
CFLAGS="-fstrict-overflow -fno-strict-overflow"
AC_CACHE_CHECK([if $CC supports -fstrict-overflow and -fno-strict-overflow],
[ac_cv_cc_supports_fstrict_overflow],
AC_COMPILE_IFELSE(
[
AC_LANG_PROGRAM([[]], [[]])
],[
STRICT_OVERFLOW_CFLAGS="-fstrict-overflow"
NO_STRICT_OVERFLOW_CFLAGS="-fno-strict-overflow"
],[
STRICT_OVERFLOW_CFLAGS=""
NO_STRICT_OVERFLOW_CFLAGS=""
])
)
_RESTORE_VAR([CFLAGS])

AC_MSG_CHECKING([for --with-strict-overflow])
AC_ARG_WITH([strict-overflow],
AS_HELP_STRING(
[--with-strict-overflow],
[choose between -fstrict-overflow or -fno-strict-overflow (default is no)]),[],
[with_strict_overflow=no])
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to pick on this further. I would like two more changes before merging:

  1. The help message is too vague. I'd like something a la: if 'yes', add -fstrict-overflow to CFLAGS, else add -fno-strict-overflow to CFLAGS (default is no).
  2. Please add a AC_MSG_WARN if ac_cv_cc_supports_fstrict_overflow=no and --with-strict-overflow=yes ("--with-strict-overflow=yes specified, but your compiler does not support the -fstrict-overflow flag"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Don't worry about picking on this, I'm just glad you care enough to make the effort!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, just ping me when you've made the changes, and I'll get this merged.

AC_MSG_RESULT([$with_strict_overflow])

# Check if CC supports -Og optimization level
_SAVE_VAR([CFLAGS])
CFLAGS="-Og"
Expand Down Expand Up @@ -2112,15 +2141,8 @@ if test "${OPT-unset}" = "unset"
then
case $GCC in
yes)
# For gcc 4.x we need to use -fwrapv so lets check if its supported
if "$CC" -v --help 2>/dev/null |grep -- -fwrapv > /dev/null; then
WRAP="-fwrapv"
fi

if test -n "${cc_is_clang}"
then
# Clang also needs -fwrapv
WRAP="-fwrapv"
# bpo-30104: disable strict aliasing to compile correctly dtoa.c,
# see Makefile.pre.in for more information
CFLAGS_ALIASING="-fno-strict-aliasing"
Expand All @@ -2131,7 +2153,7 @@ then
if test "$Py_DEBUG" = 'true' ; then
OPT="-g $PYDEBUG_CFLAGS -Wall"
else
OPT="-g $WRAP -O3 -Wall"
OPT="-g -O3 -Wall"
fi
;;
*)
Expand Down Expand Up @@ -2246,6 +2268,10 @@ AC_DEFUN([PY_CHECK_CC_WARNING], [
])

# tweak BASECFLAGS based on compiler and platform
AS_VAR_IF([with_strict_overflow], [yes],
[BASECFLAGS="$BASECFLAGS $STRICT_OVERFLOW_CFLAGS"],
[BASECFLAGS="$BASECFLAGS $NO_STRICT_OVERFLOW_CFLAGS"])

case $GCC in
yes)
CFLAGS_NODIST="$CFLAGS_NODIST -std=c11"
Expand Down Expand Up @@ -7307,7 +7333,7 @@ PY_STDLIB_MOD([_crypt],
[$LIBCRYPT_CFLAGS], [$LIBCRYPT_LIBS])
PY_STDLIB_MOD([_ctypes],
[], [test "$have_libffi" = yes],
[$LIBFFI_CFLAGS], [$LIBFFI_LIBS])
[$NO_STRICT_OVERFLOW_CFLAGS $LIBFFI_CFLAGS], [$LIBFFI_LIBS])
PY_STDLIB_MOD([_curses],
[], [test "$have_curses" != "no"],
[$CURSES_CFLAGS], [$CURSES_LIBS]
Expand Down