Skip to content

[lldb] Fix HeapCFA comparison to be symmetric #10127

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 28, 2025
Merged

[lldb] Fix HeapCFA comparison to be symmetric #10127

merged 2 commits into from
Feb 28, 2025

Conversation

felipepiovezan
Copy link 

A swift async frame A is younger than frame B if the async ctx of B can
be reached from the async ctx of A. This is implemented today.

But we fail to check the opposite:
A swift async frame A is _older_ than frame B if the async ctx of A can
be reached from the async ctx of B.

Because we lacked the opposite check, we were falling back to "vanilla"
CFA comparison, and were just being lucky that the vast majority of
cases are only interested in the "Younger" case.

@felipepiovezan felipepiovezan requested a review from a team as a code owner February 27, 2025 17:38
@felipepiovezan
Copy link
Author

@swift-ci test

felipepiovezan
felipepiovezan commented Feb 27, 2025
View reviewed changes
lldb/source/Target/StackID.cpp
@@ -76,11 +76,34 @@ bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
}

// BEGIN SWIFT
Copy link
Author

Choose a reason for hiding this comment

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

code below is just copied from the old location

kastiglione
kastiglione requested changes Feb 27, 2025
View reviewed changes
lldb/source/Target/StackID.cpp Outdated
// to check if `destination` can be reached from `source`. The search stops when
// it hits the end of the chain (parent_ctx == 0) or a safety limit in case of
// an invalid continuation chain.
static llvm::Expected<bool> ChaseContinuationsToFind(lldb::addr_t source,
Copy link

Choose a reason for hiding this comment

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

IsReachableParent or IsReachableParentContext?

lldb/source/Target/StackID.cpp Outdated
parent_ctx = process.ReadPointerFromMemory(old_parent_ctx, error);
if (error.Fail())
return llvm::createStringError(llvm::formatv(
"Failed to read parent async context of: {0:x}", old_parent_ctx));
Copy link

Choose a reason for hiding this comment

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

should this include the error's message?

lldb/source/Target/StackID.cpp Outdated
llvm::Expected<bool> lhs_younger =
ChaseContinuationsToFind(lhs_cfa, rhs_cfa, process);
if (auto E = lhs_younger.takeError())
LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "");
Copy link

Choose a reason for hiding this comment

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

Suggested change
LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "");
LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "{0}");

Copy link
Author

Choose a reason for hiding this comment

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

Ohhh nice catch

lldb/source/Target/StackID.cpp Outdated
llvm::Expected<bool> lhs_older =
ChaseContinuationsToFind(rhs_cfa, lhs_cfa, process);
if (auto E = lhs_older.takeError())
LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "");
Copy link

Choose a reason for hiding this comment

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

Suggested change
LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "");
LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "{0}");

lldb/source/Target/StackID.cpp
if (parent_ctx == destination)
return true;
}
return false;
Copy link

Choose a reason for hiding this comment

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

false is correct for the case where a null parent is reached. What about when the 512 limit is reached? Should this return an error? If not, should there be a log? As it is, there's no way of knowing the limit was reached.

Copy link
Author

Choose a reason for hiding this comment

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

I can do that, though I was hoping to just move the code as this is an NFC patch

Copy link
Author

Choose a reason for hiding this comment

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

Tried it just now, it does look much better indeed!

@felipepiovezan felipepiovezan force-pushed the felipe/fix_heap_cfa branch 2 times, most recently from b022ee1 to c8f82c0 Compare February 27, 2025 21:28
@felipepiovezan
[lldb][nfc] Factor out code comparing Heap CFAs
34622b4 
This will be re-used in a subsequent commit.
@felipepiovezan
[lldb] Fix HeapCFA comparison to be symmetric
1b3b686 
A swift async frame A is younger than frame B if the async ctx of B can
be reached from the async ctx of A. This is implemented today.

But we fail to check the opposite:
A swift async frame A is _older_ than frame B if the async ctx of A can
be reached from the async ctx of B.

Because we lacked the opposite check, we were falling back to "vanilla"
CFA comparison, and were just being lucky that the vast majority of
cases are only interested in the "Younger" case.
@felipepiovezan
Copy link
Author

Addressed feedback

@felipepiovezan
Copy link
Author

@swift-ci test

@felipepiovezan felipepiovezan merged commit 6e0f052 into swiftlang:stable/20240723 Feb 28, 2025
3 checks passed
@felipepiovezan felipepiovezan deleted the felipe/fix_heap_cfa branch February 28, 2025 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kastiglione kastiglione kastiglione approved these changes

@adrian-prantl adrian-prantl adrian-prantl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felipepiovezan @kastiglione @adrian-prantl