[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg updated this revision to Diff 233673.
mossberg added a comment.

Output error messages to console in addition to logging.

I wasn't sure, but decided to unconditionally output the  `Could not create 
return address breakpoint.` message for UX reasons. That message provides a bit 
of context for these new, lower level error messages.

Implementation is based on the one in `ThreadPlanCallFunction.cpp`.
I also slightly altered the error message for the "permissions" not found case 
to make both error messages more consistent.

Still working on adding the unit test, but I thought I'd submit this for early 
feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372

Files:
  lldb/include/lldb/Target/ThreadPlanStepOut.h
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,25 @@
     if (m_return_addr == LLDB_INVALID_ADDRESS)
       return;
 
+    // Perform some additional validation on the return address.
+    uint32_t permissions = 0;
+    if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
+                                                          permissions)) {
+      m_constructor_errors.Printf("Return address (0x%" PRIx64
+                                  ") permissions not found.",
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
+    } else if (!(permissions & ePermissionsExecutable)) {
+      m_constructor_errors.Printf("Return address (0x%" PRIx64
+                                  ") did not point to executable memory.",
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
+    }
+
     Breakpoint *return_bp = m_thread.CalculateTarget()
                                 ->CreateBreakpoint(m_return_addr, true, false)
                                 .get();
@@ -238,8 +257,13 @@
   }
 
   if (m_return_bp_id == LLDB_INVALID_BREAK_ID) {
-    if (error)
+    if (error) {
       error->PutCString("Could not create return address breakpoint.");
+      if (m_constructor_errors.GetSize() > 0) {
+        error->PutCString(" ");
+        error->PutCString(m_constructor_errors.GetString());
+      }
+    }
     return false;
   }
 
Index: lldb/include/lldb/Target/ThreadPlanStepOut.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -72,6 +72,7 @@
   std::vector<lldb::StackFrameSP> m_stepped_past_frames;
   lldb::ValueObjectSP m_return_valobj_sp;
   bool m_calculate_return_value;
+  StreamString m_constructor_errors;
 
   friend lldb::ThreadPlanSP Thread::QueueThreadPlanForStepOut(
       bool abort_other_plans, SymbolContext *addr_context, bool first_insn,



Index: lldb/source/Target/ThreadPlanStepOut.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,25 @@
     if (m_return_addr == LLDB_INVALID_ADDRESS)
       return;
 
+    // Perform some additional validation on the return address.
+    uint32_t permissions = 0;
+    if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
+                                                          permissions)) {
+      m_constructor_errors.Printf("Return address (0x%" PRIx64
+                                  ") permissions not found.",
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
+    } else if (!(permissions & ePermissionsExecutable)) {
+      m_constructor_errors.Printf("Return address (0x%" PRIx64
+                                  ") did not point to executable memory.",
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
+    }
+
     Breakpoint *return_bp = m_thread.CalculateTarget()
                                 ->CreateBreakpoint(m_return_addr, true, false)
                                 .get();
@@ -238,8 +257,13 @@
   }
 
   if (m_return_bp_id == LLDB_INVALID_BREAK_ID) {
-    if (error)
+    if (error) {
       error->PutCString("Could not create return address breakpoint.");
+      if (m_constructor_errors.GetSize() > 0) {
+        error->PutCString(" ");
+        error->PutCString(m_constructor_errors.GetString());
+      }
+    }
     return false;
   }
 
Index: lldb/include/lldb/Target/ThreadPlanStepOut.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -72,6 +72,7 @@
   std::vector<lldb::StackFrameSP> m_stepped_past_frames;
   lldb::ValueObjectSP m_return_valobj_sp;
   bool m_calculate_return_value;
+  StreamString m_constructor_errors;
 
   friend lldb::ThreadPlanSP Thread::QueueThreadPlanForStepOut(
       bool abort_other_plans, SymbolContext *addr_context, bool first_insn,

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits