Page MenuHomePhabricator

Refactor log channel registration mechanism
ClosedPublic

Authored by labath on Feb 13 2017, 9:09 AM.

Details

Summary

We currently have two log channel registration mechanisms. One uses a
set of function pointers and the other one is based on the
PluginManager.

The PluginManager dependency is unfortunate, as logging
is also used in lldb-server, and the PluginManager pulls in a lot of
classes which are not used in lldb-server.

Both approach have the problem that they leave too much to do for the
user, and so the individual log channels end up reimplementing command
line argument parsing, category listing, etc.

Here, I replace the PluginManager-based approach with a one. The new API
is more declarative, so the user only needs to specify the list of list
of channels, their descriptions, etc., and all the common tasks like
enabling/disabling categories are hadled by common code. I migrate the
LogChannelDWARF (only user of the PluginManager method) to the new API.

In the follow-up commits I'll replace the other channels with something
similar.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Feb 13 2017, 9:09 AM

I actually did add tests for that. :)

I didn't try to be super-exhaustive, but I think they provide reasonable coverage. I'd be happy to add more, if you notice parts that are missing it.

clayborg accepted this revision.Feb 13 2017, 10:07 AM

Looks good!

This revision is now accepted and ready to land.Feb 13 2017, 10:07 AM
zturner added inline comments.Feb 13 2017, 10:35 AM
include/lldb/Core/Log.h
74 ↗(On Diff #88210)

Not sure I like the idea of this being a std::atomic. Why is this necessary?

source/Commands/CommandObjectLog.cpp
278 ↗(On Diff #88210)

const auto&?

source/Core/Log.cpp
57 ↗(On Diff #88210)

LLVM guidelines say only classes, structures, and types can be in an anonymous namespace, but not functions. So this should go out of the anonymous namespace, but be declared as global statics.

Same thing withsubsequent functions.

72 ↗(On Diff #88210)

This is a little clearer if you just say flags = 0xFFFFFFFF IMO.

266 ↗(On Diff #88210)

How hard would it be to change categories to ArrayRef<StringRef>, or alternatively ArrayRef<const char*>?

284–285 ↗(On Diff #88210)

Here's an example of why I think we need a mutex instead of std::atomic. There seems to be a race condition here if two threads call EnableLogChannel and DisableLogChannel at the same time. You can get into a situation where the stream is null but the log is enabled.

source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp
32 ↗(On Diff #88210)

Where is g_log_channel_ allocated? AFAICT it's always nullptr.

labath marked an inline comment as done.Feb 14 2017, 8:27 AM
source/Core/Log.cpp
72 ↗(On Diff #88210)

UINT32_MAX (?)

266 ↗(On Diff #88210)

The second one is moderately easy, if we add the appropriate accessor to the Args class. However, I'd leave that for later. If I do that now, I'd have to update at least a dozen functions. After the refactor it should be just two.

284–285 ↗(On Diff #88210)

I was not tried to address these kinds of races here. I am fine with assuming that the calls enabling/disabling log channels are externally serialized (same as the previous implementation).

What I am tried to address is the race between someone enabling a log and someone else attempting to write to the log (e.g. GetLogIfAllCategories set). This is something that should be done with as low overhead as possible (i.e., no mutex), as that code will be running even when logging is disabled. This is why I am using an atomic variable. The original code was using a simple pointer and hoping that the update will be atomic, which is not correct, so I am trying to improve that here.

Note that this is still not completely data-race free as the flags update is not done atomically, but that is also something that I am not trying to solve here. I think I'll try to make another patch later which just deals with the thread-safety, this is just something that I wanted to sneak in as I was already modifying this part.

source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp
32 ↗(On Diff #88210)

It is set in the Log.cpp file, when we enable the logging. This was not a very good design, as I have smeared the atomic accesses over two files. The next one version of this patch should compartmentalize it better.

labath updated this revision to Diff 88381.Feb 14 2017, 8:30 AM

New version of the patch.

I can still see a bit of room for improvement, but I am not sure if I'll manage
to do it today, so this should at least enable us to do another round-trip.

labath updated this revision to Diff 88395.Feb 14 2017, 10:09 AM
labath marked an inline comment as done.

Encapsulate the atomic stuff into the Log::Channel class. Add a couple of comments.

Let me know what you think.

zturner accepted this revision.Feb 14 2017, 10:14 AM
This revision was automatically updated to reflect the committed changes.