Fix backtrace of noreturn functions situated at the end of a module

[labath]

Summary

When a call instruction is the last instruction in a function, the
backtrace PC will point past the end of the function. We already had
special code to handle that, but we did not handle the case where the PC
ends up outside of the bounds of the module containing the function,
which is a situation that occured in TestNoreturnUnwind on android for
some arch/compiler combinations.

I fix this by adding an argument to Address resolution code which states
that we are ok with addresses pointing to the end of a module/section to
resolve to that module/section.

I create a reproducible test case for this situation by hand-crafting an
executable which has a noreturn function at the end of a module.

[labath]

Jim, Jason, could one of you take a look at this please?

This part of the code is not really my specialty, so I'd appreciate your thoughts on this.

[jasonmolenda]

Sorry Pavel, I kept meaning to look at this patch this week but hadn't yet. I will later today. When I read over your description of the problem, it sounded like a good fix - haven't looked at the code yet.

[jasonmolenda]

Hi Pavel, I'd document the new flag in include/lldb/Core/Address.h where we have documentation for the other flags being used.

It seems like we're fixing this a little indirectly, and I'm not sure it's the best approach. I want to make sure I understand the situation correctly.

Say we have an object file with three sections

s1 0..99 (offset 0, size 100)
s2 100..199 (offset 100, size 100)
s3 200.299 (offset 200, size 100)

We have a noreturn function whose last instruction ends on the last byte of s2, so the saved return address is offset 200. Your change makes it so that when we say "is 200 contained within 100..199", we will say yes, it is. This gets us the correct Section for our symbol context and when we back up the pc value by one (the "decr_pc_and_recompute_addr_range = true" bit in RegisterContextLLDB::InitializeNonZerothFrame) so when we back up the offset within the section by 1, we're good.

The current behavior is that we pick the wrong section (s3) and somehow backing up the pc isn't working.

Maybe we can accomplish the same fix by looking at the if (decr_pc_and_recompute_addr_range) block of code? Is decr_pc_and_recompute_addr_range not being set for this case? Are we not correctly backing up the pc into the Section boundary correctly?

I have access to a linux machine so I can play with this myself, but it will take me a while to set up lldb on that system, could you let me know if you looked at this approach? I know RegisterContextLLDB is complicated and it's easy to miss things - or it may be that you tried this approach and it didn't look possible. (and I haven't had to touch this code in a few years so I may have forgotten about some "new Section is the same as the old Section" sanity check or something in there... I don't see it right now, but I may have missed it.)

[labath]

Thank you for looking at this. My response is below.

In D32022#733223, @jasonmolenda wrote:

Hi Pavel, I'd document the new flag in include/lldb/Core/Address.h where we have documentation for the other flags being used.

It seems like we're fixing this a little indirectly, and I'm not sure it's the best approach. I want to make sure I understand the situation correctly.

Say we have an object file with three sections

s1 0..99 (offset 0, size 100)
s2 100..199 (offset 100, size 100)
s3 200.299 (offset 200, size 100)

We have a noreturn function whose last instruction ends on the last byte of s2, so the saved return address is offset 200. Your change makes it so that when we say "is 200 contained within 100..199", we will say yes, it is. This gets us the correct Section for our symbol context and when we back up the pc value by one (the "decr_pc_and_recompute_addr_range = true" bit in RegisterContextLLDB::InitializeNonZerothFrame) so when we back up the offset within the section by 1, we're good.

The reason that decr_pc_and_recompute_addr_range is not working is because we don't find any section (or module) at all. In the object file you mention above, this would correspond to having a noreturn function at the end of s3, and then the space after s3 being empty. In this case we don't even reach the decr_pc_and_recompute_addr_range part, because we abort due to a missing module.

In my first version of this patch did something different -- I modified RegisterContextLLDB to retry the module search by decrementing the pc by one if the original search did not find any module. However, that did not seem completely clean either, as I had to do the same dance twice (StackFrame::GetFrameCodeAddress needed to do the same dance as well to display the symbol name correctly). I can go back to that implementation if you think that is better.

I have access to a linux machine so I can play with this myself, but it will take me a while to set up lldb on that system, could you let me know if you looked at this approach? I know RegisterContextLLDB is complicated and it's easy to miss things - or it may be that you tried this approach and it didn't look possible. (and I haven't had to touch this code in a few years so I may have forgotten about some "new Section is the same as the old Section" sanity check or something in there... I don't see it right now, but I may have missed it.)

