[Lldb-commits] [PATCH] D59498: [DWARF] Remove a couple of log statements

[Zachary Turner via Phabricator via lldb-commits]

zturner marked 4 inline comments as done.
zturner added a comment.

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.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp:121-144
-  Log *log(LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_ARANGES));
-  size_t orig_arange_size = 0;
-  if (log) {
-    orig_arange_size = m_aranges.GetSize();
-    log->Printf("DWARFDebugAranges::Sort(minimize = %u) with %" PRIu64
-                " entries",
-                minimize, (uint64_t)orig_arange_size);
----------------
clayborg wrote:
> These two seem useful. They are only when logging .debug_aranges with DWARF_LOG_DEBUG_ARANGES. What is left if we remove this?
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.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:93
-    }
   }
 
----------------
clayborg wrote:
> 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).
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


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:168-176
-  Log *log(
-      LogChannelDWARF::GetLogIfAny(DWARF_LOG_DEBUG_INFO | DWARF_LOG_LOOKUPS));
-  if (log) {
-    m_dwarf->GetObjectFile()->GetModule()->LogMessageVerboseBacktrace(
-        log,
-        "DWARFUnit::ExtractDIEsIfNeeded () for compile unit at "
-        ".debug_info[0x%8.8x]",
----------------
clayborg wrote:
> 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...
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.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:837-844
-    Log *log(LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_ARANGES));
-
-    if (log) {
-      m_dwarf->GetObjectFile()->GetModule()->LogMessage(
-          log,
-          "DWARFUnit::GetFunctionAranges() for compile unit at "
-          ".debug_info[0x%8.8x]",
----------------
clayborg wrote:
> This seems useful as it tells us when we manually index a compile unit for all function address ranges which can be expensive.
This information could be stored at a higher level, it doesn't have to be here.


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

https://reviews.llvm.org/D59498