Is GetLogIf**All**CategoriesSet useful?

Hi all,

In case you haven't noticed, I'd like to draw your attention to the in-flight patches (https://reviews.llvm.org/D117382, https://reviews.llvm.org/D117490) whose goal clean up/improve/streamline the logging infrastructure.

I'm don't want go into technical details here (they're on the patch), but the general idea is to replace statements like GetLogIf(Any/All)CategoriesSet(LIBLLDB_LOG_CAT1 | LIBLLDB_LOG_CAT2)
with
GetLogIf(Any/All)(LLDBLog::Cat1 | LLDBLog::Cat2)
i.e., drop macros and make use of templates to make the function calls shorter and safer.

The reason I'm writing this email is to ask about the "All" versions of these logging functions. Do you find them useful in practice?

I'm asking that because I've never used this functionality. While I can't find anything wrong with the concept in theory, practically I think it's just confusing to have some log message appear only for some combination of enabled channels. It might have made some sense when we had a "verbose" logging channel, but that one is long gone (we still have a verbose logging *flag*).

In fact, out of all our GetLogIf calls (1203), less than 1% (11*) uses the GetLogIfAll form with more than one category. Of those, three are in tests, one is definitely a bug (it combines the category with LLDB_LOG_OPTION_VERBOSE), and the others (7) are of questionable usefulness (to me anyway).

If we got rid of this, we could simplify the logging calls even further and have something like:
Log *log = GetLog(LLDBLog::Process);
everywhere.

cheers,
pl

(*) I used this command to count:
$ git grep -e LogIfAll -A 1 | fgrep -e '|' | wc -l

Hi all,

In case you haven't noticed, I'd like to draw your attention to the in-flight patches (https://reviews.llvm.org/D117382, https://reviews.llvm.org/D117490) whose goal clean up/improve/streamline the logging infrastructure.

I'm don't want go into technical details here (they're on the patch), but the general idea is to replace statements like GetLogIf(Any/All)CategoriesSet(LIBLLDB_LOG_CAT1 | LIBLLDB_LOG_CAT2)
with
GetLogIf(Any/All)(LLDBLog::Cat1 | LLDBLog::Cat2)
i.e., drop macros and make use of templates to make the function calls shorter and safer.

The reason I'm writing this email is to ask about the "All" versions of these logging functions. Do you find them useful in practice?

I'm asking that because I've never used this functionality. While I can't find anything wrong with the concept in theory, practically I think it's just confusing to have some log message appear only for some combination of enabled channels. It might have made some sense when we had a "verbose" logging channel, but that one is long gone (we still have a verbose logging *flag*).

In fact, out of all our GetLogIf calls (1203), less than 1% (11*) uses the GetLogIfAll form with more than one category. Of those, three are in tests, one is definitely a bug (it combines the category with LLDB_LOG_OPTION_VERBOSE), and the others (7) are of questionable usefulness (to me anyway).

If we got rid of this, we could simplify the logging calls even further and have something like:
Log *log = GetLog(LLDBLog::Process);
everywhere.

The only time I’ve ever “used” GetLogIfAll was when I added another LOG option to a log call, not noticing it was “All”, finding the new log didn’t work, and going back to switch “All” to “Any”.

I vote for removing it.

Jim

Hi all,

In case you haven’t noticed, I’d like to draw your attention to the in-flight patches (https://reviews.llvm.org/D117382, https://reviews.llvm.org/D117490) whose goal clean up/improve/streamline the logging infrastructure.

I’m don’t want go into technical details here (they’re on the patch), but the general idea is to replace statements like GetLogIf(Any/All)CategoriesSet(LIBLLDB_LOG_CAT1 | LIBLLDB_LOG_CAT2)
with
GetLogIf(Any/All)(LLDBLog::Cat1 | LLDBLog::Cat2)
i.e., drop macros and make use of templates to make the function calls shorter and safer.

The reason I’m writing this email is to ask about the “All” versions of these logging functions. Do you find them useful in practice?

I’m asking that because I’ve never used this functionality. While I can’t find anything wrong with the concept in theory, practically I think it’s just confusing to have some log message appear only for some combination of enabled channels. It might have made some sense when we had a “verbose” logging channel, but that one is long gone (we still have a verbose logging flag).

In fact, out of all our GetLogIf calls (1203), less than 1% (11*) uses the GetLogIfAll form with more than one category. Of those, three are in tests, one is definitely a bug (it combines the category with LLDB_LOG_OPTION_VERBOSE), and the others (7) are of questionable usefulness (to me anyway).

If we got rid of this, we could simplify the logging calls even further and have something like:
Log *log = GetLog(LLDBLog::Process);
everywhere.

The only time I’ve ever “used” GetLogIfAll was when I added another LOG option to a log call, not noticing it was “All”, finding the new log didn’t work, and going back to switch “All” to “Any”.

I vote for removing it.

+1

I also vote to remove and simplify.

Can a template function deduce the log type from an argument? Wouldn't this have to be:

Log *log = GetLog<LLDBLog>(LLDBLog::Process);

That is why I was hinting if we want to just use the enum class itself:

Log *log = LLDBLog::GetLog(LLDBLog::Process);

The template class in your second patch seems cool, but I don't understand how it worked without going and reading up on templates in C++ and spending 20 minutes trying to wrap my brain around it.

Or do we just switch to a dedicated log class with unique methods:

class LLDBLog: public Log {
  Log *Process() { return GetLog(1u << 0); }
  Log *Thread() { return GetLog(1u << 1); }
};

and avoid all the enums? Then we can't ever feed a bad enum or #define into the wrong log class.

I also vote to remove and simplify.

Sounds like it's settled then. I'll fire up my sed scripts.

If we got rid of this, we could simplify the logging calls even further and have something like:>> Log *log =
GetLog(LLDBLog::Process);

Can a template function deduce the log type from an argument? Wouldn't this have to be:

Log *log = GetLog<LLDBLog>(LLDBLog::Process);

That is why I was hinting if we want to just use the enum class itself:

Log *log = LLDBLog::GetLog(LLDBLog::Process);

The template class in your second patch seems cool, but I don't understand how it worked without going and reading up on templates
in C++ and spending 20 minutes trying to wrap my brain around it.

Template functions have always been able to deduce template arguments.
Pretty much the entire c++ standard library is made of template
functions, but you don't see <> spelled out everywhere. Class templates
have not been able to auto-deduce template arguments until c++17, and I
am still not really clear on how that works.

The way that patch works is that you have one template function
`LogChannelFor<T>`, which ties the enum to a specific channel class, and
then another one (GetLogIfAny), which returns the actual log object (and
uses the first one to obtain the channel).

But none of this is fundamentally tied to templates. One could achieve
the same thing by overloading the GetLogIfAny function (one overload for
each type). The template just saves a bit of repetition. This way, the
only thing you need to do when defining a new log channel, is to provide
the LogChannelFor function.

Or do we just switch to a dedicated log class with unique methods:

class LLDBLog: public Log { Log *Process() { return GetLog(1u << 0);
} Log *Thread() { return GetLog(1u << 1); } };

and avoid all the enums? Then we can't ever feed a bad enum or #define into the wrong log class.

That could work too, and would definitely have some advantages -- for
instance we could prefix each message with the log channel it was going
to. The downside is that we would lose the ability to send one message to multiple log channels at once, and I believe that some (Jim?) value that functionality.

pl

I also vote to remove and simplify.

Sounds like it's settled then. I'll fire up my sed scripts.

If we got rid of this, we could simplify the logging calls even further and have something like:>> Log *log =
GetLog(LLDBLog::Process);

Can a template function deduce the log type from an argument? Wouldn't this have to be:
Log *log = GetLog<LLDBLog>(LLDBLog::Process);
That is why I was hinting if we want to just use the enum class itself:
Log *log = LLDBLog::GetLog(LLDBLog::Process);
The template class in your second patch seems cool, but I don't understand how it worked without going and reading up on templates
in C++ and spending 20 minutes trying to wrap my brain around it.

Template functions have always been able to deduce template arguments.
Pretty much the entire c++ standard library is made of template
functions, but you don't see <> spelled out everywhere. Class templates
have not been able to auto-deduce template arguments until c++17, and I
am still not really clear on how that works.

The way that patch works is that you have one template function
`LogChannelFor<T>`, which ties the enum to a specific channel class, and
then another one (GetLogIfAny), which returns the actual log object (and
uses the first one to obtain the channel).

But none of this is fundamentally tied to templates. One could achieve
the same thing by overloading the GetLogIfAny function (one overload for
each type). The template just saves a bit of repetition. This way, the
only thing you need to do when defining a new log channel, is to provide
the LogChannelFor function.

Or do we just switch to a dedicated log class with unique methods:
class LLDBLog: public Log { Log *Process() { return GetLog(1u << 0);
} Log *Thread() { return GetLog(1u << 1); } };
and avoid all the enums? Then we can't ever feed a bad enum or #define into the wrong log class.

That could work too, and would definitely have some advantages -- for
instance we could prefix each message with the log channel it was going
to. The downside is that we would lose the ability to send one message to multiple log channels at once, and I believe that some (Jim?) value that functionality.

I think I’m just quibbling about terminology, I don’t think it’s possible for one site to send its log message to two channels in a single go. That would be like “lldb types” and “dwarf info” for a single log statement.
Anyway, that’s not something I see as particularly useful.

What is useful is to say “this message goes out on the lldb channel if any of these categories (“step” and “expr” for instance) is set.” I don’t really think of that as sending the message to multiple channels, since it’s only going to go out once, but the test is broader.

But, IIUC, Greg’s proposal would also make that impossible as well, so I’m still against it…

Jim

Understood, we need to be able to log if “any” bits are set.