Select the correct DWARF location list entry when looking for variable locations

[jasonmolenda]

Summary

When lldb is looking for a location list entry for a variable, it is using the pc value -- or return-pc value when up the stack -- to pick the correct DWARF location list entry. Because it is using return-pc, the entry selected may be an entirely different basic block, or function. We should be using the "pc for symbolication" -- return-pc decremented in most cases -- to select the correct block we should be looking up. I added RegisterContext::GetPCForSymbolication and StackFrame::GetFrameCodeAddressForSymbolication last year in https://reviews.llvm.org/D97644 and this is a simple patch with those in place.

The bug that drove this is in the middle of a complex project, but I can elicit incorrect behavior with a small C file that calls abort() as its last instruction, with a different function immediately after that. It must be built at -O1 with clang (on x86_64, AArch64) so that the debug info tracks variables in different registers during the function lifetime, using a DWARF location list.

I constructed a test case that does this, but it depends on clang's current codegen at -O1 on these two architectures. If the codegen were to shift around, the test could pass without correctly exercising this codepath. One possibility would be to include a test binary, DWARF for it, and corefile; that would eliminate the possibility of codegen changing. But it's a lot of data, and I'm not sure how necessary it is. Given that I get the same codegen on two architectures with clang at -O1, maybe this is a small enough exposure to the optimizer that we can use it in the testsuite. Open to suggestions.

Without this patch, this test case will show argv as unavailable when you try to print it. In the actual bug that I was tracking down, the return-pc pointed to a different location list entry for the variable so lldb showed the wrong value for that variable. A much more subtle and confusing bug for the user.

[jingham]

The change seems correct. Since the test only requires -01, and the test code is quite straightforward, the test seems fine.

[clayborg]

I wonder if this will help optimized code debugging at all!? Good catch

[jasonmolenda]

In D124597#3483197, @clayborg wrote:

I wonder if this will help optimized code debugging at all!? Good catch

It came up in a kernel debugging session iirc; it was the case of a function doing a noreturn call, and when you went up to that stack frame, the variable was in one register up through the noreturn call, and then it was in a different register after that noreturn -- so we would print the wrong register value. :/ It takes a bit to get this exact issue, but printing an incorrect value was a worst case failure mode. The API test case I came up with will only print that the variable is unavailable, instead of the wrong value.

lldb/source/Expression/DWARFExpression.cpp

bool DWARFExpression::Evaluate(ExecutionContext *exe_ctx,
                                RegisterContext *reg_ctx,
                                lldb::addr_t func_load_addr,
                                const Value *initial_value_ptr,
                                const Value *object_address_ptr, Value &result,
                                Status *error_ptr) const {
   ModuleSP module_sp = m_module_wp.lock();
 
   if (IsLocationList()) {
-    addr_t pc;
+    Address pc;
     StackFrame *frame = nullptr;
-    if (reg_ctx)
-      pc = reg_ctx->GetPC();
-    else {
+    if (!reg_ctx || !reg_ctx->GetPCForSymbolication(pc)) {
       frame = exe_ctx->GetFramePtr();
       if (!frame)
         return false;
       RegisterContextSP reg_ctx_sp = frame->GetRegisterContext();
       if (!reg_ctx_sp)
         return false;
-      pc = reg_ctx_sp->GetPC();
+      reg_ctx_sp->GetPCForSymbolication(pc);
     }
 
     if (func_load_addr != LLDB_INVALID_ADDRESS) {
-      if (pc == LLDB_INVALID_ADDRESS) {
+      if (!pc.IsValid()) {
         if (error_ptr)
           error_ptr->SetErrorString("Invalid PC in frame.");
         return false;
       }
 
-      if (llvm::Optional<DataExtractor> expr =
-              GetLocationExpression(func_load_addr, pc)) {
+      Target *target = exe_ctx->GetTargetPtr();
+      if (llvm::Optional<DataExtractor> expr = GetLocationExpression(
+              func_load_addr, pc.GetLoadAddress(target))) {
         return DWARFExpression::Evaluate(
             exe_ctx, reg_ctx, module_sp, *expr, m_dwarf_cu, m_reg_kind,
             initial_value_ptr, object_address_ptr, result, error_ptr);
       }
     }
     if (error_ptr)
       error_ptr->SetErrorString("variable not available");
     return false;
if (IsLocationList()) {
     if (!sc.function)
       return false;
 
     addr_t load_function_start =
         sc.function->GetAddressRange().GetBaseAddress().GetFileAddress();
     if (load_function_start == LLDB_INVALID_ADDRESS)
       return false;
 
-    addr_t pc = frame.GetFrameCodeAddress().GetLoadAddress(
+    addr_t pc = frame.GetFrameCodeAddressForSymbolication().GetLoadAddress(
         frame.CalculateTarget().get());
 
     if (llvm::Optional<DataExtractor> expr =
             GetLocationExpression(load_function_start, pc))
       opcodes = std::move(*expr);
     else
       return false;
   } else

lldb/test/API/functionalities/location-list-lookup/Makefile

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -O1
+include Makefile.rules

lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py

+"""Test that lldb picks the correct DWARF location list entry with a return-pc out of bounds."""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class LocationListLookupTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+    def test_loclist(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Create a target by the debugger.
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+        self.dbg.SetAsync(False)
+
+        li = lldb.SBLaunchInfo(["a.out"])
+        error = lldb.SBError()
+        process = target.Launch(li, error)
+        self.assertTrue(process.IsValid())
+        self.assertTrue(process.is_stopped)
+
+        # Find `main` on the stack, then 
+        # find `argv` local variable, then
+        # check that we can read the c-string in argv[0]
+        for f in process.GetSelectedThread().frames:
+            if f.GetDisplayFunctionName() == "main":
+                argv = f.GetValueForVariablePath("argv").GetChildAtIndex(0)
+                strm = lldb.SBStream()
+                argv.GetDescription(strm)
+                self.assertNotEqual(strm.GetData().find('a.out'), -1)

lldb/test/API/functionalities/location-list-lookup/main.c

+#include <stdio.h>
+#include <stdlib.h>
+
+// The goal with this test is:
+//  1. Have main() followed by foo()
+//  2. Have the no-return call to abort() in main be the last instruction
+//  3. Have the next instruction be the start of foo()
+//  4. The debug info for argv uses a location list.
+//     clang at -O1 on x86_64 or arm64 has debuginfo like
+//          DW_AT_location  (0x00000049:
+//              [0x0000000100003f15, 0x0000000100003f25): DW_OP_reg4 RSI
+//              [0x0000000100003f25, 0x0000000100003f5b): DW_OP_reg15 R15)
+
+void foo(int);
+int main(int argc, char **argv) {
+  char *file = argv[0];
+  char f0 = file[0];
+  printf("%c\n", f0);
+  foo(f0);
+  printf("%s %d\n", argv[0], argc);
+  abort(); /// argv is still be accessible here
+}
+void foo(int in) { printf("%d\n", in); }