[lldb][Swift] Small steps towards fixing StepOut in async functions #9138

felipepiovezan · 2024-08-16T23:07:35Z

This PR adds tests demonstrating where the debugger fails, as well as exposing swift customizations that are not properly tagged.

The two final fixes we need are listed below. I have not included them in this PR because I am missing unittests for the StackID changes in them and because this PR stands on its own regardless of whether the fixes below are considered appropriate or not.

This work builds on top the previously merged PRs improving the swift debugging story:

#9112
#9107
swiftlang/swift#75881
#9025

kastiglione · 2024-08-16T23:26:32Z

This commit message has a mistake:
felipepiovezan@a9fe88e

For two StackIDs whose CFAs are on the stack, i.e., they are both async frames,

should be "whose CFAs are on the heap,"

felipepiovezan · 2024-08-16T23:28:51Z

This commit message has a mistake: felipepiovezan@a9fe88e

For two StackIDs whose CFAs are on the stack, i.e., they are both async frames,

should be "whose CFAs are on the heap,"

Oops, good catch! I'll update it before posting the real PR

kastiglione · 2024-08-16T23:30:42Z

lldb/source/Target/StackID.cpp

+  if (lhs_cfa_on_stack && rhs_cfa_on_stack)
+    return {};


this early exit is redundant to the final return {}.

also, this function could return bool, since it currently only ever returns two values: true or nullopt. But if it can only return two values, shouldn't it be true/false?

It is true that this function only returns two values (on this patch, subsequent patches will change this though).

However, nullopt here really means: "the question isYoungerHeapCFAs is not applicable". So returning "false" would give the false impression that "isYoungerHeapCFAs" is returning "it's not younger".

The call site needs a way of knowing that it needs to try other comparisons, which is what nullopt represents here.

subsequent patches will change this though

imo that's where this change should happen

I also think this would be best served by an enum, which can be explicit about the case meanings. I think optional<bool> should be avoided in any language.

another option: instead of mixing two predicates into one, have one predicate which checks if a CFA is heap, and another that checks if one heap CFAs is younger than another heap CFA.

one predicate which checks if a CFA is heap, and another that checks if one heap CFAs is younger than another heap CFA.

Are you suggesting something like this?

if (are both heap cfas(cfa1, cfa2)) return compare heap cfas (cfa1, cfa2)

The problem with that is that we still need to handle the case where one of them is on the heap and the other one is not. We could do:

if (at least one is heap cfaa(cfa1, cfa2)) return compare heap cfas (cfa1, cfa2)

But then the "compare heap cfas" function would need to repeat the check to figure out which one is on the heap.

The other problem with this approach is that there will be another third case where we "don't have an opinion", which is when we were chasing continuation pointers and decided to give up because we reached some upper limit on the number of tries.

I'll try the enumeration approach if you believe that makes sense, though IMO the optional<bool> is quite expressive here

Changed in the newest version

kastiglione · 2024-08-16T23:34:24Z

lldb/test/API/lang/swift/async/stepping/step_out/TestSteppingOutAsync.py

+
+    @swiftTest
+    @skipIf(oslist=["windows", "linux"])
+    @skipIf("rdar://133849022")


Given the PR description:

This PR adds tests demonstrating where the debugger fails

should this be an expectedFailure?

I was debating this but we would risk having sporadic xpasses: if the stars align and the "heap cfas" numeric values are such that certain StackIDs comparisons behave in some pattern, the test could pass.

felipepiovezan · 2024-08-19T19:55:38Z

@adrian-prantl have you had a chance to look at this?
@kastiglione any other comments?

kastiglione · 2024-08-20T17:34:40Z

lldb/test/API/lang/swift/async/stepping/step_out/TestSteppingOutAsync.py

+        # Expected frames:
+        # ASYNC___0___ <- ASYNC___1___ <- ASYNC___2___ <- ASYNC___3___ <- Main
+        num_frames = 5
+        func_names = list(map(lambda frame: frame.GetFunctionName(), frames[:num_frames]))


a more pythonic formation:

Suggested change

func_names = list(map(lambda frame: frame.GetFunctionName(), frames[:num_frames]))

func_names = [frame.GetFunctionName() for frame in frames[:num_frames]]

kastiglione · 2024-08-20T17:35:52Z

