Revert "[lldb] Use the function block as a source for function ranges (#117996)"

This reverts commit ba14dac. I guess
"has no conflicts" doesn't mean "it will build".

Author: labath

lldb/include/lldb/Symbol/Function.h

@@ -653,6 +653,9 @@ class Function : public UserID, public SymbolContextScope {
   /// All lexical blocks contained in this function.
   Block m_block;
 
+  /// List of address ranges belonging to the function.
+  AddressRanges m_ranges;
+
   /// The function address range that covers the widest range needed to contain
   /// all blocks. DEPRECATED: do not use this field in new code as the range may
   /// include addresses belonging to other functions.

lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

@@ -299,7 +299,9 @@ size_t SymbolFileBreakpad::ParseBlocksRecursive(Function &func) {
   // "INLINE 0 ...", the current level is 0 and its parent block is the
   // function block at index 0.
   std::vector<Block *> blocks;
-  blocks.push_back(&func.GetBlock(false));
+  Block &block = func.GetBlock(false);
+  block.AddRange(Block::Range(0, func.GetAddressRange().GetByteSize()));
+  blocks.push_back(&block);
 
   size_t blocks_added = 0;
   addr_t func_base = func.GetAddressRange().GetBaseAddress().GetOffset();

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

@@ -1305,76 +1305,121 @@ bool SymbolFileDWARF::ParseDebugMacros(CompileUnit &comp_unit) {
   return true;
 }
 
-size_t SymbolFileDWARF::ParseBlocksRecursive(CompileUnit &comp_unit,
-                                             Block *parent_block, DWARFDIE die,
-                                             addr_t subprogram_low_pc) {
+size_t SymbolFileDWARF::ParseBlocksRecursive(
+    lldb_private::CompileUnit &comp_unit, Block *parent_block,
+    const DWARFDIE &orig_die, addr_t subprogram_low_pc, uint32_t depth) {
   size_t blocks_added = 0;
-  for (; die; die = die.GetSibling()) {
+  DWARFDIE die = orig_die;
+  while (die) {
     dw_tag_t tag = die.Tag();
 
-    if (tag != DW_TAG_inlined_subroutine && tag != DW_TAG_lexical_block)
-      continue;
+    switch (tag) {
+    case DW_TAG_inlined_subroutine:
+    case DW_TAG_subprogram:
+    case DW_TAG_lexical_block: {
+      Block *block = nullptr;
+      if (tag == DW_TAG_subprogram) {
+        // Skip any DW_TAG_subprogram DIEs that are inside of a normal or
+        // inlined functions. These will be parsed on their own as separate
+        // entities.
 
-    Block *block = parent_block->CreateChild(die.GetID()).get();
-    DWARFRangeList ranges;
-    const char *name = nullptr;
-    const char *mangled_name = nullptr;
-
-    std::optional<int> decl_file;
-    std::optional<int> decl_line;
-    std::optional<int> decl_column;
-    std::optional<int> call_file;
-    std::optional<int> call_line;
-    std::optional<int> call_column;
-    if (die.GetDIENamesAndRanges(name, mangled_name, ranges, decl_file,
-                                 decl_line, decl_column, call_file, call_line,
-                                 call_column, nullptr)) {
-      const size_t num_ranges = ranges.GetSize();
-      for (size_t i = 0; i < num_ranges; ++i) {
-        const DWARFRangeList::Entry &range = ranges.GetEntryRef(i);
-        const addr_t range_base = range.GetRangeBase();
-        if (range_base >= subprogram_low_pc)
-          block->AddRange(Block::Range(range_base - subprogram_low_pc,
-                                       range.GetByteSize()));
-        else {
-          GetObjectFile()->GetModule()->ReportError(
-              "{0:x8}: adding range [{1:x16}-{2:x16}) which has a base "
-              "that is less than the function's low PC {3:x16}. Please file "
-              "a bug and attach the file at the "
-              "start of this error message",
-              block->GetID(), range_base, range.GetRangeEnd(),
-              subprogram_low_pc);
-        }
-      }
-      block->FinalizeRanges();
-
-      if (tag != DW_TAG_subprogram &&
-          (name != nullptr || mangled_name != nullptr)) {
-        std::unique_ptr<Declaration> decl_up;
-        if (decl_file || decl_line || decl_column)
-          decl_up = std::make_unique<Declaration>(
-              comp_unit.GetSupportFiles().GetFileSpecAtIndex(
-                  decl_file ? *decl_file : 0),
-              decl_line ? *decl_line : 0, decl_column ? *decl_column : 0);
-
-        std::unique_ptr<Declaration> call_up;
-        if (call_file || call_line || call_column)
-          call_up = std::make_unique<Declaration>(
-              comp_unit.GetSupportFiles().GetFileSpecAtIndex(
-                  call_file ? *call_file : 0),
-              call_line ? *call_line : 0, call_column ? *call_column : 0);
-
-        block->SetInlinedFunctionInfo(name, mangled_name, decl_up.get(),
-                                      call_up.get());
+        if (depth > 0)
+          break;
+
+        block = parent_block;
+      } else {
+        block = parent_block->CreateChild(die.GetID()).get();
       }
+      DWARFRangeList ranges;
+      const char *name = nullptr;
+      const char *mangled_name = nullptr;
+
+      std::optional<int> decl_file;
+      std::optional<int> decl_line;
+      std::optional<int> decl_column;
+      std::optional<int> call_file;
+      std::optional<int> call_line;
+      std::optional<int> call_column;
+      if (die.GetDIENamesAndRanges(name, mangled_name, ranges, decl_file,
+                                   decl_line, decl_column, call_file, call_line,
+                                   call_column, nullptr)) {
+        if (tag == DW_TAG_subprogram) {
+          assert(subprogram_low_pc == LLDB_INVALID_ADDRESS);
+          subprogram_low_pc = ranges.GetMinRangeBase(0);
+        } else if (tag == DW_TAG_inlined_subroutine) {
+          // We get called here for inlined subroutines in two ways. The first
+          // time is when we are making the Function object for this inlined
+          // concrete instance.  Since we're creating a top level block at
+          // here, the subprogram_low_pc will be LLDB_INVALID_ADDRESS.  So we
+          // need to adjust the containing address. The second time is when we
+          // are parsing the blocks inside the function that contains the
+          // inlined concrete instance.  Since these will be blocks inside the
+          // containing "real" function the offset will be for that function.
+          if (subprogram_low_pc == LLDB_INVALID_ADDRESS) {
+            subprogram_low_pc = ranges.GetMinRangeBase(0);
+          }
+        }
 
-      ++blocks_added;
+        const size_t num_ranges = ranges.GetSize();
+        for (size_t i = 0; i < num_ranges; ++i) {
+          const DWARFRangeList::Entry &range = ranges.GetEntryRef(i);
+          const addr_t range_base = range.GetRangeBase();
+          if (range_base >= subprogram_low_pc)
+            block->AddRange(Block::Range(range_base - subprogram_low_pc,
+                                         range.GetByteSize()));
+          else {
+            GetObjectFile()->GetModule()->ReportError(
+                "{0:x8}: adding range [{1:x16}-{2:x16}) which has a base "
+                "that is less than the function's low PC {3:x16}. Please file "
+                "a bug and attach the file at the "
+                "start of this error message",
+                block->GetID(), range_base, range.GetRangeEnd(),
+                subprogram_low_pc);
+          }
+        }
+        block->FinalizeRanges();
+
+        if (tag != DW_TAG_subprogram &&
+            (name != nullptr || mangled_name != nullptr)) {
+          std::unique_ptr<Declaration> decl_up;
+          if (decl_file || decl_line || decl_column)
+            decl_up = std::make_unique<Declaration>(
+                comp_unit.GetSupportFiles().GetFileSpecAtIndex(
+                    decl_file ? *decl_file : 0),
+                decl_line ? *decl_line : 0, decl_column ? *decl_column : 0);
+
+          std::unique_ptr<Declaration> call_up;
+          if (call_file || call_line || call_column)
+            call_up = std::make_unique<Declaration>(
+                comp_unit.GetSupportFiles().GetFileSpecAtIndex(
+                    call_file ? *call_file : 0),
+                call_line ? *call_line : 0, call_column ? *call_column : 0);
+
+          block->SetInlinedFunctionInfo(name, mangled_name, decl_up.get(),
+                                        call_up.get());
+        }
+
+        ++blocks_added;
 
-      if (die.HasChildren()) {
-        blocks_added += ParseBlocksRecursive(
-            comp_unit, block, die.GetFirstChild(), subprogram_low_pc);
+        if (die.HasChildren()) {
+          blocks_added +=
+              ParseBlocksRecursive(comp_unit, block, die.GetFirstChild(),
+                                   subprogram_low_pc, depth + 1);
+        }
       }
+    } break;
+    default:
+      break;
     }
+
+    // Only parse siblings of the block if we are not at depth zero. A depth of
+    // zero indicates we are currently parsing the top level DW_TAG_subprogram
+    // DIE
+
+    if (depth == 0)
+      die.Clear();
+    else
+      die = die.GetSibling();
   }
   return blocks_added;
 }
@@ -3195,16 +3240,8 @@ size_t SymbolFileDWARF::ParseBlocksRecursive(Function &func) {
   DWARFDIE function_die =
       dwarf_cu->GetNonSkeletonUnit().GetDIE(function_die_offset);
   if (function_die) {
-    // We can't use the file address from the Function object as (in the OSO
-    // case) it will already be remapped to the main module.
-    DWARFRangeList ranges = function_die.GetDIE()->GetAttributeAddressRanges(
-        function_die.GetCU(),
-        /*check_hi_lo_pc=*/true);
-    lldb::addr_t function_file_addr =
-        ranges.GetMinRangeBase(LLDB_INVALID_ADDRESS);
-    if (function_file_addr != LLDB_INVALID_ADDRESS)
-      ParseBlocksRecursive(*comp_unit, &func.GetBlock(false),
-                           function_die.GetFirstChild(), function_file_addr);
+    ParseBlocksRecursive(*comp_unit, &func.GetBlock(false), function_die,
+                         LLDB_INVALID_ADDRESS, 0);
   }
 
   return functions_added;

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

@@ -395,7 +395,8 @@ class SymbolFileDWARF : public SymbolFileCommon {
   Function *ParseFunction(CompileUnit &comp_unit, const DWARFDIE &die);
 
   size_t ParseBlocksRecursive(CompileUnit &comp_unit, Block *parent_block,
-                              DWARFDIE die, lldb::addr_t subprogram_low_pc);
+                              const DWARFDIE &die,
+                              lldb::addr_t subprogram_low_pc, uint32_t depth);
 
   size_t ParseTypes(const SymbolContext &sc, const DWARFDIE &die,
                     bool parse_siblings, bool parse_children);

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

@@ -394,12 +394,18 @@ Block *SymbolFileNativePDB::CreateBlock(PdbCompilandSymId block_id) {
 
   switch (sym.kind()) {
   case S_GPROC32:
-  case S_LPROC32:
+  case S_LPROC32: {
     // This is a function.  It must be global.  Creating the Function entry
     // for it automatically creates a block for it.
-    if (FunctionSP func = GetOrCreateFunction(block_id, *comp_unit))
-      return &func->GetBlock(false);
+    FunctionSP func = GetOrCreateFunction(block_id, *comp_unit);
+    if (func) {
+      Block &block = func->GetBlock(false);
+      if (block.GetNumRanges() == 0)
+        block.AddRange(Block::Range(0, func->GetAddressRange().GetByteSize()));
+      return █
+    }
     break;
+  }
   case S_BLOCK32: {
     // This is a block.  Its parent is either a function or another block.  In
     // either case, its parent can be viewed as a block (e.g. a function

lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

@@ -402,32 +402,44 @@ static size_t ParseFunctionBlocksForPDBSymbol(
   assert(pdb_symbol && parent_block);
 
   size_t num_added = 0;
+  switch (pdb_symbol->getSymTag()) {
+  case PDB_SymType::Block:
+  case PDB_SymType::Function: {
+    Block *block = nullptr;
+    auto &raw_sym = pdb_symbol->getRawSymbol();
+    if (auto *pdb_func = llvm::dyn_cast<PDBSymbolFunc>(pdb_symbol)) {
+      if (pdb_func->hasNoInlineAttribute())
+        break;
+      if (is_top_parent)
+        block = parent_block;
+      else
+        break;
+    } else if (llvm::isa<PDBSymbolBlock>(pdb_symbol)) {
+      auto uid = pdb_symbol->getSymIndexId();
+      if (parent_block->FindBlockByID(uid))
+        break;
+      if (raw_sym.getVirtualAddress() < func_file_vm_addr)
+        break;
 
-  if (!is_top_parent) {
-    // Ranges for the top block were parsed together with the function.
-    if (pdb_symbol->getSymTag() != PDB_SymType::Block)
-      return num_added;
+      block = parent_block->CreateChild(pdb_symbol->getSymIndexId()).get();
+    } else
+      llvm_unreachable("Unexpected PDB symbol!");
 
-    auto &raw_sym = pdb_symbol->getRawSymbol();
-    assert(llvm::isa<PDBSymbolBlock>(pdb_symbol));
-    auto uid = pdb_symbol->getSymIndexId();
-    if (parent_block->FindBlockByID(uid))
-      return num_added;
-    if (raw_sym.getVirtualAddress() < func_file_vm_addr)
-      return num_added;
-
-    Block *block = parent_block->CreateChild(pdb_symbol->getSymIndexId()).get();
     block->AddRange(Block::Range(
         raw_sym.getVirtualAddress() - func_file_vm_addr, raw_sym.getLength()));
     block->FinalizeRanges();
-  }
-  auto results_up = pdb_symbol->findAllChildren();
-  if (!results_up)
-    return num_added;
+    ++num_added;
 
-  while (auto symbol_up = results_up->getNext()) {
-    num_added += ParseFunctionBlocksForPDBSymbol(
-        func_file_vm_addr, symbol_up.get(), parent_block, false);
+    auto results_up = pdb_symbol->findAllChildren();
+    if (!results_up)
+      break;
+    while (auto symbol_up = results_up->getNext()) {
+      num_added += ParseFunctionBlocksForPDBSymbol(
+          func_file_vm_addr, symbol_up.get(), block, false);
+    }
+  } break;
+  default:
+    break;
   }
   return num_added;
 }

lldb/source/Symbol/Function.cpp

@@ -279,14 +279,9 @@ Function::Function(CompileUnit *comp_unit, lldb::user_id_t func_uid,
                    AddressRanges ranges)
     : UserID(func_uid), m_comp_unit(comp_unit), m_type_uid(type_uid),
       m_type(type), m_mangled(mangled), m_block(*this, func_uid),
-      m_range(CollapseRanges(ranges)), m_prologue_byte_size(0) {
+      m_ranges(std::move(ranges)), m_range(CollapseRanges(m_ranges)),
+      m_frame_base(), m_flags(), m_prologue_byte_size(0) {
   assert(comp_unit != nullptr);
-  lldb::addr_t base_file_addr = m_range.GetBaseAddress().GetFileAddress();
-  for (const AddressRange &range : ranges)
-    m_block.AddRange(
-        Block::Range(range.GetBaseAddress().GetFileAddress() - base_file_addr,
-                     range.GetByteSize()));
-  m_block.FinalizeRanges();
 }
 
 Function::~Function() = default;
@@ -431,16 +426,13 @@ void Function::GetDescription(Stream *s, lldb::DescriptionLevel level,
     llvm::interleaveComma(decl_context, *s, [&](auto &ctx) { ctx.Dump(*s); });
     *s << "}";
   }
-  *s << ", range" << (m_block.GetNumRanges() > 1 ? "s" : "") << " = ";
+  *s << ", range" << (m_ranges.size() > 1 ? "s" : "") << " = ";
   Address::DumpStyle fallback_style =
       level == eDescriptionLevelVerbose
           ? Address::DumpStyleModuleWithFileAddress
           : Address::DumpStyleFileAddress;
-  for (unsigned idx = 0; idx < m_block.GetNumRanges(); ++idx) {
-    AddressRange range;
-    m_block.GetRangeAtIndex(idx, range);
+  for (const AddressRange &range : m_ranges)
     range.Dump(s, target, Address::DumpStyleLoadAddress, fallback_style);
-  }
 }
 
 void Function::Dump(Stream *s, bool show_context) const {

lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s

@@ -10,7 +10,7 @@
 
 # CHECK: 1 match found in {{.*}}
 # CHECK: Summary: {{.*}}`foo
-# CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x000000000000000e)[0x0000000000000014-0x000000000000001c)
+# CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x0000000000000007)[0x0000000000000007-0x000000000000000e)[0x0000000000000014-0x000000000000001b)[0x000000000000001b-0x000000000000001c)
 
         .text
 

lldb/test/Shell/SymbolFile/PDB/function-nested-block.test

@@ -2,6 +2,7 @@ REQUIRES: system-windows, lld
 RUN: %build --compiler=clang-cl --nodefaultlib --output=%t.exe %S/Inputs/FunctionNestedBlockTest.cpp
 RUN: lldb-test symbols -find=function -file FunctionNestedBlockTest.cpp -line 4 %t.exe | FileCheck --check-prefix=CHECK-FUNCTION %s
 RUN: lldb-test symbols -find=block -file FunctionNestedBlockTest.cpp -line 4 %t.exe | FileCheck --check-prefix=CHECK-BLOCK %s
+XFAIL: *
 
 CHECK-FUNCTION: Found 1 functions:
 CHECK-FUNCTION: name = "{{.*}}", mangled = "{{_?}}main"