Add -prefix-serialized-debugging-options

[rmaz]

This commit adds a new frontend flag that applies debug path prefixing to the
paths serialized in swiftmodule files. This makes it possible to use swiftmodule
files that have been built on different machines by applying the inverse map
when debugging, in a similar fashion to source path prefixing.

The inverse mapping in LLDB will be handled in a follow up PR.

[kastiglione]

See #17665 for a related earlier change.

[adrian-prantl]

Thanks, I can definitelty see how this can be useful. Is my understanding correct that this effectively implements LLDB's target.source-map setting for binary Swift modules, with the end-goal being that SwiftASTContext will initialize the Swift CompilerInstance with the contents of target.source-map and/or the path remapping dictionary in dSYMs? It looks like the current patch reuses the debug prefix map for this. However, I wonder if this shouldn't be a separate dictionary (are -fdebug-prefix-map options being serialized at the moment?). Finally I wonder how this will work in an expression context in LLDB if there are two dylibs with dSYMs that have different remapping dictionaries. Do we expect to need to support this use-case?

[rmaz]

Is my understanding correct that this effectively implements LLDB's target.source-map setting for binary Swift modules, with the end-goal being that SwiftASTContext will initialize the Swift CompilerInstance with the contents of target.source-map and/or the path remapping dictionary in dSYMs?

Yes, that is the idea. I haven't thought about using the dSYM remapping dictionary yet, but I think that could be handled.

It looks like the current patch reuses the debug prefix map for this. However, I wonder if this shouldn't be a separate dictionary

I considered it, but couldn't think of a case where you would want to use a different map for the swiftmodule debugging options than for the regular debugging info. Do you think this is worthwhile?

(are -fdebug-prefix-map options being serialized at the moment?)

They are, this change will skip over the flags during serialization. It may make sense to pull this into a separate commit / PR.

Finally I wonder how this will work in an expression context in LLDB if there are two dylibs with dSYMs that have different remapping dictionaries. Do we expect to need to support this use-case?

That is a case I had not considered. The complexity of handling that would be part of the upcoming PR that modifies LLDB to provide the targets source-map to the SearchPathOptions.SearchPathRemapper, so I think should not require further changes to this PR at least. For the common case of remapping the working directory to . this PR will already take care of relativizing the swiftmodule debugging info correctly and debugging should work when run from the correct working directory.

[adrian-prantl]

It looks like the current patch reuses the debug prefix map for this. However, I wonder if this shouldn't be a separate dictionary

I considered it, but couldn't think of a case where you would want to use a different map for the swiftmodule debugging options than for the regular debugging info. Do you think this is worthwhile?

(are -fdebug-prefix-map options being serialized at the moment?)

They are, this change will skip over the flags during serialization. It may make sense to pull this into a separate commit / PR.

I don't think there is a good reason to serialize -fdebug-prefix-map in a .swiftmodule, because the option is for debug info, which is irelevant when importing a serialized Swift AST. (Related side question: Is the debug prefix map applied to source locations inside the serialized AST in .swiftmodule that was compiled with -fdebug-prefix-map?)
But I'm not convinced that repurposing -fdebug-prefix-map is the right design, since our new option is supposed to affect file lookup and search paths as well as source paths in debug info. It feels wrong to have an -fdebug option affecting file lookup.

Finally I wonder how this will work in an expression context in LLDB if there are two dylibs with dSYMs that have different remapping dictionaries. Do we expect to need to support this use-case?

That is a case I had not considered. The complexity of handling that would be part of the upcoming PR that modifies LLDB to provide the targets source-map to the SearchPathOptions.SearchPathRemapper, so I think should not require further changes to this PR at least. For the common case of remapping the working directory to . this PR will already take care of relativizing the swiftmodule debugging info correctly and debugging should work when run from the correct working directory.

LLDB has two mechanisms for remapping paths that operate at different layers:

1. The target.source-map setting can be set per target (~ executable)
2. The dsym path remapping dictionary is a property of the symbol file (~ dylib) affects every query to that symbol file, =and the same symbol file could be loaded by different targets.

