Skip to content

[lldb] Deindent UnwindAssemblyInstEmulation #128874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 26, 2025
Merged

[lldb] Deindent UnwindAssemblyInstEmulation #128874

merged 2 commits into from
Feb 26, 2025

Conversation

labath
Copy link
Collaborator

@labath labath commented Feb 26, 2025

by three levels using early returns/continues.

@labath labath requested a review from jasonmolenda February 26, 2025 13:49
@labath labath requested a review from JDevlieghere as a code owner February 26, 2025 13:49
@llvmbot llvmbot added the lldb label Feb 26, 2025
by two levels using early returns.
@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

by two levels using early returns.


Patch is 22.17 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128874.diff

1 Files Affected:

  • (modified) lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp (+211-227)
diff --git a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
index 502231002f7f4..3988dcb50f353 100644
--- a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++ b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -56,251 +56,235 @@ bool UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
   if (opcode_data == nullptr || opcode_size == 0)
     return false;
 
-  if (range.GetByteSize() > 0 && range.GetBaseAddress().IsValid() &&
-      m_inst_emulator_up.get()) {
+  if (range.GetByteSize() == 0 || !range.GetBaseAddress().IsValid() ||
+      !m_inst_emulator_up)
+    return false;
 
-    // The instruction emulation subclass setup the unwind plan for the first
-    // instruction.
-    m_inst_emulator_up->CreateFunctionEntryUnwind(unwind_plan);
+  // The instruction emulation subclass setup the unwind plan for the first
+  // instruction.
+  m_inst_emulator_up->CreateFunctionEntryUnwind(unwind_plan);
 
-    // CreateFunctionEntryUnwind should have created the first row. If it
-    // doesn't, then we are done.
-    if (unwind_plan.GetRowCount() == 0)
-      return false;
+  // CreateFunctionEntryUnwind should have created the first row. If it doesn't,
+  // then we are done.
+  if (unwind_plan.GetRowCount() == 0)
+    return false;
 
-    const bool prefer_file_cache = true;
-    DisassemblerSP disasm_sp(Disassembler::DisassembleBytes(
-        m_arch, nullptr, nullptr, nullptr, nullptr, range.GetBaseAddress(),
-        opcode_data, opcode_size, 99999, prefer_file_cache));
+  const bool prefer_file_cache = true;
+  DisassemblerSP disasm_sp(Disassembler::DisassembleBytes(
+      m_arch, nullptr, nullptr, nullptr, nullptr, range.GetBaseAddress(),
+      opcode_data, opcode_size, 99999, prefer_file_cache));
 
-    Log *log = GetLog(LLDBLog::Unwind);
+  if (!disasm_sp)
+    return false;
 
-    if (disasm_sp) {
+  Log *log = GetLog(LLDBLog::Unwind);
 
-      m_range_ptr = ⦥
-      m_unwind_plan_ptr = &unwind_plan;
+  m_range_ptr = ⦥
+  m_unwind_plan_ptr = &unwind_plan;
+
+  const uint32_t addr_byte_size = m_arch.GetAddressByteSize();
+  const bool show_address = true;
+  const bool show_bytes = true;
+  const bool show_control_flow_kind = false;
+  m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+      unwind_plan.GetRegisterKind(), unwind_plan.GetInitialCFARegister());
+  m_fp_is_cfa = false;
+  m_register_values.clear();
+  m_pushed_regs.clear();
+
+  // Initialize the CFA with a known value. In the 32 bit case it will be
+  // 0x80000000, and in the 64 bit case 0x8000000000000000. We use the address
+  // byte size to be safe for any future address sizes
+  m_initial_sp = (1ull << ((addr_byte_size * 8) - 1));
+  RegisterValue cfa_reg_value;
+  cfa_reg_value.SetUInt(m_initial_sp, m_cfa_reg_info.byte_size);
+  SetRegisterValue(m_cfa_reg_info, cfa_reg_value);
+
+  const InstructionList &inst_list = disasm_sp->GetInstructionList();
+  const size_t num_instructions = inst_list.GetSize();
+
+  if (num_instructions > 0) {
+    Instruction *inst = inst_list.GetInstructionAtIndex(0).get();
+    const lldb::addr_t base_addr = inst->GetAddress().GetFileAddress();
+
+    // Map for storing the unwind plan row and the value of the registers at a
+    // given offset. When we see a forward branch we add a new entry to this map
+    // with the actual unwind plan row and register context for the target
+    // address of the branch as the current data have to be valid for the target
+    // address of the branch too if we are in the same function.
+    std::map<lldb::addr_t, std::pair<UnwindPlan::RowSP, RegisterValueMap>>
+        saved_unwind_states;
+
+    // Make a copy of the current instruction Row and save it in m_curr_row so
+    // we can add updates as we process the instructions.
+    UnwindPlan::RowSP last_row =
+        std::make_shared<UnwindPlan::Row>(*unwind_plan.GetLastRow());
+    m_curr_row = std::make_shared<UnwindPlan::Row>(*last_row);
+
+    // Add the initial state to the save list with offset 0.
+    saved_unwind_states.insert({0, {last_row, m_register_values}});
+
+    // cache the stack pointer register number (in whatever register numbering
+    // this UnwindPlan uses) for quick reference during instruction parsing.
+    RegisterInfo sp_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+        eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP);
+
+    // The architecture dependent condition code of the last processed
+    // instruction.
+    EmulateInstruction::InstructionCondition last_condition =
+        EmulateInstruction::UnconditionalCondition;
+    lldb::addr_t condition_block_start_offset = 0;
+
+    for (size_t idx = 0; idx < num_instructions; ++idx) {
+      m_curr_row_modified = false;
+      m_forward_branch_offset = 0;
+
+      inst = inst_list.GetInstructionAtIndex(idx).get();
+      if (inst) {
+        lldb::addr_t current_offset =
+            inst->GetAddress().GetFileAddress() - base_addr;
+        auto it = saved_unwind_states.upper_bound(current_offset);
+        assert(it != saved_unwind_states.begin() &&
+               "Unwind row for the function entry missing");
+        --it; // Move it to the row corresponding to the current offset
+
+        // If the offset of m_curr_row don't match with the offset we see in
+        // saved_unwind_states then we have to update m_curr_row and
+        // m_register_values based on the saved values. It is happening after we
+        // processed an epilogue and a return to caller instruction.
+        if (it->second.first->GetOffset() != m_curr_row->GetOffset()) {
+          UnwindPlan::Row *newrow = new UnwindPlan::Row;
+          *newrow = *it->second.first;
+          m_curr_row.reset(newrow);
+          m_register_values = it->second.second;
+          // re-set the CFA register ivars to match the new m_curr_row.
+          if (sp_reg_info.name &&
+              m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
+            uint32_t row_cfa_regnum =
+                m_curr_row->GetCFAValue().GetRegisterNumber();
+            lldb::RegisterKind row_kind = m_unwind_plan_ptr->GetRegisterKind();
+            // set m_cfa_reg_info to the row's CFA reg.
+            m_cfa_reg_info =
+                *m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum);
+            // set m_fp_is_cfa.
+            if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
+              m_fp_is_cfa = false;
+            else
+              m_fp_is_cfa = true;
+          }
+        }
 
-      const uint32_t addr_byte_size = m_arch.GetAddressByteSize();
-      const bool show_address = true;
-      const bool show_bytes = true;
-      const bool show_control_flow_kind = false;
-      m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
-          unwind_plan.GetRegisterKind(), unwind_plan.GetInitialCFARegister());
-      m_fp_is_cfa = false;
-      m_register_values.clear();
-      m_pushed_regs.clear();
-
-      // Initialize the CFA with a known value. In the 32 bit case it will be
-      // 0x80000000, and in the 64 bit case 0x8000000000000000. We use the
-      // address byte size to be safe for any future address sizes
-      m_initial_sp = (1ull << ((addr_byte_size * 8) - 1));
-      RegisterValue cfa_reg_value;
-      cfa_reg_value.SetUInt(m_initial_sp, m_cfa_reg_info.byte_size);
-      SetRegisterValue(m_cfa_reg_info, cfa_reg_value);
-
-      const InstructionList &inst_list = disasm_sp->GetInstructionList();
-      const size_t num_instructions = inst_list.GetSize();
-
-      if (num_instructions > 0) {
-        Instruction *inst = inst_list.GetInstructionAtIndex(0).get();
-        const lldb::addr_t base_addr = inst->GetAddress().GetFileAddress();
-
-        // Map for storing the unwind plan row and the value of the registers
-        // at a given offset. When we see a forward branch we add a new entry
-        // to this map with the actual unwind plan row and register context for
-        // the target address of the branch as the current data have to be
-        // valid for the target address of the branch too if we are in the same
-        // function.
-        std::map<lldb::addr_t, std::pair<UnwindPlan::RowSP, RegisterValueMap>>
-            saved_unwind_states;
-
-        // Make a copy of the current instruction Row and save it in m_curr_row
-        // so we can add updates as we process the instructions.
-        UnwindPlan::RowSP last_row =
-            std::make_shared<UnwindPlan::Row>(*unwind_plan.GetLastRow());
-        m_curr_row = std::make_shared<UnwindPlan::Row>(*last_row);
-
-        // Add the initial state to the save list with offset 0.
-        saved_unwind_states.insert({0, {last_row, m_register_values}});
-
-        // cache the stack pointer register number (in whatever register
-        // numbering this UnwindPlan uses) for quick reference during
-        // instruction parsing.
-        RegisterInfo sp_reg_info = *m_inst_emulator_up->GetRegisterInfo(
-            eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP);
-
-        // The architecture dependent condition code of the last processed
-        // instruction.
-        EmulateInstruction::InstructionCondition last_condition =
-            EmulateInstruction::UnconditionalCondition;
-        lldb::addr_t condition_block_start_offset = 0;
-
-        for (size_t idx = 0; idx < num_instructions; ++idx) {
-          m_curr_row_modified = false;
-          m_forward_branch_offset = 0;
-
-          inst = inst_list.GetInstructionAtIndex(idx).get();
-          if (inst) {
-            lldb::addr_t current_offset =
-                inst->GetAddress().GetFileAddress() - base_addr;
-            auto it = saved_unwind_states.upper_bound(current_offset);
-            assert(it != saved_unwind_states.begin() &&
-                   "Unwind row for the function entry missing");
-            --it; // Move it to the row corresponding to the current offset
-
-            // If the offset of m_curr_row don't match with the offset we see
-            // in saved_unwind_states then we have to update m_curr_row and
-            // m_register_values based on the saved values. It is happening
-            // after we processed an epilogue and a return to caller
-            // instruction.
-            if (it->second.first->GetOffset() != m_curr_row->GetOffset()) {
-              UnwindPlan::Row *newrow = new UnwindPlan::Row;
-              *newrow = *it->second.first;
-              m_curr_row.reset(newrow);
-              m_register_values = it->second.second;
-              // re-set the CFA register ivars to match the
-              // new m_curr_row.
-              if (sp_reg_info.name &&
-                  m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
-                uint32_t row_cfa_regnum =
-                    m_curr_row->GetCFAValue().GetRegisterNumber();
-                lldb::RegisterKind row_kind =
-                    m_unwind_plan_ptr->GetRegisterKind();
-                // set m_cfa_reg_info to the row's CFA reg.
-                m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
-                    row_kind, row_cfa_regnum);
-                // set m_fp_is_cfa.
-                if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
-                  m_fp_is_cfa = false;
-                else
-                  m_fp_is_cfa = true;
-              }
-            }
+        m_inst_emulator_up->SetInstruction(inst->GetOpcode(),
+                                           inst->GetAddress(), nullptr);
+
+        if (last_condition != m_inst_emulator_up->GetInstructionCondition()) {
+          if (m_inst_emulator_up->GetInstructionCondition() !=
+                  EmulateInstruction::UnconditionalCondition &&
+              saved_unwind_states.count(current_offset) == 0) {
+            // If we don't have a saved row for the current offset then save our
+            // current state because we will have to restore it after the
+            // conditional block.
+            auto new_row = std::make_shared<UnwindPlan::Row>(*m_curr_row.get());
+            saved_unwind_states.insert(
+                {current_offset, {new_row, m_register_values}});
+          }
 
-            m_inst_emulator_up->SetInstruction(inst->GetOpcode(),
-                                               inst->GetAddress(), nullptr);
-
-            if (last_condition !=
-                m_inst_emulator_up->GetInstructionCondition()) {
-              if (m_inst_emulator_up->GetInstructionCondition() !=
-                      EmulateInstruction::UnconditionalCondition &&
-                  saved_unwind_states.count(current_offset) == 0) {
-                // If we don't have a saved row for the current offset then
-                // save our current state because we will have to restore it
-                // after the conditional block.
-                auto new_row =
-                    std::make_shared<UnwindPlan::Row>(*m_curr_row.get());
-                saved_unwind_states.insert(
-                    {current_offset, {new_row, m_register_values}});
-              }
-
-              // If the last instruction was conditional with a different
-              // condition then the then current condition then restore the
-              // condition.
-              if (last_condition !=
-                  EmulateInstruction::UnconditionalCondition) {
-                const auto &saved_state =
-                    saved_unwind_states.at(condition_block_start_offset);
-                m_curr_row =
-                    std::make_shared<UnwindPlan::Row>(*saved_state.first);
-                m_curr_row->SetOffset(current_offset);
-                m_register_values = saved_state.second;
-                // re-set the CFA register ivars to match the
-                // new m_curr_row.
-                if (sp_reg_info.name &&
-                    m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
-                  uint32_t row_cfa_regnum =
-                      m_curr_row->GetCFAValue().GetRegisterNumber();
-                  lldb::RegisterKind row_kind =
-                      m_unwind_plan_ptr->GetRegisterKind();
-                  // set m_cfa_reg_info to the row's CFA reg.
-                  m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
-                      row_kind, row_cfa_regnum);
-                  // set m_fp_is_cfa.
-                  if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
-                    m_fp_is_cfa = false;
-                  else
-                    m_fp_is_cfa = true;
-                }
-                bool replace_existing =
-                    true; // The last instruction might already
-                          // created a row for this offset and
-                          // we want to overwrite it.
-                unwind_plan.InsertRow(
-                    std::make_shared<UnwindPlan::Row>(*m_curr_row),
-                    replace_existing);
-              }
-
-              // We are starting a new conditional block at the actual offset
-              condition_block_start_offset = current_offset;
+          // If the last instruction was conditional with a different condition
+          // then the then current condition then restore the condition.
+          if (last_condition != EmulateInstruction::UnconditionalCondition) {
+            const auto &saved_state =
+                saved_unwind_states.at(condition_block_start_offset);
+            m_curr_row = std::make_shared<UnwindPlan::Row>(*saved_state.first);
+            m_curr_row->SetOffset(current_offset);
+            m_register_values = saved_state.second;
+            // re-set the CFA register ivars to match the new m_curr_row.
+            if (sp_reg_info.name &&
+                m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
+              uint32_t row_cfa_regnum =
+                  m_curr_row->GetCFAValue().GetRegisterNumber();
+              lldb::RegisterKind row_kind =
+                  m_unwind_plan_ptr->GetRegisterKind();
+              // set m_cfa_reg_info to the row's CFA reg.
+              m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+                  row_kind, row_cfa_regnum);
+              // set m_fp_is_cfa.
+              if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
+                m_fp_is_cfa = false;
+              else
+                m_fp_is_cfa = true;
             }
+            // The last instruction might already created a row for this offset
+            // and we want to overwrite it.
+            bool replace_existing = true;
+            unwind_plan.InsertRow(
+                std::make_shared<UnwindPlan::Row>(*m_curr_row),
+                replace_existing);
+          }
 
-            if (log && log->GetVerbose()) {
-              StreamString strm;
-              lldb_private::FormatEntity::Entry format;
-              FormatEntity::Parse("${frame.pc}: ", format);
-              inst->Dump(&strm, inst_list.GetMaxOpcocdeByteSize(), show_address,
-                         show_bytes, show_control_flow_kind, nullptr, nullptr,
-                         nullptr, &format, 0);
-              log->PutString(strm.GetString());
-            }
+          // We are starting a new conditional block at the actual offset
+          condition_block_start_offset = current_offset;
+        }
 
-            last_condition = m_inst_emulator_up->GetInstructionCondition();
-
-            m_inst_emulator_up->EvaluateInstruction(
-                eEmulateInstructionOptionIgnoreConditions);
-
-            // If the current instruction is a branch forward then save the
-            // current CFI information for the offset where we are branching.
-            if (m_forward_branch_offset != 0 &&
-                range.ContainsFileAddress(inst->GetAddress().GetFileAddress() +
-                                          m_forward_branch_offset)) {
-              auto newrow =
-                  std::make_shared<UnwindPlan::Row>(*m_curr_row.get());
-              newrow->SetOffset(current_offset + m_forward_branch_offset);
-              saved_unwind_states.insert(
-                  {current_offset + m_forward_branch_offset,
-                   {newrow, m_register_values}});
-              unwind_plan.InsertRow(newrow);
-            }
+        if (log && log->GetVerbose()) {
+          StreamString strm;
+          lldb_private::FormatEntity::Entry format;
+          FormatEntity::Parse("${frame.pc}: ", format);
+          inst->Dump(&strm, inst_list.GetMaxOpcocdeByteSize(), show_address,
+                     show_bytes, show_control_flow_kind, nullptr, nullptr,
+                     nullptr, &format, 0);
+          log->PutString(strm.GetString());
+        }
 
-            // Were there any changes to the CFI while evaluating this
-            // instruction?
-            if (m_curr_row_modified) {
-              // Save the modified row if we don't already have a CFI row in
-              // the current address
-              if (saved_unwind_states.count(
-                      current_offset + inst->GetOpcode().GetByteSize()) == 0) {
-                m_curr_row->SetOffset(current_offset +
-                                      inst->GetOpcode().GetByteSize());
-                unwind_plan.InsertRow(m_curr_row);
-                saved_unwind_states.insert(
-                    {current_offset + inst->GetOpcode().GetByteSize(),
-                     {m_curr_row, m_register_values}});
-
-                // Allocate a new Row for m_curr_row, copy the current state
-                // into it
-                UnwindPlan::Row *newrow = new UnwindPlan::Row;
-                *newrow = *m_curr_row.get();
-                m_curr_row.reset(newrow);
-              }
-            }
+        last_condition = m_inst_emulator_up->GetInstructionCondition();
+
+        m_inst_emulator_up->EvaluateInstruction(
+            eEmulateInstructionOptionIgnoreConditions);
+
+        // If the current instruction is a branch forward then save the current
+        // CFI information for the offset where we are branching.
+        if (m_forward_branch_offset != 0 &&
+            range.ContainsFileAddress(inst->GetAddress().GetFileAddress() +
+                                      m_forward_branch_offset)) {
+          auto newrow = std::make_...
[truncated]

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⬅️

@labath labath merged commit 30b021f into llvm:main Feb 26, 2025
10 checks passed
@labath labath deleted the inst branch February 26, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants