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 ↗

(On Diff #166032)

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

[davide]

include/lldb/Target/StackFrame.h

506–507 ↗

(On Diff #166032)

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 ↗

(On Diff #166032)

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 ↗

(On Diff #166032)

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

source/Target/StackFrame.cpp

1712 ↗

(On Diff #166032)

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

1737 ↗

(On Diff #166032)

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 ↗

(On Diff #166032)

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 ↗

(On Diff #166194)

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

lldb/trunk/include/lldb/Target/StackFrame.h

public:
   ///
   /// @return
   ///   The ValueObject if found.  If valid, it has a valid ExpressionPath.
   //------------------------------------------------------------------
   lldb::ValueObjectSP GuessValueForRegisterAndOffset(ConstString reg,
                                                      int64_t offset);
 
   //------------------------------------------------------------------
+  /// Attempt to reconstruct the ValueObject for a variable with a given \a name
+  /// from within the current StackFrame, within the current block. 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.
+  ///
+  /// @params [in] name
+  ///   The name of the variable.
+  ///
+  /// @return
+  ///   The ValueObject if found.
+  //------------------------------------------------------------------
+  lldb::ValueObjectSP FindVariable(ConstString name);
+
+  //------------------------------------------------------------------
   // lldb::ExecutionContextScope pure virtual functions
   //------------------------------------------------------------------
   lldb::TargetSP CalculateTarget() override;
 
   lldb::ProcessSP CalculateProcess() override;
 
   lldb::ThreadSP CalculateThread() override;
 

lldb/trunk/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(ConstString(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");

lldb/trunk/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(ConstString name) {
+  ValueObjectSP value_sp;
+
+  if (!name)
+    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(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();
   }