Don't enable -Wzero-as-null-pointer-constant on apple clang
Needs RevisionPublic

Authored by vonreth on Jan 26 2019, 11:41 AM.

Details

Summary

The unknown argument breaks the cmake visibility macro detection.

22:12:55 warning: unknown warning option '-Wzero-as-null-pointer-constant'; did you mean '-Wint-to-void-pointer-cast'? [-Wunknown-warning-option]
22:12:55 1 warning generated.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7544
Build 7562: arc lint + arc unit
vonreth created this revision.Jan 26 2019, 11:41 AM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptJan 26 2019, 11:41 AM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
vonreth requested review of this revision.Jan 26 2019, 11:41 AM
vonreth edited the summary of this revision. (Show Details)Jan 26 2019, 11:45 AM
vonreth added reviewers: aacid, apol, dfaure, rjvbb, bcooksley.

Why?

Why it breaks the cmake macro or why apple clang doesn't support it?
The second one I can't answer, for the first, its the same reason why we had to bump the cmake version requirement to 3 recently.
The cmake detection for the visibility flag is looking for the exit code and for warnings.
If the test fails it asumes that there are no visibility flags...

rjvbb requested changes to this revision.Jan 26 2019, 1:13 PM

See also https://phabricator.kde.org/D16894 which (initially) aimed to tackle this in a more general fashion.

IMHO the way forward (if my proposal is not acceptable) is to replace the existing clang version check with an actual test if the compiler supports the flac (CXXCompilerCheckFlag IIRC):

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") 
  if (CXXCompilerCheckFlag("-foo")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} foo")
  endif()
endif()

That avoids the overhead when GCC is used but also won't give false positives or negatives when clang is used because the compiler/version detection was just off.

kde-modules/KDEFrameworkCompilerSettings.cmake
75

I'm amazed that this works because CMake *may* detect Apple's clang as AppleClang depending on version and how policy 25 (IIRC) is set. Annoyingly that policy can only be set before the toplevel project statement, i.e. not in the ECM.

In addition, Apple's versioning is very hard if not impossible to match with upstream versioning.

This revision now requires changes to proceed.Jan 26 2019, 1:13 PM

This change is somewhat urgent - as can be seen at https://binary-factory.kde.org/view/MacOS/ all Mac builds are currently broken due to this issue.

aacid added a comment.Jan 26 2019, 6:51 PM

You sure about that? AFAICS from https://binary-factory.kde.org/view/MacOS/job/Kate_Release_macos/346/console it's just a warning, otherwise, ninja would stop after the first error, and it continues until a linking failure happens

aacid added a comment.Jan 26 2019, 6:52 PM

Oh, Hannah updated the summary, so an unknown option makes visibility stuff fail?

Wow Apple really did a bad fork of clang if that's what's happening.

rjvbb added a comment.Jan 26 2019, 7:18 PM

This is in fact cmake's fault, or ECM's for not taking a cmake quirk into account.

If you add an unknown option then all subsequent tests to see if the compiler supports a given argument will fail because cmake does not take into account which of the options generates a warning or error.

It's probably also a build system issue that builds break because of this. Instead of adapting it continues to add the visibility options, but the headerfiles that should add export magic to public symbols do not contain that magic.

Back when I found the origin of the issue (and created my diff, a few months ago) this affected only relatively few projects and I could for instance build most if not all of the frameworks with an Apple clang that vaguely corresponds to v3.5 .

The most appropriate quick fix is in my previous comment - evidently this supposes that no unsupported compiler argument is already present in the arguments list.

dfaure accepted this revision.Jan 27 2019, 6:55 PM

If it fixes the issue, this can go in, but like René says, this is quite surprising, since the default behavior (CMP0025 off) is that the compiler is called "Clang" on Apple platforms as well.
See cmake --help-policy CMP0025

rjvbb added a comment.Jan 27 2019, 9:04 PM

like René says, this is quite surprising

Hmmm, did I say exactly that? :)

The surpising bit is that this hasn't been an issue for much longer although maybe even that is not so surprising.

I continue to think that the check is not reliable as is. For instance, on my 10.9.5 system:

> /usr/bin/clang --version
Apple LLVM version 6.0 (clang-600.0.57) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin13.4.0
Thread model: posix

this compiler version does NOT support the flag in question here, as evident from the corresponding stock version (3.5). Later Apple clang versions no longer show the equivalence info but continue to use a versioning that is ahead of stock version numbers.
This particular version is reported by CMake as AppleClang 6.0.0.6000057 on the terminal, and CMAKE_CXX_COMPILER_VERSION "6.0.0.6000057" internally, iow, "not less than 5.0.0" ...