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

Support extra compiler flags during building #39191

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

omajid
Copy link
Member

@omajid omajid commented Jul 13, 2020

Many Linux distributions like to use an extra set of compiler flags (via CFLAGS, CXXFLAGS and LDFLAGS) to produce builds that are hardened against vulnerabilities and exploits. The flags sometimes also enable extra warnings to inform packagers about potential memory issues.

This pach adds support for that to dotnet/runtime.

The obvious method to make this work is to just export the CFLAGS, CXXFLAGS, and LDFLAGS directly. This, however, enables those flags during configure-time (aka cmake without --build too). That means several cmake configure tests get executed with unexpected compiler flags. These configure tests can then report incorrect results.

For example, https://bugzilla.redhat.com/show_bug.cgi?id=1712158 demonstrates an issue where the check for strerror_r in the runtime comes to the wrong conclusion because -Wall is enabled and a variable is unused.

A slightly longer fix is to support another set of environment variables, and use them to set CFLAGS, CXXFLAGS, LDFLAGS, but only for the invocation of cmake --build/make.

See #35727 for the complete details.

Fixes #35727

Some questions:

  • Is this the only call to cmake --build/make in the runtime build? I couldn't find any other place where cmake --build is called.
  • Is EXTRA_CFLAGS a good enough name? Any suggestions for a better name?
  • Should I file an issue for the Windows-side of this fix? Is one needed?
  • What would be a good place to document this?

@omajid
Copy link
Member Author

omajid commented Jul 22, 2020

What would be a good place to document this feature?

@jkoritzinsky
Copy link
Member

We currently don't have documentation for directly using the build-runtime.sh script since we direct people to use the root build scripts, so there isn't actually a good place to put docs.

Can you add some comments around the changes you've made to explain them? That should cover it enough.

Many Linux distributions like to use an extra set of compiler flags (via
`CFLAGS`, `CXXFLAGS` and `LDFLAGS`) to produce builds that are hardened
against vulnerabilities and exploits. The flags sometimes also enable
extra warnings to inform packagers about potential memory issues.

This pach adds support for that to dotnet/runtime.

The obvious method to make this work is to just export the `CFLAGS`,
`CXXFLAGS`, and `LDFLAGS` directly. This, however, enables those flags
during configure-time (aka `cmake` without `--build` too). That means
several cmake configure tests get executed with unexpected compiler
flags. These configure tests can then report incorrect results.

For example, https://bugzilla.redhat.com/show_bug.cgi?id=1712158
demonstrates an issue where the check for `strerror_r` in the runtime
comes to the wrong conclusion because `-Wall` is enabled and a variable
is unused.

A slightly longer fix is to support another set of environment
variables, and use them to set `CFLAGS`, `CXXFLAGS`, `LDFLAGS`, but only
for the invocation of `cmake --build`/`make`.

See dotnet#35727 for the complete details.

Fixes dotnet#35727
@omajid
Copy link
Member Author

omajid commented Aug 5, 2020

Do the changes look okay?

@omajid
Copy link
Member Author

omajid commented Aug 7, 2020

Assuming there are no objections or further reviews required, can someone please merge this? I don't have merge powers.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 1ec6939 into dotnet:master Aug 10, 2020
@am11
Copy link
Member

am11 commented Aug 10, 2020

@janvorli, for couple of years, the extra flags support has been via -cmakeargs -DCMAKE_C_FLAGS={value}. Top level build.sh supports -cmakeargs, as well as the internal build-runtime scripts. Did we still need this new implementation?

@janvorli
Copy link
Member

Hmm, I've forgotten about that. @omajid I suppose that would work for you as well, so we can revert this change, right?

@omajid
Copy link
Member Author

omajid commented Aug 10, 2020

I thought my use case was not being met. But maybe I was wrong. If we revert this now, can I try do another PR if it turns out the -cmakeargs -DCMAKE_C_FLAGS= isn't sufficient?

@janvorli
Copy link
Member

I wonder if you could try the other way first and we would revert the change only if the other way doesn't work for you?

@am11
Copy link
Member

am11 commented Aug 10, 2020

In addition to cmakeargs support; the environment variable support is actually quite the opposite: we fixed Clear Linux build issue: #2188, where the default environment had CFLAGS, CFFLAGS and CXXFLAGS etc. set, and was conflicting with one of the requirements of coreclr. We cleared that conflicting value explicitly in cmake script.

This means this PR is not needed for any of the flags, we can set the flags in the environment and it should just work:

# option 1
CFLAGS={some value} ./build.sh  # should work without this change

# option 2
./build.sh -cmakeargs -DCMAKE_C_FLAGS={some value}   # should work without this change

@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let builders provide customized CFLAGS for use by the runtime build
6 participants