gh-112301: Enable compiler flags with low performance impact and no warnings #120975
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
All commit authors signed the Contributor License Agreement. |
|
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 |
thesamesam
reviewed
Jun 24, 2024
configure.ac
Outdated
| # These flags should be enabled by default for all builds. | ||
| AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [BASECFLAGS="$BASECFLAGS -Wimplicit-fallthrough"], [AC_MSG_WARN([-Wimplicit-fallthrough not supported])]) | ||
| AX_CHECK_COMPILE_FLAG([-fstack-protector-strong], [BASECFLAGS="$BASECFLAGS -fstack-protector-strong"], [AC_MSG_WARN([-fstack-protector-strong not supported])]) | ||
| AX_CHECK_COMPILE_FLAG([-fno-strict-overflow], [BASECFLAGS="$BASECFLAGS -fno-strict-overflow"], [AC_MSG_WARN([-fno-strict-overflow not supported])]) |
There was a problem hiding this comment.
But -fno-strict-overflow is already handled right above, isn't it?
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([[]], [[]])],
[ac_cv_cc_supports_fstrict_overflow=yes],
[ac_cv_cc_supports_fstrict_overflow=no]
)
)
_RESTORE_VAR([CFLAGS])
AS_VAR_IF([ac_cv_cc_supports_fstrict_overflow], [yes],
[STRICT_OVERFLOW_CFLAGS="-fstrict-overflow"
NO_STRICT_OVERFLOW_CFLAGS="-fno-strict-overflow"],
[STRICT_OVERFLOW_CFLAGS=""
NO_STRICT_OVERFLOW_CFLAGS=""])
AC_MSG_CHECKING([for --with-strict-overflow])
AC_ARG_WITH([strict-overflow],
AS_HELP_STRING(
[--with-strict-overflow],
[if 'yes', add -fstrict-overflow to CFLAGS, else add -fno-strict-overflow (default is no)]
),
[
AS_VAR_IF(
[ac_cv_cc_supports_fstrict_overflow], [no],
[AC_MSG_WARN([--with-strict-overflow=yes requires a compiler that supports -fstrict-overflow])],
[]
)
],
[with_strict_overflow=no]
)
AC_MSG_RESULT([$with_strict_overflow])There was a problem hiding this comment.
Yes that is true. I only checked that it was enabled when compiling and it looks like it is redundant. I have removed my addition
|
I'm also wondering why these 3 specific flags are being done as part of this batch. One is a warning and one is already set. A warning should never make any runtime difference, although it could impact compile-time speed. It would make sense to do that by itself, I'd say. |
|
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 |
I am investigating enabling options suggested in the OpenSSF guidance for compiler hardening linked in the issue. These options just a few of the suggested options that both don't impact performance and don't generate any new warnings at compile time. Some of the other flags that I am considering enabling either generate warnings or impact pyperf benchmarks and the tradeoff needs to be discussed further before making a decision on those. |
corona10
reviewed
Jun 25, 2024
There was a problem hiding this comment.
I didn't take a look at it deeply, but if it costs 3% geometric mean performance overhead, it's not a low-performance impact when considering the efforts of the faster-CPython team.
Why not just provide as the optional flag instead of default option?
I think those measurements were for a larger set of changes. The measurement for this PR shows no measurable change in performance. |
|
This is why as pointed out by @thesamesam, it would make sense to incrementally add different options via separate PRs, so that flags which don't produce different machine code and only activate diagnostics could be easily landed without discussing the performance impact they don't have. |
corona10
requested changes
Jun 25, 2024
corona10
reviewed
Jun 25, 2024
| @@ -0,0 +1 @@ | |||
| Add default compiler options to improve security | |||
There was a problem hiding this comment.
Please list concrete compiler options you added.
There was a problem hiding this comment.
Added options to news file
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
corona10
reviewed
Jun 25, 2024
hugovk
reviewed
Jun 25, 2024
Misc/NEWS.d/next/Security/2024-06-25-04-42-43.gh-issue-112301.god4IC.rst
Outdated
Show resolved
Hide resolved
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @corona10: please review the changes made to this pull request. |
corona10
approved these changes
Jun 26, 2024
|
|
|
|
I've created #121026 because |
mrahtz
pushed a commit
to mrahtz/cpython
that referenced
this pull request
Jun 30, 2024
noahbkim
pushed a commit
to hudson-trading/cpython
that referenced
this pull request
Jul 11, 2024
estyxx
pushed a commit
to estyxx/cpython
that referenced
this pull request
Jul 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enables a few default compiler options in an effort to improve security. The options enabled here are:
-Wimplicit-fallthrough -fstack-protector-strong -fno-strict-overflow -WtrampolinesThese have been found to have little to no pyperformance benchmark impact and don't introduce any new warnings.
The pyperf benchmark for this configuration can be viewed here: https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20240620-3.14.0a0-98d9ea0
This will be one of several PRs addressing the issue: #112301