This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Cleanup log channel stuff to all be consistent and use 64 bit log channel mask.
AbandonedPublic

Authored by clayborg on Jan 14 2022, 6:58 PM.

Details

Summary

Long ago we first stated with only one log channel: the LLDB log channel. Then we slowly added more log channels and all those new log channels used the lldb_private::Log class and all call sites would access methods in each log class except the LLDB log channel.

After this change:

  • Log.h contains the Log class definition and no longer inlcudes the old "Logging.h" file which contained the LLDB log channel bits and functions
  • the log mask is now expanded to 64 bits which allows for more categories within a log channel (we were out of available category bits in the LLDB log channel)
  • Logging.h has been renamed to LLDBLog.h
  • Logging.cpp has been renamed to LLDBLog.cpp
  • all call sites that were using the old top level functions GetLogIfAllCategoriesSet and GetLogIfAnyCategoriesSet now call LLDBLog::GetLogIfAllCategoriesSet and LLDBLog::GetLogIfAnyCategoryIsSetrespectively.

This cleans up our logging implementation and caught a few cases where we were getting the wrong log from the wrong channel (ThreadGDBRemote for instance) and makes it harder to use the wrong log. This also allows us to add more log channels to LLDB's main logging categories now.

Diff Detail

Event Timeline

clayborg created this revision.Jan 14 2022, 6:58 PM
clayborg requested review of this revision.Jan 14 2022, 6:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 6:58 PM

Nice improvement!

lldb/include/lldb/Utility/LLDBLog.h
1

LLDBLog.h

58

Even if it's a NOOP, should we add a Terminate for consistency?

lldb/source/Core/ValueObject.cpp
45–46

Dupe

It is definitely an improvement, but I can't help but wonder, since we're going to be touching every GetLog line with this, if we couldn't go even further.

The various LLDB_LOG have helped a lot, but one of the things that still bothers me is the verbosity of the GetLog statements. GetLogIfAllCategoriesSet(LIBLLDB_LOG_WHATEVER) is long enough as it is, and this prepends LLDBLog:: on top of that.

Here's a straw-man proposal to make that shorter (and safer). Instead of macros, we use (scoped) enums for denoting log categories. enums are not a preprocessor construct, so we can keep the identifiers relatively short. E.g. LLDBLog::API instead of LLDB_LOG_API. They also have types, which means we can use the compiler to check that the values are used appropriately. And thanks to that, we also don't need to have a long name for the log getter function. The logging could be as simple as

Log *log = LogIfAny(LLDBLog::API); // Uses template magic to find the appropriate log class based on the type

If the name sounds too cryptic, we can also choose something longer like GetLogIfAnySet or variation thereof.

If you (all) think something like that would be useful, but don't have the time to do that, I can try to whip something up.

D117490 is pretty much what I had in mind. It needs a bit of a cleanup, and doesn't actually change any interface (that'd be better done as a separate patch anyway), but all the major functionality is already there.

clayborg abandoned this revision.Jan 24 2022, 4:20 PM

Abandoning after discussing on the mailing list in lieu of Pavel's patch: https://reviews.llvm.org/D117490