This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add support for the DW_AT_trampoline attribute with mangled names
AbandonedPublic

Authored by augusto2112 on Mar 22 2023, 5:58 PM.

Details

Summary

This patch adds support for the DW_AT_trampoline attribute whose values
are mangled names. This patch makes it so stepping into a trampoline
will step through to the target subprogram instead, stepping out from the
target subprogram will also step out of the trampoline, and that setting
breakpoints by name will (by default) ignore trampolines.

Diff Detail

Event Timeline

augusto2112 created this revision.Mar 22 2023, 5:58 PM
Herald added a project: Restricted Project. · View Herald Transcript
augusto2112 requested review of this revision.Mar 22 2023, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 5:58 PM
bulbazord added inline comments.
lldb/include/lldb/Symbol/Function.h
439
lldb/source/Core/Module.cpp
782

The logic here is a little confusing. If the function is not a trampoline, then is_trampoline is true? Below, keep_it can only be true if we're not looking at a trampoline. So if a function is not a trampoline, we cannot keep it. Am I understanding the logic here incorrectly?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2450–2451

nit: this comment could be updated to indicate that it's referring to func_range.

lldb/source/Target/TargetProperties.td
187
JDevlieghere added inline comments.Mar 23 2023, 9:13 AM
lldb/include/lldb/Symbol/Function.h
665

I'd like to see a doxygen comment (1) explaining what the trampoline target is (i.e. according to the summary the mangled name of the trampoline target) and also why it's safe to store a StringRef. I didn't trace the lifetime but it seems like the StringRef is coming from a SymbolContext. Is the lifetime of the SC guaranteed to exceed that of the function object? If not, should this store an owning std::string?

lldb/include/lldb/Target/Target.h
253

What does trampoline "support" mean? Could this be named something more descriptive? From the description of the patch it sounds like this guards whether you step through trampolines, so maybe something like GetEnableStepThroughTrampolines or something?

lldb/source/Core/Module.cpp
779–783

+1 on what Alex said. Also I would either write

bool is_trampoline = Target::GetGlobalProperties().GetEnableTrampolineSupport() && sc.function && !sc.function->IsTrampoline()

or

if (Target::GetGlobalProperties().GetEnableTrampolineSupport()) 
  is_trampoline = sc.function && !sc.function->IsTrampoline();

if you really wanted to split it up

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2443

The return type of GetTrampolineTargetName isn't obvious from the name so I don't think this is a good place to use auto (in accordance with the LLVM style guide)

lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
208–211

No else-after-return. I would write:

return IsValid() ? m_die->GetTrampolineTargetName(m_cu) : nullptr;
211

nvm, I see this is done for consistency with the existing code...

lldb/source/Target/ThreadPlanRunToAddress.cpp
217

This is more of a preference but I would make this const to avoid accidentally modifying this value.

221–222

Not sure how hot this is, but would it be worthwhile moving this out of the loop and reusing the same object for every iteration?

226–232

Nit: I would just inline this. I'm sure the compiler will optimize this for you, but knowing the macro has the same if(log) check, I try avoid wrapping it in if(log).

lldb/source/Target/ThreadPlanStepThrough.cpp
126

Should we check that the current frame is not null?

augusto2112 marked 14 inline comments as done.

Addressed comments

lldb/include/lldb/Target/Target.h
253

Yeah I agree the name is pretty generic. This guards both stepping and setting breakpoints on trampolines (by name). We could potentially have two different settings (one for stepping on trampolines, one for breakpoints), but at the same time I don't think there are many users who would want to change only of or the other. I'll add a doxygen comment here with the explanation of what this does, but I'd be happy to change the setting name if anyone has a better suggestion.