Skip to content

[lldb] Use the function block as a source for function ranges #117996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

labath
Copy link
Collaborator

@labath labath commented Nov 28, 2024

This is a follow-up/reimplementation of #115730. While working on that patch, I did not realize that the correct (discontinuous) set of ranges is already stored in the block representing the whole function. The catch -- ranges for this block are only set later, when parsing all of the blocks of the function.

This patch changes that by populating the function block ranges eagerly -- from within the Function constructor. This also necessitates a corresponding change in all of the symbol files -- so that they stop populating the ranges of that block. This allows us to avoid some unnecessary work (not parsing the function DW_AT_ranges twice) and also results in some simplification of the parsing code.

This is a follow-up/reimplementation of llvm#115730. While working on that
patch, I did not realize that the correct (discontinuous) set of ranges
is already stored in the block representing the whole function. The
catch -- ranges for this block are only set later, when parsing all of
the blocks of the function.

This patch changes that by populating the function block ranges eagerly
-- from within the Function constructor. This also necessitates a
corresponding change in all of the symbol files -- so that they stop
populating the ranges of that block. This allows us to avoid some
unnecessary work (not parsing the function DW_AT_ranges twice) and also
results in some simplification of the parsing code.
@labath labath requested a review from jasonmolenda November 28, 2024 11:49
@labath labath requested a review from JDevlieghere as a code owner November 28, 2024 11:49
@llvmbot llvmbot added the lldb label Nov 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 28, 2024

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

This is a follow-up/reimplementation of #115730. While working on that patch, I did not realize that the correct (discontinuous) set of ranges is already stored in the block representing the whole function. The catch -- ranges for this block are only set later, when parsing all of the blocks of the function.

This patch changes that by populating the function block ranges eagerly -- from within the Function constructor. This also necessitates a corresponding change in all of the symbol files -- so that they stop populating the ranges of that block. This allows us to avoid some unnecessary work (not parsing the function DW_AT_ranges twice) and also results in some simplification of the parsing code.


Full diff: https://github.com/llvm/llvm-project/pull/117996.diff

9 Files Affected:

  • (modified) lldb/include/lldb/Symbol/Function.h (-3)
  • (modified) lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp (+1-3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+72-108)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+1-2)
  • (modified) lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp (+3-9)
  • (modified) lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp (+21-32)
  • (modified) lldb/source/Symbol/Function.cpp (+12-4)
  • (modified) lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s (+1-1)
  • (modified) lldb/test/Shell/SymbolFile/PDB/function-nested-block.test (-1)
diff --git a/lldb/include/lldb/Symbol/Function.h b/lldb/include/lldb/Symbol/Function.h
index 70f51a846f8d96..e71dc2348b1734 100644
--- a/lldb/include/lldb/Symbol/Function.h
+++ b/lldb/include/lldb/Symbol/Function.h
@@ -650,9 +650,6 @@ 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.
diff --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
index df3bf157278daf..bc886259d6fa5f 100644
--- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
+++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
@@ -299,9 +299,7 @@ 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;
-  Block &block = func.GetBlock(false);
-  block.AddRange(Block::Range(0, func.GetAddressRange().GetByteSize()));
-  blocks.push_back(&block);
+  blocks.push_back(&func.GetBlock(false));
 
   size_t blocks_added = 0;
   addr_t func_base = func.GetAddressRange().GetBaseAddress().GetOffset();
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index fe711c56958c44..d814b635f2e5cd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1305,121 +1305,76 @@ bool SymbolFileDWARF::ParseDebugMacros(CompileUnit &comp_unit) {
   return true;
 }
 
-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 SymbolFileDWARF::ParseBlocksRecursive(CompileUnit &comp_unit,
+                                             Block *parent_block, DWARFDIE die,
+                                             addr_t subprogram_low_pc) {
   size_t blocks_added = 0;
-  DWARFDIE die = orig_die;
-  while (die) {
+  for (; die; die = die.GetSibling()) {
     dw_tag_t tag = die.Tag();
 
-    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.
-
-        if (depth > 0)
-          break;
+    if (tag != DW_TAG_inlined_subroutine && tag != DW_TAG_lexical_block)
+      continue;
 
-        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);
-          }
-        }
-
-        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());
+    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());
+      }
 