You don't need a linux machine to debug this -- you should be able to open the core file in the test from any system (last time I tried, I had problems downloading binary files from phabricator, so I am going to also send them to you directly, just in case). You could probably also create a darwin executable reproducing this using assembly similar to the one in the test.

source/Plugins/Process/Utility/RegisterContextLLDB.cpp

339 ↗

(On Diff #95139)

Here we don't get a valid module and we abort the search.

[labath]

Jason, any thoughts on my comments above?

[labath]

Add documentation.

[labath]

I'd like to commit this this week. If you have any objections to how I implemented this, let me know, so I can adjust the approach.

[jasonmolenda]

Pavel, my apologies for not following up on this, we had a big push to get ready for a beta release/conference this week, but it wasn't fair to leave you hanging with this change. Please commit. I'd like to play with this a little when I get free time (hahahaha) to see if I can't isolate the change a little more, but it'll be on me to play with the corefile test case on my own time instead of blocking you.

[labath]

Thanks for the review. I'm not entirely proud of how I implemented this, but I hope it's not too ugly either. The reason I opted for this approach is that I needed to implement the "if the module is null then decrement the address and recompute" logic in two places. RegisterContextLLDB (computes the backtrace) and StackFrame (computes the symbol name to display). This avoids the duplication, but adds another argument to a bunch of functions. I don't really have an idea on how to make this better, but maybe you will think of something.

lldb/trunk/include/lldb/Core/Address.h

#endif
   /// @param[in] load_addr
   ///     A load address from a current process.
   ///
   /// @param[in] target
   ///     The target to use when trying resolve the address into
   ///     a section + offset. The Target's SectionLoadList object
   ///     is used to resolve the address.
   ///
+  /// @param[in] allow_section_end
+  ///     If true, treat an address pointing to the end of the module as
+  ///     belonging to that module.
+  ///
   /// @return
   ///     Returns \b true if the load address was resolved to be
   ///     section/offset, \b false otherwise. It is often ok for an
   ///     address no not resolve to a section in a module, this often
   ///     happens for JIT'ed code, or any load addresses on the stack
   ///     or heap.
   //------------------------------------------------------------------
-  bool SetLoadAddress(lldb::addr_t load_addr, Target *target);
+  bool SetLoadAddress(lldb::addr_t load_addr, Target *target,
+                      bool allow_section_end = false);
 
   bool SetOpcodeLoadAddress(
       lldb::addr_t load_addr, Target *target,
-      lldb::AddressClass addr_class = lldb::eAddressClassInvalid);
+      lldb::AddressClass addr_class = lldb::eAddressClassInvalid,
+      bool allow_section_end = false);
 
   bool SetCallableLoadAddress(lldb::addr_t load_addr, Target *target);
 
   //------------------------------------------------------------------
   /// Get accessor for the module for this address.
   ///
   /// @return
   ///     Returns the Module pointer that this address is an offset

lldb/trunk/include/lldb/Core/Section.h

public:
   const SectionList &GetChildren() const { return m_children; }
 
   void Dump(Stream *s, Target *target, uint32_t depth) const;
 
   void DumpName(Stream *s) const;
 
   lldb::addr_t GetLoadBaseAddress(Target *target) const;
 
-  bool ResolveContainedAddress(lldb::addr_t offset, Address &so_addr) const;
+  bool ResolveContainedAddress(lldb::addr_t offset, Address &so_addr,
+                               bool allow_section_end = false) const;
 
   lldb::offset_t GetFileOffset() const { return m_file_offset; }
 
   void SetFileOffset(lldb::offset_t file_offset) {
     m_file_offset = file_offset;
   }
 
   lldb::offset_t GetFileSize() const { return m_file_size; }

lldb/trunk/include/lldb/Target/SectionLoadList.h

public:
   void operator=(const SectionLoadList &rhs);
 
   bool IsEmpty() const;
 
   void Clear();
 
   lldb::addr_t GetSectionLoadAddress(const lldb::SectionSP &section_sp) const;
 
