Movatterモバイル変換

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])])

Copy link

Contributor

Choose a reason for hiding this comment

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

But-fno-strict-overflow is already handled right above, isn't it?

dnl Historically, some of our code assumed that signed integer overflowdnl 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])

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Contributor

thesamesam commentedJun 24, 2024•
edited
Loading

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.

Remove redundant -fno-strict-overflow option from config

1e84a87

Copy link

bedevere-appbot commentedJun 25, 2024

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

Copy link

ContributorAuthor

nohlson commentedJun 25, 2024

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.

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.

📜🤖 Added by blurb_it.

257e3d7

corona10 requested a review frommdboom

June 25, 2024 13:56

corona10 reviewed

Copy link

Member

corona10 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I didn't take a look at it deeply, but if itcosts 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?

Copy link

Contributor

mdboom commentedJun 25, 2024

I didn't take a look at it deeply, but if itcosts 3% geometric mean performance overhead, it's not a low-performance impact when considering the efforts of the faster-CPython team.

I think those measurements were for a larger set of changes. The measurement forthis PR shows no measurable change in performance.

Copy link

Contributor

eli-schwartz commentedJun 25, 2024

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 approved these changes

Copy link

Member

corona10 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

LGTM, I can not see any side effect, and thank you for cross-checking about the performance issue@mdboom

bedevere-appbot added awaiting merge and removed awaiting review labels

corona10 requested changes

Copy link

Member

corona10 left a comment

Choose a reason for hiding this comment

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

Overall changes lgtm but let's add more explain through the NEWS.d

corona10 reviewed

Misc/NEWS.d/next/Security/2024-06-25-04-42-43.gh-issue-112301.god4IC.rst Outdated

		@@ -0,0 +1 @@
		Add default compiler options to improve security

Copy link

Member

corona10Jun 25, 2024

Choose a reason for hiding this comment

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

Please list concrete compiler options you added.

Copy link

ContributorAuthor

nohlsonJun 25, 2024

Choose a reason for hiding this comment

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

Added options to news file

bedevere-appbot removed the awaiting merge label

Copy link

bedevere-appbot commentedJun 25, 2024

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 phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-appbot added the awaiting changes label

corona10 reviewed

configure.acShow resolvedHide resolved

Include explicit compiler options in news

154fa05

hugovk reviewed

Misc/NEWS.d/next/Security/2024-06-25-04-42-43.gh-issue-112301.god4IC.rst OutdatedShow resolvedHide resolved

Remove trailing whitespace in news

1917b93

Copy link

ContributorAuthor

nohlson commentedJun 25, 2024

I have made the requested changes; please review again

bedevere-appbot added awaiting change review and removed awaiting changes labels

Copy link

bedevere-appbot commentedJun 25, 2024

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

bedevere-appbot requested a review fromcorona10

June 25, 2024 21:56

corona10 approved these changes

bedevere-appbot added awaiting merge and removed awaiting change review labels

corona10 merged commit7fb32e0 intopython:main

bedevere-appbot removed the awaiting merge label