[cmake] Enable -Werror=return-type for clang

[kastiglione]

Summary

Turn -Wreturn-type into an error.

Follow up to D98224, but making this an error only for clang.

Making -Wreturn-type an error is currently done by libcxx, libcxxabi, and libunwind, and for the same reasons makes be a good default for all of llvm. I'm not aware of any cases where this shouldn't be an error. This ensures different build configs, merges, and downstream branches catch issues sooner.

As mentioned in the comments, gcc basically doesn't support the notion of exhasutive switches over enums. After initially commiting D98224, downstream compilation errors resulted.

[xbolva00]

Thanks

[dblaikie]

I don't think this is appropriate/the right direction - we have a general mechanism for turning on warnings-as-errors for a build that I'd expect people to use if they want that behavior. I don't think we want to start opting in specific warnings to warnings-as-error status overriding the users request via the general flag.

[xbolva00]

If any code contains control paths that do not return a value, compiler optimizations can exploit this fact. I think it is dangerous to ignore this type of warning, hard error should be safe.

[xbolva00]

Unlike in C, in C++, flowing off the end of a non-void function other than main results in undefined behavior even when the value of the function is not used.

[kastiglione]

1. I've updated the summary to include wording from the original diff D98224 which points out that -Werror=return-type is used in libcxx, libcxxabi, and libunwind.
2. Warnings have a spectrum of severity, -Wreturn-type is one of the most severe, but turning on -Werror for more mundane warnings would be a deal breaker for some projects. I think it's more reasonable to turn errors on individually, rather than require a codebase to be all or nothing.
3. There are already other -Werror=... in this same file, so there is precedent.

Are there downsides to this? I don't see them, I only seem benefits.

[dblaikie]

In D99825#2671982, @kastiglione wrote:

1. I've updated the summary to include wording from the original diff D98224 which points out that -Werror=return-type is used in libcxx, libcxxabi, and libunwind.

Ah, thanks for the prior context - sorry I missed that review.

2. Warnings have a spectrum of severity, -Wreturn-type is one of the most severe, but turning on -Werror for more mundane warnings would be a deal breaker for some projects. I think it's more reasonable to turn errors on individually, rather than require a codebase to be all or nothing.

Generally we try pretty hard to be -Werror clean for clang at least (& some folks do so for gcc too).

3. There are already other -Werror=... in this same file, so there is precedent.

The two I see are date-time and unguarded-availability-new which I'd be similarly a bit suspicious of, but I guess they're constraint enforcement which is a bit different from warnings.

Are there downsides to this? I don't see them, I only seem benefits.

I'd rather not get into the territory of classifying different warnings - and encourage people to turn on -Werror generally, as many buildbots do.

[kastiglione]

Generally we try pretty hard to be -Werror clean for clang at least (& some folks do so for gcc too)

With this kind of change, it's one less place to _try_, the compiler would enforce it.

The two I see are date-time and unguarded-availability-new

There's also non-virtual-dtor and suggest-override.

I'd rather not get into the territory of classifying different warnings - and encourage people to turn on -Werror generally, as many buildbots do

I agree it would be good if -Werror were used pretty much everywhere. But with it not being used everywhere, downstream projects have warnings and can't flip the switch. A change like this is a small incremental improvement that prevents a certain kind of error at compile time, like the other -Werror=... flags. I think there's an argument to be made that this warning is more severe than some of the others marked as errors. And again some projects in llvm already turn it on, so there is evidence supporting it as a good thing.

Unfortunately I don't know what you mean by "I'd rather not get into the territory". Will this have a cost to you? Or do you mean you wish to preemptively prevent debates about which warnings should/shouldn't be errors?

Did you have any comments on @xbolva00's comments? I thought those were pretty compelling.

[aaron.ballman]

In D99825#2671963, @xbolva00 wrote:

Unlike in C, in C++, flowing off the end of a non-void function other than main results in undefined behavior even when the value of the function is not used.

FWIW, that suggests to me that -Wreturn-type should be an error in C++ by default.

[xbolva00]

In D99825#2672581, @aaron.ballman wrote:

In D99825#2671963, @xbolva00 wrote:

Unlike in C, in C++, flowing off the end of a non-void function other than main results in undefined behavior even when the value of the function is not used.

FWIW, that suggests to me that -Wreturn-type should be an error in C++ by default.

+1.

[dblaikie]

The two I see are date-time and unguarded-availability-new

There's also non-virtual-dtor and suggest-override.

I think those are only turned into errors for the feature test (the compiler's being run during the cmake configuration process with a sample that /should/ produce a specific warning/error (or shouldn't produce a specific error, if the test is validating a certain false positive case is not diagnosed) - it's easier for the script to test for non-zero exit code (& so uses -Werror) than parsing the diagnostic output.

I'd rather not get into the territory of classifying different warnings - and encourage people to turn on -Werror generally, as many buildbots do

I agree it would be good if -Werror were used pretty much everywhere. But with it not being used everywhere, downstream projects have warnings and can't flip the switch.

Not sure I follow this - presumably downstream users wouldn't be using LLVM's warning or werror flags, and would configure their own?

A change like this is a small incremental improvement that prevents a certain kind of error at compile time, like the other -Werror=... flags. I think there's an argument to be made that this warning is more severe than some of the others marked as errors.

I mean, there are lots of other warnings that represent pretty severe results - -Wnull-dereference, -Warray-bounds ?

And again some projects in llvm already turn it on, so there is evidence supporting it as a good thing.

Unfortunately I don't know what you mean by "I'd rather not get into the territory". Will this have a cost to you? Or do you mean you wish to preemptively prevent debates about which warnings should/shouldn't be errors?

Yep, pretty much that (don't want to start trying to classify some warnings as more severe than others, etc - generally I'm all for treating all warnings on a self-hosted clang build as errors and we either fix clang if the warning is bad or fix the code if the warning is good)

Did you have any comments on @xbolva00's comments? I thought those were pretty compelling.

There are many other warnings that diagnose UB the compiler can do bad things to - so I don't agree it's a good justification for this particular warning.

@aaron.ballman's point that maybe this should be -Werror by default in clang sounds plausible - because not all projects will be using -Werror, etc. If that helps some users who work on LLVM itself but don't want to turn on -Werror (not sure why - the buildbots will do it and so things will break/you'll have to fix stuff if you're not building with -Werror using a fairly recent clang) that's OK.

[kastiglione]

In D99825#2672670, @dblaikie wrote:

@aaron.ballman's point that maybe this should be -Werror by default in clang sounds plausible - because not all projects will be using -Werror, etc. If that helps some users who work on LLVM itself but don't want to turn on -Werror (not sure why - the buildbots will do it and so things will break/you'll have to fix stuff if you're not building with -Werror using a fairly recent clang) that's OK.

thanks, I'll open a diff to make return-type and error by default and see how that goes.

llvm/cmake/modules/HandleLLVMOptions.cmake

CHECK_C_SOURCE_COMPILES("// \\\\\\n//\\nint main() {return 0;}"
                           C_WCOMMENT_ALLOWS_LINE_WRAP)
   set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
   if (NOT C_WCOMMENT_ALLOWS_LINE_WRAP)
     append("-Wno-comment" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endif()
 
   # Enable -Wstring-conversion to catch misuse of string literals.
   add_flag_if_supported("-Wstring-conversion" STRING_CONVERSION_FLAG)
+
+  # Disabled for gcc. Clang and gcc differ in handling of switch statements
+  # that exhaustively handle all enumerations. This diagnostic is emitted by
+  # gcc in such cases, while clang does not. For gcc, exhaustive switch
+  # statements must be followed by a return statement or llvm_unreachable().
+  if (NOT CMAKE_COMPILER_IS_GNUCXX)
+    # Enable -Werror=return-type if available, to catch functions that contain
+    # control paths that do not return a value.
+    add_flag_if_supported("-Werror=return-type" WERROR_RETURN_TYPE)
+  endif()
 endif (LLVM_ENABLE_WARNINGS AND (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL))
 
 if (LLVM_COMPILER_IS_GCC_COMPATIBLE AND NOT LLVM_ENABLE_WARNINGS)
   append("-w" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 endif()
 
 macro(append_common_sanitizer_flags)
   if (NOT MSVC)