[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Hans Wennborg via Phabricator via lldb-commits]

hans added a comment.

There were some concerns raised on this patch, and also in PR44780.

I think at this point it's safer to revert these changes and start over. I've 
pushed the revert in 916be8fd6a0a0feea4cefcbeb0c22c65848d7a2e 
 and will 
merge that to 10.x after it's baked a little.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Khem Raj via Phabricator via lldb-commits]

raj.khem added a comment.

In D70764#1807405 , @smeenai wrote:

> In D70764#1803559 , @raj.khem wrote:
>
> > this is now in master, and I am seeing build failures in cross-building 
> > clang, e.g. when building clang for arm on a x86_64 host. its resorting to 
> > finding, libz from buildhost instead of target sysroot ( using --sysroot) 
> > and failing in link step. e.g.
> >
> > FAILED: bin/llvm-config
> >  ...
> >   -o bin/llvm-config  -Wl,-rpath,"\$ORIGIN/../lib"  
> > lib/libLLVMSupport.a  /usr/lib/libz.so  -lrt  -ldl  -ltinfo  -lm  
> > lib/libLLVMDemangle.a
> >  ...
> >
> > aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error 
> > adding symbols: file in wrong format
> >  clang-10: error: linker command failed with exit code 1 (use -v to see 
> > invocation)
> >
> > you can see that its adding /usr/lib/libz.so to linker cmdline while cross 
> > linking.
>
>
> Have you set `CMAKE_SYSROOT` appropriately?


That is not the problem. Since in cases of target its finding is properly.
there are multiple pieces where some are being built for native( build host) 
some for target so it thinks its building for
buildhost (native) llvm-config e.g. is one such case. Replacing it with -lz 
fixes it becasue compile environment is providing
right --sysroot for native or cross cases and it always links with correct libs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Shoaib Meenai via Phabricator via lldb-commits]

smeenai added a comment.

In D70764#1803559 , @raj.khem wrote:

> this is now in master, and I am seeing build failures in cross-building 
> clang, e.g. when building clang for arm on a x86_64 host. its resorting to 
> finding, libz from buildhost instead of target sysroot ( using --sysroot) and 
> failing in link step. e.g.
>
> FAILED: bin/llvm-config
>  ...
>   -o bin/llvm-config  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMSupport.a  
> /usr/lib/libz.so  -lrt  -ldl  -ltinfo  -lm  lib/libLLVMDemangle.a
>  ...
>
> aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error 
> adding symbols: file in wrong format
>  clang-10: error: linker command failed with exit code 1 (use -v to see 
> invocation)
>
> you can see that its adding /usr/lib/libz.so to linker cmdline while cross 
> linking.


Have you set `CMAKE_SYSROOT` appropriately?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Khem Raj via Phabricator via lldb-commits]

raj.khem added a comment.

this is now in master, and I am seeing build failures in cross-building clang, 
e.g. when building clang for arm on a x86_64 host. its resorting to finding, 
libz from buildhost instead of target sysroot ( using --sysroot) and failing in 
link step. e.g.

FAILED: bin/llvm-config
...
 -o bin/llvm-config  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMSupport.a  
/usr/lib/libz.so  -lrt  -ldl  -ltinfo  -lm  lib/libLLVMDemangle.a
...

aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error 
adding symbols: file in wrong format
clang-10: error: linker command failed with exit code 1 (use -v to see 
invocation)

you can see that its adding /usr/lib/libz.so to linker cmdline while cross 
linking.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd closed this revision.
compnerd added a comment.

GIT 68a235d07f9 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Chris Bieneman via Phabricator via lldb-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

I think this is good, and it has been sitting a while.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Chris Bieneman via Phabricator via lldb-commits]

beanz added a comment.

I conferred off-list with @compnerd. He and I talked through my concerns, and I 
believe this patch is fine as-is, so LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

