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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I will let Jason comment on the unwind specifics since this is his area. I caught a few other things that need to be cleaned up.



================
Comment at: lldb/include/lldb/Target/Unwind.h:40
     uint32_t idx;
+    bool behaves_like_zeroth_frame;
 
----------------
initialize with value just in case.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:153-155
+  AddressRange addr_range;
+  m_sym_ctx_valid = TryResolveSymbolContextAndAddressRange(
+      m_current_pc, m_sym_ctx, addr_range);
----------------
See comment for RegisterContextLLDB::TryResolveSymbolContextAndAddressRange, but if we change as suggested this will become:
```
m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx, nullptr);
```


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:430-432
+  AddressRange addr_range;
+  m_sym_ctx_valid = TryResolveSymbolContextAndAddressRange(
+      m_current_pc, m_sym_ctx, addr_range);
----------------
See comment for RegisterContextLLDB::TryResolveSymbolContextAndAddressRange, but if we change as suggested this will become:
```
m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx, nullptr);
```


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:486-487
     m_sym_ctx.Clear(false);
-    m_sym_ctx_valid = false;
-    SymbolContextItem resolve_scope =
-        eSymbolContextFunction | eSymbolContextSymbol;
-
-    ModuleSP temporary_module_sp = temporary_pc.GetModule();
-    if (temporary_module_sp &&
-        temporary_module_sp->ResolveSymbolContextForAddress(
-            temporary_pc, resolve_scope, m_sym_ctx) &
-            resolve_scope) {
-      if (m_sym_ctx.GetAddressRange(resolve_scope, 0, false, addr_range))
-        m_sym_ctx_valid = true;
-    }
+    m_sym_ctx_valid = TryResolveSymbolContextAndAddressRange(
+        temporary_pc, m_sym_ctx, addr_range);
+
----------------
See comment for RegisterContextLLDB::TryResolveSymbolContextAndAddressRange, but if we change as suggested this will become:
```
m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx, &addr_range);
```


================
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)
----------------
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);  
}
```


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1760
+void RegisterContextLLDB::PropagateTrapHandlerFlag(
+    lldb::UnwindPlanSP unwind_plan) {
+  if (unwind_plan->GetUnwindPlanForSignalTrap() != eLazyBoolYes) {
----------------
JosephTremoulet wrote:
> I'm a bit ambivalent about adding this part -- the downside is that it's not concretely helping today, because if an eh_frame record has the 'S' flag in its augmentation (which is what `unwind_Plan->GetUnwindPlanForSignalTrap()` reports), we'll have already decremented pc and generated unwind plans based on the prior function when initializing the register context.  But the upside is that this connects the dots between the otherwise-unused bit on the unwind plan and the frame type, and will be in place should we subsequently add code to try the second function's unwind plan as a fallback.
I will let Jason comment on this one.


================
Comment at: lldb/source/Target/StackFrameList.cpp:453
     lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+    bool behaves_like_zeroth_frame;
     if (idx == 0) {
----------------
init with value


================
Comment at: lldb/source/Target/StackFrameList.cpp:670
         addr_t pc, cfa;
-        if (unwinder->GetFrameInfoAtIndex(idx, cfa, pc)) {
+        bool behaves_like_zeroth_frame;
+        if (unwinder->GetFrameInfoAtIndex(idx, cfa, pc,
----------------
init with value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64993