Skip to content

build: Do not set _FORTIFY_SOURCE if it causes compiler warnings #20824

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
Closed
hebasto wants to merge 1 commit into from
Closed

build: Do not set _FORTIFY_SOURCE if it causes compiler warnings #20824

hebasto wants to merge 1 commit into from

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Jan 1, 2021

On master (e75f91e) passing CXXFLAGS variable to the configure script causes the default -O2 optimization flag is not set in the AC_PROG_CXX macro:

If output variable CXXFLAGS was not already set, set it to -g -O2 for the GNU C++ compiler (-O2 on systems where G++ does not accept -g), or -g for other compilers.

Such behavior leads to multiple warnings when compiling with clang 11.0 (on Fedora 33):

/usr/include/features.h:397:4: warning: _FORTIFY_SOURCE requires compiling with optimization (-O) [-W#warnings]
#  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
   ^
1 warning generated.

This PR ensures that -D_FORTIFY_SOURCE flag won't cause such warnings.

@luke-jr
Copy link
Member

luke-jr commented Jan 1, 2021

Not sure this is a good idea. Better the builder knows he isn't getting the fortification?

Also, pretty sure the PR as-is breaks when the compiler has _FORTIFY_SOURCE enabled by default (with just -D, it causes a warning, hence the check for -U)

@fanquake
Copy link
Member

fanquake commented Jan 4, 2021

Not sure this is a good idea. Better the builder knows he isn't getting the fortification?

I agree with Luke. This change is (essentially silently) disabling a security feature in lieu of emitting compiler warnings in situations where you'd actually want them.

If someone has overridden CXXFLAGS without understanding what they are doing, or any potential consequences, the warnings will alert them to that fact.

If someone knows what they are doing, and is providing their own CXXFLAGS, they know they can "fix" the warnings by adding an optimize option (-Ox), or, if they don't want any optimization / don't want to be using FORTIFY_SOURCE at all, they can add -U_FORTIFY_SOURCE to their CPPFLAGS etc.

@hebasto
Copy link
Member Author

hebasto commented Jan 4, 2021

@luke-jr @fanquake Thanks for your comments. Agree with both.

Closing.

@hebasto hebasto closed this Jan 4, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hebasto @luke-jr @fanquake @DrahtBot