[CMake] Use find_package to find zlib

[phosek]

Summary

Use the FindZLIB CMake function with find_package to find zlib.
This is a builtin module provided by CMake which allows overriding
the zlib location through standard mechanisms.

[delcypher]

The idea behind this change is fine. The main problem in the implementation is not considering the scenario where LLVM_ENABLE_ZLIB is initially TRUE but is then changed at a later date. The current implementation would ignore the value of LLVM_ENABLE_ZLIB because the HAVE_LIBZ cache variable is still present from a previous configure run.

llvm/cmake/config-ix.cmake

112

If LLVM_ENABLE_ZLIB but ZLIB_FOUND is FALSE we should emit an error telling the user that Zlib couldn't be found and that should either tell CMake how to find it or set LLVM_ENABLE_ZLIB to FALSE. You also use the REQUIRED argument to find_package() as an alternative.

113

We need to handle the case that Zlib was found previously but LLVM_ENABLE_ZLIB is now FALSE. You can either do it here so that HAVE_LIBZ is set to FALSE or you can update all uses of HAVE_LIBZ to also check if LLVM_ENABLE_ZLIB is set to TRUE.

llvm/lib/Support/CMakeLists.txt

2

You shouldn't drop LLVM_ENABLE_ZLIB in the condition here, at least in the current implementation anyway. AFAIK both LLVM_ENABLE_ZLIB and HAVE_LIBZ are cache variables.

What could happen is that the user configures with LLVM_ENABLE_ZLIB and ZLIB is found and used.
Then at a later date with an existing build directory the user decides to set LLVM_ENABLE_ZLIB to FALSE.
If that happens HAVE_LIBZ will still be TRUE causing the code to continue to use Zlib despite LLVM_ENABLE_ZLIB being
set to FALSE.

llvm/lib/Support/Compression.cpp

21

Shouldn't this be HAVE_LIBZ?

27

Shouldn't this be HAVE_LIBZ?

[delcypher]

llvm/cmake/config-ix.cmake

113

Oh wait. HAVE_LIBZ is a regular variable and not a cache variable. Sorry I was confusing it with ZLIB_FOUND. In that case I think your implementation should work fine in the scenario I mentioned.

[delcypher]

llvm/lib/Support/CMakeLists.txt

2

Again. I think I confused HAVE_LIBZ with ZLIB_FOUND so your implementation is probably fine in the scenario I mentioned.

[phosek]

llvm/cmake/config-ix.cmake

112

I'm fine doing that, but it'd would be a different behavior from the current one where we silently ignore the missing library?

[delcypher]

llvm/cmake/config-ix.cmake

112

Yes that is different behaviour. I'm pretty convinced that the current behaviour is wrong.

In KLEE we use a slightly different pattern. We initialize the default option for enabling ZLIB depending on whether or not the library is available.

See https://github.com/klee/klee/blob/843e9be8fc10c6ffb30218c5a826aab192a31955/CMakeLists.txt#L344

The advantage of this approach is that if the system has the library we use it by default, but if the user explicitly sets the option to use zlib and its not available then we emit an error.

Given that it appears the existing behaviour of LLVM right now is to silently ignore missing libraries, you don't have to fix this right now if you don't want to. This could be addressed in a separate patch. Although I'd personally like a warning at the very least instead of LLVM's build system silently ignoring the request to use zlib.

[phosek]

llvm/cmake/config-ix.cmake

112

I agree that the approach you're following in KLEE is better, but in that case we'll probably have to change all the code to check LLVM_ENABLE_ZLIB rather than HAVE_LIBZ because even if HAVE_LIBZ is set to TRUE and we use it to se the default value of LLVM_ENABLE_ZLIB, user may still override that and set LLVM_ENABLE_ZLIB to FALSE. Does that sound fine to you?

[labath]

llvm/lib/Support/CMakeLists.txt

3

What would be the new value of ZLIB_LIBRARIES here? One of my previous attempts to refactor this failed because we have code which assumes system_libs is a list bare library names z dl ... that can then be prefixed with -l and passed to the linker. This fails horribly if you specify a full path to a library (which cmake likes to to).
Please make sure that llvm-config --system-libs produces something reasonable after this patch.

[delcypher]

llvm/cmake/config-ix.cmake

112

Sounds good to me. You may have to refactor the code a bit more because it looks like the declaration for LLVM_ENABLE_ZLIB is nowhere near where it's used. In fact it's not even in the same file.

llvm/lib/Support/CMakeLists.txt

3

Sounds like we should have a better test case then. It looks like test/tools/llvm-config/system-libs.test is too weak to check what you asking for.