lldb/test/API/lang/swift/async/stepping/step_out/TestSteppingOutAsync.py

+        func_names = list(map(lambda frame: frame.GetFunctionName(), frames[:num_frames]))
+        for idx in range(num_frames - 1):
+            self.assertRegex(func_names[idx], f"ASYNC___{idx}___")
+        self.assertRegex(func_names[num_frames - 1], "Main")


these assertRegex can be assertIn since there's no regular expressions in play.

Good point!

felipepiovezan · 2024-08-20T18:21:26Z

Addressed review comments

felipepiovezan · 2024-08-20T20:08:45Z

Changed "IsYounger" function to return an enumeration

kastiglione · 2024-08-20T21:34:47Z

lldb/source/Target/StackID.cpp

-  if (lhs.IsCFAOnStack(process) && !rhs.IsCFAOnStack(process))
+  if (lhs_cfa_on_stack && !rhs_cfa_on_stack)
+    return HeapCFAComparisonResult::Younger;
+  return {};


should this be NoOpinion?

(I'm not sure what {} evaluates to for an enum, but I presume the first case)

kastiglione · 2024-08-20T21:35:51Z

lldb/test/API/lang/swift/async/stepping/step_out/TestSteppingOutAsync.py

+        num_frames = 5
+        func_names = [frame.GetFunctionName() for frame in frames[:num_frames]]
+        for idx in range(num_frames - 1):
+            self.assertRegex(func_names[idx], f"ASYNC___{idx}___")


this can also be assertIn if you want

…rison The code moved in this patch is a swift customization, even though it is missing the customization markers. This simple refactor should also make it clear that there is a bug in this function: it is not symmetric.

While this could, theoretically, be sent upstream, we have unfortunately renamed some of the core APIs downstream ("IsYounger" doesn't exist upstream, nor anything "heap cfa" related).

felipepiovezan · 2024-08-20T21:51:33Z

Update test and address error in the previous update

felipepiovezan requested review from kastiglione and adrian-prantl August 16, 2024 23:07

felipepiovezan requested a review from JDevlieghere as a code owner August 16, 2024 23:07

felipepiovezan changed the title ~~[lldb] Small steps towards fixing StepOut in async functions~~ [lldb][Swift] Small steps towards fixing StepOut in async functions Aug 16, 2024

kastiglione reviewed Aug 16, 2024

View reviewed changes

adrian-prantl approved these changes Aug 20, 2024

View reviewed changes

kastiglione reviewed Aug 20, 2024

View reviewed changes

felipepiovezan force-pushed the felipe/step_out_preliminaries branch from da5facc to 52f2fbc Compare August 20, 2024 18:21

felipepiovezan force-pushed the felipe/step_out_preliminaries branch from 52f2fbc to f44c46e Compare August 20, 2024 20:07

kastiglione approved these changes Aug 20, 2024

View reviewed changes

felipepiovezan added 3 commits August 20, 2024 14:51

[lldb][swift] Add test demonstrating how step out fails

d282f3f

[lldb] Add unittest for StackID comparisons

874b20b

While this could, theoretically, be sent upstream, we have unfortunately renamed some of the core APIs downstream ("IsYounger" doesn't exist upstream, nor anything "heap cfa" related).

felipepiovezan force-pushed the felipe/step_out_preliminaries branch from f44c46e to 874b20b Compare August 20, 2024 21:51

felipepiovezan merged commit 53abe0d into swiftlang:stable/20240723 Aug 20, 2024

felipepiovezan deleted the felipe/step_out_preliminaries branch August 20, 2024 22:04

	func_names = list(map(lambda frame: frame.GetFunctionName(), frames[:num_frames]))
	func_names = [frame.GetFunctionName() for frame in frames[:num_frames]]

[lldb][Swift] Small steps towards fixing StepOut in async functions #9138

[lldb][Swift] Small steps towards fixing StepOut in async functions #9138

Uh oh!

Conversation

felipepiovezan commented Aug 16, 2024

Uh oh!

kastiglione commented Aug 16, 2024

Uh oh!

felipepiovezan commented Aug 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Aug 20, 2024

Uh oh!

felipepiovezan commented Aug 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Aug 20, 2024

Uh oh!

Uh oh!

kastiglione Aug 20, 2024 •

edited

Loading