-  bool ResolveLoadAddress(lldb::addr_t load_addr, Address &so_addr) const;
+  bool ResolveLoadAddress(lldb::addr_t load_addr, Address &so_addr,
+                          bool allow_section_end = false) const;
 
   bool SetSectionLoadAddress(const lldb::SectionSP &section_sp,
                              lldb::addr_t load_addr,
                              bool warn_multiple = false);
 
   // The old load address should be specified when unloading to ensure we get
   // the correct instance of the section as a shared library could be loaded
   // at more than one location.

lldb/trunk/packages/Python/lldbsuite/test/functionalities/unwind/noreturn/TestNoreturnUnwind.py

 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class NoreturnUnwind(TestBase):
     mydir = TestBase.compute_mydir(__file__)
 
     @skipIfWindows  # clang-cl does not support gcc style attributes.
-    @expectedFailureAndroid(bugnumber="llvm.org/pr31192")
-    @expectedFailureAll(bugnumber="llvm.org/pr31192", oslist=['linux'], compiler="gcc", archs=['arm'])
     def test(self):
         """Test that we can backtrace correctly with 'noreturn' functions on the stack"""
         self.build()
         self.setTearDownCleanup()
 
         exe = os.path.join(os.getcwd(), "a.out")
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)

lldb/trunk/packages/Python/lldbsuite/test/functionalities/unwind/noreturn/main.c

 func_a (void)
 {
   func_b ();
 }
 
 int
 main (int argc, char *argv[])
 {
-    sleep (2);
-
   func_a ();
 
   return 0;
 }

lldb/trunk/packages/Python/lldbsuite/test/functionalities/unwind/noreturn/module-end/TestNoReturnModuleEnd.py

+"""
+Test that we properly display the backtrace when a noreturn function happens to
+be at the end of the stack.
+"""
+
+from __future__ import print_function
+
+import shutil
+import struct
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestNoreturnModuleEnd(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        super(TestNoreturnModuleEnd, self).setUp()
+        self._initial_platform = lldb.DBG.GetSelectedPlatform()
+
+    def tearDown(self):
+        lldb.DBG.SetSelectedPlatform(self._initial_platform)
+        super(TestNoreturnModuleEnd, self).tearDown()
+
+    def test(self):
+        target = self.dbg.CreateTarget("test.out")
+        process = target.LoadCore("test.core")
+        self.assertTrue(process.IsValid(), PROCESS_IS_VALID)
+        self.assertEqual(process.GetNumThreads(), 1)
+
+        thread = process.GetSelectedThread()
+        self.assertTrue(thread.IsValid())
+
+        backtrace = [
+            ["func2", 3],
+            ["func1", 8],
+            ["_start", 8],
+        ]
+        self.assertEqual(thread.GetNumFrames(), len(backtrace))
+        for i in range(len(backtrace)):
+            frame = thread.GetFrameAtIndex(i)
+            self.assertTrue(frame.IsValid())
+            symbol = frame.GetSymbol()
+            self.assertTrue(symbol.IsValid())
+            self.assertEqual(symbol.GetName(), backtrace[i][0])
+            function_start = symbol.GetStartAddress().GetLoadAddress(target)
+            self.assertEquals(function_start + backtrace[i][1], frame.GetPC())
+
+        self.dbg.DeleteTarget(target)

lldb/trunk/packages/Python/lldbsuite/test/functionalities/unwind/noreturn/module-end/a.s

+# compile this with:
+# as a.s -o a.o --32 && ld a.o -m elf_i386
+# generate core file with:
+# ulimit -s 12 && ./a.out
+
+.text
+
+.globl func2
+.type func2, @function
+func2:
+  pushl %ebp
+  movl  %esp, %ebp
+  movl  0,    %eax
+  popl  %ebp
+  ret
+.size func2, .-func2
+
+.globl _start
+.type _start, @function
+_start:
+  pushl %ebp
+  movl  %esp, %ebp
+  call  func1
+  popl  %ebp
+  ret
+.size _start, .-_start
+
+.globl func1
+.type func1, @function
+func1:
+  pushl %ebp
+  movl  %esp, %ebp
+  call  func2
+.size func1, .-func1
+

lldb/trunk/source/Core/Address.cpp

if (code_addr != LLDB_INVALID_ADDRESS) {
     if (addr_class == eAddressClassInvalid)
       addr_class = GetAddressClass();
     code_addr = target->GetOpcodeLoadAddress(code_addr, addr_class);
   }
   return code_addr;
 }
 
 bool Address::SetOpcodeLoadAddress(lldb::addr_t load_addr, Target *target,
-                                   AddressClass addr_class) {
-  if (SetLoadAddress(load_addr, target)) {
+                                   AddressClass addr_class,
+                                   bool allow_section_end) {
+  if (SetLoadAddress(load_addr, target, allow_section_end)) {
     if (target) {
       if (addr_class == eAddressClassInvalid)
         addr_class = GetAddressClass();
       m_offset = target->GetOpcodeLoadAddress(m_offset, addr_class);
     }
     return true;
   }
   return false;
if (obj_file) {
       // Give the symbol vendor a chance to add to the unified section list.
       module_sp->GetSymbolVendor();
       return obj_file->GetAddressClass(GetFileAddress());
     }
   }
   return eAddressClassUnknown;
 }
 