Supporting (2) is tricky because in the expression context, we don't have a ready mapping between symbol file and imported Swift modules, so it can be tricky to discern which remapping dictionary to apply. However, I think that if you are trying to build a distributed build system, then properly supporting (2) is the way to go, because a properly implemented system where dsyms are automatically fitted with that e correct remapping dictionary can be completely transparent to the end-user.

[keith]

It feels wrong to have an -fdebug option affecting file lookup.

clang has -ffile-prefix-map now that implies a few of these, do you think we should add something similar to swift? Today it would imply -debug-prefix-map and -coverage-prefix-map. Happy to add that if there's interest

[rmaz]

Related side question: Is the debug prefix map applied to source locations inside the serialized AST in .swiftmodule that was compiled with -fdebug-prefix-map?

I'm not 100% I follow, but the debug prefixes we are using are the ones passed via -debug-prefix-map, which get propagated to the Clang importer as -fdebug-prefix-map, which we then avoid serializing.

But I'm not convinced that repurposing -fdebug-prefix-map is the right design, since our new option is supposed to affect file lookup and search paths as well as source paths in debug info. It feels wrong to have an -fdebug option affecting file lookup.

Yes, I see your point. It seems a little messy to add yet another mapping flag for this (this would make 3 now I think) but how about -serialized-debugging-options-prefix-map or similar? We could consider adding a master flag later as @keith suggests to set them all at once.

However, I think that if you are trying to build a distributed build system, then properly supporting (2) is the way to go, because a properly implemented system where dsyms are automatically fitted with that e correct remapping dictionary can be completely transparent to the end-user.

As far as our use case goes we do not need to support dSYMs or even apply inverse mappings, all paths except Xcode
for a Swift compilation are kept inside a single folder, so prefixing the working directory is sufficient. We only use dSYMs for shipped applications, not for local builds, so they are generally only used for stack trace population. I am happy to have a go at getting this working though. My current patch is just applying the target's SourcePathMap.

[adrian-prantl]

I would like to avoid duplicating functionality that already exists in LLDB in the Swift compiler, because it can lead to hard-to-debug issues. I think we could do the following: This patch is about remapping Clang options. Those options need to get passed to the Clang compiler instance that is owned by ClangImporter. ClangImporter is created in LLDB by SwiftASTContext. If we modified ClangImporter to take a callback for remapping compiler flags, we could set that to a function similar to SwiftASTContext::RemapClangImporterOptions(). LLDB would no longer manually call RemapClangImporterOptions(), it would get called through the callback. Then we only have a single implementation of path remapping, which is owned by LLDB, and it will tie in nicely with logging and make it much more transparent what's going on.

[rmaz]

I would like to avoid duplicating functionality that already exists in LLDB in the Swift compiler, because it can lead to hard-to-debug issues.

Agreed.

If we modified ClangImporter to take a callback for remapping compiler flags, we could set that to a function similar to SwiftASTContext::RemapClangImporterOptions(). LLDB would no longer manually call RemapClangImporterOptions(), it would get called through the callback. Then we only have a single implementation of path remapping, which is owned by LLDB, and it will tie in nicely with logging and make it much more transparent what's going on.

It seems in both the Swift serialization path and LLDB we are getting ClangImporterOptions via CompilerInvocation::getClangImporterOptions(). Would it make sense to add a method to ClangImporterOptions to handle the flag splitting and joining, with a callback for the path remapping? Something like:

std::vector<std::string> remapExtraArgs(std::function< std::string(std::string) > remapCallback) {
	// do the splitting logic for the joined args
	// and call `remapCallback` for each path 
	// and return a vector of the results 
}

Then we could call this function in both the Swift serialization path and LLDB to handle
their respective remapping dictionaries or whatever custom logic is required.

[rmaz]

I updated this to use a separate -debugging-options-prefix-map flag, and to extract the flag splitting logic into a separate function that can be used from LLDB. @adrian-prantl lmk what you think of this approach.

