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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
