[Lldb-commits] [PATCH] D135621: [lldb] Add an always-on log channel

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D135621#3875218 <https://reviews.llvm.org/D135621#3875218>, @JDevlieghere wrote:

> In D135621#3850248 <https://reviews.llvm.org/D135621#3850248>, @clayborg wrote:
>
>> I am also questioning if the these even belong in the LLDB logging stuff? Seems like it would be just as easy to create a diagnostic message by calling Diagnostics::Report(...). Do we really want to modify the log channels here? Seems like always on diagnostics should just a dedicated API.
>
> I expect the majority of places where we want to log to the diagnostic log channel to be places where we already log today.

Yeah, but that could be achieved by having the "dedicated diagnostic API" forward the message to a regular log when it deems it necessary (or always).

Speaking of dedicated APIs, we already have the Debugger::ReportWarning/Error functions. I would expect we want those to show up in the diagnostic log, as these are things that we've already deemed important enough to print to the user. Wouldn't it be better to use that as a starting point? Possibly extend it by adding some sort of an "info" priority, which does not get printed to the user (but makes its way to the log)?

> Similarly, I also plan to change `LLDB_LOG_ERROR` to always log to the diagnostic channel. I didn't include it in this patch as it requires moving the macro in the diagnostics header and generates a lot of churn.

I'm not sure that's a good default. Some of those messages could be fairly innocuous, and the ratio of the innocuous ones will probably increase as we use llvm::Error more.



================
Comment at: lldb/source/Utility/Diagnostics.cpp:23
+static constexpr Log::Category g_categories[] = {
+    {{"lldb"}, {"diagnostics log for lldb"}, DiagnosticsLog::LLDB},
+};
----------------
JDevlieghere wrote:
> kastiglione wrote:
> > To me, it's not ideal that there's an "lldb" channel, and this channel has an "lldb" category.
> > 
> > Do you have other categories in mind for later updates, that should be moved into this patch? For example, will there eventually be "warning" and "error" categories? If so, maybe start with those instead of "lldb".
> Yes, for the errors I was going to add an error category. Maybe we can call this one general or something?
I'm not entirely sure of what's the purpose of having multiple categories, if they're all supposed to be on all the time (the category name is not recorded anywhere, so the only thing they do is enable one to selectively enable/disable some log messages).


================
Comment at: lldb/test/Shell/Diagnostics/TestLogChannel.test:2
+# RUN: %lldb -o 'log list' 2>&1 | FileCheck %s
+# CHECK-NOT: Logging categories for 'diagnostics'
----------------
Judging by the patch, I think the user will still be able to disable diagnostics if he is able to "guess" that there is a log channel called that way (`log disable diagnostics`). Is that intentional?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135621/new/

https://reviews.llvm.org/D135621