[lldb][swift] Evaluate entry_value(async_reg) in terms of CFA #9403
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
felipepiovezan
commented
Oct 9, 2024
| @@ -37,6 +37,27 @@ def check_pcs(self, async_frames, process, target): | |||
| prologue_to_skip = parent_frame.GetFunction().GetPrologueByteSize() | |||
| self.assertEqual(continuation_ptr + prologue_to_skip, parent_frame.GetPC()) | |||
|
|
|||
| def check_async_regs_one_frame(self, frame, process): | |||
| is_x86 = 'x86' in process.GetTarget().GetTriple() | |||
There was a problem hiding this comment.
Jason suggested some variant of if self.getArchitecture() in ["x86_64"]:
There was a problem hiding this comment.
could the architecture ever return x86_64h?
There was a problem hiding this comment.
I honestly did not know this was a thing!
There was a problem hiding this comment.
I've skipped the test on non arm64, arm64e, x86_x64 archs (as done in other swift tests) and used Jason's suggestion
felipepiovezan
force-pushed
the
felipe/entry_val_as_cfa
branch
from
f5b1cf3 to
6d68345
Compare
|
(I had forgotten to include an NFC commit on the test) |
| return false; | ||
|
|
||
| return (triple == llvm::Triple::x86_64 && op_async_ctx_reg == DW_OP_reg14) || | ||
| (triple == llvm::Triple::aarch64 && op_async_ctx_reg == DW_OP_reg22); |
There was a problem hiding this comment.
We should at least throw in a FIXME here: I feel like this should be a method of the ABI plugin. No idea how to get from here to the ABI of the Process.
| const uint32_t subexpr_len = opcodes.GetULEB128(&new_offset); | ||
| const void *subexpr_data = opcodes.GetData(&new_offset, subexpr_len); | ||
| if (!subexpr_data) | ||
| return {}; |
There was a problem hiding this comment.
Should this function return expected and these should all bit StringErrors?
| return {}; | ||
| } | ||
|
|
||
| static const uint8_t cfa_opcode = DW_OP_call_frame_cfa; |
felipepiovezan
force-pushed
the
felipe/entry_val_as_cfa
branch
from
6d68345 to
83ac4c5
Compare
felipepiovezan
force-pushed
the
felipe/entry_val_as_cfa
branch
2 times, most recently
from
13673d4 to
604af0f
Compare
| @@ -532,6 +532,73 @@ bool DWARFExpression::LinkThreadLocalStorage( | |||
| return true; | |||
| } | |||
|
|
|||
| /// Returns true if `opdcodes` contains the opcode for the register used for the | |||
There was a problem hiding this comment.
Suggested change
| /// Returns true if `opdcodes` contains the opcode for the register used for the | |
| /// Returns true if \c opcodes contains the opcode for the register used for the |
| (triple == llvm::Triple::aarch64 && op_async_ctx_reg == DW_OP_reg22); | ||
| } | ||
|
|
||
| /// If `opcodes` contain the location of asynchronous contexts in Swift, |
There was a problem hiding this comment.
Suggested change
| /// If `opcodes` contain the location of asynchronous contexts in Swift, | |
| /// If \c opcodes contain the location of asynchronous contexts in Swift, |
| /// evaluates DW_OP_call_frame_cfa and returns its result. Otherwise, does | ||
| /// nothing. This is possible because, for async frames, the language unwinder | ||
| /// treats the asynchronous context as the CFA of the frame. | ||
| static llvm::Expected<Value> SwiftAsyncEvaluate_DW_OP_entry_value( |
There was a problem hiding this comment.
Idea: In the future, languages could be able to register a DWARF expression plugin that matches sequences like this and returns a value?
There was a problem hiding this comment.
Yup, that could be an interesting idea, though I wonder how many languages would use this...
adrian-prantl
approved these changes
Oct 14, 2024
Prior to this commit, evaluating the dwarf expression `entry_value(async_reg)` is done by finding the value of the asynchronous register in the parent frame. To enable the above, the unwinder must pretend there is a real function call between the parent frame and the current frame, and that the async register is set by the parent frame prior to making the 'call'. None of this is actually true, and it creates a lot of work for the unwinder (see the amount of code deleted there). Here is further evidence of how awkward this is. Suppose you have this call stack: ``` A <--- younger frame, top of the stack B C <--- older frame, bottom of the stack ``` When the unwinder is creating the frame of C from the register state of B, it must know whether A was an indirect (Q) funclet or not, because that determined how the frame of B was produced from the register state of A. This is very unusual, in fact, the unwinder doesn't even have access to such information (we had to use a "dummy" register for this). This patch changes how `entry_value(async_reg)` (or `entry_value(async_reg),deref` for Q_funclets) is evaluated: this expression is equivalent to the CFA (the async context) of the current frame. Since we no longer need to peek at the parent frame, the unwinder no longer needs to perform the work described previously. The unwinder can instead provide the continuation funclet with the register contents they will _actually_ have when the funclet runs. This patch also addresses a more subtle issue. In Q funclets, after a certain instruction, `entry_value(async_reg)` produces a pointer to memory that has been freed, as Q funclets free the async context of funclet that just finished executing. If the debugger attempts to evaluate `entry_value(async_reg), deref` as two separate operations, it will be accessing freed heap memory. By converting that operation sequence into `DW_OP_call_frame_cfa`, we bypass the issue.
felipepiovezan
force-pushed
the
felipe/entry_val_as_cfa
branch
from
604af0f to
bbc1484
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this commit, evaluating the dwarf expression
entry_value(async_reg)is done by finding the value of the asynchronous register in the parent frame.To enable the above, the unwinder must pretend there is a real function call between the parent frame and the current frame, and that the async register is set by the parent frame prior to making the 'call'. None of this is actually true, and it creates a lot of work for the unwinder (see the amount of code deleted there).
Here is further evidence of how awkward this is. Suppose you have this call stack:
When the unwinder is creating the frame of C from the register state of B, it must know whether A was an indirect (Q) funclet or not, because that determined how the frame of B was produced from the register state of A. This is very unusual, in fact, the unwinder doesn't even have access to such information (we had to use a "dummy" register for this).
This patch changes how
entry_value(async_reg)(orentry_value(async_reg),dereffor Q_funclets) is evaluated: this expression is equivalent to the CFA (the async context) of the current frame. Since we no longer need to peek at the parent frame, the unwinder no longer needs to perform the work described previously. The unwinder can instead provide the continuation funclet with the register contents they will actually have when the funclet runs.This patch also addresses a more subtle issue. In Q funclets, after a certain instruction,
entry_value(async_reg)produces a pointer to memory that has been freed, as Q funclets free the async context of funclet that just finished executing. If the debugger attempts to evaluateentry_value(async_reg), derefas two separate operations, it will be accessing freed heap memory. By converting that operation sequence intoDW_OP_call_frame_cfa, we bypass the issue.