Refactor FindVariable() core functionality into StackFrame out of SBFrame

[shafik]

Summary

SBFrame should be a thin wrapper around the core functionality in StackFrame. Currently FindVariable() core functionality is implemented in SBFrame and this will move that into StackFrame.

This is step two in enabling stepping into std::function.

[davide]

LGTM modulo minor.

source/Target/StackFrame.cpp

1733–1738

This is fairly unreadable IMHO. If I were you, I would hoist the lambda out.

[davide]

include/lldb/Target/StackFrame.h

506–507

Also, I think you might want to add a doxygen comment here, as this function is now part of the API.

[clayborg]

include/lldb/Target/StackFrame.h

506

Make this a "ConstString name" since we need that for lookup anyway? We couldn't do that in SBFrame cause we don't expose our constant strings outside the API, but internally we can use ConstString

source/API/SBFrame.cpp

669

value_sp = frame->FindVariable(ConstString(name));

source/Target/StackFrame.cpp

1712

lldb::ValueObjectSP StackFrame::FindVariable(ConstString name) {

1737

remove ConstString if we change parameter

[jingham]

I agree with Greg, this is the sort of function that we'd pass a ConstString to on the lldb_private side.

We haven't documented all the API's we should. This one is pretty self-explanatory, but still we should try to document all the new functions we add, Davide's right there.

Other than those nits, this seems fine.

[shafik]

Addressing comments:

- Adding documentation to FindVariable()
- Using ConstString instead of const char *

[shafik]

@jingham @clayborg @davide addressed comments w/ the exception of the lambda one which I politely disagree with.

source/Target/StackFrame.cpp

1733–1738

This is exactly the case lambda were meant to address, moving it out would just add boilerplate code :-(

[jingham]

This is fine by me.

[davide]

Sure.

[clayborg]

Just a documentation suggestion, but looks good.

include/lldb/Target/StackFrame.h

508

We might want to say how the search is done. Something like "The search for the variable starts in the deepest block corresponding to the current PC in the stack frame and traverse through all parent blocks stopping at inlined function boundaries"

[shafik]

Adjusting documentation based on feedback

[clayborg]

Looks good

include/lldb/Target/StackFrame.h

public:
   ///   The offset from the register.  Particularly important for sp...
   ///
   /// @return
   ///   The ValueObject if found.  If valid, it has a valid ExpressionPath.
   //------------------------------------------------------------------
   lldb::ValueObjectSP GuessValueForRegisterAndOffset(ConstString reg,
                                                      int64_t offset);
 
+  lldb::ValueObjectSP FindVariable(const char *name);
+
   //------------------------------------------------------------------
   // lldb::ExecutionContextScope pure virtual functions
   //------------------------------------------------------------------
   lldb::TargetSP CalculateTarget() override;
 
   lldb::ProcessSP CalculateProcess() override;
 
   lldb::ThreadSP CalculateThread() override;
 

source/API/SBFrame.cpp

SBValue SBFrame::FindVariable(const char *name,
   StackFrame *frame = nullptr;
   Target *target = exe_ctx.GetTargetPtr();
   Process *process = exe_ctx.GetProcessPtr();
   if (target && process) {
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process->GetRunLock())) {
       frame = exe_ctx.GetFramePtr();
       if (frame) {
-        VariableList variable_list;
-        SymbolContext sc(frame->GetSymbolContext(eSymbolContextBlock));
-
-        if (sc.block) {
-          const bool can_create = true;
-          const bool get_parent_variables = true;
-          const bool stop_if_block_is_inlined_function = true;
+        value_sp = frame->FindVariable(name);
 
-          if (sc.block->AppendVariables(
-                  can_create, get_parent_variables,
-                  stop_if_block_is_inlined_function,
-                  [frame](Variable *v) { return v->IsInScope(frame); },
-                  &variable_list)) {
-            var_sp = variable_list.FindVariable(ConstString(name));
-          }
-        }
-
-        if (var_sp) {
-          value_sp =
-              frame->GetValueObjectForFrameVariable(var_sp, eNoDynamicValues);
+        if (value_sp)
           sb_value.SetSP(value_sp, use_dynamic);
-        }
       } else {
         if (log)
           log->Printf("SBFrame::FindVariable () => error: could not "
                       "reconstruct frame object for this SBFrame.");
       }
     } else {
       if (log)
         log->Printf("SBFrame::FindVariable () => error: process is running");

source/Target/StackFrame.cpp

lldb::ValueObjectSP StackFrame::GuessValueForRegisterAndOffset(ConstString reg,
   if (!variables) {
     return ValueObjectSP();
   }
 
   return DoGuessValueAt(*this, reg, offset, *disassembler_sp, *variables,
                         GetFrameCodeAddress());
 }
 
+lldb::ValueObjectSP StackFrame::FindVariable(const char *name) {
+  ValueObjectSP value_sp;
+
+  if (name == nullptr || name[0] == '\0')
+    return value_sp;
+
+  TargetSP target_sp = CalculateTarget();
+  ProcessSP process_sp = CalculateProcess();
+
+  if (!target_sp && !process_sp)
+    return value_sp;
+
+  VariableList variable_list;
+  VariableSP var_sp;
+  SymbolContext sc(GetSymbolContext(eSymbolContextBlock));
+
+  if (sc.block) {
+    const bool can_create = true;
+    const bool get_parent_variables = true;
+    const bool stop_if_block_is_inlined_function = true;
+
+    if (sc.block->AppendVariables(
+            can_create, get_parent_variables, stop_if_block_is_inlined_function,
+            [this](Variable *v) { return v->IsInScope(this); },
+            &variable_list)) {
+      var_sp = variable_list.FindVariable(ConstString(name));
+    }
+
+    if (var_sp)
+      value_sp = GetValueObjectForFrameVariable(var_sp, eNoDynamicValues);
+  }
+
+  return value_sp;
+}
+
 TargetSP StackFrame::CalculateTarget() {
   TargetSP target_sp;
   ThreadSP thread_sp(GetThread());
   if (thread_sp) {
     ProcessSP process_sp(thread_sp->CalculateProcess());
     if (process_sp)
       target_sp = process_sp->CalculateTarget();
   }