-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement -debug-prefix-map flag. #17665
Conversation
This flag is based on Clang's -fdebug-prefix-map, which lets the user remap absolute paths in debug info. This is necessary for reproducible builds and allows debugging to work on a different machine than the one that built the code when paths to the source may be different.
You analysis that this is difficult is correct, but I'm afraid, we can't ignore it. Debugging Swift code currently depends on LLDB's embedded swift compiler importing Swift modules, and that depends on Swift's embedded Clang importing Clang modules. There are two approaches to apply the remapping:
The second variant would be cleaner.
Maybe the right answer is to realpath them before serialization, too, just like the other paths that go into debug info?
As a general guidance: Everything we can do to make compilation and debugging more similar is a win. I would not want to make any changes that would make it easier to have search path issues that only reproduce when in the debugger. |
I'm with Adrian on the "only use search paths when debugging" thing. We used to do that, and it led to a bunch of weird cases where you could compile but not debug. ClangImporter flags are another big mess. I don't want to have to recreate Clang parsing or guess at what's a path. And really, a whole bunch of Clang flags are probably not interesting for debugging. It's just hard to know, and even weirder for Swift to know what they would be. I'd rather take a step back and think about how debugging with Clang flags is supposed to work at all. (Sorry, I think I've said this to both of you multiple times in the past, even though we've never actually set a time for such a rethink/discussion.) That said, this patch is still useful just to improve In the near term, how about something brute force for the swiftmodule side: if some flag is passed, we don't serialize search paths (or -Xcc options) at all, and then it's part of setting up a proper debug environment? I don't think that's a good enough answer to be the default mode for everyone yet, but it might be good enough for what you're doing. |
include/swift/Basic/PathRemapper.h
Outdated
/// the original path is returned. | ||
std::string remapPath(StringRef Path) const { | ||
for (const auto &Mapping : PathMappings) | ||
if (Path.startswith(Mapping.first)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't obvious to me that this should be using startswith
instead of checking for a path delimiter, but it looks like that's what Clang's mapping does too (albeit without preserving order). Can you leave a comment to note that this was thought about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
include/swift/Option/Options.td
Outdated
@@ -470,6 +470,9 @@ def gline_tables_only : Flag<["-"], "gline-tables-only">, | |||
def gdwarf_types : Flag<["-"], "gdwarf-types">, | |||
Group<g_Group>, Flags<[FrontendOption]>, | |||
HelpText<"Emit full DWARF type info.">; | |||
def debug_prefix_map : Separate<["-"], "debug-prefix-map">, | |||
Flags<[FrontendOption]>, | |||
HelpText<"remap source paths and search paths in debug info">; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Option help strings should be capitalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
for (auto A : Args.getAllArgValues(OPT_debug_prefix_map)) { | ||
// Forward -debug-prefix-map arguments from Swift to Clang as | ||
// -fdebug-prefix-map. This is required to ensure DIFiles created there, | ||
// like "<swift-imported-modules>", have their paths remapped properly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I feel like I would have missed this. :-)
Note that Clang does not preserve the order of options today; it probably should in the future. At least that's backwards-compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; I updated the comment to mention the (admittedly rare?) situations where the difference matters.
Thanks for the feedback! I've reverted the pieces that remapped paths inside .swiftmodules and left only the DebugInfo bits, so I think this is what you were looking for. I'm happy to work on this incrementally—the paths in .swiftmodules are an issue for us not only because of remote debugging, but for reproducible builds in general because these the files can't be reliably cached. I understand the desire to keep module loading the same during compilation and debugging, although based on my exploration in SR-7845, it still seems problematic to me that serialized debugging options would modify how modules are found during compilation. Maybe this is a bigger issue for us based on how we use Bazel to build Swift, though? Unlike Xcode where an entire application is typically a single module (excepting dynamic frameworks), we build multiple small modules as static libraries, always with an Objective-C header for interop, and then use Clang, not ...which means that we can actually explore your suggestion of disabling serialization today by just not passing that flag. Once this change is in, maybe we can configure the rest of the debugging environment enough that it could be made to work. |
Yes, this is what I meant for an uncontroversial first step. @adrian-prantl, what do you think? @swift-ci Please smoke test |
@adrian-prantl Friendly bump 🙂 |
Bump again—we would love to see this make it into Swift 4.2 if possible, since it opens the way for us to debug distributed builds. If there's anything else I need to change before it can be merged, please let me know. Thanks! |
I'll take a look at this today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bits in IRGenDebugInfo.cpp
are fine, but I can't comment on the Driver changes as I'm not really familiar with them, so you'll need somebody else to approve that part.
include/swift/Basic/PathRemapper.h
Outdated
namespace swift { | ||
|
||
class PathRemapper { | ||
private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess private is redundant here, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! Removed.
I'm good with the Driver bits. In the interest of expediency, I'm okay with merging this to master today, but I'm not sure Ted will still want to take it on the 4.2 branch at this point. Doesn't hurt to ask, though (via PR). |
@swift-ci please test |
Build failed |
Build failed |
Thanks all! Once this merged, I'll float a PR to cherry-pick it and see what the release manager thinks. There's still a minor behavioral change even when the I'm not familiar enough with LLDB's test suite to know whether passing tests there are a good measure of whether that change worked as intended; I did some local testing to verify it by hand and hoped that I covered everything. @dcci, do you have some additional insight? |
The swift-pr cycle runs the swift lldb tests as part of this so you should be fine.
|
If you want to add a testcase to the lldb testsuite where you build a source file with this command and make sure everything works as intended, though, it will be really appreciated as a follow-up. |
From glancing over the OS X log, it looks like the failure is unrelated (but I'm not sure why it occurred). |
@allevato That assertion has been committed 4 days ago. Have you tried to reproduce this locally on ToT? |
(although I agree it doesn't seem immediately related). |
@swift-ci please test macos |
Unfortunately I'm not in front of that particular development machine right now; if the re-run fails the same way, I'll try to reproduce it later today and report back. |
It passed this time, so I'm going to merge to give Tony the opportunity to make a 4.2 PR. |
Implement -debug-prefix-map flag.
Like Clang's
-fdebug-prefix-map
, this lets the user remap absolute paths in debug info. (The implementation here is based largely on the Clang implementation.) This is necessary for reproducible builds and allows debugging to work correctly on a machine other than the one that built the code because absolute paths to the source may be different.Addressing search paths/ClangImporter flags in .swiftmodule files will happen separately; I've preserved my original discussion items below.
In addition to the path changes in the emitted debug info, this also necessarily remaps the search paths serialized in theINPUT_BLOCK
of .swiftmodules so that those paths can also be resolved correctly when LLDB loads the ASTs.I've done some local testing of this, remapping the debug info paths and search paths of a multi-module executable, and LLDB finds
boththe sourceand the dependent modulescorrectly when started in the appropriate CWD.Open question: Remapping ClangImporter flags
I looked into remapping the full set of ClangImporter flags in the
OPTIONS_BLOCK
as well, but this is somewhat challenging:-I foo
, but not-D foo
).-debug-prefix-map foo=bar
, then-Ifoo
needs to become-Ibar
and-isystemfoo
needs to become-isystembar
. This would require keeping an explicit whitelist of such flags.Remapping the ClangImporter flags isn't necessary in all cases. They're passed to ClangImporter verbatim as the user typed them in the invocation, and therefore aren't converted to absolute paths like the Swift import search paths are. So in situations where the user is using
-debug-prefix-map
to remove an absolute path prefix (by remapping it to the empty string or.
), they can avoid the need to remap ClangImporter flags by simply passing those relative paths to begin with. But in the case where you need to actually change a path prefix, it may not be sufficient. I'm not sure if that's a blocking issue here.Known issue: Duplicate search paths
As currently written, this PR causes duplicate search paths to appear in some .swiftmodules. The problem is this:
-I foo -debug-prefix-map foo=bar
. The compiler addsfoo
to the AST context's import search paths, and it is remapped tobar
when the module is serialized.-I foo -debug-prefix-map foo=bar
, and it imports A. First, the compiler addsfoo
from the command line to the AST context's search paths. Then module A is loaded, which addsbar
to the context's search paths. When B.swiftmodule is serialized, there's no attempt to uniquify these;foo
becomesbar
, the secondbar
stays the same, and we end up with two identical entries in the module.I can think of a couple ways to address this:
@jrose-apple @adrian-prantl You've both given me some conceptual input on this already; is there anything I'm overlooking?