Page MenuHomePhabricator

Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication
ClosedPublic

Authored by teemperor on Jan 21 2019, 4:11 AM.

Details

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.

Event Timeline

teemperor created this revision.Jan 21 2019, 4:11 AM

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 added subscribers: beanz, labath.
labath added inline comments.
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 marked an inline comment as done.Jan 21 2019, 6:49 AM
teemperor added inline comments.
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 added inline comments.Jan 21 2019, 7:05 AM
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 updated this revision to Diff 182797.Jan 21 2019, 8:15 AM
  • Moved define to Xcode's Config.h (Thanks Pavel!)
labath accepted this revision.Jan 21 2019, 10:01 AM

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?

This revision is now accepted and ready to land.Jan 21 2019, 10:01 AM

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)

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 updated this revision to Diff 183265.Jan 24 2019, 1:29 AM
  • Added missing include.

Thanks for testing Jason!

This revision was automatically updated to reflect the committed changes.