[labath]

llvm/lib/Support/CMakeLists.txt

3

Yes, a better test case would be useful certainly (but don't worry you will get test failures, they will just be in compiler-rt, or other projects which use llvm-config to invoke the linker :)).

Anyway, I am not asking for anything. I don't have a horse in this game; as long as the main cmake project builds I am happy. I'm just giving you a heads up that you are likely to run into issues down this road.

[phosek]

llvm/lib/Support/CMakeLists.txt

3

Isn't this actually more correct? In our case, I'd like to use our own version of zlib and libxml2 rather than the system ones using the CMAKE_FIND_ROOT_PATH option. If llvm-config --system-libs prints -lz -lxml2, it's ambiguous which zlib and libxml2 it's referring to. Furthermore, in our case we want to link zlib and libxml2 statically, so presumably llvm-config --system-libs shouldn't print -lz -lxml2 at all but that's not currently the case.

[labath]

llvm/lib/Support/CMakeLists.txt

3

Isn't this actually more correct?

You could argue it's more correct if you also fixed --system-libs to output the library as /usr/lib/libz.so or something else that the linker can consume. As it stands now it will print -l/usr/lib/libz.so which will just blow up. I don't know much about the scenarios in which llvm-config is used (I just know that when I tried something like this I found a dozen angry emails waiting for me the next day), but this change might actually be enough to appease people relying on it.

Furthermore, in our case we want to link zlib and libxml2 statically, so presumably llvm-config --system-libs shouldn't print -lz -lxml2 at all but that's not currently the case.

The intended audience of llvm-config is people who want to link against llvm. For that the modality (shared/static) of libz doesn't matter. What matters is the modality of llvm. If you built llvm as a bunch of static libraries, when using them, you will need to pass libz to the linker (static or otherwise). If you built llvm as a single shared library, then you don't need to pass anything as shared libraries can track their dependencies (and I believe in this case the output of --system-libs will already be empty).

[phosek]

This was done in D70764.

llvm/cmake/config-ix.cmake

 check_include_file(sys/param.h HAVE_SYS_PARAM_H)
 check_include_file(sys/resource.h HAVE_SYS_RESOURCE_H)
 check_include_file(sys/stat.h HAVE_SYS_STAT_H)
 check_include_file(sys/time.h HAVE_SYS_TIME_H)
 check_include_file(sys/types.h HAVE_SYS_TYPES_H)
 check_include_file(termios.h HAVE_TERMIOS_H)
 check_include_file(unistd.h HAVE_UNISTD_H)
 check_include_file(valgrind/valgrind.h HAVE_VALGRIND_VALGRIND_H)
-check_include_file(zlib.h HAVE_ZLIB_H)
 check_include_file(fenv.h HAVE_FENV_H)
 check_symbol_exists(FE_ALL_EXCEPT "fenv.h" HAVE_DECL_FE_ALL_EXCEPT)
 check_symbol_exists(FE_INEXACT "fenv.h" HAVE_DECL_FE_INEXACT)
 
 check_include_file(mach/mach.h HAVE_MACH_MACH_H)
 check_include_file(histedit.h HAVE_HISTEDIT_H)
 check_include_file(CrashReporterClient.h HAVE_CRASHREPORTERCLIENT_H)
 if(APPLE)
if(HAVE_LIBPTHREAD)
   set(THREADS_HAVE_PTHREAD_ARG Off)
   find_package(Threads REQUIRED)
   set(LLVM_PTHREAD_LIB ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
 # Don't look for these libraries if we're using MSan, since uninstrumented third
 # party code may call MSan interceptors like strlen, leading to false positives.
 if(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
-  set(HAVE_LIBZ 0)
   if(LLVM_ENABLE_ZLIB)
-    foreach(library z zlib_static zlib)
-      string(TOUPPER ${library} library_suffix)
-      check_library_exists(${library} compress2 "" HAVE_LIBZ_${library_suffix})
-      if(HAVE_LIBZ_${library_suffix})
-        set(HAVE_LIBZ 1)
-        set(ZLIB_LIBRARIES "${library}")
-        break()
-      endif()
-    endforeach()
+    find_package(ZLIB)
+    if (ZLIB_FOUND)
+      set(HAVE_LIBZ TRUE)
+      include_directories(${ZLIB_INCLUDE_DIR})
   endif()
 
   # Don't look for these libraries on Windows.
   if (NOT PURE_WINDOWS)
     # Skip libedit if using ASan as it contains memory leaks.
     if (LLVM_ENABLE_LIBEDIT AND HAVE_HISTEDIT_H AND NOT LLVM_USE_SANITIZER MATCHES ".*Address.*")
       check_library_exists(edit el_init "" HAVE_LIBEDIT)
     else()
       set(HAVE_LIBEDIT 0)

 endif()
 
 if( LLVM_ENABLE_THREADS )
   message(STATUS "Threads enabled.")
 else( LLVM_ENABLE_THREADS )
   message(STATUS "Threads disabled.")
 endif()
 
-if (LLVM_ENABLE_ZLIB )
-  # Check if zlib is available in the system.
-  if ( NOT HAVE_ZLIB_H OR NOT HAVE_LIBZ )
-    set(LLVM_ENABLE_ZLIB 0)
-  endif()
-endif()
-
 if (LLVM_ENABLE_DOXYGEN)
   message(STATUS "Doxygen enabled.")
   find_package(Doxygen REQUIRED)
 
   if (DOXYGEN_FOUND)
     # If we find doxygen and we want to enable doxygen by default create a
     # global aggregate doxygen target for generating llvm and any/all
     # subprojects doxygen documentation.

llvm/include/llvm/Config/config.h.cmake

 #cmakedefine HAVE_TERMIOS_H ${HAVE_TERMIOS_H}
 
 /* Define to 1 if you have the <unistd.h> header file. */
 #cmakedefine HAVE_UNISTD_H ${HAVE_UNISTD_H}
 
 /* Define to 1 if you have the <valgrind/valgrind.h> header file. */
 #cmakedefine HAVE_VALGRIND_VALGRIND_H ${HAVE_VALGRIND_VALGRIND_H}
 
-/* Define to 1 if you have the <zlib.h> header file. */
-#cmakedefine HAVE_ZLIB_H ${HAVE_ZLIB_H}
-
 /* Have host's _alloca */
 #cmakedefine HAVE__ALLOCA ${HAVE__ALLOCA}
 
 /* Define to 1 if you have the `_chsize_s' function. */
 #cmakedefine HAVE__CHSIZE_S ${HAVE__CHSIZE_S}
 
 /* Define to 1 if you have the `_Unwind_Backtrace' function. */
 #cmakedefine HAVE__UNWIND_BACKTRACE ${HAVE__UNWIND_BACKTRACE}

 
 /* Linker version detected at compile time. */
 #cmakedefine HOST_LINK_VERSION "${HOST_LINK_VERSION}"
 
 /* Target triple LLVM will generate code for by default */
 /* Doesn't use `cmakedefine` because it is allowed to be empty. */
 #define LLVM_DEFAULT_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}"
 
-/* Define if zlib compression is available */
-#cmakedefine01 LLVM_ENABLE_ZLIB
-
 /* Define if overriding target triple is enabled */
 #cmakedefine LLVM_TARGET_TRIPLE_ENV "${LLVM_TARGET_TRIPLE_ENV}"
 
 /* LLVM version information */
 #cmakedefine LLVM_VERSION_INFO "${LLVM_VERSION_INFO}"
 
 /* Whether tools show host and target info when invoked with --version */
 #cmakedefine01 LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO

llvm/lib/Support/CMakeLists.txt

 set(system_libs)
-if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
+if ( HAVE_LIBZ )
   set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
 endif()
 if( MSVC OR MINGW )
   # libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
   # advapi32 required for CryptAcquireContextW in lib/Support/Windows/Path.inc.
   set(system_libs ${system_libs} psapi shell32 ole32 uuid advapi32)
 elseif( CMAKE_HOST_UNIX )
   if( HAVE_LIBRT )
     set(system_libs ${system_libs} rt)

llvm/lib/Support/Compression.cpp

 
 #include "llvm/Support/Compression.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_ZLIB_H
+#if HAVE_LIBZ
 #include <zlib.h>
 #endif
 
 using namespace llvm;
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if HAVE_LIBZ
 static Error createError(StringRef Err) {
   return make_error<StringError>(Err, inconvertibleErrorCode());
 }
 
 static int encodeZlibCompressionLevel(zlib::CompressionLevel Level) {
   switch (Level) {
     case zlib::NoCompression: return 0;
     case zlib::BestSpeedCompression: return 1;

llvm/unittests/Support/CompressionTest.cpp

 #include "llvm/Config/config.h"
 #include "llvm/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if HAVE_LIBZ
 
 void TestZlibCompression(StringRef Input, zlib::CompressionLevel Level) {
   SmallString<32> Compressed;
   SmallString<32> Uncompressed;
 
   Error E = zlib::compress(Input, Compressed, Level);
   EXPECT_FALSE(E);
   consumeError(std::move(E));