[lldb-dev] logging in lldb

[Pavel Labath via lldb-dev]

Hello again,

I'd to get back to the question of unifying llvm's and lldb's logging
mechanisms that Chris asked about. In the way these two are
implemented now, they have a number of similarities, but also a number
of differences. Among the differences, there is one that I think will
be most painful to resolve, so I'll start with that one:

I am talking about how to disable logging at compile-time. Currently,
llvm's logging mechanism can be controlled both at runtime and
compile-time. lldb's can be only controlled at runtime. While we may
not want/need the compile-time knob, it is a very hard requirement for
llvm, which tries to squeeze every ounce of performance from the
hardware. So, if we are going to have a common logging API, we will
need to support being compiled without it.

This has impact on the kind of syntax we are able to use. I see two
problems here.

1. The first one is that our log patterns are split into independent
parts. Currently the pattern is:
Log *log = GetLogIf(Flag);
...
if (log) log->Printf(...);

The API we (mostly?) converged to above is:
Log *log = GetLogIf(Flag);
...
LLDB_LOG(log, ...);

If we want to compile the logging away, getting rid of the second part
is easy, as it is already a macro. However, for a completely clean
compile, we would need to remove the first part as well. Since
wrapping it in #ifdef would be too ugly, I think the easiest solution
would be to just make it go away completely.

The way I understand it, the reason we do it in two steps now is to
make the code fast if logging is off. My proposal here would be to
make the code very fast even without the additional local variable. If
we could use the macro like:
LLDB_LOG(Flag, ...)
where the macro would expand to something like:
if (LLVM_UNLIKELY(Flag & lldb_private::enabled_channels)) log_stuff(...)
where `enabled_channels` is just a global integral variable then the
overhead of a disabled log statement would be three instructions
(load, and, branch), some of which could be reused if we had more
logging statements in a function. Plus the macro could hint the cpu
and compiler to optimize for the "false" case. This is still an
increase over the status quo, where the overhead of a log statement is
one or two instructions, but I am not sure if this is significant.

2. The second, and probably bigger, issue is the one mentioned by
Jason earlier in this thread -- the ability to insert random code into
if(log) blocks. Right writing the following is easy:
if (log) {
  do_random_stuff();
  log->Printf(...);
}

In the first version of the macro, this is still easy to write, as we
don't have to worry about compile-time. But if we need this to go
away, we will need to resort to the same macro approach as llvm:
LLDB_DEBUG( { do_random_stuff(); LLDB_LOG(...); });
Which has all the disadvantages Jason mentioned. Although, I don't
think this has to be that bad, as we probably will not be doing this
very often, and the issues can be mitigated by putting the actual code
in a function, and only putting the function calls inside the macro.



So, my question to you is: Do you think these two drawbacks are worth
sacrificing for the sake of having a unified llvm-wide logging
infrastructure? I am willing to drive this, and implement the llvm
side of things, but I don't want to force this onto everyone, if it is
not welcome. If you do not think this is a worthwhile investment then
I'd rather proceed with the previous lldb-only solution we discussed
above, as that is something I am more passionate above, will already
be a big improvement, and a good stepping stone towards implementing
an llvm-wide solution in the future.

Of course, if you have alternative proposals on how to implement
llvm-wide logging, I'd love to hear about it.

Let me know what you think,
pavel