Skip to content

[lldb] Extract logic out of GetFunctionDisplayName into a helper function #10766

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

Conversation

charles-zablit
Copy link

This patch splits the logic inside SwiftLanguage::GetFunctionDisplayName into reusable helper functions.

  • SwiftLanguage::GetFunctionName returns the name of the function as a string.
  • SwiftLanguage::GetFunctionDisplayArgs returns the generics and arguments of the function as a string.

This will be needed for #10710

@charles-zablit charles-zablit requested a review from a team as a code owner May 30, 2025 11:13
@charles-zablit
Copy link
Author

I'm not sure about the changes to the Language::FunctionNameRepresentation::eNameWithNoArgs case. It seems like it should just print the name of the function without its parameters given the name of the enum entry. The patch respects that and will only print the name of the function.

However, this is not what's happening prior to the patch, as the parameters are printed as well.

This does not seem to break any tests in check-lldb but it's probably not tested at all.

if (open_paren && generic && generic < open_paren)
return std::string(cstr, generic);
if (open_paren)
return std::string(cstr, open_paren);

Choose a reason for hiding this comment

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

Hmmm why is this new logic needed?

Choose a reason for hiding this comment

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

We didn't previously do this in the NoArgs case right?

Copy link
Author

@charles-zablit charles-zablit Jun 3, 2025

Choose a reason for hiding this comment

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

I am a bit confused by the original implementation of the NoArgs case: if the frame is foo(a: 1) I would expect the NoArgs case to return foo and nothing else. But in the original implementation, it returns foo(a: Int).

This additional logic is due to me splitting the the logic into 2 helper functions. Say we have foo<T>(a: T). GetFunctionName will return foo and GetFunctionDisplayArgs will return <Int>(a=1).

Hence: no, we didn't previously do this in the NoArgs case, but I think this was a mistake. Since there are no tests for the way backtraces are printed in the Swift plugin, I have no idea how much of a breaking change this is.

Choose a reason for hiding this comment

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

Oh I see. In that case, if eNoArgs was always broken, lets just remove that case entirely? That'll mean less code to change in the other PR (and we won't be breaking any users)

Copy link
Author

Choose a reason for hiding this comment

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

I reverted the changes that were in the eNoArgs case so that the PR does not break existing users. Is that what you meant?

Choose a reason for hiding this comment

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

I meant we should be able to remove the case entirely. If it never worked, it won't break any users 🤷

Copy link
Author

Choose a reason for hiding this comment

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

Is it possible that a client is using this case incorrectly? They would not get any string from this function if that's the case.

Choose a reason for hiding this comment

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

And lets just have 1 helper function that factors out only the stuff in the eNameWithArgs case. Cause that's what you intended to do in #10710 anyway

Choose a reason for hiding this comment

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

Is it possible that a client is using this case incorrectly? They would not get any string from this function if that's the case.

You're saying, if we removed the case, then a client would just not get any string if eFunctionNameNoArgs was used? Yea if that's the case lets just leave it alone for now

Copy link
Author

Choose a reason for hiding this comment

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

And lets just have 1 helper function that factors out only the stuff in the eNameWithArgs case. Cause that's what you intended to do in #10710 anyway

I think it's better to keep them as 2 separate functions. My end goal here is to have a function which takes a function's SymbolContext and returns the arguments parts of the demangled name, i.e (a=1, b=2), without the name.

The initial implementation was tightly coupled with the part that gets the name, meaning you would have to pass in foo(a: Int, b: Int) to GetFunctionDisplayArgs in order to get the desired output.

I think that splitting it into 2 separate components makes it easier to work with.

@Michael137
Copy link

@swift-ci test

@charles-zablit charles-zablit force-pushed the charles-zablit/lldb/extract-logic-out-of-GetFunctionDisplayName branch from 66d4cc7 to 0e9262d Compare June 10, 2025 14:14
@charles-zablit
Copy link
Author

@swift-ci please test

return true;

s << display_name;
if (inline_info) {

Choose a reason for hiding this comment

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

This can be removed now

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks

@charles-zablit charles-zablit force-pushed the charles-zablit/lldb/extract-logic-out-of-GetFunctionDisplayName branch from 0e9262d to 3ad0a46 Compare June 10, 2025 14:59
@Michael137
Copy link

@swift-ci test

@adrian-prantl adrian-prantl merged commit 87ed310 into swift/release/6.2 Jun 11, 2025
3 checks passed
@adrian-prantl adrian-prantl deleted the charles-zablit/lldb/extract-logic-out-of-GetFunctionDisplayName branch June 11, 2025 20:06
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