[Lldb-commits] [PATCH] D64993: Fix PC adjustment in StackFrame::GetSymbolContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:595-624
+bool RegisterContextLLDB::TryResolveSymbolContextAndAddressRange(
+    lldb_private::Address pc, lldb_private::SymbolContext &sym_ctx,
+    lldb_private::AddressRange &addr_range) {
+  ModuleSP pc_module_sp = pc.GetModule();
+
+  // Can't resolve context without a module.
+  if (!pc_module_sp)
----------------
JosephTremoulet wrote:
> clayborg wrote:
> > This function doesn't belong in RegisterContextLLDB. It think lldb_private::Address would be a better place:
> > 
> > ```
> > /// if "addr_range_ptr" is not NULL, then fill in with the address range of the function.
> > bool lldb_private::Address::ResolveFunctionScope(lldb_private::SymbolContext &sym_ctx, lldb_private::AddressRange *addr_range_ptr) {
> >   constexpr bool resolve_tail_call_address = false;
> >   constexpr SymbolContextItem resolve_scope = eSymbolContextFunction | eSymbolContextSymbol;
> >   if (!CalculateSymbolContext(&sym_ctx, resolve_scope))
> >    return false;
> >   if (!addr_range_ptr)
> >     return true;
> >   return sym_ctx.GetAddressRange(resolve_scope, 0, false, *addr_range_ptr);  
> > }
> > ```
> Sure, will update.  Just to double-check a couple points:
> 
>  #  I'll actually want to pass `&addr_range` at all three callsites, rather than passing `nullptr` at two of them, correct? (at the callsite on 154 to preserve the `addr_range` previously defined on 175, and at the callsite on 431to preserve the `addr_range` previously defined on 470)
> 
>  # I see you removed the `resolve_scope & resolved_scope` check.  Am I correct that that was intentional and it's redundant with the checks in `GetAddressRange` that require `function` or `symbol` to be non-null, or is there something more subtle going on here?
> 
> Thanks.
For 1) above, it seems like you were making a new local variable just so you could pass it. If it is actually used somewhere, then yes, do include it. It seemed like you were just making a new local for no reason.

For 2) above: You can restore the resolve_scope & resolved_scope if needed. Can't remember if we will return eSymbolContextModule if we fail to find eSymbolContextFunction or eSymbolContextSymbol. So problably safest to restore that so we don't change functionality


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64993