Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New rustc and Cargo options to allow path sanitisation by default #3127
New rustc and Cargo options to allow path sanitisation by default #3127
Changes from 23 commits
139d7f0
bcbd131
97e4104
d8344ef
408dc50
f92a321
2bd2792
998ecf4
ba6b2d8
d790948
d33e029
0688580
aee42a6
8e33a46
0b59e5c
604dcb0
2d49c09
23394fc
e15308c
dec1901
bb079f4
7c533d3
6e45014
34d4386
785c229
a357827
5286008
8ba3510
3f59b7b
16edfb2
cbcb1df
451f163
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 precompiled standard library libraries would use the virtual prefix even if rust-src is installed, so you have to add the remap for debuggers either way.
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.
When
rust-src
is installed, rustc internally does a hack to map the virtual prefix to the corresponding real path on the user's local fs: https://github.com/rust-lang/rust/blob/d1a9a9551741c3e888d350d8d4f4821a5addccb2/compiler/rustc_metadata/src/rmeta/decoder.rs#L1477-L1478Though this is real path is not emitted because it wasn't affected by
--remap-path-prefix
which makes reproducible builds impossible: rust-lang/rust#73167.However, some people want to see the real path for debugging: rust-lang/rust#85463.
The point here is to satisfy both use cases: without
rust-src
you don't have a choice but to see the virtual path. Withrust-src
you'll see the real path if you don't do anything else, but if you want build reproducibility you can use--remap-path-prefix
to remap them.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.
@cbeuw i think rust-analyzer requires rust-src. does this mean people using rust-analyzer will not have sanitization by default which i thought would happen by default from other comments
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.
Sanitisation by default will happen when you do
cargo build --release
(orcargo test --release
), regardless of the presence of rust-src. For default debug builds people with rust-src will now see the local paths.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.
ok sounds like i was mixing things up then, sorry.
so regardless of rust-analyzer/rust-src if someone is doing
--release
then its still clean. just reiterating to confirm i have things straight since the discussion was confusing for me.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.
Correct.
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 think what @bjorn3 is referring to is that the virtual prefixes already embedded in the debuginfo of the standard library, that is, rustc won't have access to them anymore and does not have a way of remapping those paths. Only the linker will touch that debuginfo.
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.
This is not really a descriptive name.
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.
panic
is what most users care about, but it's not really accurate for those usingstd::file!()
directly or indirectly for other things. Sofile-macro
maybe?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.
file!()
isn't the only way to get a source location anymore (I don't know if its even still used for panics), we now also haveintrinsics::caller_location
(including stable wrappers).I don't have any good naming suggestions (maybe
intrinsics
?), but I believe it would be good to also mention the intrinsic or its wrappers explicitly in the RFC.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.
expansion
maybe?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.
Is this useful on it's own? It can't prevent the original path from ending up in the crate metadata without also requiring all other remappings.
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.
rust-lang/rust#88363
Maybe people won't use it on its own, but it can't be merged with other options either so it has to be there
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.
since analyzer is popular that means nothing is stripped by default then? will it be a single option and not a mess of remap-prefix options then to strip the paths? otherwise it will be a choice of privacy/reproducibility or the usefulness of analyzer
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.
--remap-path-prefix
is a stable option and will do the same things as documented after this RFC (i.e. remap everything everywhere). You only need--remap-path-scopes
if you want finer grained control.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.
@davidtwco, do you know if we can actually implement this (when using the LLVM backend)? I.e. can we control what paths show up in what context? As far as I know, we produce a single LLVM metadata description and then LLVM takes care of splitting things apart, right?
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 don't think there is a need for all those different options. I think having "codegen", "debuginfo" and "all" as the only three options would be fine. "codegen" would mean that stripping an executable, cdylib or staticlib from it's debuginfo (if any) will remove all unmapped paths and thus would be a good option when shipping programs without debuginfo. "debuginfo" will mean that both the executable, cdylib or staticlib and it's debuginfo only contain mapped paths and thus would be a good option when shipping programs with debuginfo. "all" would mean that all intermediate artifacts (like rlibs or rust dylibs), rustc diagnostics and everything only contain mapped paths and would be a good option for cloud builds or rust's CI.
note: For rust dylibs the all scope would be necessary as they contain crate metadata which needs to contain unmapped paths if diagnostics use unmapped paths.
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 think we can reasonably experiment with having all of these available, but I think that when we go to stabilize this, we should consider just a subset rather than full granularity.
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.
name-version
is only unique - at best - within a single registry. And for code coming from git, the specific commit id is probably important to encode, since it can change without version changes.Ideally it would be nice to encode a hash of the source file itself somewhere, but that probably doesn't fit well into this scheme. (I think Dwarf has a way to encode this, so it can be done in a case by case basis.)
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.
Maybe it should use the full package id as used by cargo? Package id's look like
libc 0.2.126 (registry+https://github.com/rust-lang/crates.io-index)
. The remapping could then be/cargo/libc 0.2.126 (registry+https://github.com/rust-lang/crates.io-index)/src/lib.rs
.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.
Reading this entire section, it doesn't seem to me that this functionality needs to live in Cargo. rustc could directly provide a
-C trim-paths
option with this behavior, since AFAICT rustc has all the information needed to do so. Cargo can then just pass through trim-paths to rustc.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.
Is rustc aware of the path to the current package? This is relevant to dependencies living under
$HOME/.cargo/registry
. Currently there is also the possibility (mentioned in unresolved questions) to encode more information about the package to the sanitised path, such as registry name and git commit hashThere 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.
Hmmm, fair point. I thought that rustc had the necessary information, but perhaps not.
It might make sense to pass the requisite information into rustc, but then let rustc make the decision for how to use that information. I'd rather not have cargo making that decision.
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.
rustc always knows the absolute path to files, but only the build tool can tell which part of it is environment-sensitive. E.g. rustc knows it's using
/home/cbeuw/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rand_core-0.6.3/src/lib.rs
, but the/home/cbeuw/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rand_core-0.6.3
part is up to Cargo, and it would be different if a different registry were used, or the structure of that path may be completely different for Bazel dependencies - but rustc doesn't know that.This "requisite information" is already included in provided mappings in
--remap-path-prefix
. I can't see how a separate argument that lets the build tool pass in "the sensitive part of the current path" would look like. It'd probably be either too build tool-specific, requiring rustc to know about things like Cargo registries or Bazel repositories, or too "free", ultimately being the same as the existing--remap-path-prefix
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'm not sure what you mean by "path introduced by include!() will be remapped". The path that's actually included is not subject to remapping. Do you mean a
file!()
macro in aninclude!()
ed file will be remapped? If so, yeah, I think that would be the expected behaviour. Presumably if the path is out of the scope of any of the remappings (include!("/tmp/randomcode.rs")
) then it will be left as-is.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.
When a panic is raised inside a file that's been
include!()
ed, it's the file that directly contains the panic gets embedded and printed, not the includer. This sentence in the RFC means that the includee path is sanitisede.g.
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 think it is somewhat redundant. The remapping happens for the file corresponding to the source span.
include!()
puts the included file in the source span, and only keeps the file which contained theinclude!()
as macro expansion source. Other macros may make other choices.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.
Rust-analyzer uses expect-test which does exactly this to update snapshot tests. Most of the time tests would run in debug mode I think though.
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.
test
profile is inherited fromdev
so the default behaviour ofcargo test
won't change. The line "We only need to change the behaviour for Test and Build compile modes." means other compile modes likecargo check
can simply ignore the newtrim-paths
, though the wording is a bit ambiguous.