[lldb-dev] logging in lldb

[Jason Molenda via lldb-dev]

Hi Pavel, sorry for not keeping up with the thread, I've been super busy all this week.  I'm not going to object to where this proposal has ended up.  I personally have a preference for the old system but not based on any justifiable reasons.


> On Dec 15, 2016, at 7:13 AM, Pavel Labath <labath at google.com> wrote:
> 
> Just to let you know, I will be on vacation until the end of the year,
> so probably will not respond to any comments until January. If you
> have any concerns, do let me know, as I'd like to get started when I
> get back.
> 
> pl
> 
> On 13 December 2016 at 16:32, Pavel Labath <labath at google.com> wrote:
>> 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