-bool Address::SetLoadAddress(lldb::addr_t load_addr, Target *target) {
-  if (target &&
-      target->GetSectionLoadList().ResolveLoadAddress(load_addr, *this))
+bool Address::SetLoadAddress(lldb::addr_t load_addr, Target *target,
+                             bool allow_section_end) {
+  if (target && target->GetSectionLoadList().ResolveLoadAddress(
+                    load_addr, *this, allow_section_end))
     return true;
   m_section_wp.reset();
   m_offset = load_addr;
   return false;
 }

lldb/trunk/source/Core/Section.cpp

addr_t Section::GetLoadBaseAddress(Target *target) const {
   }
   if (load_base_addr == LLDB_INVALID_ADDRESS) {
     load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress(
         const_cast<Section *>(this)->shared_from_this());
   }
   return load_base_addr;
 }
 
-bool Section::ResolveContainedAddress(addr_t offset, Address &so_addr) const {
+bool Section::ResolveContainedAddress(addr_t offset, Address &so_addr,
+                                      bool allow_section_end) const {
   const size_t num_children = m_children.GetSize();
-  if (num_children > 0) {
-    for (size_t i = 0; i < num_children; i++) {
-      Section *child_section = m_children.GetSectionAtIndex(i).get();
 
-      addr_t child_offset = child_section->GetOffset();
-      if (child_offset <= offset &&
-          offset - child_offset < child_section->GetByteSize())
-        return child_section->ResolveContainedAddress(offset - child_offset,
-                                                      so_addr);
-    }
+        offset - child_offset <
+            child_section->GetByteSize() + (allow_section_end ? 1 : 0))
+                                                    so_addr, allow_section_end);
   }
   so_addr.SetOffset(offset);
   so_addr.SetSection(const_cast<Section *>(this)->shared_from_this());
 
 #ifdef LLDB_CONFIGURATION_DEBUG
   // For debug builds, ensure that there are no orphaned (i.e., moduleless)
   // sections.
   assert(GetModule().get());

lldb/trunk/source/Plugins/Process/Utility/RegisterContextLLDB.cpp

