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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to