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

Fix fortify source warning #1164

Closed

Conversation

@greenius
Copy link

greenius commented Jul 16, 2017

Fixes problem with warnings when _FORTIFIED_SOURCE is already defined by the compiler by undefining it on the command line before defining it again.

@greenius

This comment has been minimized.

Copy link
Author

greenius commented Jul 16, 2017

This also prevents the "Compiler must support a thread local storage class for trivially constructible types" error that you get on systems that automatically set _FORTIFIED_SOURCE without using --disable-warnings-as-errors command line option.

@acmorrow

This comment has been minimized.

Copy link
Contributor

acmorrow commented Jul 16, 2017

@greenius - Thanks for the PR. This was on my queue to fix (it is currently ticketed as https://jira.mongodb.org/browse/SERVER-29982), so I appreciate your sharing your fix with us. A few things we will want to do:

I'll take a closer look at the changes themselves later today or tomorrow.

@greenius greenius force-pushed the greenius:fix-_FORTIFY_SOURCE-warning branch from e83b589 to 12589d5 Jul 17, 2017
@greenius

This comment has been minimized.

Copy link
Author

greenius commented Jul 17, 2017

@acmorrow I have now squashed the commits, changed the message and signed the contributor agreement.

@@ -2490,6 +2502,10 @@ def doConfigure(myenv):
# http://lists.llvm.org/pipermail/cfe-dev/2015-November/045852.html
#
if env.TargetOSIs('posix') and not env.ToolchainIs('clang') and conf.CheckForFortify():
if conf.CheckForFortifyDefined():
conf.env.Prepend(
CPPFLAGS='-U_FORTIFY_SOURCE'

This comment has been minimized.

Copy link
@acmorrow

acmorrow Jul 17, 2017

Contributor

Rather than just having -U_FORTIFY_SOURCE -DFORTIFY_SOURCE, which feels a little strange, would it be better if we just didn't set our own _FORTIFY_SOURCE if the compiler has already done it?

In other words, just write:

if !conf.CheckForForifyDefined():
    conf.env.Append(
        CPPDEFINES=[
            ('_FORTIFY_SOURCE', 2)

This comment has been minimized.

Copy link
@greenius

greenius Jul 17, 2017

Author

That's what I did initially, but I think the default predefined value for _FORTIFY_SOURCE is 1 and this is setting it to 2, which enables more checking.

From http://man7.org/linux/man-pages/man7/feature_test_macros.7.html it says:

If _FORTIFY_SOURCE is set to 1, with compiler optimization level 1 (gcc -O1) and above, checks that shouldn't change the behavior of conforming programs are performed. With _FORTIFY_SOURCE set to 2, some more checking is added, but some conforming programs might fail.

This comment has been minimized.

Copy link
@acmorrow

acmorrow Jul 21, 2017

Contributor

Having thought about this a bit more, this really feels like a bug in the packaging of the compiler on this system. I'm fine with the distro toolchain wanting to default _FORTIFY_SOURCE=1, but it shouldn't cause a compilation error if I then chose to set _FORTIFY_SOURCE=2 on the command line, as I'm entirely entitled to do. That should simply cause the compiler to honor my choice rather than its default, not raise an error. While I'd be willing to pull this PR as a workaround, I think it might be better to try to first raise this as an issue with your distro toolchain maintainer.

This comment has been minimized.

Copy link
@acmorrow

acmorrow Jul 21, 2017

Contributor

@greenius - FYI I've added a similar comment to https://jira.mongodb.org/browse/SERVER-29982, if you want to follow along there.

@acmorrow acmorrow self-assigned this Jul 17, 2017
@Schubes

This comment has been minimized.

Copy link
Collaborator

Schubes commented Jan 8, 2018

I'm closing this pull request per Drew's comment on SERVER-29982

@Schubes Schubes closed this Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.