JDevlieghere added inline comments.
================
Comment at: lldb/include/lldb/Symbol/Function.h:664
+ llvm::StringRef m_trampoline_target;
+
----------------
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?
================
Comment at: lldb/include/lldb/Target/Target.h:253
+ bool GetEnableTrampolineSupport() const;
+
----------------
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?
================
Comment at: lldb/source/Core/Module.cpp:779-783
+ bool is_trampoline = false;
+ if (Target::GetGlobalProperties().GetEnableTrampolineSupport() &&
+ sc.function) {
+ is_trampoline = !sc.function->IsTrampoline();
+ }
----------------
+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
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2443
+ auto *trampoline_target = die.GetTrampolineTargetName();
+
----------------
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)
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:208-211
+ if (IsValid())
+ return m_die->GetTrampolineTargetName(m_cu);
+ else
+ return nullptr;
----------------
No else-after-return. I would write:
```
return IsValid() ? m_die->GetTrampolineTargetName(m_cu) : nullptr;
```
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:211
+ else
+ return nullptr;
+}
----------------
nvm, I see this is done for consistency with the existing code...
================
Comment at: lldb/source/Target/ThreadPlanRunToAddress.cpp:217
+ images.FindSymbolsWithNameAndType(symbol_name, eSymbolTypeCode,
code_symbols);
+ size_t num_code_symbols = code_symbols.GetSize();
+
----------------
This is more of a preference but I would make this `const` to avoid
accidentally modifying this value.
================
Comment at: lldb/source/Target/ThreadPlanRunToAddress.cpp:221-222
+ for (uint32_t i = 0; i < num_code_symbols; i++) {
+ SymbolContext context;
+ AddressRange addr_range;
+ if (code_symbols.GetContextAtIndex(i, context)) {
----------------
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?
================
Comment at: lldb/source/Target/ThreadPlanRunToAddress.cpp:226-232
+ if (log) {
+ addr_t load_addr =
+ addr_range.GetBaseAddress().GetLoadAddress(target_sp.get());
+
+ LLDB_LOGF(log, "Found a trampoline target symbol at 0x%" PRIx64 ".",
+ load_addr);
+ }
----------------
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)`.
================
Comment at: lldb/source/Target/ThreadPlanStepThrough.cpp:126
+ TargetSP target_sp(thread.CalculateTarget());
+ StackFrame *current_frame = thread.GetStackFrameAtIndex(0).get();
+ const SymbolContext ¤t_context =
----------------
Should we check that the current frame is not null?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146679/new/
https://reviews.llvm.org/D146679
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