void RegisterContextLLDB::InitializeNonZerothFrame() {
   ExecutionContext exe_ctx(m_thread.shared_from_this());
   Process *process = exe_ctx.GetProcessPtr();
   // Let ABIs fixup code addresses to make sure they are valid. In ARM ABIs
   // this will strip bit zero in case we read a PC from memory or from the LR.
   ABI *abi = process->GetABI().get();
   if (abi)
     pc = abi->FixCodeAddress(pc);
 
-  m_current_pc.SetLoadAddress(pc, &process->GetTarget());
+  const bool allow_section_end = true;
+  m_current_pc.SetLoadAddress(pc, &process->GetTarget(), allow_section_end);
 
   // If we don't have a Module for some reason, we're not going to find
   // symbol/function information - just
   // stick in some reasonable defaults and hope we can unwind past this frame.
   ModuleSP pc_module_sp(m_current_pc.GetModule());
   if (!m_current_pc.IsValid() || !pc_module_sp) {
     UnwindLogMsg("using architectural default unwind method");
 
void RegisterContextLLDB::InitializeNonZerothFrame() {
 
   // If the symbol lookup failed...
   if (m_sym_ctx_valid == false)
     decr_pc_and_recompute_addr_range = true;
 
   // Or if we're in the middle of the stack (and not "above" an asynchronous
   // event like sigtramp),
   // and our "current" pc is the start of a function...
-  if (m_sym_ctx_valid && GetNextFrame()->m_frame_type != eTrapHandlerFrame &&
+  if (GetNextFrame()->m_frame_type != eTrapHandlerFrame &&
       GetNextFrame()->m_frame_type != eDebuggerFrame &&
-      addr_range.GetBaseAddress().IsValid() &&
+      (!m_sym_ctx_valid ||
+       (addr_range.GetBaseAddress().IsValid() &&
         addr_range.GetBaseAddress().GetSection() == m_current_pc.GetSection() &&
-      addr_range.GetBaseAddress().GetOffset() == m_current_pc.GetOffset()) {
+        addr_range.GetBaseAddress().GetOffset() == m_current_pc.GetOffset()))) {
     decr_pc_and_recompute_addr_range = true;
   }
 
   // We need to back up the pc by 1 byte and re-search for the Symbol to handle
   // the case where the "saved pc"
   // value is pointing to the next function, e.g. if a function ends with a CALL
   // instruction.
   // FIXME this may need to be an architectural-dependent behavior; if so we'll

lldb/trunk/source/Target/SectionLoadList.cpp

bool SectionLoadList::SetSectionUnloaded(const lldb::SectionSP &section_sp,
   if (ats_pos != m_addr_to_sect.end()) {
     erased = true;
     m_addr_to_sect.erase(ats_pos);
   }
 
   return erased;
 }
 
-bool SectionLoadList::ResolveLoadAddress(addr_t load_addr,
-                                         Address &so_addr) const {
+bool SectionLoadList::ResolveLoadAddress(addr_t load_addr, Address &so_addr,
+                                         bool allow_section_end) const {
   // First find the top level section that this load address exists in
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   if (!m_addr_to_sect.empty()) {
     addr_to_sect_collection::const_iterator pos =
         m_addr_to_sect.lower_bound(load_addr);
     if (pos != m_addr_to_sect.end()) {
       if (load_addr != pos->first && pos != m_addr_to_sect.begin())
         --pos;
       const addr_t pos_load_addr = pos->first;
       if (load_addr >= pos_load_addr) {
         addr_t offset = load_addr - pos_load_addr;
-        if (offset < pos->second->GetByteSize()) {
+        if (offset < pos->second->GetByteSize() + (allow_section_end ? 1 : 0)) {
           // We have found the top level section, now we need to find the
           // deepest child section.
-          return pos->second->ResolveContainedAddress(offset, so_addr);
+          return pos->second->ResolveContainedAddress(offset, so_addr,
+                                                      allow_section_end);
         }
       }
     } else {
       // There are no entries that have an address that is >= load_addr,
       // so we need to check the last entry on our collection.
       addr_to_sect_collection::const_reverse_iterator rpos =
           m_addr_to_sect.rbegin();
       if (load_addr >= rpos->first) {
         addr_t offset = load_addr - rpos->first;
-        if (offset < rpos->second->GetByteSize()) {
+        if (offset <
+            rpos->second->GetByteSize() + (allow_section_end ? 1 : 0)) {
           // We have found the top level section, now we need to find the
           // deepest child section.
-          return rpos->second->ResolveContainedAddress(offset, so_addr);
+          return rpos->second->ResolveContainedAddress(offset, so_addr,
+                                                       allow_section_end);
         }
       }
     }
   }
   so_addr.Clear();
   return false;
 }
 

lldb/trunk/source/Target/StackFrame.cpp

if (m_flags.IsClear(RESOLVED_FRAME_CODE_ADDR) &&
     m_flags.Set(RESOLVED_FRAME_CODE_ADDR);
 
     // Resolve the PC into a temporary address because if ResolveLoadAddress
     // fails to resolve the address, it will clear the address object...
     ThreadSP thread_sp(GetThread());
     if (thread_sp) {
       TargetSP target_sp(thread_sp->CalculateTarget());
       if (target_sp) {
+        const bool allow_section_end = true;
         if (m_frame_code_addr.SetOpcodeLoadAddress(
                 m_frame_code_addr.GetOffset(), target_sp.get(),
-                eAddressClassCode)) {
+                eAddressClassCode, allow_section_end)) {
           ModuleSP module_sp(m_frame_code_addr.GetModule());
           if (module_sp) {
             m_sc.module_sp = module_sp;
             m_flags.Set(eSymbolContextModule);
           }
         }
       }
     }