Page MenuHomePhabricator

Add auto deduce source map setting
ClosedPublic

Authored by yinghuitan on Aug 31 2022, 12:17 PM.

Details

Summary

This patch adds a new "target.auto-deduce-source-map" setting.

If enabled, this setting may auto deduce a source map entry based on requested
breakpoint path and the original path stored in debug info for resolved
breakpoint.

As an example, if debug info contains "./a/b/c/main.cpp", user sets a source
breakpoint at "/root/repo/x/y/z/a/b/c/main.cpp". The breakpoint will resolve
correctly now with Greg's patch https://reviews.llvm.org/D130401. However, the
resolved breakpoint will use "./a/b/c/main.cpp" to locate source file during
stop event which would fail most of the time.

With the new "target.auto-deduce-source-map" setting enabled, a auto deduced
source map entry "." => "/root/repo/x/y/z" will be added. This new mapping will
help lldb to map resolved breakpoint path "./a/b/c/main.cpp" back to
"/root/repo/x/y/z/a/b/c/main.cpp" and locate it on disk.

If an existing source map entry is used the patch also concatenates the auto
deduced entry with any stripped reverse mapping prefix (see example below).

As a second example, debug info contains "./a/b/c/main.cpp" and user sets
breakpoint at "/root/repo/x/y/z/a/b/c/main.cpp". Let's say there is an existing
source map entry "." => "/root/repo"; this mapping would strip the prefix out of
"/root/repo/x/y/z/a/b/c/main.cpp" and use "x/y/z/a/b/c/main.cpp" to resolve
breakpoint. "target.auto-deduce-source-map" setting would auto deduce a new
potential mapping of "." => "x/y/z", then it detects that there is a stripped
prefix from reverse mapping and concatenates it as the new mapping:
"." => "/root/repo/x/y/z" which would correct map "./a/b/c/main.cpp" path to
new path in disk.

This patches depends on https://reviews.llvm.org/D133038 to use new added
SBTarget::GetSourceMap() API for testing.

Diff Detail

Event Timeline

yinghuitan created this revision.Aug 31 2022, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2022, 12:17 PM
yinghuitan requested review of this revision.Aug 31 2022, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2022, 12:17 PM
yinghuitan edited the summary of this revision. (Show Details)Sep 1 2022, 12:05 AM
clayborg requested changes to this revision.Sep 14 2022, 12:06 PM

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?

lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
28
69

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;
70

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.

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
31

Remove this and use target when needed on demand.

195–196
215

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?

215

Move the "request_file" below to this line and use it to avoid copying it each time through the loop.

220

Should we quickly continue here if "sc_file" is not relative?

223

Move this out of the for loop. No need to calculate it each time.

237

please use the llvm::sys::path::append stuff to append paths to each other so it can worry about adding any needed directory delimiters

244

If it is empty, then assign

250

use llvm::sys::path::append

305

I would remove this check and just call DeduceSourceMapping all the time and check for this setting in the function itself.

lldb/source/Target/PathMappingList.cpp
82–83

Call Normalize on "path" and "replacement" outside of this loop instead of doing it each time through

This revision now requires changes to proceed.Sep 14 2022, 12:06 PM

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

lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
69

Your later comment reminds me that we can get target from GetBreakpoint()->GetTarget() so we do not need to store target point at all.

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
215

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

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.

Address review comments

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

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
225

If the directory is empty on the requested file, should we be doing anything here? Can we early return?

305

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?

lldb/source/Target/Target.cpp
405

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????

lldb/source/Target/TargetProperties.td
40
41
42

@clayborg, now I see you want to make this option true by default while full paths one to be optional. Then sure, it makes sense.

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
225

This should be handled by check at line 221, right? If request_file.GetDirectory().IsEmpty() then request_file.IsRelative() should be true and early return already?

lldb/source/Target/Target.cpp
405

It is initialized as nullptr here but CreateBreakpoint() call in next line will call BreakpointResolver::SetBreakpoint() to initialize it. Actually in BreakpointResolver::GetBreakpoint() explicitly has an assertion to ensure it can't be nullptr so I assume we should be safe here.

clayborg added inline comments.Sep 15 2022, 4:50 PM
lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
225

Then you can just set full to true all the time then right?

lldb/source/Target/Target.cpp
405

Sounds good. I looked around the code. Looks like the breakpoint in the constructor is used for the "create from StructuredData".

Address review feedback

Just one nit about double checking if auto deducing is enabled. Fix that and this is good to go.

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
305

Either remove this, or leave the check in the DeduceSourceMapping function. I would vote to just always call DeduceSourceMapping in case someone else calls it from anywhere else.

clayborg accepted this revision.Sep 19 2022, 12:14 PM
This revision is now accepted and ready to land.Sep 19 2022, 12:14 PM
This revision was automatically updated to reflect the committed changes.