-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[RFC] build: turn down the _FORTIFY_SOURCE level on gcc >= 4.0 #1390
[RFC] build: turn down the _FORTIFY_SOURCE level on gcc >= 4.0 #1390
Conversation
It seems that every couple of months someone is running into this problem. This will help reduce the number of reports like #1368. |
It would be really nice of this finally fixes it until we can reach a consensus about using the gcc extension (and making the changes that this would require). |
@@ -42,6 +42,15 @@ if(APPLE) | |||
endif() | |||
endif() | |||
|
|||
# gcc 4.0 and better turn on _FORTIFY_SOURCE=2 automatically. This currently | |||
# does not work with Neovim due to some uses of dynamically-sized structures. | |||
if(CMAKE_COMPILER_IS_GNUCC AND NOT CMAKE_C_COMPILER_VERSION VERSION_LESS "4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe link to #223 here or in the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I squashed in a pointer to the issue in the comment. Thanks for the review!
LGTM! |
Unrelated but heads up (didn't want to create an issue out of this): I noticed that coverity started providing support for read-only defects by default. So I activated that setting. Now everyone can see our defects! |
🎅 |
LGTM! |
build: turn down the _FORTIFY_SOURCE level on gcc >= 4.0
Thanks @aktau! |
# -U in add_definitions doesn't end up in the correct spot, so we add it to | ||
# the flags variable instead. | ||
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1") | ||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jszakmeister Just curious, do we need to worry about CXX for MSVC? I thought I was only for C++ compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need to worry about it at the moment. There had been some mention of using C++ at some point, so I included it. I can back that part out, if you'd rather we didn't have it. It doesn't affect anything right now. And this block is only for GNU compilers, so MSVC is unaffected.
vim uses some dynamic structures that seem to trigger false positives with fortify-sources macro expansions when compiling in 64 bits. This tones it down to '1' from the default, which is '2'. More info: neovim/neovim#1390 Change-Id: I26c0f15479e67519178a52bca0c76dee5e2f9177
No description provided.