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

Jump to bottom

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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 36 commits into from
+104 −18

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Sep 14, 2022 •
edited

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

Let me remove the special handling for _struct, because that PR is already merged. And then we can merge this one?

@matthiasgoergens
Copy link
Contributor Author

OK, this is ready to merge as far as I am concerned. @erlend-aasland

@erlend-aasland
Copy link
Contributor

I'll wait for a final thumbs-up from @ericsnowcurrently before merging.

@erlend-aasland erlend-aasland self-assigned this Oct 10, 2022
@ericsnowcurrently
Copy link
Member

This seems good to me. However, I don't have the expertise to review the configure script changes.

@erlend-aasland
Copy link
Contributor

@matthiasgoergens, can you please document the new configure option and add a link to those docs in the NEWS entry? See Doc/using/configure.rst and the :option: Sphinx directive.

AA-Turner
AA-Turner reviewed Oct 11, 2022
View changes
Doc/using/configure.rst Outdated Show resolved Hide resolved
matthiasgoergens and others added 3 commits Oct 12, 2022
@matthiasgoergens @AA-Turner
Update Doc/using/configure.rst
ca6e6ba 
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@matthiasgoergens @AA-Turner
Update Misc/NEWS.d/next/Build/2022-09-14-10-38-15.gh-issue-96821.Zk2a…
fa14763 
…9c.rst

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@matthiasgoergens
Merge branch 'main' into strict_overflow
6b09fa7
@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 changes
erlend-aasland
erlend-aasland reviewed Oct 14, 2022
View changes
configure.ac Outdated
[--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 Oct 19, 2022

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

@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

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericsnowcurrently ericsnowcurrently

@AA-Turner AA-Turner

@erlend-aasland erlend-aasland

@CAM-Gerlach CAM-Gerlach

Assignees

@erlend-aasland erlend-aasland

Labels
awaiting merge build The build process and cross-build DO-NOT-MERGE performance Performance or resource usage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@matthiasgoergens @erlend-aasland @ericsnowcurrently @smontanaro @hauntsaninja @AA-Turner @CAM-Gerlach @bedevere-bot