[adrian-prantl]

It seems in both the Swift serialization path and LLDB we are getting ClangImporterOptions via CompilerInvocation::getClangImporterOptions(). Would it make sense to add a method to ClangImporterOptions to handle the flag splitting and joining, with a callback for the path remapping? Something like:

std::vector<std::string> remapExtraArgs(std::function< std::string(std::string) > remapCallback) {
	// do the splitting logic for the joined args
	// and call `remapCallback` for each path 
	// and return a vector of the results 
}

Then we could call this function in both the Swift serialization path and LLDB to handle
their respective remapping dictionaries or whatever custom logic is required.

https://github.com/apple/swift/blob/7c653e4fec06069c47ce70438a0e1be4bce68b67/include/swift/Frontend/Frontend.h#L85

I think CompilerInvocation is supposed to store the result of having built a full compiler invocation so the callback might fit better into either a class that builds a CompilerInvocation, or into ClangImporter, who receives the extra args. I thought ClangImporter's constructor would be a natural place to put the callback because it's LLDB who is creating ClangImporter:

https://github.com/apple/llvm-project/blob/c304b0469181552dfbdf745264c7277b0d23f6a7/lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp#L3331

[rmaz]

I think CompilerInvocation is supposed to store the result of having built a full compiler invocation so the callback might fit better into either a class that builds a CompilerInvocation, or into ClangImporter, who receives the extra args. I thought ClangImporter's constructor would be a natural place to put the callback because it's LLDB who is creating ClangImporter

IIUC I think what you are suggesting is that we would add a callback param as part of the constructor to the ClangImporter and then we feed the ClangImporter the unmapped args and when they are actually used internally by the ClangImporter it would call the callback function to do path fixup, is that the idea? I can see how that would work for the LLDB case, but for Swift we want the ClangImporter to keep using the existing unmodified paths, its only during serialization that we would want to remap the paths to have prefixes.

Maybe I am not following 100% though.

[adrian-prantl]

To summarize an offline conversation. I was conflating two things here:

1. we need an option to apply the debug prefix map to search paths that are serialized in .swiftmodule files
2. we need a way to let LLDB handle the "inverse" transformation to map the remapped paths back to something meaningful on the machine where the debugger runs.

This PR deals with (1). My initial concern was that having an -fdebug option affect .swiftmodule files would be unexpected. However, there serialized paths are there primarily for the debugger, so I think I've come around to being okay with just unconditionally applying the prefix map to .swiftmodule serialized search paths, too.

[rmaz]

@artemcm @adrian-prantl just checking where we stand with this one, are further changes required or is this good for testing and merging?

[artemcm]

This change makes sense and looks good to me.

[artemcm]

Please add a brief comment to explain why we skip this flag.

[artemcm]

@swift-ci please test

[drodriguez]

@swift-ci please test

[edudnyk]

Hi @rmaz,

Thanks for this feature.

As @adrian-prantl pointed,

2. we need a way to let LLDB handle the "inverse" transformation to map the remapped paths back to something meaningful on the machine where the debugger runs.

Is this already supported by LLDB? I am testing debugger with -prefix-serialized-debug-info and it seems like remapping via target.source-map in .lldbinit does not affect the prefixed paths stored in .swiftmodule files.

For instance, if to take the sample project from this issue and to add -Xfrontend -prefix-serialized-debugging-options to SWIFT_FLAGS in all targets, the path in .swiftmodule becomes correctly prefixed, but the error from lldb stays the same (the vfsoverlay file is not found because its path does not get remapped).

You've mentioned that

The inverse mapping in LLDB will be handled in a follow up PR.

was there a follow-up on this?

[rmaz]

The inverse mapping in LLDB will be handled in a follow up PR.

was there a follow-up on this?

Apologies, that one slipped off the radar, our use case requires no remapping as we prefix all the paths to be relative to the repo root.

I can take a pass at adding remapping via the target.source-map later this week.