This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Remove a couple of log statements
ClosedPublic

Authored by zturner on Mar 18 2019, 10:04 AM.

Details

Summary

I don't find these specific log statements particularly high value, as they appear more just like simple function tracing calls (i.e. they log low level implementation details, not high level program flow) so I'm removing them from the low level DWARF parsing libraries.

I'm prepared to be convinced that we actually should do something other than outright delete them, but I did look at each one specifically as well as the surrounding context before deciding that they were probably ok to delete.

My reasoning for deleting these is that they are all on the success codepath and have a very limited number of call-sites (in most cases actually just 1), so in most cases a previous log statement would necessarily be followed by the deleted log statement, so therefore it doesn't actually add much (if any) diagnostic value.

Diff Detail

Event Timeline

zturner created this revision.Mar 18 2019, 10:04 AM

Most of these logs seem useful. See inline comments.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp
121–144

These two seem useful. They are only when logging .debug_aranges with DWARF_LOG_DEBUG_ARANGES. What is left if we remove this?

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
93

All above logs seem useful. Sometimes we don't get a .debug_aranges section which means we must parse the address ranges manually. When we are logging .debug_aranges, it is handy to know where the info came from (.debug_aranges, or manual index).

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
168–176

It is nice to know when we parse all DIEs for a compile unit. This helps us to verify that our lazy DWARF parsing is working. When we have good accelerator tables, like the Apple ones or DWARF 5 ones, then we will only parse the DIEs in a compile unit when we need to. Seems like we need one when it starts and one when it ends so we could see timestamp times...

837–844

This seems useful as it tells us when we manually index a compile unit for all function address ranges which can be expensive.

zturner marked 4 inline comments as done.Mar 18 2019, 10:58 AM

In all cases, I think the question worth asking is not "could it be used for X", but rather "how often is it actually used for X". Otherwise, it's just technical debt IMO. There's a lot of things that are possible in theory, but unless it exhibits value in practice, YAGNI.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp
121–144

What useful information does it tell you though? That the function was called? That's the part that doesn't seem useful. It gives you some information about bytes saved and entries combined, which could arguably be useful, but are you aware of any situations where anyone actually looked at this log statement? One option is moving it up to a higher level if so, but I think it's worth seriously asking why you would want to know this, and if you did want to know it, why running LLDB in a debugger and/or adding some temporary logging to diagnose a specific problem wouldn't be sufficient.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
93

In that case, we can move the log up to a much higher level, and print one log all the way up in SymbolFileDWARF that says "aranges not present, using range info from manual index". It doesn't need to be in the low level parsing code

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
168–176

This function already has this at the beginning:

static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
  static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
  Timer scoped_timer(func_cat, "%8.8x: DWARFUnit::ExtractDIEsIfNeeded()", m_offset);

if timing information is needed. There are also other options, like running a profiler or function tracer. For verifying that the lazy parsing is working, we should just have tests, not log statements that require someone to manually inspect.

837–844

This information could be stored at a higher level, it doesn't have to be here.

JDevlieghere accepted this revision.Mar 18 2019, 2:19 PM
JDevlieghere added a subscriber: davide.

In all cases, I think the question worth asking is not "could it be used for X", but rather "how often is it actually used for X". Otherwise, it's just technical debt IMO. There's a lot of things that are possible in theory, but unless it exhibits value in practice, YAGNI.

I strongly agree with this. Unless this is something we want to know from a user, we can probably get this information by setting a breakpoint.

This is bad advertising. It's like a butcher that sells vegan meat. (Quote @davide)

This revision is now accepted and ready to land.Mar 18 2019, 2:19 PM
clayborg accepted this revision.Mar 18 2019, 2:27 PM

I haven't used it in a long time. I can add it back temporarily if I ever need to.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 9:25 AM
Herald added a subscriber: abidh. · View Herald Transcript