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

[jeffrey tan via Phabricator via lldb-commits]

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