Skip to content

[lldb] Refactor Module::LookupInfo constructor #3240

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
Open
wants to merge 2 commits into
base: stable/20210726
Choose a base branch
from
Open

[lldb] Refactor Module::LookupInfo constructor #3240

wants to merge 2 commits into from

Conversation

bulbazord
Copy link

@bulbazord bulbazord commented Sep 9, 2021 •
edited
Loading

A few weeks ago I wrote a patch refactoring Module::LookupInfo's constructor 1. I had to revert it because the patch had some issues but now I am ready to re-land a revised version of it to llvm.org. In addition to that patch, I have another patch that should preserve swift functionality here.

Please let me know if I should change something (code, different branch, etc). I don't think I have the ability to trigger tests anymore, so if somebody could do that for me I would greatly appreciate it.

cc @adrian-prantl

@bulbazord
Re-land "[lldb] Refactor Module::LookupInfo constructor"
d64b243 
Module::LookupInfo's constructor currently goes over supported languages
trying to figure out the best way to search for a symbol name. This
seems like a great candidate for refactoring. Specifically, this is work
that can be delegated to language plugins.

Once again, the goal here is to further decouple plugins from
non-plugins. The idea is to have each language plugin take a name and
give you back some information about the name from the perspective of
the language. Specifically, each language now implements a
`GetFunctionNameInfo` method which returns an object of type
`Language::FunctionNameInfo`. Right now, it consists of a basename
and a FunctionNameType. Module::LookupInfo's constructor will
call `GetFunctionNameInfo` with the appropriate language plugin(s) and
then decide what to do with that information. I have attempted to maintain
existing behavior as best as possible.

A nice side effect of this change is that lldbCore no longer links
against the ObjC Language plugin.

Differential Revision: https://reviews.llvm.org/D108229
@bulbazord
[lldb] Implement SwiftLanguage::GetFunctionNameInfo
9e613d3
@adrian-prantl
Copy link

Thank you! You will need to also land this on the next branch.

@gmittert
Copy link

@swift-ci test

@bulbazord
Copy link
Author

The builds completed but I can't tell if my patch introduced the build failures unfortunately. I'm having a hard time reproducing the failures on my local machine. Any insight here @adrian-prantl?

I'll open a PR against next when I have more confidence that this is ready.

@bulbazord
Copy link
Author

@swift-ci test

1 similar comment
@bulbazord
Copy link
Author

@swift-ci test

@bulbazord
Copy link
Author

@swift-ci please test

@bulbazord
Copy link
Author

I've had the chance to actually run the tests on my local machine now and my patch(es) cause 2 tests to fail for sure:

Failed Tests (2):
  lldb-api :: functionalities/breakpoint/objc/TestObjCBreakpoints.py
  lldb-api :: functionalities/tail_call_frames/sbapi_support/TestTailCallFrameSBAPI.py

When debugging exactly what caused the failures, I found some interesting behavior in SwiftLanguageRuntime::MethodName::Parse(). Specifically, as it parses a name, there are some codepaths that cause it to think some names are valid swift method names. For example, in debugging TestObjCBreakpoints, it will incorrectly think that -[MyClass myCategoryFunction] is a basename of a swift method (which is why the test fails). TestTailCallFrameSBAPI fails for a similar reason: _Z5func3v is considered a swift method basename.

@adrian-prantl I believe my patch to implement SwiftLanguage::GetFunctionNameInfo is probably correct but it will require some adjustments to SwiftLanguageRuntime::MethodName::Parse.

@bulbazord bulbazord mentioned this pull request Mar 25, 2025
Merged
slydiman added a commit to slydiman/llvm-project that referenced this pull request Apr 3, 2025
@slydiman
[LLDB] Refactor Module::LookupInfo constructor and move out CPlusPlus…
ba94c68 
…Language::MethodName to break lldb-server dependencies

This patch addresses the issue llvm#129543.
After this patch the size of lldb-server is reduced by 9MB.

Based on swiftlang#3240 by @bulbazord Alex Langford
slydiman added a commit to slydiman/llvm-project that referenced this pull request Apr 3, 2025
@slydiman
[LLDB] Refactor Module::LookupInfo constructor and move out CPlusPlus…
a4d62ca 
…Language::MethodName to break lldb-server dependencies

This patch addresses the issue llvm#129543.
After this patch the size of lldb-server is reduced by 9MB.

Based on swiftlang#3240 by @bulbazord Alex Langford
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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
@bulbazord @adrian-prantl @gmittert