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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 38 commits into from
Mar 4, 2023
Merged

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

merged 38 commits into from
Mar 4, 2023

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Sep 14, 2022 •
edited
Loading

Please see the issue for details.

This PR marks the modules that need -fno-strict-overflow and adds machinery to force -fstrict-overflow, if the users wants that.

As far as is currently known, the three remaining modules that rely on defined integer overflow are fixed by:

@matthiasgoergens matthiasgoergens changed the title gh-96821: Mark modules that need -fno-strict-overflow gh-96821: Mark modules that need -fno-strict-overflow Sep 14, 2022
@matthiasgoergens
Copy link
Contributor Author

@ericsnowcurrently Please have a look.

I think perhaps we should only add the flag when we detect that the compiler supports it? What do you think?

@erlend-aasland
Copy link
Contributor

I think perhaps we should only add the flag when we detect that the compiler supports it? What do you think?

Yes, please add a compiler flag check. FWIW, the autoconf changes themselves look good.

ericsnowcurrently
ericsnowcurrently reviewed Sep 16, 2022
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

This seems okay to me too.

First, though:

How can we verify (now or in the future) that these modules can't use strict overflow or that everything else can?

@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 16, 2022
Open
@ericsnowcurrently
Copy link
Member

How can we verify (now or in the future) that these modules can't use strict overflow or that everything else can?

Let's discuss this over on gh-96821.

@matthiasgoergens matthiasgoergens changed the title gh-96821: Mark modules that need -fno-strict-overflow gh-96821: Machinery for -fstrict-overflow Sep 17, 2022
@CAM-Gerlach CAM-Gerlach added performance Performance or resource usage build The build process and cross-build labels Sep 18, 2022
erlend-aasland
erlend-aasland reviewed Sep 20, 2022
View reviewed changes
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@matthiasgoergens
Copy link
Contributor Author

@AA-Turner Thanks for the suggestions! Much appreciated.

erlend-aasland
erlend-aasland previously approved these changes Oct 14, 2022
View reviewed changes
erlend-aasland
erlend-aasland reviewed Oct 14, 2022
View reviewed changes
configure.ac Outdated
Comment on lines 2109 to 2111
[--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.

@ericsnowcurrently
Copy link
Member

Thanks for shepherding this through, @erlend-aasland!

@ericsnowcurrently
Copy link
Member

...and @matthiasgoergens for doing the work, of course. 😄

@erlend-aasland erlend-aasland dismissed their stale review October 19, 2022 08:33

Awaiting final changes

@smontanaro
Copy link
Contributor

(I'm working my way through some PRs which have been approved and are labeled "awaiting merge", hence my seemingly bolt from the blue comment. Why? Read here.)

This work looks to be completed. Can someone merge?

@erlend-aasland
Copy link
Contributor

(I'm working my way through some PRs which have been approved and are labeled "awaiting merge", hence my seemingly bolt from the blue comment. Why? Read here.)

This work looks to be completed. Can someone merge?

I'm awaiting final changes. See #96823 (comment)

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Jan 20, 2023 •
edited
Loading

@erlend-aasland I was interested in picking this up, so I added the help message change you suggested here #96823 (comment) to the PR

I was looking into adding the warn as well, but I'm not familiar with autoconf. The existing PR looks like it's missing something to me... should we be setting ac_cv_cc_supports_fstrict_overflow in the branches of the AC_COMPILE_IFELSE?

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 20, 2023 •
edited
Loading

@erlend-aasland I was interested in picking this up, so I added the help message change you suggested here #96823 (comment) to the PR

Great, thanks :)

I was looking into adding the warn as well, but I'm not familiar with autoconf. The existing PR looks like it's missing something to me... should we be setting ac_cv_cc_supports_fstrict_overflow in the branches of the AC_COMPILE_IFELSE?

Yes; if we don't, configure will execute this check for every run, even if we're using the -C flag. Also, we must not set STRICT_OVERFLOW_CFLAGS and NO_STRICT_OVERFLOW_CFLAGS inside the AC_CACHE_CHECK; instead, add a AS_VAR_IF after the check, and set those two variables if ac_cv_cc_supports_fstrict_overflow is yes.

@matthiasgoergens
Copy link
Contributor Author

Please pick this up, if you can. Thanks!

I don't have as much time as I would like these days.

@hauntsaninja
Copy link
Contributor

Okay, I think this is ready. I'm not familiar with autoconf, so apologies for any issues!

@erlend-aasland
Copy link
Contributor

Thanks, Shantanu! It looks good, AFAICS. As an improvement, we could rewrite the logic to only check for the compiler switch if --with-strict-overflow was given.

@CAM-Gerlach
Copy link
Member

Gentle ping @hauntsaninja @erlend-aasland This came up in discussion and it looks like we're really close to being -fstrict-overflow clean here. Anything else this needs to be merged for 3.12, besides reviewing and merging #96925 also? From a (minimal) docs review perspective, this looks good. Thanks!

@hauntsaninja
Copy link
Contributor

I think this is ready to merge if we need it. Erlend's last comment is a nice to have and I can address it in a follow up PR.

@matthiasgoergens
Copy link
Contributor Author

Nice to see this progressing!

@hauntsaninja hauntsaninja merged commit eff9f43 into python:main Mar 4, 2023
@erlend-aasland
Copy link
Contributor

Thanks Shantanu and Matthias!

hugovk pushed a commit to hugovk/cpython that referenced this pull request Mar 6, 2023
@matthiasgoergens @CAM-Gerlach
@erlend-aasland @AA-Turner @hauntsaninja @hugovk
pythongh-96821: Add config option --with-strict-overflow (python#96823
c9931ce 
)

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Shantanu <hauntsaninja@gmail.com>
carljm added a commit to carljm/cpython that referenced this pull request Mar 6, 2023
@carljm
Merge branch 'main' into nodeaddict
33dda2a 
* main: (21 commits)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives in sub interpreters module (python#102472)
  pythongh-95672: Fix versionadded indentation of get_pagesize in test.rst (pythongh-102455)
  pythongh-102416: Do not memoize incorrectly loop rules in the parser (python#102467)
  pythonGH-101362: Optimise PurePath(PurePath(...)) (pythonGH-101667)
  pythonGH-101362: Check pathlib.Path flavour compatibility at import time (pythonGH-101664)
  pythonGH-101362: Call join() only when >1 argument supplied to pathlib.PurePath() (python#101665)
  pythongh-102444: Fix minor bugs in `test_typing` highlighted by pyflakes (python#102445)
  pythonGH-102341: Improve the test function for pow (python#102342)
  Fix unused classes in a typing test (pythonGH-102437)
  pythongh-101979: argparse: fix a bug where parentheses in metavar argument of add_argument() were dropped (python#102318)
  pythongh-102356: Add thrashcan macros to filter object dealloc (python#102426)
  Move around example in to_bytes() to avoid confusion (python#101595)
  pythonGH-97546: fix flaky asyncio `test_wait_for_race_condition` test (python#102421)
  pythongh-96821: Add config option `--with-strict-overflow` (python#96823)
  pythongh-101992: update pstlib module documentation (python#102133)
  pythongh-63301: Set exit code when tabnanny CLI exits on error (python#7699)
  pythongh-101863: Fix wrong comments in EUC-KR codec (pythongh-102417)
  pythongh-102302 Micro-optimize `inspect.Parameter.__hash__` (python#102303)
  pythongh-102179: Fix `os.dup2` error reporting for negative fds (python#102180)
  pythongh-101892: Fix `SystemError` when a callable iterator call exhausts the iterator (python#101896)
  ...
carljm added a commit to carljm/cpython that referenced this pull request Mar 7, 2023
@carljm
Merge branch 'main' into inlinecomp2
ecb313c 
* main: (37 commits)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives in sub interpreters module (python#102472)
  pythongh-95672: Fix versionadded indentation of get_pagesize in test.rst (pythongh-102455)
  pythongh-102416: Do not memoize incorrectly loop rules in the parser (python#102467)
  pythonGH-101362: Optimise PurePath(PurePath(...)) (pythonGH-101667)
  pythonGH-101362: Check pathlib.Path flavour compatibility at import time (pythonGH-101664)
  pythonGH-101362: Call join() only when >1 argument supplied to pathlib.PurePath() (python#101665)
  pythongh-102444: Fix minor bugs in `test_typing` highlighted by pyflakes (python#102445)
  pythonGH-102341: Improve the test function for pow (python#102342)
  Fix unused classes in a typing test (pythonGH-102437)
  pythongh-101979: argparse: fix a bug where parentheses in metavar argument of add_argument() were dropped (python#102318)
  pythongh-102356: Add thrashcan macros to filter object dealloc (python#102426)
  Move around example in to_bytes() to avoid confusion (python#101595)
  pythonGH-97546: fix flaky asyncio `test_wait_for_race_condition` test (python#102421)
  pythongh-96821: Add config option `--with-strict-overflow` (python#96823)
  pythongh-101992: update pstlib module documentation (python#102133)
  pythongh-63301: Set exit code when tabnanny CLI exits on error (python#7699)
  pythongh-101863: Fix wrong comments in EUC-KR codec (pythongh-102417)
  pythongh-102302 Micro-optimize `inspect.Parameter.__hash__` (python#102303)
  pythongh-102179: Fix `os.dup2` error reporting for negative fds (python#102180)
  pythongh-101892: Fix `SystemError` when a callable iterator call exhausts the iterator (python#101896)
  ...
@matthiasgoergens matthiasgoergens mentioned this pull request Mar 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@AA-Turner AA-Turner AA-Turner left review comments

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Assignees

@erlend-aasland erlend-aasland

Labels
build The build process and cross-build performance Performance or resource usage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@matthiasgoergens @erlend-aasland @ericsnowcurrently @smontanaro @hauntsaninja @CAM-Gerlach @AA-Turner @bedevere-bot