Page MenuHomePhabricator

[flang] Fix release build flags.
ClosedPublic

Authored by DavidTruby on May 29 2020, 6:16 AM.

Details

Summary

This patch removes the custom CMAKE_RELEASE_CXX_FLAGS variable.
This variable being set was having the effect of removing other important
Release flags, notably -DNDEBUG.

This patch may need to be accompanied by fixes for the macOS issues that
the removed comment mentions; I don't have a mac to test this on though so
hopefully a reviewer can help with that.

Diff Detail

Event Timeline

DavidTruby created this revision.May 29 2020, 6:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 6:16 AM
tskeith added a project: Restricted Project.May 29 2020, 7:30 AM

This builds for me with clang 9 on macos and I'm seeing -O3 -DNDEBUG on the clang++ command line.

@tskeith Did you or perhaps @pmccormick report the original problem on mac?

@tskeith Did you or perhaps @pmccormick report the original problem on mac?

I don't think I reported it.

@tskeith Did you or perhaps @pmccormick report the original problem on mac?

I don't think I reported it.

I had it listed as a TODO item in the CMake file but did not file an issue...

I had it listed as a TODO item in the CMake file but did not file an issue...

I didn't mean to make anyone feel guilty :) I'm just curious is this is a mac-only problem (which Tim reports is fixed in his environment) or if we observed it elsewhere.

LGTM

Builds fine on my Mac (Mac OS Mojave) with Apple Clang 10.0.1. For reference:

cmake -G Ninja -DLLVM_CCACHE_BUILD=On -DLLVM_APPEND_VC_REV=Off -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_PROJECTS="clang;flang;mlir" -DCMAKE_BUILD_TYPE=Release ../../llvm/
ninja
tskeith accepted this revision.May 30 2020, 9:58 AM
This revision is now accepted and ready to land.May 30 2020, 9:58 AM
This revision was automatically updated to reflect the committed changes.

The CMAKE_XYZ_FLAGS are supposed to be in control by the user, not by the project. Modifying them in the CMakeLists.txt is an anti-pattern. It has global effects, that is -DDEBUGF18 will be added to all LLVM projects in debug builds, not just flang. Did you consider using add_compile_definitions or target_compile_definitions instead?

The CMAKE_XYZ_FLAGS are supposed to be in control by the user, not by the project. Modifying them in the CMakeLists.txt is an anti-pattern. It has global effects, that is -DDEBUGF18 will be added to all LLVM projects in debug builds, not just flang. Did you consider using add_compile_definitions or target_compile_definitions instead?

I can look into that as well; I didn't write the original code I just spotted that we were not defining NDEBUG in Release builds and found a fix for that.

isuruf added a subscriber: isuruf.Jun 4 2020, 3:03 PM
It has global effects, that is -DDEBUGF18 will be added to all LLVM projects in debug builds, not just flang

Are you sure about this? CMake variables are scoped and would apply only to flang.