Skip to content

Support mapping path prefixes for index stores #21893

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

Conversation

kastiglione
Copy link
Contributor

@kastiglione kastiglione commented Jan 15, 2019

This change builds on #17665, which added -debug-prefix-map to rewrite source paths embedded in the debug info of outputs. This change uses same flag to also remap source paths embedded in the index store.

The goal of this is just like the purpose of -debug-prefix-map, to produce artifacts (index store) that contain paths that refer to a known source path, not the build system's chosen build location. The end goal is to have indexes provided from a build cache.

I haven't discussed this with anyone on the Swift team, there are a few things to discuss:

  1. First, are there reasons this can't or shouldn't be done
  2. Which paths in the index should be mapped, currently only source file paths are remapped
  3. Are there any cases where there debug info and index store need separate prefix maps?
  4. Is it ok to expand -debug-prefix-map to include more than debug info? I think "debug" could be taken to mean outputs used for "development"

TODO

  • Add tests
  • Add equivalent support to clang

@adrian-prantl
Copy link
Contributor

Do you understand how/if the same problem is solved in Clang?

I think the generally idea behind it sounds perfectly reasonable.

It's odd for a debugging option to affect how the index is being built, but like the debugger an IDE is source-based tooling and can run into the similar problems... Would there ever be a reason to want remapped paths in the debug info bot not in the index?

@kastiglione
Copy link
Contributor Author

kastiglione commented Jan 16, 2019

Do you understand how/if the same problem is solved in Clang?

I think clang needs to be updated too, hopefully to match what's done here. I can propose this additional behavior to -fdebug-prefix-map.

It's odd for a debugging option to affect how the index is being built

Right, what we want is an option that affects development outputs, which includes debug info, index store, any any other artifacts used by development tools. Personally I'm fine with "debug" meaning "development", but I wouldn't be surprised if others aren't. In which case is the right choice to deprecate -debug-prefix-map and introduce a new one with an appropriate name? Or should there be a flag that opts index store writing to use the debug prefix map, like -index-store-with-debug-prefix-map (I don't like this name, but it illustrates the idea).

Would there ever be a reason to want remapped paths in the debug info bot not in the index?

Maybe. If you had a post build job/action that used the index, that tooling might want the paths as they exist at build time. This is speculative, I don't know of any build tools that operate post build.

Having replied, I'm liking the idea of an -index-store-* flag that enables this behavior. It would give control and if named right would show the connection between -debug-prefix-map and this non-debug-info rewriting behavior.

@benlangmuir
Copy link
Contributor

I'm very much in favour of having a way to create reproducible index data. Are you also planning to add a way for libIndexStore to remap the paths at load time, or do you map directly to a path that you know (somehow) will exist on the machine that will receive the artifacts? We've talked in the past about adding load-time path remapping to libIndexStore, but haven't done anything concrete there.

In which case is the right choice to deprecate -debug-prefix-map and introduce a new one with an appropriate name? Or should there be a flag that opts index store writing to use the debug prefix map, like -index-store-with-debug-prefix-map (I don't like this name, but it illustrates the idea).

I'd be interested in @jrose-apple 's thoughts on the driver argument names. Personally, I would have preferred us not to put debug in the name if we want this to cover other things as well. I'd probably lean toward adding a more general name and leaving the debug-prefix-map options alias that.

I think clang needs to be updated too, hopefully to match what's done here. I can propose this additional behavior to -fdebug-prefix-map.

I would flip this around: swift uses libIndex from clang to implement the low-level details of writing the index data, so I would plan to do both concurrently. I don't know if you need to add anything to upstream (llvm.org) clang for this at the moment since we haven't yet pushed the index data store code upstream (there is index code upstream, but not the record reader/writer stuff). It may be we only need to change swift-clang for now?

@jrose-apple
Copy link
Contributor

I'd be interested in @jrose-apple's thoughts on the driver argument names. Personally, I would have preferred us not to put debug in the name if we want this to cover other things as well. I'd probably lean toward adding a more general name and leaving the debug-prefix-map options alias that.

I think I'm with Ben on that. We can't remove driver options, but we can pick more appropriate names and use those going forward. -source-prefix-map? input-prefix-map?

I'll stay out of the rest of the discussion, about whether this is the right way to go about this at all.

@kastiglione
Copy link
Contributor Author

@benlangmuir

I'm very much in favour of having a way to create reproducible index data.

🇨🇦

Are you also planning to add a way for libIndexStore to remap the paths at load time, or do you map directly to a path that you know (somehow) will exist on the machine that will receive the artifacts?

The plan is to map directly to a known path, since we'll have to do this anyway for debug info. Not yet sure of the specifics, but we'll have to figure something out. Until support for this makes it into an official Swift release, we may have to rewrite unit files to correct their paths.

We've talked in the past about adding load-time path remapping to libIndexStore, but haven't done anything concrete there.

Would be nice.

plan to do both concurrently.
It may be we only need to change swift-clang for now

Sounds good, I'll open a PR for swift-clang soon

From Jordan:

-source-prefix-map? -input-prefix-map?

+1 for -source-prefix-map.

@benlangmuir
Copy link
Contributor

The plan is to map directly to a known path, since we'll have to do this anyway for debug info. Not yet sure of the specifics, but we'll have to figure something out

Interesting. I thought with debug info you would normally rely on the debugger being able to remap the path again (e.g. gdb searches cwd for sources by default and you can use directory to add more paths [0].

@kastiglione
Copy link
Contributor Author

I thought with debug info you would normally rely on the debugger being able to remap the path again

I've seen it done this way, but for lldb it requires local lldb config be correct. We are currently using lldb's source-map to remap paths, because -debug-prefix-map isn't available until Swift 5. At that point, I was expecting to try to not remap, presumably by using a stable path and a symlink. Once I saw that the index store also needed path mapping, and had no way to remap at load time, it seemed like stable path + symlink was the way to achieve both. I'm happy to work toward any solution, any means to the end works for me.

@allevato are you planning to remap the debug info path using lldb source-map?

@allevato
Copy link
Member

In our case, we were planning to remap Bazel's remote view of the workspace root directory to . and then invoke lldb with the CWD as the user's local workspace root so the sources would be at the same relative paths. I don't know if we'll need source-map beyond that (I suppose we might, if there are some invocations where we can't force/guarantee the CWD).

Regarding the overall question of this PR, I think it definitely makes sense to remap index store paths for the same reason, and I wish it had occurred to me when I implemented -debug-prefix-map so that I could have chosen a better name at that time 😄

Adding -source-prefix-map and keeping -debug-prefix-map as a legacy alias sounds great to me, and it would be nice to get the same capability added to upstream Clang too.

@kastiglione
Copy link
Contributor Author

kastiglione commented Mar 4, 2019

Thanks @allevato. It's unknown whether we could use relative paths in the index store Perhaps someone from Apple might answer this. This pull request would only be useful if Xcode does support relative paths (ex MainFilePath). If not, then indexstore unit files provided by a remote cache will need to be rewritten to be used from Xcode. We're currently exploring this approach.

@kastiglione
Copy link
Contributor Author

I've learned more here, after making a tool to convert index store unit files. It ends up that other paths need to be remapped, not just source paths, including the OutputFile (the .o path). Since remapping source paths isn't enough, this PR would get more complicated and would be specific to just the index. Closing with the plan to use a separate tool to solve this.

@kastiglione kastiglione closed this Mar 8, 2019
@kastiglione kastiglione deleted the dl/support-mapping-path-prefixes-for-index-stores branch March 8, 2019 18:15
@kastiglione
Copy link
Contributor Author

For anyone curious, we've open sourced the tool that remaps paths of an index-store: https://github.com/lyft/index-import

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants