[flang] Use LLVM's flags

[isuruf]

Summary

The only difference is that LLVM_ENABLE_WERROR is set to OFF
by default, but we enable this in a standalone flang build

This commit fixes some windows issues with the flags

[isuruf]

Remove -Wno-unused-parameter as LLVM adds it anyways

[tskeith]

flang/CMakeLists.txt

239

-pedantic should still be included in the compilation options.

[isuruf]

flang/CMakeLists.txt

239

There's an option LLVM_ENABLE_PEDANTIC which is enabled by default.

[ChinouneMehdi]

flang/CMakeLists.txt

62

LLVM_ENABLE_WARNINGS is enabled by default.

63

Are you sure this could be enabled with MSVC.
Even LLVM can't be built with it.

[ChinouneMehdi]

flang/unittests/Evaluate/CMakeLists.txt

96

add_flang_executable should be sufficient.

[tskeith]

flang/CMakeLists.txt

239

When I do an out-of-tree build of flang, this is a compilation command that I'm seeing:

clang++ -DGTEST_HAS_RTTI=0 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I../../include -Iinclude -isystem $LLVM/mlir/include -isystem $LLVM/build/Release-clang-9/tools/mlir/include -isystem $LLVM/llvm/include -isystem $LLVM/build/Release-clang-9/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -Wno-string-conversion -Wno-unused-command-line-argument -Wstring-conversion -Wcovered-switch-default -Wno-nested-anon-types -O2 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -fno-exceptions -fno-rtti -std=c++17 -MD -MT lib/Common/CMakeFiles/obj.FortranCommon.dir/Fortran.cpp.o -MF lib/Common/CMakeFiles/obj.FortranCommon.dir/Fortran.cpp.o.d -o lib/Common/CMakeFiles/obj.FortranCommon.dir/Fortran.cpp.o -c ../../lib/Common/Fortran.cpp

There is no -pedantic there.

[isuruf]

flang/unittests/Evaluate/CMakeLists.txt

96

That doesn't work. add_flang_executable is meant for executables to be installed. Not for test executables.

Fails with a bunch of

CMake Error at /home/isuru/projects/llvm-project/llvm/cmake/modules/LLVMProcessSources.cmake:112 (message):
  Found unknown source file uint128.cpp

  Please update
  /home/isuru/projects/llvm-project/flang/unittests/Evaluate/CMakeLists.txt

Call Stack (most recent call first):
  /home/isuru/projects/llvm-project/llvm/cmake/modules/LLVMProcessSources.cmake:62 (llvm_check_source_file_list)
  /home/isuru/projects/llvm-project/llvm/cmake/modules/AddLLVM.cmake:766 (llvm_process_sources)
  /home/isuru/projects/llvm-project/flang/cmake/modules/AddFlang.cmake:101 (add_llvm_executable)
  /home/isuru/projects/llvm-project/flang/unittests/Evaluate/CMakeLists.txt:93 (add_flang_executable)

[isuruf]

Enable pedantic on standalone build and disable werror on msvc

[isuruf]

flang/CMakeLists.txt

239

Can you check now?

[isuruf]

Add option for enable exceptions and rtti

[tskeith]

flang/CMakeLists.txt

239

Yes, it's there now. So is -Wno-long-long. Is that intended?

[isuruf]

Remove last change

[isuruf]

flang/CMakeLists.txt

239

I don't think -Wlong-long is enabled in C++11 or above with -pedantic. So, that's harmless.

From gcc docs,

Warn if long long type is used. This is enabled by either -Wpedantic or -Wtraditional in ISO C90 and C++98 modes. To inhibit the warning messages, use -Wno-long-long.

[DavidTruby]

I don't think we should enable Werror by default anymore now that we're in LLVM. It often prevents building with newer compilers (LLVM sees issues with this a lot already) and can be enabled by hand anyway for developer builds.

[isuruf]

Don't enable -Werror by default even on standalone builds

[sscalpone]

Please give an example of system where this is a problem.

[sscalpone]

If Windows, are we able to make this change conditional on Windows? Are we able to fix the warnings instead?

[isuruf]

If Windows, are we able to make this change conditional on Windows?

Yes, that's what I had before the last change.

[isuruf]

Let me know what to do here (I can revert to the commit where Werror is turned on by default to non-MSVC platforms)

[isuruf]

Revert to werror on for standalone builds

[isuruf]

I reverted back. After this is accepted I will make a new differential to change the default where it can be discussed.
This change allows MSVC builds to proceed further and I don't want this to be held back by a decision on the default for werror.

[tskeith]

In D78306#1992905, @isuruf wrote:

I reverted back. After this is accepted I will make a new differential to change the default where it can be discussed.
This change allows MSVC builds to proceed further and I don't want this to be held back by a decision on the default for werror.

It looks like you're still removing -Werror for in-tree builds. Is that correct? That should also not be done without discussion.

[isuruf]

That's fair. Let me know what you think should happen here.

[DavidTruby]

I'm in favour of not having Werror on by default so that we don't hard fail on new compiler releases. I'm not massively fussed right now while we're not heading towards a release, but once we're in the official releases of LLVM this is really important; we absolutely should not hard fail in released versions when new versions of other compilers are released. In my opinion, an example like LLVM 11 failing to compile with GCC 12 out of the box because that added new warnings would be a really big problem. I'd rather just remove the Werror now to stop this situation ever happening.

[DavidTruby]

I'm still happy for this to go ahead without removing Werror, if we open another diff to remove Werror to discuss that separately. I do think the discussion needs having though.

[Meinersbur]

flang/CMakeLists.txt

62

Projects having Werror enabled by default one of the reasons why it is hard to implement new warnings. Please don't.

For instance, I fixed false-negative warnings in Clang. The patch was reverted because it broke the Chrome build.

[sscalpone]

Please make sure LLVM_ENABLE_WERROR, LLVM_ENABLE_PEDANTIC, etc work for out-of-tree builds.

flang/CMakeLists.txt

62

Flang is built in tree with Clang, so such a change would not escape.

[Meinersbur]

flang/CMakeLists.txt

62

Flang and clang in the same repository gives the unique possibility that with adding a warning flag, I can fix the cause of the warning in flang. But generally, e.g. a new version of gcc emitting new warnings when compiling flang, this is not the case.

[mehdi_amini]

@isuruf ping on this revision? I was about to start doing a similar cleanup.

[isuruf]

This is ready from my end. Just needs a decision on what to do about werror. My preference is to not enable it by default, but others may disagree.

[tskeith]

With this change is it possible to enable -Werror for flang (in an in-tree build) without enabling it for all of LLVM? Also, -pedantic?

[DavidTruby]

As I said before, my strong preference is also to not have Werror by default; however I'm happy to let this patch go ahead and open a separate patch for that if this fixes people's builds.

[isuruf]

@tskeith, no. That can only be done in an out-of-tree build. LLVM doesn't have a mechanism to do it for a specific subproject and adding it here would mean copying that code from LLVM here.
(Using the current code in master is not desirable as the flags in master do not work for some compilers like MSVC.)

[DavidTruby]

In D78306#2018241, @tskeith wrote:

With this change is it possible to enable -Werror for flang (in an in-tree build) without enabling it for all of LLVM? Also, -pedantic?

Could you let us know why you think this is necessary when LLVM_ENABLE_WERROR and LLVM_ENABLE_PEDANTIC exist?

It's perfectly acceptable to stand up build bots that enable these two flags and enforce that those build bots always pass in master; I just really don't think we want these flags by default when new compiler releases after our own releases (and therefore completely out of our control) can cause build failures with these flags.

[tskeith]

In D78306#2018333, @DavidTruby wrote:

In D78306#2018241, @tskeith wrote:

With this change is it possible to enable -Werror for flang (in an in-tree build) without enabling it for all of LLVM? Also, -pedantic?

Could you let us know why you think this is necessary when LLVM_ENABLE_WERROR and LLVM_ENABLE_PEDANTIC exist?

It's perfectly acceptable to stand up build bots that enable these two flags and enforce that those build bots always pass in master; I just really don't think we want these flags by default when new compiler releases after our own releases (and therefore completely out of our control) can cause build failures with these flags.

I want to make sure that my flang changes (and everyone else's) build without warnings from the compilers I test with. LLVM_ENABLE_WERROR etc. is fine if LLVM and MLIR don't have any warnings, but that hasn't been my experience.

[mehdi_amini]

In D78306#2018686, @tskeith wrote:

I want to make sure that my flang changes (and everyone else's) build without warnings from the compilers I test with. LLVM_ENABLE_WERROR etc. is fine if LLVM and MLIR don't have any warnings, but that hasn't been my experience.

Note that we have a disjoint set of host environments, for me ninja check-mlir does not produce any warning locally and passes with -DLLVM_ENABLE_WERROR=ON (using clang-10 on Linux), but that does not mean it'll work for everyone else. Me building with Werror won't guarantee that your environment will be warning free.

Patches are welcome to fix warnings everywhere though if you encounter some! Folks have been sending patches to MLIR to fix warnings that they see on their platforms (Windows frequently, but also other Unix systems than Linux).

Looking at one clang invocation, these flags are used for me when building LLVM right now FYI: -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -fvisibility=hidden -fno-exceptions -fno-rtti -UNDEBUG -std=c++14

[DavidTruby]

In D78306#2018686, @tskeith wrote:

In D78306#2018333, @DavidTruby wrote:

In D78306#2018241, @tskeith wrote:

With this change is it possible to enable -Werror for flang (in an in-tree build) without enabling it for all of LLVM? Also, -pedantic?

Could you let us know why you think this is necessary when LLVM_ENABLE_WERROR and LLVM_ENABLE_PEDANTIC exist?

It's perfectly acceptable to stand up build bots that enable these two flags and enforce that those build bots always pass in master; I just really don't think we want these flags by default when new compiler releases after our own releases (and therefore completely out of our control) can cause build failures with these flags.

I want to make sure that my flang changes (and everyone else's) build without warnings from the compilers I test with. LLVM_ENABLE_WERROR etc. is fine if LLVM and MLIR don't have any warnings, but that hasn't been my experience.

I'm not opposed to adding a FLANG_ENABLE_WERROR and FLANG_ENABLE_PEDANTIC, there's even precedent for this in other LLVM projects. That way we could fine-tune which projects should have -Werror and -pedantic turned on and only enable it for those we care about. Then we can add BuildBots with these flags turned on for the compilers we test with to ensure all changes pass that.

What I'm opposed to is having this on by default; this is bad for users that just download the source of x release and find they can't build it with y compiler that came out after that release because of warnings promoted to errors.

[richard.barton.arm]

We discussed this on the technical call on 18 May - see https://docs.google.com/document/d/1Z2U5UAtJ-Dag5wlMaLaW1KRmNgENNAYynJqLW2j2AZQ

Summarising the discussion:

• We should split the flags fixes (which are agreed) from the flipping of the default for -Werror into separate patches. Suggest this patch be modified to fix the issues seen on Windows without changing the default value of -Werror.
• On the call we proposed a compromise of keeping -Werror on by default for Flang but having a local Flang CMake flag to disable it that folks can use that. @isuruf would that unblock you and fix the issue that caused you to submit the patch originally?
• We wondered whether the way the Cmake files are written already supports this through adding -DLLVM_ENABLE_WARNINGS=0 to the build line. Pat McCormick is going to look and reply.

[isuruf]

On the call we proposed a compromise of keeping -Werror on by default for Flang but having a local Flang CMake flag to disable it that folks can use that. @isuruf would that unblock you and fix the issue that caused you to submit the patch originally?

For out-of-tree builds, this is easy to do. For in-tree builds, I'll have to duplicate the logic of https://github.com/llvm/llvm-project/blob/e3e0367f9ba142b5a3d4c2362f11b3fcbacfc98d/llvm/cmake/modules/HandleLLVMOptions.cmake#L413-L415 and https://github.com/llvm/llvm-project/blob/e3e0367f9ba142b5a3d4c2362f11b3fcbacfc98d/llvm/cmake/modules/HandleLLVMOptions.cmake#L473-L477

If you are okay with duplicating code like this I'll add an option.

We wondered whether the way the Cmake files are written already supports this through adding -DLLVM_ENABLE_WARNINGS=0 to the build line. Pat McCormick is going to look and reply.

What's LLVM_ENABLE_WARNINGS got to do with anything?

[isuruf]

Turn on -Werror by default on non MSVC platforms

[isuruf]

Fix MSVC flag

[isuruf]

I've added a FLANG_ENABLE_WERROR which is turned on by default on non MSVC builds.

Please test.

[isuruf]

Simplify logic

[mehdi_amini]

What is the plan for incompatible host compiler version (clang vs gcc-9 that I mentioned before for example)?

[isuruf]

@mehdi_amini, I'm sorry, what issue is this?

[mehdi_amini]

In D78306#2042318, @isuruf wrote:

@mehdi_amini, I'm sorry, what issue is this?

Different compilers have different set of warnings: enabling Werror by default means it increases the likelihood that the build will just fails for a user that has a compiler version (+stdlib) you're not testing against continuously. This isn't a good experience to offer as a *default* settings.

[isuruf]

@mehdi_amini, after this is merged, I will make a new differential to change the default and people can argue there.
I'd really like to get this merged and this doesn't change the status quo.

[sscalpone]

flang/CMakeLists.txt

62

How about only default to ON if the user has not already set LLVM_ENABLE_WARNING? That lets the user say LLVM_ENABLE_WARNING=OFF.

63

How about only default to ON if the user has not already set LLVM_ENABLE_PEDANTIC?

79

How about preserving this line if LLVM_ENABLE_WARNINGS isn't otherwise set? This way, people who don't want warnings can just -DLLVM_ENABLE_WARNING=OFF.

239

It would be nice if these options were preserved with LLVM_COMPILER_IS_GCC_COMPATIBLE, beside -Werror, which is controlled elsewhere, and also, maybe -Wpedantic.

248

Is there a tradition of documenting a single-letter option like /X?

flang/lib/Optimizer/CMakeLists.txt

1

Why not include this under LLVM_COMPILER_IS_GCC_COMPATIBLE?

flang/unittests/Evaluate/CMakeLists.txt

92

Is rtti really needed here?

flang/unittests/Runtime/CMakeLists.txt

5

Is rtti really needed here?

[mehdi_amini]

In D78306#2042464, @isuruf wrote:

@mehdi_amini, after this is merged, I will make a new differential to change the default and people can argue there.
I'd really like to get this merged and this doesn't change the status quo.

Yes absolutely! Thanks for fixing this.

[isuruf]

flang/CMakeLists.txt

62

That is how CMake options work. The value we are setting here is a default and the user can override it on the command line.

63

Same as before

79

On a in-tree build, this option is already defined. In an out-of-tree build, this option is defined in Line 70

239

-Wall -Wextra -Wcast-qual -Wimplicit-fallthrough -Wdelete-non-virtual-dtor are all covered by LLVM_ENABLE_WARNINGS=ON which is the default.
-fno-rtti is covered by LLVM_ENABLE_RTTI=OFF which is the default.
-fno-exceptions is covered by LLVM_ENABLE_EH=OFF which is the default.

flang/lib/Optimizer/CMakeLists.txt

1

-Wno-unused-parameter is added by LLVM_ENABLE_WARNINGS

flang/unittests/Evaluate/CMakeLists.txt

92

Otherwise, there's a warning. See https://github.com/llvm/llvm-project/blob/6701993027f8af172d7ba697884459261b00e3c6/llvm/cmake/modules/AddLLVM.cmake#L17

flang/unittests/Runtime/CMakeLists.txt

5

Same as before

[DavidTruby]

@isuruf I think you're ok to merge this? Are there any objections left as long as we open another separate ticket to discuss -Werror?

[sscalpone]

@DavidTruby Do you want to merge this one?

[DavidTruby]

In D78306#2072045, @sscalpone wrote:

@DavidTruby Do you want to merge this one?

I would need @isuruf's email address and approval to merge this

[isuruf]

Yes, I'd like this to be merged. My email address is [e-mail].

[tskeith]

flang/CMakeLists.txt

255

What's the point of this warning? Is there a problem with having FLANG_ENABLE_WERROR set and LLVM_ENABLE_WERROR not set?

[mehdi_amini]

In D78306#2042464, @isuruf wrote:

@mehdi_amini, after this is merged, I will make a new differential to change the default and people can argue there.
I'd really like to get this merged and this doesn't change the status quo.

Do you already have the follow-up here?

flang/CMakeLists.txt

project(Flang)
   # We need a pre-built/installed version of LLVM.
   find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_PATH}")
   list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
 
   # If LLVM links to zlib we need the imported targets so we can too.
   if(LLVM_ENABLE_ZLIB)
     find_package(ZLIB REQUIRED)
   endif()
+  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
+  if (MSVC)
+    set(_FLANG_ENABLE_WERROR_DEFAULT OFF)
+  else ()
+    set(_FLANG_ENABLE_WERROR_DEFAULT ON)
+  endif()
+  option(LLVM_ENABLE_WERROR "Fail and stop if a warning is triggered." "${_FLANG_ENABLE_WERROR_DEFAULT}")
+  option(LLVM_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
 
   include(CMakeParseArguments)
   include(AddLLVM)
   include(HandleLLVMOptions)
   include(VersionFromVCS)
 
   if(LINK_WITH_FIR)
     include(TableGen)
     find_package(MLIR REQUIRED CONFIG)
     # Use SYSTEM for the same reasons as for LLVM includes
     include_directories(SYSTEM ${MLIR_INCLUDE_DIRS})
     list(APPEND CMAKE_MODULE_PATH ${MLIR_DIR})
     include(AddMLIR)
     find_program(MLIR_TABLEGEN_EXE "mlir-tblgen" ${LLVM_TOOLS_BINARY_DIR}
       NO_DEFAULT_PATH)
   endif()
 
-  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
   option(LLVM_INSTALL_TOOLCHAIN_ONLY
     "Only include toolchain files in the 'install' target." OFF)
   option(LLVM_FORCE_USE_OLD_HOST_TOOLCHAIN
     "Set to ON to force using an old, unsupported host toolchain." OFF)
 
 
   # Add LLVM include files as if they were SYSTEM because there are complex unused
   # parameter issues that may or may not appear depending on the environments and

 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/include/flang/Version.inc.in
   ${CMAKE_CURRENT_BINARY_DIR}/include/flang/Version.inc)
 # Configure Flang's version info header file.
 configure_file(
   ${FLANG_SOURCE_DIR}/include/flang/Config/config.h.cmake
   ${FLANG_BINARY_DIR}/include/flang/Config/config.h)
 
-# Add global F18 flags.
-set(CMAKE_CXX_FLAGS "-fno-rtti -fno-exceptions -pedantic -Wall -Wextra -Werror -Wcast-qual -Wimplicit-fallthrough -Wdelete-non-virtual-dtor ${CMAKE_CXX_FLAGS}")
-
 # Builtin check_cxx_compiler_flag doesn't seem to work correctly
 macro(check_compiler_flag flag resultVar)
   unset(${resultVar} CACHE)
   check_cxx_compiler_flag("${flag}" ${resultVar})
 endmacro()
 
 check_compiler_flag("-Werror -Wno-deprecated-copy" CXX_SUPPORTS_NO_DEPRECATED_COPY_FLAG)
 if (CXX_SUPPORTS_NO_DEPRECATED_COPY_FLAG)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-copy")
 endif()
 check_compiler_flag("-Wstring-conversion" CXX_SUPPORTS_NO_STRING_CONVERSION_FLAG)
 if (CXX_SUPPORTS_NO_STRING_CONVERSION_FLAG)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-string-conversion")
 endif()
 
 # Add appropriate flags for GCC
 if (LLVM_COMPILER_IS_GCC_COMPATIBLE)
 
   if (NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-strict-aliasing -fno-semantic-interposition")
   else()

flang/lib/Optimizer/CMakeLists.txt

-# Sources generated by tablegen have unused parameters.
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-parameter")
-
 add_subdirectory(Dialect)
 add_subdirectory(Support)

flang/unittests/Evaluate/CMakeLists.txt

target_link_libraries(logical-test
   FortranSemantics
   LLVMSupport
 )
 
 # GCC -fno-exceptions breaks the fenv.h interfaces needed to capture
 # IEEE exception flags (different use of the word "exception")
 # in the actual hardware floating-point status register, so ensure that
 # C++ exceptions are enabled for this test.
-set_source_files_properties(real.cpp PROPERTIES COMPILE_FLAGS -fexceptions)
+set(LLVM_REQUIRES_EH ON)
+set(LLVM_REQUIRES_RTTI ON)
 add_executable(real-test
   real.cpp
 )
+llvm_update_compile_flags(real-test)
 
 target_link_libraries(real-test
   FortranEvaluateTesting
   FortranEvaluate
   FortranDecimal
   FortranSemantics
   LLVMSupport
 )

flang/unittests/Runtime/CMakeLists.txt

-if(CMAKE_COMPILER_IS_GNUCXX OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang"))
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fexceptions")
-endif()
 set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
 
+# RuntimeTesting needs exceptions enabled
+set(LLVM_REQUIRES_EH ON)
+set(LLVM_REQUIRES_RTTI ON)
 add_library(RuntimeTesting
   testing.cpp
   )
+llvm_update_compile_flags(RuntimeTesting)
 
 add_executable(format-test
   format.cpp
 )
 
 target_link_libraries(format-test
   FortranRuntime
   RuntimeTesting