lldb/Instrumentation: NFC-ish use GetFrameCodeAddressForSymbolication()

[friss]

Summary

A couple of our Instrumentation runtimes were gathering backtraces,
storing it in a StructuredData array and later creating a HistoryThread
using this data. By deafult HistoryThread will consider the history PCs
as return addresses and thus will substract 1 from them to go to the
call address.

This is usually correct, but it's also wasteful as when we gather the
backtraces ourselves, we have much better information to decide how
to backtrace and symbolicate. This patch uses the new
GetFrameCodeAddressForSymbolication() to gather the PCs that should
be used for symbolication and configures the HistoryThread to just
use those PCs as-is.

(The MTC plugin was actaully applying a -1 itself and then the
HistoryThread would do it again, so this actaully fixes a bug there.)

rdar://77027680

[JDevlieghere]

lldb/source/Plugins/InstrumentationRuntime/MainThreadChecker/InstrumentationRuntimeMainThreadChecker.cpp

271–273

[friss]

Use make_shared

[JDevlieghere]

Thanks for addressing my critical feedback. LGTM.

[shafik]

lldb/source/Plugins/InstrumentationRuntime/MainThreadChecker/InstrumentationRuntimeMainThreadChecker.cpp

273

A nitpick but you could have also done:

ThreadSP new_thread_sp = std::make_shared<HistoryThread>(
     *process_sp, tid, PCs, true /*pcs_are_call_addresses*/);

Otherwise I would have made pcs_are_call_addresses a constant since you are using purely for documentation.

[teemperor]

lldb/source/Plugins/InstrumentationRuntime/MainThreadChecker/InstrumentationRuntimeMainThreadChecker.cpp

273

I think the format /*pc_are_call_addresses=*/true); is what LLVM is using (with that format clang-tidy will check that the argument fits the parameter name).

lldb/source/Plugins/InstrumentationRuntime/MainThreadChecker/InstrumentationRuntimeMainThreadChecker.cpp

InstrumentationRuntimeMainThreadChecker::RetrieveReportData(
   }
 
   // Gather the PCs of the user frames in the backtrace.
   StructuredData::Array *trace = new StructuredData::Array();
   auto trace_sp = StructuredData::ObjectSP(trace);
   StackFrameSP responsible_frame;
   for (unsigned I = 0; I < thread_sp->GetStackFrameCount(); ++I) {
     StackFrameSP frame = thread_sp->GetStackFrameAtIndex(I);
-    Address addr = frame->GetFrameCodeAddress();
+    Address addr = frame->GetFrameCodeAddressForSymbolication();
     if (addr.GetModule() == runtime_module_sp) // Skip PCs from the runtime.
       continue;
 
     // The first non-runtime frame is responsible for the bug.
     if (!responsible_frame)
       responsible_frame = frame;
 
-    // First frame in stacktrace should point to a real PC, not return address.
-    if (I != 0 && trace->GetSize() == 0) {
-      addr.Slide(-1);
-    }
-
     lldb::addr_t PC = addr.GetLoadAddress(&target);
     trace->AddItem(StructuredData::ObjectSP(new StructuredData::Integer(PC)));
   }
 
   auto *d = new StructuredData::Dictionary();
   auto dict_sp = StructuredData::ObjectSP(d);
   d->AddStringItem("instrumentation_class", "MainThreadChecker");
   d->AddStringItem("api_name", apiName);
InstrumentationRuntimeMainThreadChecker::GetBacktracesFromExtendedStopInfo(
 
   if (PCs.empty())
     return threads;
 
   StructuredData::ObjectSP thread_id_obj =
       info->GetObjectForDotSeparatedPath("tid");
   tid_t tid = thread_id_obj ? thread_id_obj->GetIntegerValue() : 0;
 
-  HistoryThread *history_thread = new HistoryThread(*process_sp, tid, PCs);
-  ThreadSP new_thread_sp(history_thread);
+  // We gather symbolication addresses above, so no need for HistoryThread to
+  // try to infer the call addresses.
+  bool pcs_are_call_addresses = true;
+  ThreadSP new_thread_sp = std::make_shared<HistoryThread>(
+      *process_sp, tid, PCs, pcs_are_call_addresses);
 
   // Save this in the Process' ExtendedThreadList so a strong pointer retains
   // the object
   process_sp->GetExtendedThreadList().AddThread(new_thread_sp);
   threads->AddThread(new_thread_sp);
 
   return threads;
 }

lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp

target.GetDebugger().GetAsyncOutputStream()->Printf(
         eval_error.AsCString());
     return StructuredData::ObjectSP();
   }
 
   // Gather the PCs of the user frames in the backtrace.
   StructuredData::Array *trace = new StructuredData::Array();
   auto trace_sp = StructuredData::ObjectSP(trace);
   for (unsigned I = 0; I < thread_sp->GetStackFrameCount(); ++I) {
-    const Address FCA =
-        thread_sp->GetStackFrameAtIndex(I)->GetFrameCodeAddress();
+    const Address FCA = thread_sp->GetStackFrameAtIndex(I)
+                            ->GetFrameCodeAddressForSymbolication();
     if (FCA.GetModule() == runtime_module_sp) // Skip PCs from the runtime.
       continue;
 
     lldb::addr_t PC = FCA.GetLoadAddress(&target);
     trace->AddItem(StructuredData::ObjectSP(new StructuredData::Integer(PC)));
   }
 
   std::string IssueKind = RetrieveString(main_value, process_sp, ".issue_kind");
InstrumentationRuntimeUBSan::GetBacktracesFromExtendedStopInfo(
 
   if (PCs.empty())
     return threads;
 
   StructuredData::ObjectSP thread_id_obj =
       info->GetObjectForDotSeparatedPath("tid");
   tid_t tid = thread_id_obj ? thread_id_obj->GetIntegerValue() : 0;
 
-  HistoryThread *history_thread = new HistoryThread(*process_sp, tid, PCs);
-  ThreadSP new_thread_sp(history_thread);
+  // We gather symbolication addresses above, so no need for HistoryThread to
+  // try to infer the call addresses.
+  bool pcs_are_call_addresses = true;
+  ThreadSP new_thread_sp = std::make_shared<HistoryThread>(
+      *process_sp, tid, PCs, pcs_are_call_addresses);
   std::string stop_reason_description = GetStopReasonDescription(info);
   new_thread_sp->SetName(stop_reason_description.c_str());
 
   // Save this in the Process' ExtendedThreadList so a strong pointer retains
   // the object
   process_sp->GetExtendedThreadList().AddThread(new_thread_sp);
   threads->AddThread(new_thread_sp);
 
   return threads;
 }