Page MenuHomePhabricator

[lldb] [test] Fix tests to use more portable LLVM_ENABLE_ZLIB
ClosedPublic

Authored by mgorny on Jan 4 2018, 5:54 AM.

Details

Summary

The HAVE_LIBZ variable is not exported by LLVM, and therefore is not
available in stand-alone builds of other tools. Use LLVM_ENABLE_ZLIB
which is the name under which the effective value is exported.

Additional, use llvm_canonicalize_cmake_booleans() to make sure that
a correct (Python-safe) boolean value is passed down to lit.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Jan 4 2018, 5:54 AM
labath added a comment.Jan 6 2018, 1:04 AM

Maybe we could just change the lit script to use the value of LLVM_ENABLE_ZLIB as the trigger? That should work in both standalone and non-standalone builds (and it seems more clean -- I'm not sure why llvm does not do the same)?

mgorny added a comment.Jan 6 2018, 1:28 AM

Maybe we could. I presumed people are using the separate variables for some reason. We'd have to check that it's properly sanitized when building in-tree and given to cmake though.

labath added a comment.Jan 6 2018, 1:44 AM

Maybe we could. I presumed people are using the separate variables for some reason. We'd have to check that it's properly sanitized when building in-tree and given to cmake though.

I would expect that reason is purely historical. While working on that patch I got the feeling that there is a lot of confusion in how to check for zlib presence. All everyone wants to know is whether zlib is present and working, so there should not be a need for different checks. We certainly don't need anything fancy here.

mgorny added a comment.Jan 6 2018, 2:15 AM

Well, it's actually more complex. In Gentoo we want to let the user choose whether he wants to build against zlib or without zlib support. So in the end there are two uses to be considered:

  1. When the value is merely passed down from LLVM to indicate state of compression support in LLVM. AFAIR that was the case with clang -- we merely wanted to know if we can run tests that require LLVM to have compression support.
  2. When the value is actually used to use zlib locally. To be honest, in this case I believe we should have a separate check & switch. FWICS, this is the case with LLDB.

So I don't think using LLVM_ENABLE_ZLIB here is correct, and my patch isn't correct as well.

That said, is there a reason for LLDB to use zlib directly instead of using the LLVM support compression library?

labath added a comment.Jan 7 2018, 1:01 AM

That said, is there a reason for LLDB to use zlib directly instead of using the LLVM support compression library?

We aren't using zlib directly. We only need to know whether llvm was configured with zlib support, so we can know whether we can expect some tests to pass. That is what this piece of code does.

mgorny added a comment.Jan 7 2018, 2:11 AM

I was talking of:

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
tools/debugserver/source/RNBRemote.cpp
mgorny updated this revision to Diff 128883.Jan 7 2018, 11:57 AM
mgorny retitled this revision from [lldb] [test] Fix missing HAVE_LIBZ for tests in stand-alone builds to [lldb] [test] Fix tests to use more portable LLVM_ENABLE_ZLIB.
mgorny edited the summary of this revision. (Show Details)

Updated the patch to use LLVM_ENABLE_ZLIB directly.

labath added a comment.Jan 9 2018, 5:10 AM

I was talking of:

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

Ah, right. These uses were introduced before we developed a preference for llvm primitives. That said, I see a lot of compression stuff there, but I don't see mention of zlib directly.

tools/debugserver/source/RNBRemote.cpp

Debugserver does not use llvm on purpose, but that should only worry you if your targetting darwin.

In any case, lgtm.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 9 2018, 6:45 AM
This revision was automatically updated to reflect the committed changes.
mgorny added a comment.Jan 9 2018, 6:51 AM

I was talking of:

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

Ah, right. These uses were introduced before we developed a preference for llvm primitives. That said, I see a lot of compression stuff there, but I don't see mention of zlib directly.

In the first of those files there are two HAVE_LIBZ conditionals that use zlib. There's also HAVE_LIBCOMPRESSION for the other library, I suppose it's relevant to Darwin.

Therefore, I have three questions:

  1. Should we add a separate check/switch for zlib here, or just use LLVM_ENABLE_ZLIB?
  2. Should I post a patch rewriting this little zlib bit to reuse libLLVMSupport?
  3. Should I look into merging libcompression support into libLLVMSupport?
tools/debugserver/source/RNBRemote.cpp

Debugserver does not use llvm on purpose, but that should only worry you if your targetting darwin.

In any case, lgtm.

Thanks, merged.