[Lldb-commits] [PATCH] D146679: [lldb] Add support for the DW_AT_trampoline attribute with mangled names

[Jonas Devlieghere via Phabricator via lldb-commits]

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 &current_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