`/usr/lib/x86_64-linux-gnu/libz.so` is definitely better than 
`-l/usr/lib/x86_64-linux-gnu/libz.so`, as it's at least a valid link line. It's 
not completely equivalent, and may not work in all cases -- last time I 
accidentally changed this, I got about a dozen emails from people I broke, and 
I wouldn't be surprised if one of them depended on the subtle differences here. 
However, the fact that libxml is also spelled out this way gives me some hope...

That said, this still leaves us with the "problem" that the default cmake 
configuration will be broken for people who don't have zlib installed (pretty 
much everyone on windows, at least). You can try that out by 
deleting/renaming/uninstalling zlib.h from your system. With this patch you get 
a configuration error whereas previously zlib support was automatically 
disabled. Whether this "problem" is a **problem** or "working as intended" 
depends on what do you expect out of your build system, but it's definitely not 
consistent with how other llvm dependencies are managed.

Anyway, I don't really have a horse in this race, but I figured I should at 
least warn you about the problems you're likely to run into. (and I really wish 
@beanz would say something here, as he's the one who's complaining about build 
system inconsistencies...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Chris Bieneman via Phabricator via lldb-commits]

beanz added a comment.

@labath sorry for not replying on-list. I've actually been discussing offline 
with @compnerd. If `llvm-config` is printing an absolute path, I'm concerned 
about how that will interact with binary package distributions for the llvm-dev 
packages. Not sure if @tstellar has any thoughts on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D70764#1767413 , @compnerd wrote:

> In D70764#1767395 , @JDevlieghere 
> wrote:
>
> > Having one canonical variable controlling zlib support seems indeed 
> > desirable.
> >
> > In D70519#1754618 , @labath wrote:
> >
> > > With this patch, what is the output of `llvm-config --system-libs` ?
> >
> >
> > @compnerd What's the answer to this for this patch?
>
>
> Sorry, didn't see the question.  From my local build:
>
>$ ./bin/llvm-config --system-libs
>   /usr/lib/x86_64-linux-gnu/libz.so -lrt -ldl -ltinfo -lpthread -lm 
> /usr/lib/x86_64-linux-gnu/libxml2.so
>


So that's the issue Pavel raised in the other review. Can we make this return 
`-lz` with the current approach?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D70764#1767395 , @JDevlieghere 
wrote:

> Having one canonical variable controlling zlib support seems indeed desirable.
>
> In D70519#1754618 , @labath wrote:
>
> > With this patch, what is the output of `llvm-config --system-libs` ?
>
>
> @compnerd What's the answer to this for this patch?


Sorry, didn't see the question.  From my local build:

   $ ./bin/llvm-config --system-libs
  /usr/lib/x86_64-linux-gnu/libz.so -lrt -ldl -ltinfo -lpthread -lm 
/usr/lib/x86_64-linux-gnu/libxml2.so


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Having one canonical variable controlling zlib support seems indeed desirable.

In D70519#1754618 , @labath wrote:

> With this patch, what is the output of `llvm-config --system-libs` ?


@compnerd What's the answer to this for this patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

@labath I think you are misunderstanding the patch.  This is not autoselecting 
the dependencies.  It is simply doing that based on an existing option that we 
have - `LLVM_ENABLE_ZLIB`.  We could always search for zlib and override the 
results with `LLVM_ENABLE_ZLIB` as well.  The current build will continue to 
just work - zlib is used only for the compressed debug sections (which requires 
the user to opt-in).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Please take a look at D70519  for the issues 
with this approach.

Also, while I do agree with you that we should not auto-select dependencies, I 
think this runs contrary to the llvm philosophy that the default built should 
"just work" (I don't know if that's formalized anywhere, but I know it's 
certainly what (some?) people expect).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70764/new/

https://reviews.llvm.org/D70764



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: beanz, smeenai.
Herald added subscribers: lldb-commits, Sanitizers, hiraditya, mgorny.
Herald added projects: clang, Sanitizers, LLDB, LLVM.

Rather than handling zlib handling manually, use `find_package` from CMake to 
find zlib properly.  Use this to normalize the `LLVM_ENABLE_ZLIB`, `HAVE_ZLIB`, 
`HAVE_ZLIB_H`.  Furthermore, require zlib if `LLVM_ENABLE_ZLIB` is set to 
`YES`, which requires the distributor to explicitly select whether zlib is 
enabled or not.  This simplifies the CMake handling and usage in the rest of 
the tooling.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70764

Files:
  clang/test/CMakeLists.txt
  clang/test/lit.site.cfg.py.in
  compiler-rt/test/lit.common.configured.in
  lld/test/CMakeLists.txt
  lld/test/lit.site.cfg.py.in
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  llvm/cmake/config-ix.cmake
  llvm/include/llvm/Config/config.h.cmake
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/CRC.cpp
  llvm/lib/Support/Compression.cpp
  llvm/test/CMakeLists.txt
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp

Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -21,7 +21,7 @@
 
 namespace {
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if LLVM_ENABLE_ZLIB
 
 void TestZlibCompression(StringRef Input, int Level) {
   SmallString<32> Compressed;
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -33,7 +33,7 @@
 config.host_ldflags = '@HOST_LDFLAGS@'
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
-config.have_zlib = @HAVE_LIBZ@
+config.have_zlib = @LLVM_ENABLE_ZLIB@
 config.have_libxar = @HAVE_LIBXAR@
 config.have_dia_sdk = @LLVM_ENABLE_DIA_SDK@
 config.enable_ffi = @LLVM_ENABLE_FFI@
Index: llvm/test/CMakeLists.txt
===
--- llvm/test/CMakeLists.txt
+++ llvm/test/CMakeLists.txt
@@ -1,12 +1,12 @@
 llvm_canonicalize_cmake_booleans(
   BUILD_SHARED_LIBS
   HAVE_LIBXAR
-  HAVE_LIBZ
   HAVE_OCAMLOPT
   HAVE_OCAML_OUNIT
   LLVM_ENABLE_DIA_SDK
   LLVM_ENABLE_FFI
   LLVM_ENABLE_THREADS
+  LLVM_ENABLE_ZLIB
   LLVM_INCLUDE_GO_TESTS
   LLVM_LIBXML2_ENABLED
   LLVM_LINK_LLVM_DYLIB
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -17,13 +17,13 @@
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_ZLIB_H
+#if LLVM_ENABLE_ZLIB
 #include 
 #endif
 
 using namespace llvm;
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if LLVM_ENABLE_ZLIB
 static Error createError(StringRef Err) {
   return make_error(Err, inconvertibleErrorCode());
 }
Index: llvm/lib/Support/CRC.cpp
===
--- llvm/lib/Support/CRC.cpp
+++ llvm/lib/Support/CRC.cpp
@@ -25,7 +25,7 @@
 
 using namespace llvm;
 
-#if LLVM_ENABLE_ZLIB == 0 || !HAVE_ZLIB_H
+#if !LLVM_ENABLE_ZLIB
 
 static const uint32_t CRCTable[256] = {
 0x, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, 0x706af48f,
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -1,7 +1,4 @@
-set(system_libs)
-if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
-  set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
-endif()
+set(system_libs ${ZLIB_LIBRARY})
 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.
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -109,9 +109,6 @@
 /* Define to 1 if you have the `pthread_setname_np' function. */
 #cmakedefine HAVE_PTHREAD_SETNAME_NP ${HAVE_PTHREAD_SETNAME_NP}
 
-/* Define to 1 if you have the `z' library (-lz). */
-#cmakedefine HAVE_LIBZ ${HAVE_LIBZ}
-
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_LINK_H ${HAVE_LINK_H}
 
@@ -226,9 +223,6 @@
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_VALGRIND_VALGRIND_H ${HAVE_VALGRIND_VALGRIND_H}
 
-/* Define to 1 if you have the  header file. */
-#cmakedefine HAVE_ZLIB_H ${HAVE_ZLIB_H}
-
 /* Have host's _alloca */
 #cmakedefine HAVE__ALLOCA ${HAVE__ALLOCA}
 
Index: llv