[lldb][Format] Display only the inlined Swift frame name in backtraces if available #10786
Conversation
lldb/source/Core/Mangled.cpp
Outdated
| @@ -352,7 +352,7 @@ ConstString Mangled::GetDemangledNameImpl(bool force, // BEGIN SWIFT | |||
| Log *log = GetLog(LLDBLog::Demangle); | |||
| LLDB_LOGF(log, "demangle swift: %s", mangled_name); | |||
| std::string demangled(SwiftLanguageRuntime::DemangleSymbolAsString( | |||
| mangled_name, SwiftLanguageRuntime::eTypeName, sc)); | |||
| mangled_name, SwiftLanguageRuntime::eSimplified, sc)); | |||
There was a problem hiding this comment.
This change is needed to properly return the name of the demangled name and nothing else. Otherwise, we get an output that's too verbose like main.foo instead of just foo.
There was a problem hiding this comment.
Since this PR is just about making the inlined frame name cleaner, should this be done in a separate PR?
There was a problem hiding this comment.
To give some context:
The call stack for this method is:
Mangled::GetDemangledNameImplMangled::GetDemangledNameSwiftLanguage::GetDemangledFunctionNameWithoutArgumentsMangled::GetNameSwiftLanguage::GetFunctionDisplayName
With eTypeName this is what the output looks like:
With eSimplified:
This is more uniform with the rest of the backtrace.
I will bring this change in a separate PR 👍
There was a problem hiding this comment.
I take that back, sorry, notice that the frame #0 with eTypeName is test.baz(...
but with eSimplified it's simply baz(... which is the same as the current behavior of lldb.
I think this commit should stay in this patch.
There was a problem hiding this comment.
Do we not want the return type and scope in the name though? I thought we'll make it configurable in #10710. But for now we should show as much info as we can? I'm probably still missing something
There was a problem hiding this comment.
Just a bit surprised the issue is all the way down in Mangled::GetDemangledNameImpl
There was a problem hiding this comment.
I replied to the wrong thread here: #10786 (comment)
To add on that, we don't remove the scope entirely, we just remove the redundant test part which is just the name of the program. If the frame is a method foo on the class Bar, we still print Bar.foo for both inlined and non inlined methods.
There was a problem hiding this comment.
There's some test failures here on the macOS CI:
14:35:40 lldb-api :: lang/swift/async/frame/variables_multiple_frames/TestSwiftAsyncFrameVarMultipleFrames.py
14:35:40 lldb-api :: lang/swift/async/stepping/step-in/task-switch/TestSwiftTaskSwitch.py
14:35:40 lldb-api :: lang/swift/async/tasks/TestSwiftTaskSelect.py
14:35:40 lldb-api :: lang/swift/async/unwind/short_unwind/TestSwiftShortAsyncUnwind.py
14:35:40 lldb-api :: lang/swift/async/unwind/unwind_in_all_instructions/TestSwiftAsyncUnwindAllInstructions.py
14:35:40 lldb-api :: lang/swift/bt_printing/TestSwiftBacktracePrinting.py
14:35:40 lldb-api :: lang/swift/generic_function_name/TestSwiftGenericFunctionName.py
14:35:40 lldb-api :: lang/swift/macro/TestSwiftMacro.py
14:35:40 lldb-api :: lang/swift/protocols/class_protocol/TestSwiftClassConstrainedProtocolArgument.py
14:35:40 lldb-api :: lang/swift/protocols/stepping_through_witness/TestSwiftSteppingThroughWitness.py
14:35:40 lldb-api :: lang/swift/step_into_override/TestStepIntoOverride.py
14:35:40 lldb-api :: lang/swift/swift_callback/TestSwiftCallback.py
14:35:40 lldb-api :: lang/swift/variables/func/TestSwiftFunctionVariables.py
14:35:40 lldb-shell :: SwiftREPL/NSObjectSubclass.test
I suspect they might be related to this part here. But haven't looked closely
There was a problem hiding this comment.
They seem to all be related. There are 2 failures:
- Tests that fail because
main.foobecomesfoo, which is what we want. - Tests that fail because async functions were printed with their return types, so
foo() async -> Intbecomesfoo()which is not a desirable change.
There was a problem hiding this comment.
After more testing, it seems that too many methods depend on Mangled::GetDemangledNameImpl to change its behavior.
c6b8ffa introduces a better approach, keeping the same results as the screenshots above, but without breaking the API and tests that depend on Mangled::GetDemangledNameImpl.
|
@swift-ci test |
|
@swift-ci test |
|
The tests are failing because on macOS, the frame is represented as: But on other systems, it's: I marked the tests as XFAIL on non Darwin systems, but I'm not sure if it's the right call, we might want to fix this. This is not a regression in this patch. |
| s << display_name; | ||
| s.PutCString(" [inlined] "); | ||
| display_name = inline_info->GetName().GetString(); | ||
| s.PutCString("[inlined] "); |
There was a problem hiding this comment.
Sorry for the back and forth, but now that we merged #10789 into 6.2, we can remove this
If it's not a regression then marking them as XFAIL is the right call. Actually the way it's formatted on Darwin is worse UX in my opinion, since we don't see the value. Something to do with the way we're formatting the builtin |
|
@swift-ci test |
This PR is a port of the changes made in llvm#135343 to the Swift plugin.
We remove the prefix of
[inlined]to cut on the noise but keep the[inlined]itself until we implement a way to use a formatter to display or not.Also notice that on the screenshots below, the frame had a return type for inlined functions, but not for regular functions. Both now have the same behavior.
Before:
After:
