yinghuitan added a comment.
@clayborg , it is my intention to make `target.auto-deduce-source-map` boolean
flag ultimately working for both relative paths and two full paths (two full
paths are really important for off build host debugging, e.g. dump or
production debugging). In this patch, I focuses on getting relative path
working because that's the default behavior; a follow-up patch can get two full
paths working.
In my opinion boolean flag setting is dead simple to use (to enable both) and
an enumeration setting would add extra barrier for adoption.
I can add description to `target.auto-deduce-source-map` to explain it only
works for relative paths.
================
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;
----------------
clayborg wrote:
> 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;
> ```
>
Your later comment reminds me that we can get target from
`GetBreakpoint()->GetTarget()` so we do not need to store target point at all.
================
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:
> 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.
I do not think we can because if an existing reverse source mapping is applied
`m_location_spec` will become a relative path even though original request is
full path (Remember the prefix is stripped and stored in `removed_prefix_opt`? )
I guess I can check:
1. If `removed_prefix_opt` is not available, then return if
m_location_spec.IsRelative() is true.
2. If `removed_prefix_opt` is available, do nothing.
================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:222
+
+ FileSpec sc_file = sc.line_entry.file;
+ FileSpec request_file = m_location_spec.GetFileSpec();
----------------
clayborg wrote:
> Should we quickly continue here if "sc_file" is not relative?
I do not think so. Here is an example:
```
dwarf sc_file:
/build/repo/x/y/z/foo.cpp
breakpoint request:
/user/root/new_repo_location/x/y/z/foo.cpp
User has an existing source mapping entry so a reverse mapping is applied:
original: .
replacement: /user/root/new_repo_location
After reverse mapping:
Breakpoint::m_location_spec: x/y/z/foo.cpp
With the new auto-deduce-source-map a new source mapping entry is added:
original: /build/repo
replacement: /user/root/new_repo_location
You can see the sc_list is full path in this example.
```
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
