Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication

[teemperor]

Summary

The field m_decompression_scratch_type is only used when HAVE_LIBCOMPRESSION is
defined, which caused a warning which I fixed in rLLDB350675 by just marking the variable as always used.

This patch fixes this in a better way by only defining the variable (and the related m_decompression_scratch
variable) when HAVE_LIBCOMPRESSION is defined. This also required changing the way we handle
HAVE_LIBCOMPRESSION works, as this was previously always defined on macOS within the source file
but not in the header. Now it's always defined from within our config header when CMake defines it or when
we are on macOS.

The field initialization was moved to the header to prevent that we have #ifdef within our initializer list.

[teemperor]

I currently don't have access to a macOS machine and this only affects macOS, so I would appreciate if someone could check if this still compiles/works as intended. Otherwise I can also just commit it and watch the macOS bots.

[labath]

include/lldb/Host/Config.h.cmake

32–40 ↗

(On Diff #182769)

Why is this even necessary? If libcompression is always available on apple platforms, then the cmake check should always succeed, and there should be no need to fix it up. Maybe the check needs to be fixed/improved?

[teemperor]

include/lldb/Host/Config.h.cmake

32–40 ↗

(On Diff #182769)

I don't think I found a good answer for that in the git log. Maybe it's a workaround for the Xcode project?

[labath]

include/lldb/Host/Config.h.cmake

32–40 ↗

(On Diff #182769)

That shouldn't be necessary. XCode has a special version of this file at include/lldb/Host/Config.h. However, it looks like this macro is not defined there. So maybe all that's needed is to hardcode #define HAVE_LIBCOMPRESSION there?

[teemperor]

• Moved define to Xcode's Config.h (Thanks Pavel!)

[labath]

This looks much better. Thanks.

(I haven't tested on a mac, but it doesn't seem like a particularly dangerous change to make).

Maybe, if this sticks, https://reviews.llvm.org/D48977 could be backed out too?

[jasonmolenda]

I'll test this later today. The cleanup looks good; the goal of the most recent changes was to make HAVE_LIBCOMPESSION always enabled on Darwin systems - the setting would occasionally get lost in the Xcode project settings and we'd have slow channels using uncompressed communication until we looked into the perf regressions. After this sequence happened twice, I wanted to make it always-on (it also needed to be conditional for a while because these API were only added to Darwin ~4 years ago)

[jasonmolenda]

Sorry for the delay in checking this out. It works fine. You need to add a #include "lldb/Host/Config.h" to GDBRemoteCommunication.h.

[teemperor]

• Added missing include.

Thanks for testing Jason!

lldb/trunk/include/lldb/Host/Config.h

 #define LLDB_HAVE_EL_RFUNC_T 1
 
 #define HAVE_SYS_EVENT_H 1
 
 #define HAVE_PPOLL 0
 
 #define HAVE_SIGACTION 1
 
+#define HAVE_LIBCOMPRESSION 1
+
 #else
 
 #error This file is only used by the Xcode build.
 
 #endif
 
 #endif // #ifndef LLDB_HOST_CONFIG_H

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

 
 #include <condition_variable>
 #include <mutex>
 #include <queue>
 #include <string>
 #include <vector>
 
 #include "lldb/Core/Communication.h"
+#include "lldb/Host/Config.h"
 #include "lldb/Host/HostThread.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/Listener.h"
 #include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 #include "lldb/lldb-public.h"
 
 namespace lldb_private {
private:
   std::queue<StringExtractorGDBRemote> m_packet_queue; // The packet queue
   std::mutex m_packet_queue_mutex; // Mutex for accessing queue
   std::condition_variable
       m_condition_queue_not_empty; // Condition variable to wait for packets
 
   HostThread m_listen_thread;
   std::string m_listen_url;
 
-  CompressionType m_decompression_scratch_type;
-  void *m_decompression_scratch;
+#if defined(HAVE_LIBCOMPRESSION)
+  CompressionType m_decompression_scratch_type = CompressionType::None;
+  void *m_decompression_scratch = nullptr;
+#endif
 
   DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunication);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 namespace llvm {

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

 #include "ProcessGDBRemoteLog.h"
 
 #if defined(__APPLE__)
 #define DEBUGSERVER_BASENAME "debugserver"
 #else
 #define DEBUGSERVER_BASENAME "lldb-server"
 #endif
 
-#if defined(__APPLE__)
-#define HAVE_LIBCOMPRESSION
+#if defined(HAVE_LIBCOMPRESSION)
 #include <compression.h>
 #endif
 
 #if defined(HAVE_LIBZ)
 #include <zlib.h>
 #endif
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 
 //----------------------------------------------------------------------
 // GDBRemoteCommunication constructor
 //----------------------------------------------------------------------
 GDBRemoteCommunication::GDBRemoteCommunication(const char *comm_name,
                                                const char *listener_name)
     : Communication(comm_name),
 #ifdef LLDB_CONFIGURATION_DEBUG
       m_packet_timeout(1000),
 #else
       m_packet_timeout(1),
 #endif
       m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
       m_send_acks(true), m_compression_type(CompressionType::None),
-      m_listen_url(), m_decompression_scratch_type(CompressionType::None),
-      m_decompression_scratch(nullptr) {
-  // Unused unless HAVE_LIBCOMPRESSION is defined.
-  (void)m_decompression_scratch_type;
+      m_listen_url() {
 }
 
 //----------------------------------------------------------------------
 // Destructor
 //----------------------------------------------------------------------
 GDBRemoteCommunication::~GDBRemoteCommunication() {
   if (IsConnected()) {
     Disconnect();
   }
 
+#if defined(HAVE_LIBCOMPRESSION)
   if (m_decompression_scratch)
     free (m_decompression_scratch);
+#endif
 
   // Stop the communications read thread which is used to parse all incoming
   // packets.  This function will block until the read thread returns.
   if (m_read_thread_enabled)
     StopReadThread();
 }
 
 char GDBRemoteCommunication::CalculcateChecksum(llvm::StringRef payload) {