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

Thu Sep 15 15:50:54 PDT 2022

clayborg added a comment.

In D133042#3791146 <https://reviews.llvm.org/D133042#3791146>, @yinghuitan wrote:

> @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.

So I think we should have two settings if this is the case. Why? Because I think the default values for "target.auto-deduce-source-map" should be true for relative paths in debug info, but not true by default for two different full paths in the debug info. We have no way to doing a great job at trying to match up two different full paths. What if we have "/a/b/c/foo.cpp" and "/d/e/f/foo.cpp". If this setting is enabled, I don't expect us to make an auto source map between "/a/b/c" -> "/d/e/f". We would need to specify a minimum directory depth and that gets really messy and involved more settings, and I don't think the full path stuff should be enabled by default.

> I can add description to `target.auto-deduce-source-map` to explain it only works for relative paths.

So not sure if we need to rename this setting to something like "target.auto-source-map-relative" to make this clear, and then this can and should default to true. Then if we later add full path remapping we can use "target.auto-source-map-absolute" and default it to false, and there will need to be many other settings for directory depth and other things. I just think there is too much that can go wrong with the auto remapping of full paths to group them both into the same bucket. I would also rather teach the production build toolchains to emit relative debug info and then never have to add the really hard auto source maps for different paths. That is unless we expose the DW_AT_comp_dir value in lldb_private::CompileUnit and can make a rock solid auto path mapping tool from those settings.

================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:225
+  const bool case_sensitive = request_file.IsCaseSensitive();
+  const bool full = !request_file.GetDirectory().IsEmpty();
+  for (uint32_t i = 0; i < sc_list.GetSize(); ++i) {
----------------
If the directory is empty on the requested file, should we be doing anything here? Can we early return?

================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:307

+  if (GetBreakpoint()->GetTarget().GetAutoDeduceSourceMap())
+    DeduceSourceMapping(sc_list);
----------------
See my inline comment below, but the BreakpointResolverFileLine is created with a NULL for the breakpoint. Does the breakpoint get set properly prior any of these calls? I am sure it must?

================
Comment at: lldb/source/Target/Target.cpp:405
   BreakpointResolverSP resolver_sp(new BreakpointResolverFileLine(
-      nullptr, offset, skip_prologue, location_spec));
+      nullptr, offset, skip_prologue, location_spec, removed_prefix_opt));
   return CreateBreakpoint(filter_sp, resolver_sp, internal, hardware, true);
----------------
The breakpoint is initialized with NULL here. Does it get set to something valid before we try to use it somehow? I am worried we won't be able to get a target from the BreakpointResolver's stored breakpoint????

================
Comment at: lldb/source/Target/TargetProperties.td:40
     Desc<"Source path remappings apply substitutions to the paths of source files, typically needed to debug from a different host than the one that built the target.  The source-map property consists of an array of pairs, the first element is a path prefix, and the second is its replacement.  The syntax is `prefix1 replacement1 prefix2 replacement2...`.  The pairs are checked in order, the first prefix that matches is used, and that prefix is substituted with the replacement.  A common pattern is to use source-map in conjunction with the clang -fdebug-prefix-map flag.  In the build, use `-fdebug-prefix-map=/path/to/build_dir=.` to rewrite the host specific build directory to `.`.  Then for debugging, use `settings set target.source-map . /path/to/local_dir` to convert `.` to a valid local path.">;
+  def AutoDeduceSourceMap: Property<"auto-deduce-source-map", "Boolean">,
+    DefaultFalse,
----------------

================
Comment at: lldb/source/Target/TargetProperties.td:41
+  def AutoDeduceSourceMap: Property<"auto-deduce-source-map", "Boolean">,
+    DefaultFalse,
+    Desc<"Automatically deduce source path mappings based on source file breakpoint resolution. It only deduces source mapping if source file breakpoint request is using full path.">;
----------------

================
Comment at: lldb/source/Target/TargetProperties.td:42
+    DefaultFalse,
+    Desc<"Automatically deduce source path mappings based on source file breakpoint resolution. It only deduces source mapping if source file breakpoint request is using full path.">;
   def ExecutableSearchPaths: Property<"exec-search-paths", "FileSpecList">,
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133042