Skip to content

Conversation

pveras
Copy link

@pveras pveras commented Sep 23, 2025

Specially when dealing with different address spaces, CFAs could start from addresses like 0. ABIs could already specify what they consider to be valid CFA values but this was never used in these parts of the unwinder code. Note that this COULD be a breaking change for ABISysV_arc and ABISysV_riscv since these are the only two ABIs that don't explicitly disallow 0 values.

@walter-erquinigo
Copy link
Collaborator

Do you think you can upstream this change?

@pveras
Copy link
Author

pveras commented Sep 25, 2025

I can try. Should I just submit a PR upstream? I'll check if upstream has any additional requirements

@walter-erquinigo
Copy link
Collaborator

Yeah, resubmit the PR upstream. They'll likely ask for a unit test, but we should have that upstream conversation first.
I don't want to wait until too late to upstream unwinder changes because unwinding is one of the few areas where we are modifying existing code instead of just adding new functionality.

Comment on lines 454 to 456
if (m_cfa == LLDB_INVALID_ADDRESS ||
(abi_sp && !abi_sp->CallFrameAddressIsValid(m_cfa)) ||
(!abi_sp && (m_cfa == 0 || m_cfa == 1))) {
Copy link
Owner

@clayborg clayborg Sep 25, 2025

Choose a reason for hiding this comment

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

This logic should go into a function as this code is duplicated below many times. Just create a static function in this file:

static bool CFAIsValid(ABISP abi_sp, lldb::addr_t cfa) {
  if (cfa == LLDB_INVALID_ADDRESS)
    return false;
  if (abi_sp)
    return abi_sp->CallFrameAddressIsValid(cfa)
  else
    return m_cfa != 0 && m_cfa != 1;
}

@pveras pveras changed the title Defer to ABI plugin to check if call frame addresses are valid Delegate to ABI plugin to check if call frame addresses are valid Sep 30, 2025
@pveras
Copy link
Author

pveras commented Sep 30, 2025

Addressed comments and created an upstream pull request here: llvm#161398 but will keep this PR around for reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants