[Lldb-commits] [PATCH] D56737: [SymbolFile] Split ParseVariablesForContext into two functions.

[Zachary Turner via Phabricator via lldb-commits]

zturner created this revision.
zturner added reviewers: clayborg, labath, jingham.

This function was being used in 2 separate use cases.  The first is parsing variables in a function, and the second is parsing global variables.  To clarify the intent, I've split this into two functions, ParseLocalVariablesRecursive and ParseGlobalVariables.

In doing so, I found what may actually be a bug.   This bug was very subtle in the old implementation, and an example of exactly why I think it's important to clean up these interfaces.   There was a disconnect in what the caller expected to happen and what the implementation actually did.

Specifically, before when calling `Block::GetBlockVariableList` it was implemented this way:

  SymbolContext sc;
  CalculateSymbolContext(&sc);
  assert(sc.module_sp);
  sc.module_sp->GetSymbolVendor()->ParseVariablesForContext(sc);

This is logical, because it expects that `ParseVariablesForContext` will see that the block is set, then only parse variables for the given block.  However, the implementation of this function did not actually handle the case where `sc.m_block` was set.  Instead, this would fall to the `sc.m_function` code path, and parse all variables for the entire function, recursively, even though only 1 block was requested.

I expect this will be a performance problem if you have large function with many blocks and you put a breakpoint in one of the leaf-blocks and try to print a variable.  It would have to parse potentially thousands of other blocks' variables even though only 1 is needed.

After this patch, there is no functional change, but the bug is more obvious, because `Block::GetVariableList` is implemented like this:

  VariableListSP Block::GetBlockVariableList(bool can_create) {
    if (!m_parsed_block_variables) {
      if (m_variable_list_sp.get() == nullptr && can_create) {
        m_parsed_block_variables = true;
        Function *func = CalculateSymbolContextFunction();
        ModuleSP module = CalculateSymbolContextModule();
        module->GetSymbolVendor()->ParseLocalVariablesRecursive(*func);
      }
    }
    return m_variable_list_sp;
  }

This way, a person so motivated can go through and add another overload such as `ParseVariablesForBlock` (or change `ParseVariablesForFunction` to `ParseVariablesForBlock(Block& block, bool recursive)` so that we can pass false there.`


https://reviews.llvm.org/D56737

Files:
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/source/Symbol/Block.cpp
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/SymbolVendor.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56737.181840.patch
Type: text/x-patch
Size: 21475 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190115/0ffeed39/attachment-0001.bin>