-        ++blocks_added;
+      ++blocks_added;
 
-        if (die.HasChildren()) {
-          blocks_added +=
-              ParseBlocksRecursive(comp_unit, block, die.GetFirstChild(),
-                                   subprogram_low_pc, depth + 1);
-        }
+      if (die.HasChildren()) {
+        blocks_added += ParseBlocksRecursive(
+            comp_unit, block, die.GetFirstChild(), subprogram_low_pc);
       }
-    } 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;
 }
@@ -3240,8 +3195,17 @@ size_t SymbolFileDWARF::ParseBlocksRecursive(Function &func) {
   DWARFDIE function_die =
       dwarf_cu->GetNonSkeletonUnit().GetDIE(function_die_offset);
   if (function_die) {
-    ParseBlocksRecursive(*comp_unit, &func.GetBlock(false), function_die,
-                         LLDB_INVALID_ADDRESS, 0);
+    // 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);
   }
 
   return functions_added;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index ac25a0c48ee7d7..76f4188fdf4afb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -395,8 +395,7 @@ class SymbolFileDWARF : public SymbolFileCommon {
   Function *ParseFunction(CompileUnit &comp_unit, const DWARFDIE &die);
 
   size_t ParseBlocksRecursive(CompileUnit &comp_unit, Block *parent_block,
-                              const DWARFDIE &die,
-                              lldb::addr_t subprogram_low_pc, uint32_t depth);
+                              DWARFDIE die, lldb::addr_t subprogram_low_pc);
 
   size_t ParseTypes(const SymbolContext &sc, const DWARFDIE &die,
                     bool parse_siblings, bool parse_children);
diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
index d17fedf26b4c48..27d51bbd1cb563 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -394,18 +394,12 @@ 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.
-    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 &block;
-    }
+    if (FunctionSP func = GetOrCreateFunction(block_id, *comp_unit))
+      return &func->GetBlock(false);
     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
diff --git a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
index 4935b0fbdfd877..952c1eb35ccb56 100644
--- a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -402,44 +402,33 @@ 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;
 
-      block = parent_block->CreateChild(pdb_symbol->getSymIndexId()).get();
-    } else
-      llvm_unreachable("Unexpected PDB symbol!");
+  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;
 
+    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();
-    ++num_added;
 
-    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;
+  }
+  auto results_up = pdb_symbol->findAllChildren();
+  if (!results_up)
+    return num_added;
+
+  while (auto symbol_up = results_up->getNext()) {
+    num_added += ParseFunctionBlocksForPDBSymbol(func_file_vm_addr,
+                                                 symbol_up.get(), parent_block, false);
   }
   return num_added;
 }
diff --git a/lldb/source/Symbol/Function.cpp b/lldb/source/Symbol/Function.cpp
index b346749ca06ec3..0c067a0126571d 100644
--- a/lldb/source/Symbol/Function.cpp
+++ b/lldb/source/Symbol/Function.cpp
@@ -279,9 +279,14 @@ 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_ranges(std::move(ranges)), m_range(CollapseRanges(m_ranges)),
-      m_frame_base(), m_flags(), m_prologue_byte_size(0) {
+      m_range(CollapseRanges(ranges)), 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;
@@ -426,13 +431,16 @@ void Function::GetDescription(Stream *s, lldb::DescriptionLevel level,
     llvm::interleaveComma(decl_context, *s, [&](auto &ctx) { ctx.Dump(*s); });
     *s << "}";
   }
-  *s << ", range" << (m_ranges.size() > 1 ? "s" : "") << " = ";
+  *s << ", range" << (m_block.GetNumRanges() > 1 ? "s" : "") << " = ";
   Address::DumpStyle fallback_style =
       level == eDescriptionLevelVerbose
           ? Address::DumpStyleModuleWithFileAddress
           : Address::DumpStyleFileAddress;
-  for (const AddressRange &range : m_ranges)
+  for (unsigned idx = 0; idx < m_block.GetNumRanges(); ++idx) {
+    AddressRange range;
+    m_block.GetRangeAtIndex(idx, range);
     range.Dump(s, target, Address::DumpStyleLoadAddress, fallback_style);
+  }
 }
 
 void Function::Dump(Stream *s, bool show_context) const {
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s b/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s
index 2584158207cc8e..b03d5d12ad2a1d 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s
+++ b/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-0x0000000000000007)[0x0000000000000007-0x000000000000000e)[0x0000000000000014-0x000000000000001b)[0x000000000000001b-0x000000000000001c)
+# CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x000000000000000e)[0x0000000000000014-0x000000000000001c)
 
         .text
 
