labath marked an inline comment as done.
labath added a comment.
================
Comment at: source/Core/Log.cpp:72
+ if (llvm::StringRef("all").equals_lower(categories[i])) {
+ flags |= ~0;
+ continue;
----------------
zturner wrote:
> This is a little clearer if you just say `flags = 0xFFFFFFFF` IMO.
UINT32_MAX (?)
================
Comment at: source/Core/Log.cpp:266
const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
- uint32_t log_options, const char *channel, const char **categories,
+ uint32_t log_options, llvm::StringRef channel, const char **categories,
Stream &error_stream) {
----------------
zturner wrote:
> How hard would it be to change `categories` to `ArrayRef<StringRef>`, or
> alternatively `ArrayRef<const char*>`?
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.
================
Comment at: source/Core/Log.cpp:284-285
+ if (ch->second.log.m_mask_bits.Get()) {
+ ch->second.log.SetStream(log_stream_sp);
+ ch->second.log_ptr.store(&ch->second.log, std::memory_order_release);
+ }
----------------
zturner wrote:
> 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.
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.
================
Comment at: source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp:32
void LogChannelDWARF::Initialize() {
- PluginManager::RegisterPlugin(GetPluginNameStatic(),
- GetPluginDescriptionStatic(),
- LogChannelDWARF::CreateInstance);
-}
-
-void LogChannelDWARF::Terminate() {
- PluginManager::UnregisterPlugin(LogChannelDWARF::CreateInstance);
-}
-
-LogChannel *LogChannelDWARF::CreateInstance() { return new LogChannelDWARF(); }
-
-lldb_private::ConstString LogChannelDWARF::GetPluginNameStatic() {
- return SymbolFileDWARF::GetPluginNameStatic();
+ Log::Register("dwarf", g_categories, DWARF_LOG_DEFAULT, g_log_channel);
}
----------------
zturner wrote:
> Where is `g_log_channel_` allocated? AFAICT it's always `nullptr`.
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.
https://reviews.llvm.org/D29895
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
