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

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet marked an inline comment as done.
JosephTremoulet 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)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64993