diff --git a/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test b/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
index 1cb20a40363827..9057d01c25840f 100644
--- a/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
+++ b/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
@@ -2,7 +2,6 @@ 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"

@@ -2,7 +2,6 @@ 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: *
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't directly related to what this patch is trying to do, but it looks like my refactor fixed the bug. :D

@@ -10,7 +10,7 @@

# CHECK: 1 match found in {{.*}}
# CHECK: Summary: {{.*}}`foo
# CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x0000000000000007)[0x0000000000000007-0x000000000000000e)[0x0000000000000014-0x000000000000001b)[0x000000000000001b-0x000000000000001c)
# CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x000000000000000e)[0x0000000000000014-0x000000000000001c)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the block ranges get automatically coalesced (which is a good thing, I guess).

@@ -3240,8 +3195,17 @@ size_t SymbolFileDWARF::ParseBlocksRecursive(Function &func) {
DWARFDIE function_die =
dwarf_cu->GetNonSkeletonUnit().GetDIE(function_die_offset);
if (function_die) {
ParseBlocksRecursive(*comp_unit, &func.GetBlock(false), function_die,
LLDB_INVALID_ADDRESS, 0);
// We can't use the file address from the Function object as (in the OSO
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filing a complaint about the complete lack of test coverage for this code path on non-mac hosts. :P

Copy link

github-actions bot commented Nov 28, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@jasonmolenda jasonmolenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me.

@labath labath merged commit ba14dac into llvm:main Dec 3, 2024
7 checks passed
@labath labath deleted the fn-range branch December 3, 2024 09:21
labath added a commit that referenced this pull request Dec 3, 2024
…#117996)"

This reverts commit ba14dac. I guess
"has no conflicts" doesn't mean "it will build".
@llvm-ci
Copy link
Collaborator

llvm-ci commented Dec 3, 2024

LLVM Buildbot has detected a new failure on builder llvm-x86_64-debian-dylib running on gribozavr4 while building lldb at step 5 "build-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/14176

Here is the relevant piece of the build log for the reference
Step 5 (build-unified-tree) failure: build (failure)
...
57.552 [335/96/6617] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/CompilerDecl.cpp.o
57.563 [334/96/6618] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/CompilerDeclContext.cpp.o
57.578 [333/96/6619] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/CompilerType.cpp.o
57.594 [332/96/6620] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/SaveCoreOptions.cpp.o
57.609 [331/96/6621] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/DWARFCallFrameInfo.cpp.o
57.758 [330/96/6622] Linking CXX executable bin/clangd-indexer
57.787 [329/96/6623] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/DeclVendor.cpp.o
58.531 [328/96/6624] Linking CXX executable bin/clangd-fuzzer
58.659 [327/96/6625] Linking CXX executable bin/clangd
60.318 [326/96/6626] Building CXX object tools/lldb/source/Core/CMakeFiles/lldbCore.dir/FileLineResolver.cpp.o
FAILED: tools/lldb/source/Core/CMakeFiles/lldbCore.dir/FileLineResolver.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/source/Core -I/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source/Core -I/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/include -I/b/1/llvm-x86_64-debian-dylib/build/include -I/b/1/llvm-x86_64-debian-dylib/llvm-project/llvm/include -I/b/1/llvm-x86_64-debian-dylib/llvm-project/llvm/../clang/include -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/../clang/include -I/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/source -isystem /usr/include/libxml2 -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-vla-extension -O3 -DNDEBUG  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/lldb/source/Core/CMakeFiles/lldbCore.dir/FileLineResolver.cpp.o -MF tools/lldb/source/Core/CMakeFiles/lldbCore.dir/FileLineResolver.cpp.o.d -o tools/lldb/source/Core/CMakeFiles/lldbCore.dir/FileLineResolver.cpp.o -c /b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source/Core/FileLineResolver.cpp
In file included from /b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source/Core/FileLineResolver.cpp:11:
In file included from /b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Symbol/CompileUnit.h:15:
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Symbol/Function.h:450:58: error: use of undeclared identifier 'm_ranges'; did you mean 'm_range'?
  const AddressRanges &GetAddressRanges() const { return m_ranges; }
                                                         ^~~~~~~~
                                                         m_range
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Symbol/Function.h:659:16: note: 'm_range' declared here
  AddressRange m_range;
               ^
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Symbol/Function.h:450:58: error: no viable conversion from returned value of type 'const lldb_private::AddressRange' to function return type 'const lldb_private::AddressRanges'
  const AddressRanges &GetAddressRanges() const { return m_ranges; }
                                                         ^~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:625:7: note: candidate inherited constructor not viable: no known conversion from 'const lldb_private::AddressRange' to 'initializer_list<std::vector<lldb_private::AddressRange, std::allocator<lldb_private::AddressRange>>::value_type>' (aka 'initializer_list<lldb_private::AddressRange>') for 1st argument
      vector(initializer_list<value_type> __l,
      ^
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Core/AddressRange.h:256:50: note: constructor from base class 'vector<lldb_private::AddressRange, std::allocator<lldb_private::AddressRange>>' inherited here
  using std::vector<lldb_private::AddressRange>::vector;
                                                 ^
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Core/AddressRange.h:254:7: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'const lldb_private::AddressRange' to 'const lldb_private::AddressRanges &' for 1st argument
class AddressRanges : public std::vector<lldb_private::AddressRange> {
      ^
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Core/AddressRange.h:254:7: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'const lldb_private::AddressRange' to 'lldb_private::AddressRanges &&' for 1st argument
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:497:7: note: explicit constructor is not a candidate
      vector(const allocator_type& __a) _GLIBCXX_NOEXCEPT
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:510:7: note: explicit constructor is not a candidate
      vector(size_type __n, const allocator_type& __a = allocator_type())
      ^
2 errors generated.
60.520 [326/95/6627] Building CXX object tools/lldb/source/Core/CMakeFiles/lldbCore.dir/AddressResolverFileLine.cpp.o
FAILED: tools/lldb/source/Core/CMakeFiles/lldbCore.dir/AddressResolverFileLine.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/source/Core -I/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source/Core -I/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/include -I/b/1/llvm-x86_64-debian-dylib/build/include -I/b/1/llvm-x86_64-debian-dylib/llvm-project/llvm/include -I/b/1/llvm-x86_64-debian-dylib/llvm-project/llvm/../clang/include -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/../clang/include -I/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source -I/b/1/llvm-x86_64-debian-dylib/build/tools/lldb/source -isystem /usr/include/libxml2 -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-vla-extension -O3 -DNDEBUG  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/lldb/source/Core/CMakeFiles/lldbCore.dir/AddressResolverFileLine.cpp.o -MF tools/lldb/source/Core/CMakeFiles/lldbCore.dir/AddressResolverFileLine.cpp.o.d -o tools/lldb/source/Core/CMakeFiles/lldbCore.dir/AddressResolverFileLine.cpp.o -c /b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source/Core/AddressResolverFileLine.cpp
In file included from /b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/source/Core/AddressResolverFileLine.cpp:13:
In file included from /b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Symbol/CompileUnit.h:15:
/b/1/llvm-x86_64-debian-dylib/llvm-project/lldb/include/lldb/Symbol/Function.h:450:58: error: use of undeclared identifier 'm_ranges'; did you mean 'm_range'?
  const AddressRanges &GetAddressRanges() const { return m_ranges; }
                                                         ^~~~~~~~

labath added a commit that referenced this pull request Dec 3, 2024
#117996)"

This reverts commit 2526d5b, reapplying
ba14dac after fixing the conflict with
 #117532. The change is that Function::GetAddressRanges now recomputes
the returned value instead of returning the member. This means it now
returns a value instead of a reference type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants