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

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Jul 31 09:00:41 PDT 2019

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

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list