Address feedback

Author: Jeffrey Tan

lldb/source/Plugins/Process/amdgpu-core/ProcessAmdGpuCore.cpp

@@ -43,29 +43,42 @@ void ProcessAmdGpuCore::Initialize() {
   llvm::call_once(g_once_flag, []() {
     // Register as GPU Process plugin (for merged CPU+GPU cores)
     // AMD only supports merged mode, not standalone GPU-only cores
-    ProcessElfGpuCore::RegisterEmbeddedCorePlugin(
+    ProcessElfEmbeddedCore::RegisterEmbeddedCorePlugin(
         GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance);
   });
 }
 
 void ProcessAmdGpuCore::Terminate() {
-  ProcessElfGpuCore::UnregisterEmbeddedCorePlugin(
+  ProcessElfEmbeddedCore::UnregisterEmbeddedCorePlugin(
       ProcessAmdGpuCore::CreateInstance);
 }
 
-std::shared_ptr<ProcessElfGpuCore> ProcessAmdGpuCore::CreateInstance(
+static std::optional<CoreNote>
+GetAmdGpuNote(std::shared_ptr<ProcessElfCore> cpu_core_process) {
+  const auto &notes = cpu_core_process->GetCoreNotes();
+  auto it = std::find_if(notes.begin(), notes.end(), [](const CoreNote &note) {
+    return note.info.n_name == "AMDGPU" &&
+           note.info.n_type == llvm::ELF::NT_AMDGPU_KFD_CORE_STATE;
+  });
+
+  if (it != notes.end()) {
+    return *it;
+  }
+  return std::nullopt;
+}
+
+std::shared_ptr<ProcessElfEmbeddedCore> ProcessAmdGpuCore::CreateInstance(
     std::shared_ptr<ProcessElfCore> cpu_core_process,
     lldb::ListenerSP listener_sp, const FileSpec *crash_file) {
   // Check if this core file has AMD GPU notes (type 33 =
   // NT_AMDGPU_KFD_CORE_STATE)
-  if (!cpu_core_process->GetAmdGpuNote().has_value()) {
+  if (!GetAmdGpuNote(cpu_core_process).has_value()) {
     return nullptr;
   }
   llvm::Expected<lldb::TargetSP> gpu_target_or_err =
-      CreateGpuTarget(cpu_core_process->GetTarget().GetDebugger());
+      CreateEmbeddedCoreTarget(cpu_core_process->GetTarget().GetDebugger());
   if (!gpu_target_or_err) {
-    LLDB_LOGF(GetLog(LLDBLog::Process),
-              "Failed to create GPU target: %s",
+    LLDB_LOGF(GetLog(LLDBLog::Process), "Failed to create GPU target: %s",
               llvm::toString(gpu_target_or_err.takeError()).c_str());
     return nullptr;
   }
@@ -83,6 +96,15 @@ std::shared_ptr<ProcessElfGpuCore> ProcessAmdGpuCore::CreateInstance(
   // Set up the CPU-GPU connection
   cpu_core_process->GetTarget().SetGPUPluginTarget(
       gpu_process_sp->GetPluginName(), gpu_target_sp);
+
+  // Load the GPU core - report errors to user if loading fails
+  Status error = gpu_process_sp->LoadCore();
+  if (error.Fail()) {
+    cpu_core_process->GetTarget().GetDebugger().ReportError(
+        llvm::formatv("Failed to load AMD GPU core: {0}", error.AsCString()));
+    return nullptr;
+  }
+
   return gpu_process_sp;
 }
 
@@ -120,7 +142,7 @@ static amd_dbgapi_status_t amd_dbgapi_client_process_get_info_callback(
     amd_dbgapi_core_state_data_t *core_state_data =
         (amd_dbgapi_core_state_data_t *)value;
     std::optional<CoreNote> amd_gpu_note_opt =
-        process->GetCpuProcess()->GetAmdGpuNote();
+        GetAmdGpuNote(process->GetCpuProcess());
 
     if (!amd_gpu_note_opt.has_value())
       return AMD_DBGAPI_STATUS_ERROR_NOT_AVAILABLE;

lldb/source/Plugins/Process/amdgpu-core/ProcessAmdGpuCore.h

@@ -16,10 +16,10 @@
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_ELF_CORE_PROCESSAMDGPUCORE_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_ELF_CORE_PROCESSAMDGPUCORE_H
 
-#include "../elf-core/ProcessElfGpuCore.h"
+#include "../elf-core/ProcessElfEmbeddedCore.h"
 #include <amd-dbgapi/amd-dbgapi.h>
 
-class ProcessAmdGpuCore : public ProcessElfGpuCore {
+class ProcessAmdGpuCore : public ProcessElfEmbeddedCore {
 public:
   static void Initialize();
 
@@ -29,7 +29,7 @@ class ProcessAmdGpuCore : public ProcessElfGpuCore {
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
-  static std::shared_ptr<ProcessElfGpuCore>
+  static std::shared_ptr<ProcessElfEmbeddedCore>
   CreateInstance(std::shared_ptr<ProcessElfCore> cpu_core_process,
                  lldb::ListenerSP listener_sp,
                  const lldb_private::FileSpec *crash_file);
@@ -38,8 +38,8 @@ class ProcessAmdGpuCore : public ProcessElfGpuCore {
   ProcessAmdGpuCore(std::shared_ptr<ProcessElfCore> cpu_core_process,
                     lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
                     const lldb_private::FileSpec &core_file)
-      : ProcessElfGpuCore(target_sp, /*cpu_core_process=*/cpu_core_process,
-                          listener_sp, core_file) {}
+      : ProcessElfEmbeddedCore(target_sp, /*cpu_core_process=*/cpu_core_process,
+                               listener_sp, core_file) {}
 
   ~ProcessAmdGpuCore() override;
 
@@ -59,8 +59,6 @@ class ProcessAmdGpuCore : public ProcessElfGpuCore {
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
-  std::optional<lldb_private::CoreNote> GetAmdGpuNote();
-
 protected:
   bool DoUpdateThreadList(lldb_private::ThreadList &old_thread_list,
                           lldb_private::ThreadList &new_thread_list) override;

lldb/source/Plugins/Process/elf-core/CMakeLists.txt

@@ -1,6 +1,6 @@
 add_lldb_library(lldbPluginProcessElfCore PLUGIN
   ProcessElfCore.cpp
-  ProcessElfGpuCore.cpp
+  ProcessElfEmbeddedCore.cpp
   ThreadElfCore.cpp
   RegisterContextLinuxCore_x86_64.cpp
   RegisterContextPOSIXCore_arm.cpp

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

@@ -28,11 +28,11 @@
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Support/Threading.h"
 
-#include "ProcessElfGpuCore.h"
 #include "Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h"
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/Process/elf-core/RegisterUtilities.h"
 #include "ProcessElfCore.h"
+#include "ProcessElfEmbeddedCore.h"
 #include "ThreadElfCore.h"
 
 using namespace lldb_private;
@@ -273,12 +273,9 @@ Status ProcessElfCore::DoLoadCore() {
       }
     }
   }
-  llvm::Expected<std::shared_ptr<ProcessElfGpuCore>> gpu_process =
-      ProcessElfGpuCore::LoadGpuCore(
-          std::static_pointer_cast<ProcessElfCore>(shared_from_this()),
-          GetCoreFile());
-  if (!gpu_process)
-    return Status::FromError(gpu_process.takeError());
+  ProcessElfEmbeddedCore::LoadEmbeddedCoreFiles(
+      std::static_pointer_cast<ProcessElfCore>(shared_from_this()),
+      GetCoreFile());
 
   return error;
 }
@@ -437,7 +434,7 @@ size_t ProcessElfCore::DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   const lldb::addr_t file_start = address_range->data.GetRangeBase();
   const lldb::addr_t file_end = address_range->data.GetRangeEnd();
   size_t bytes_to_read = size; // Number of bytes to read from the core file
-  size_t bytes_copied = 0;   // Number of bytes actually read from the core file
+  size_t bytes_copied = 0; // Number of bytes actually read from the core file
   lldb::addr_t bytes_left =
       0; // Number of bytes available in the core file from the given address
 
@@ -520,8 +517,7 @@ lldb::addr_t ProcessElfCore::GetImageInfoAddress() {
 
 // Parse a FreeBSD NT_PRSTATUS note - see FreeBSD sys/procfs.h for details.
 static void ParseFreeBSDPrStatus(ThreadData &thread_data,
-                                 const DataExtractor &data,
-                                 bool lp64) {
+                                 const DataExtractor &data, bool lp64) {
   lldb::offset_t offset = 0;
   int pr_version = data.GetU32(&offset);
 
@@ -548,8 +544,7 @@ static void ParseFreeBSDPrStatus(ThreadData &thread_data,
 
 // Parse a FreeBSD NT_PRPSINFO note - see FreeBSD sys/procfs.h for details.
 static void ParseFreeBSDPrPsInfo(ProcessElfCore &process,
-                                 const DataExtractor &data,
-                                 bool lp64) {
+                                 const DataExtractor &data, bool lp64) {
   lldb::offset_t offset = 0;
   int pr_version = data.GetU32(&offset);
 
@@ -568,8 +563,7 @@ static void ParseFreeBSDPrPsInfo(ProcessElfCore &process,
 }
 
 static llvm::Error ParseNetBSDProcInfo(const DataExtractor &data,
-                                       uint32_t &cpi_nlwps,
-                                       uint32_t &cpi_signo,
+                                       uint32_t &cpi_nlwps, uint32_t &cpi_signo,
                                        uint32_t &cpi_siglwp,
                                        uint32_t &cpi_pid) {
   lldb::offset_t offset = 0;
@@ -737,8 +731,8 @@ llvm::Error ProcessElfCore::parseNetBSDNotes(llvm::ArrayRef<CoreNote> notes) {
 
     if (name == "NetBSD-CORE") {
       if (note.info.n_type == NETBSD::NT_PROCINFO) {
-        llvm::Error error = ParseNetBSDProcInfo(note.data, nlwps, signo,
-                                                siglwp, pr_pid);
+        llvm::Error error =
+            ParseNetBSDProcInfo(note.data, nlwps, signo, siglwp, pr_pid);
         if (error)
           return error;
         SetID(pr_pid);
@@ -964,7 +958,9 @@ llvm::Error ProcessElfCore::parseLinuxNotes(llvm::ArrayRef<CoreNote> notes) {
       Status status = prpsinfo.Parse(note.data, arch);
       if (status.Fail())
         return status.ToError();
-      thread_data.name.assign (prpsinfo.pr_fname, strnlen (prpsinfo.pr_fname, sizeof (prpsinfo.pr_fname)));
+      thread_data.name.assign(
+          prpsinfo.pr_fname,
+          strnlen(prpsinfo.pr_fname, sizeof(prpsinfo.pr_fname)));
       SetID(prpsinfo.pr_pid);
       m_executable_name = thread_data.name;
       break;
@@ -1019,7 +1015,7 @@ llvm::Error ProcessElfCore::ParseThreadContextsFromNoteSegment(
   assert(segment_header.p_type == llvm::ELF::PT_NOTE);
 
   auto notes_or_error = parseSegment(segment_data);
-  if(!notes_or_error)
+  if (!notes_or_error)
     return notes_or_error.takeError();
   switch (GetArchitecture().GetTriple().getOS()) {
   case llvm::Triple::FreeBSD:
@@ -1152,14 +1148,15 @@ bool ProcessElfCore::GetProcessInfo(ProcessInstanceInfo &info) {
   return true;
 }
 
-std::optional<CoreNote> ProcessElfCore::GetAmdGpuNote() {
+std::vector<CoreNote> ProcessElfCore::GetCoreNotes() {
+  std::vector<CoreNote> notes;
   ObjectFileELF *core = (ObjectFileELF *)(m_core_module_sp->GetObjectFile());
   if (core == nullptr)
-    return std::nullopt;
+    return notes;
   llvm::ArrayRef<elf::ELFProgramHeader> program_headers =
       core->ProgramHeaders();
   if (program_headers.size() == 0)
-    return std::nullopt;
+    return notes;
 
   for (const elf::ELFProgramHeader &H : program_headers) {
     if (H.p_type == llvm::ELF::PT_NOTE) {
@@ -1170,11 +1167,11 @@ std::optional<CoreNote> ProcessElfCore::GetAmdGpuNote() {
         llvm::consumeError(notes_or_error.takeError());
         continue;
       }
-      for (const auto &note : *notes_or_error) {
-        if (note.info.n_type == llvm::ELF::NT_AMDGPU_KFD_CORE_STATE)
-          return note;
+      // Append all notes from this segment to our collection
+      for (auto &note : *notes_or_error) {
+        notes.push_back(std::move(note));
       }
     }
   }
-  return std::nullopt;
+  return notes;
 }

lldb/source/Plugins/Process/elf-core/ProcessElfCore.h

@@ -98,7 +98,7 @@ class ProcessElfCore : public lldb_private::PostMortemProcess {
 
   bool GetProcessInfo(lldb_private::ProcessInstanceInfo &info) override;
 
-  std::optional<lldb_private::CoreNote> GetAmdGpuNote();
+  std::vector<lldb_private::CoreNote> GetCoreNotes();
 
 protected:
   void Clear();