[Lldb-commits] [PATCH] D133042: Add auto deduce source map setting

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

The main question for me here is if "target.auto-deduce-source-map" should be a boolean or an enum. This path, if I read it correctly, will only auto deduce source maps if the debug info contains relative paths and a full path is specified. So maybe it should be:

  (lldb) settings set target.auto-deduce-source-map disabled
  (lldb) settings set target.auto-deduce-source-map relative-debug-info

If we plan to use the target.auto-deduce-source-map to later handle cases where we have two different full paths, the user might not want to enable this setting.

Or we can clarify that this setting deduces source mapping only for relative debug info paths by renaming it and then the "bool" value makes more sense?



================
Comment at: lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h:28
+      llvm::Optional<llvm::StringRef> removed_prefix_opt = llvm::None,
+      lldb::TargetSP target = nullptr);
 
----------------



================
Comment at: lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h:70
+  llvm::Optional<llvm::StringRef> m_removed_prefix_opt;
+  lldb::TargetSP m_target = nullptr;
+  bool m_auto_deduce_source_map = false;
----------------
Store this as a weak pointer to a target to avoid a target that owns an object that will keep the target object from ever being able to destruct itself. Anytime you need to use the target then you use a local variable that is a shared pointer:
```
TargetSP target_sp = m_target_wp.lock();
if (!target_sp)
  return;
```



================
Comment at: lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h:71
+  lldb::TargetSP m_target = nullptr;
+  bool m_auto_deduce_source_map = false;
 
----------------
Remove this. No need to store this as a member variable, just ask the breakpoints target for it when you need it since you only use this once.


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:31
+      m_removed_prefix_opt(removed_prefix_opt), m_target(target) {
+  m_auto_deduce_source_map = target && target->GetAutoDeduceSourceMap();
+}
----------------
Remove this and use target when needed on demand.


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:197-198
+    SymbolContextList &sc_list) {
+  if (!m_auto_deduce_source_map || !m_target)
+    return;
+
----------------




================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:217
+
+  const bool case_sensitive = m_location_spec.GetFileSpec().IsCaseSensitive();
+  for (uint32_t i = 0; i < sc_list.GetSize(); ++i) {
----------------
Can we just return if m_location_spec.IsRelative() returns true here to short circuit this entire function? Many users type "b main.cpp:12" and we probably don't need to do any auto source map stuff if the request starts as a relative path like "main.cpp" or "foo/bar/baz.cpp" right?


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:217
+
+  const bool case_sensitive = m_location_spec.GetFileSpec().IsCaseSensitive();
+  for (uint32_t i = 0; i < sc_list.GetSize(); ++i) {
----------------
clayborg wrote:
> Can we just return if m_location_spec.IsRelative() returns true here to short circuit this entire function? Many users type "b main.cpp:12" and we probably don't need to do any auto source map stuff if the request starts as a relative path like "main.cpp" or "foo/bar/baz.cpp" right?
Move the "request_file" below to this line and use it to avoid copying it each time through the loop.


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:222
+
+    FileSpec sc_file = sc.line_entry.file;
+    FileSpec request_file = m_location_spec.GetFileSpec();
----------------
Should we quickly continue here if "sc_file" is not relative?


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:225
+
+    const bool full = !request_file.GetDirectory().IsEmpty();
+    if (FileSpec::Equal(sc_file, request_file, full))
----------------
Move this out of the for loop. No need to calculate it each time.


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:239
+    if (m_removed_prefix_opt.hasValue())
+      new_mapping_to.append(m_removed_prefix_opt.getValue());
+
----------------
please use the llvm::sys::path::append stuff to append paths to each other so it can worry about adding any needed directory delimiters


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:246
+      if (new_mapping_to.empty())
+        new_mapping_to.append(".");
+    } else {
----------------
If it is empty, then assign


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:252
+        new_mapping_from = ".";
+        new_mapping_to.append(new_mapping_to_opt.getValue());
+      }
----------------
use llvm::sys::path::append 


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:301
 
+  if (m_auto_deduce_source_map)
+    DeduceSourceMapping(sc_list);
----------------
I would remove this check and just call DeduceSourceMapping all the time and check for this setting in the function itself.


================
Comment at: lldb/source/Target/PathMappingList.cpp:82-83
+  for (const auto &pair : m_pairs) {
+    if (pair.first.GetStringRef().equals(NormalizePath(path)) &&
+        pair.second.GetStringRef().equals(NormalizePath(replacement)))
+      return;
----------------
Call Normalize on "path" and "replacement" outside of this loop instead of doing it each time through


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133042/new/

https://reviews.llvm.org/D133042